Friendly Provider determination #21
Conversation
This will add flexibility when testing.
This verifies the caching based on userAgentProvider.
With the caching, this should be safe.
It's testing two things, so the argument could be made to split it, with the tradeoff that we would have some duplicated test code.
|
@yacaovsnc: Please validate in your downstream library as part of your code review. |
|
|
||
| if (userAgentProvider != null) { | ||
| for (final Provider provider : providers) { | ||
| if (provider.getClassName().equals(userAgentProvider)) { | ||
| return provider; | ||
| result = provider; |
There was a problem hiding this comment.
Let me know if my understanding isn't right.
According to the Javadoc findCompatibleProvider(final String userAgentProvider) will find a provider that works, only preferring the provider specified as userAgentProvider. However in this case, wouldn't this always return userAgentProvider regardless? For example, if I prefer JavaFX provider, I could always make a call findCompatibleProvider(javaFxProvider) and I will always get that provider, even though on some platform it doesn't work? From reading the Javadoc, I was under the impression that it will check if JavaFXProvider meets all the requirement, and only return it when it works.
Also another general comment, how expensive are those checks? It seems we are adding many states to UserAgentImpl because of this change, I am wondering if the checks are cheap, can we make it stateless? We could just call scanProvider() every time and check the destinationUnmetRequirements? It probably will make the code easier to read.
There was a problem hiding this comment.
According to the Javadoc findCompatibleProvider(final String userAgentProvider) will find a provider that works, only preferring the provider specified as userAgentProvider. However in this case, wouldn't this always return userAgentProvider regardless?
You're correct: I preserved the override behaviour but the documentation to findCompatibleProvider() makes it sound like a requirements check will always be performed.
It seems to me that I can preserve the existing behaviour (while also making my documentation accurate) by adding a checkOverrideIsCompatible parameter to the determineProvider() method, such that encode() will supply false and findCompatibleProvider() will supply true.
There was a problem hiding this comment.
Also another general comment, how expensive are those checks?
Given the recently-added issue #22, it appears the checkRequirements() for JavaFX will most likely need to try to instantiate a WebView, making the check expensive. I suspect an SWT provider might also need to do the same, given that some third-party dependencies sometimes need to be installed separately.
When I first wrote checkRequirements() and determineProvider(), there didn't seem to be any fixes you could make without restarting the process.
For example:
- You're running the program via a remote shell - there's no desktop, so you need to start over from the desktop.
- You've launched from an installation of Java that doesn't have JavaFX, so you need to launch from another installation of Java, possibly after downloading and installing it first.
Otherwise, once you're deemed compatible (or incompatible), it's unlikely to stop (or start) working, so there's no need to keep checking.
There was a problem hiding this comment.
(Using my personal account since I don't want to do 2FA)
I personally prefer not to complicate this further. What I had in mind was a simple check that the consumer calls and can get a true / false decision. Everything else is icing on the cake - I don't think they are essential to my use case.
I will leave it up to you to decide what kind of checks you provide, but I don't oppose just change the Javadoc to make the description accurate. :)
There was a problem hiding this comment.
Otherwise, once you're deemed compatible (or incompatible), it's unlikely to stop (or start) working, so there's no need to keep checking.
It's understood that there is no need to keep checking, the comment is more about code structure -- remove states IMHO will make the code easier to read.
"Expensive" is relative, the process we are about to invoke is a user interaction, which could run in minutes. As long as the check is tolerable (sub second?), it may not matter that much.
Another option is instead of "true/false", how about determineProvider returns the compatible provider (null to signify no provider is compatible), and then consumer can pass this Provider to requestAuthorizationCode, or create a new function named requestAuthorizationCodeWithProvider and just use it. This way you might be able to avoid storing state?
The documentation for findCompatibleProvider() overloads says that a compatible provider will be found, meaning an override should also be checked for compatibility. The checkOverrideIsCompatible flag was added to preserve the existing functionality when skipping the provider scanning and going directly to requesting the authorization code.
|
I made the changes I was suggesting, such that a call to |
|
Tested, works as advertised! |
Implements the spirit of issue #13, with slight deviations/improvements from the original plan.
The good news is the new methods are written based on fragments of the old methods, so I was able to preserve most of the existing functionality.