-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #4691 - use static methods with javadoc to get MethodHandle lookups #4694
Issue #4691 - use static methods with javadoc to get MethodHandle lookups #4694
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall good work, a few things to correct.
@@ -374,7 +374,7 @@ private void assertBasicTypeNotRegistered(byte basicWebSocketType, Object messag | |||
try | |||
{ | |||
// TODO: move methodhandle lookup to container? | |||
MethodHandles.Lookup lookup = MethodHandles.publicLookup(); | |||
MethodHandles.Lookup lookup = JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you still need the TODO above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove it, I don't know why we would want to do this in the container, it seems better to contain the MethodHandle stuff in the FrameHandler.
@@ -432,7 +432,7 @@ else if (String.class.isAssignableFrom(clazz)) | |||
try | |||
{ | |||
// TODO: move MethodHandle lookup to container? | |||
MethodHandles.Lookup lookup = MethodHandles.publicLookup(); | |||
MethodHandles.Lookup lookup = JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
/** | ||
* <p> | ||
* Gives a {@link MethodHandles.Lookup} instance to be used to find methods in server classes. | ||
* For lookups on application classes use {@link #getApplicationMethodHandleLookup(Class)} ()} instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an additional ()}
in the javadoc.
* to server classes we need to use and will give access permissions to private methods as well. | ||
* </p> | ||
* | ||
* @return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing description of what it returns.
* to server classes we need to use and will give access permissions to private methods as well. | ||
* </p> | ||
* | ||
* @return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid duplicating these javadocs, I would rather @see JavaxWebSocketFrameHandlerFactory#getServerMethodHandleLookup()
.
So that if we improve the javadocs, we only do it in one place.
Check that we can actually do this @see
because it will reference a class in a different module that is not dependent on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can't be done, I think they will have to be duplicated.
Symbol 'org.eclipse.jetty.websocket.javax.common.JavaxWebSocketFrameHandlerFactory' is inaccessible from here.
* </p> | ||
* @param lookupClass the desired lookup class for the new lookup object. | ||
* @return a lookup object to be used to find methods on the lookupClass. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
Signed-off-by: Lachlan Roberts <[email protected]>
Issue #4691
Method handle lookups for websocket are now all created from static methods on both the Jetty and Javax FrameHandlerFactory. Two static methods are given, one for looking up methods on server classes and one for looking up methods on application classes.
These methods now have javadoc to describe the reasons why these specific MethodHandle lookups are the correct ones to use for the given situation.