-
-
Notifications
You must be signed in to change notification settings - Fork 113
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
[WebPayments] Introduce OverlayWidget & Use it by PaymentHandler #1654
Conversation
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.
LGTM but I'd like to defer to others who take a look with other points of view.
what should we do with #1646 then? |
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've done a general review and while the code is clean and easy to understand I have the feeling that it's a bit ad-hoc. Anyway, as I am not totally sure, and provided that it deserves a more in depth review, I'll neither approve nor request changes yet.
Context context = mWebContents.get().getTopLevelNativeWindow().getContext().get(); | ||
final TabCompositorView compositorView = new TabCompositorView(context); | ||
compositorView.onNativeLibraryLoaded(windowAndroid); | ||
windowAndroid.setAnimationPlaceholderView(compositorView); |
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.
Can't we do this kind of things on the Chromium side? I believe we do that when we create a new Tab for example.
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 moved this logic to the Tab class, as a new static method. It's now evident that this code is identical to the Tab constructor, so we can apply some refactoring in the future.
The Tab.createNewTab is an horrible name, so suggestions are really welcome.
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.
Tab.getNewCompositorView
or Tab.createCompositorView
?
This patch applies border and brightness effects to OverlayContentWidget. The border will overlap 2px on the left/right/top/bottom of WebContents because the browser's compositor implementation doesn't consider the x, y coordinates when setting up the surface. It's same with Widget for Tab, and we maybe consider to have the inner Widget to avoid this issue later. Having a border effect will bring about a better UX even if it loses 2 pixels.
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.
The new changes to add borders to the PaymentHandler window work fine and the code looks good.
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 personally don't like very much some parts of the current design like:
- getActiveWebContents()
- storing an additional web contents in the tab
- the ovorlaywidget which is basically a reimplementation of our window
- the new tab defined in chromium is only used to get the display and reuse code in chromium but not as a regular tab in Wolvic
That said it works and our limited resources do not allow us to spend much more time on this so I guess we can land it and see.
A new OverlayWidget class is defined to render the payment handler WebContents instance, provided by the web engine, as a result of the call to the Payment Request API.
The tab that initiates the payment request needs to handle 2 WebContents instances, determining which one is active during the process. Hence, The OverlayWidget is implemented as modal dialog, preventing the original web-content to be updated until the payment request is completed.
The TabsWebContentObserver must implement a new WebContentsObserver method, defined to notify Wolvic that the new payment handler WebContents instance is ready.
This PR depends on the PR #143 in the Chromium backend.
This feature can be tested using the Web Payments demo at https://paymentrequest.show/demo