Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2893823004:
[Payments] Implement openWindow for service worker based payment handler (Closed)
|
Created:
3 years, 7 months ago by gogerald1 Modified:
3 years, 6 months ago Reviewers:
falken, gogerald, please use gerrit instead, dcheng, Ted C, Yusuf, zino, haraken, Marijn Kruisselbrink CC:
agrieve+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-worker-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, falken+watch_chromium.org, gogerald+paymentswatch_chromium.org, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kinuko+watch, kinuko+serviceworker, mahmadi+paymentswatch_chromium.org, michaeln, mlamouri+watch-content_chromium.org, nhiroki, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, serviceworker-reviews, shimazu+serviceworker_chromium.org, tzik Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement openWindow for web based payment handler
1, This CL implements openWindow in PaymentRequestEvent.idl for service
worker based payment handler.
The spec: https://github.com/w3c/payment-handler.
The intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2ojnMk_T9_c/h7QfIhTeCAAJ
2, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and
chrome://flags/#service_worker_payment_apps flags
3, openWindow opens the url in custom tab with sliding up and down animation.
4, The custom tab is opened within the foreground ChromeActivity so as to not list in recent apps
(Intent.FLAG_ACTIVITY_NEW_TASK is not set).
5, Do not open the url if the foreground activity is not ChromeActivity.
6, Test video record
https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWhjRjY1LWpTeEdwZ1c0/view?usp=sharing
BUG=720027, 661608
Review-Url: https://codereview.chromium.org/2893823004
Cr-Commit-Position: refs/heads/master@{#477180}
Committed: https://chromium.googlesource.com/chromium/src/+/1a4b4420868253582467b228f5df4bf167b3ecff
Patch Set 1 #Patch Set 2 : format #
Total comments: 35
Patch Set 3 : address comments #
Total comments: 20
Patch Set 4 : address comments and rebase #Patch Set 5 : format #
Total comments: 16
Patch Set 6 : fix nit #Patch Set 7 : address yusuf's comments #Patch Set 8 : correct file changes #
Total comments: 14
Patch Set 9 : rename #Patch Set 10 : use popup window for payment handler #
Total comments: 20
Patch Set 11 : address comments #
Total comments: 15
Patch Set 12 : addressed nits #Patch Set 13 : use CompleteURL #
Total comments: 4
Patch Set 14 : move createPopupCustomTab to ServiceTabLauncher #Patch Set 15 : remove canDisplay check #Patch Set 16 : add todos #
Total comments: 6
Patch Set 17 : use const reference #Patch Set 18 : rename ipc messages #
Total comments: 1
Messages
Total messages: 186 (140 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== test BUG= ========== to ========== Implement openWindow for web based payment handler BUG=720027 ==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Description was changed from ========== Implement openWindow for web based payment handler BUG=720027 ========== to ========== Implement openWindow for web based payment handler This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow It is based on the CL https://codereview.chromium.org/2887713002/ BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow It is based on the CL https://codereview.chromium.org/2887713002/ BUG=720027 ========== to ========== Implement openWindow for web based payment handler This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow It is based on the CL https://codereview.chromium.org/2887713002/ openWindow opens the new tab in custom tab with sliding up and down animation BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow It is based on the CL https://codereview.chromium.org/2887713002/ openWindow opens the new tab in custom tab with sliding up and down animation BUG=720027 ========== to ========== Implement openWindow for web based payment handler This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. It is based on the CL https://codereview.chromium.org/2887713002/. openWindow opens the new tab in custom tab with sliding up and down animation. BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. It is based on the CL https://codereview.chromium.org/2887713002/. openWindow opens the new tab in custom tab with sliding up and down animation. BUG=720027 ========== to ========== Implement openWindow for web based payment handler This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. It is based on the CL https://codereview.chromium.org/2887713002/. openWindow opens the new tab in custom tab with sliding up and down animation. The new tab is shown over the tab showing merchant website without showing in recent apps, so Intent.FLAG_ACTIVITY_NEW_TASK is not set. BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. It is based on the CL https://codereview.chromium.org/2887713002/. openWindow opens the new tab in custom tab with sliding up and down animation. The new tab is shown over the tab showing merchant website without showing in recent apps, so Intent.FLAG_ACTIVITY_NEW_TASK is not set. BUG=720027 ========== to ========== Implement openWindow for web based payment handler This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. It is based on the CL https://codereview.chromium.org/2887713002/. openWindow opens the new tab in custom tab with sliding up and down animation. The new tab is shown over the tab showing merchant website without showing in recent apps, so Intent.FLAG_ACTIVITY_NEW_TASK is not set. BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. It is based on the CL https://codereview.chromium.org/2887713002/. openWindow opens the new tab in custom tab with sliding up and down animation. The new tab is shown over the tab showing merchant website without showing in recent apps, so Intent.FLAG_ACTIVITY_NEW_TASK is not set. BUG=720027 ========== to ========== Implement openWindow for web based payment handler This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. It is based on the CL https://codereview.chromium.org/2887713002/. openWindow opens the new tab in custom tab with sliding up and down animation. The new tab is shown over the tab showing merchant website without showing in recent apps, so Intent.FLAG_ACTIVITY_NEW_TASK is not set. Do not open the new tab is the tab showing merchant website is not active. BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. It is based on the CL https://codereview.chromium.org/2887713002/. openWindow opens the new tab in custom tab with sliding up and down animation. The new tab is shown over the tab showing merchant website without showing in recent apps, so Intent.FLAG_ACTIVITY_NEW_TASK is not set. Do not open the new tab is the tab showing merchant website is not active. BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1,This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the new tab in custom tab with sliding up and down animation. 4, The new tab is shown over the tab showing merchant website without showing in recent apps, so Intent.FLAG_ACTIVITY_NEW_TASK is not set. 5, Do not open the new tab is the tab showing merchant website is not active. BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler 1,This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the new tab in custom tab with sliding up and down animation. 4, The new tab is shown over the tab showing merchant website without showing in recent apps, so Intent.FLAG_ACTIVITY_NEW_TASK is not set. 5, Do not open the new tab is the tab showing merchant website is not active. BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the new tab in custom tab with sliding up and down animation. 4, The new tab is shown over the tab showing merchant website without showing in recent apps, so Intent.FLAG_ACTIVITY_NEW_TASK is not set. 5, Do not open the new tab is the tab showing merchant website is not active. BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the new tab in custom tab with sliding up and down animation. 4, The new tab is shown over the tab showing merchant website without showing in recent apps, so Intent.FLAG_ACTIVITY_NEW_TASK is not set. 5, Do not open the new tab is the tab showing merchant website is not active. BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The new tab is shown over the tab showing merchant website without showing in recent apps, so Intent.FLAG_ACTIVITY_NEW_TASK is not set. 5, Do not open the new tab is the tab showing merchant website is not active. BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The new tab is shown over the tab showing merchant website without showing in recent apps, so Intent.FLAG_ACTIVITY_NEW_TASK is not set. 5, Do not open the new tab is the tab showing merchant website is not active. BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the new tab is the tab showing merchant website is not active. BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the new tab is the tab showing merchant website is not active. BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the new tab is the tab showing merchant website is not active. BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the new tab is the tab showing merchant website is not active. BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the tab showing merchant website is not active. BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the tab showing merchant website is not active. BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the tab showing merchant website is not active. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the tab showing merchant website is not active. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the tab showing merchant website is not active. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ==========
gogerald@chromium.org changed reviewers: + dpranke@chromium.org, jinho.bang@samsung.com, mek@chromium.org, rouslan@chromium.org, tedchoc@chromium.org
Hi, mek@ and dpranke@, please review changes in content/* and third_party/* tedchoc@, please review changes in chrome/android/* rouslan@ and jinho.bang@ please review overall changes,
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for web based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the tab showing merchant website is not active. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the tab showing merchant website is not active. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the tab showing merchant website is not active. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the tab showing merchant website is not active. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ==========
On 2017/05/24 21:36:23, gogerald1 wrote: > Hi, > > mek@ and dpranke@, please review changes in content/* and third_party/* > > tedchoc@, please review changes in chrome/android/* > > rouslan@ and jinho.bang@ please review overall changes, I don't think you want me reviewing content and WebKit changes these days; is there some reason you thought I'd be an apt OWNER/reviewer? I might need to update some owners files somewhere ...
gogerald@chromium.org changed reviewers: - dpranke@chromium.org
gogerald@chromium.org changed reviewers: + dpranke@chromium.org, haraken@chromium.org
Ah, miss trust my extension, removing dpranke@, Hi haraken@, please review changes in third_party/WebKit/*
gogerald@chromium.org changed reviewers: - dpranke@chromium.org
WebKit LGTM. If RuntimeEnabledFeatures=PaymentApp is enabled on Android, you need to get an approval from an API owner for the IDL change.
RuntimeEnabledFeatures=PaymentApp is currently disabled on all platforms.
Thank you for the patch! This looks promising. I have a general question about your design. It seems that the whole patch hinges on the new parameter "redirect_url", which is used to determine how to open the CCT window. Can you explain in more detail what is contained in this URL and why this parameter is necessary? It's not immediately obvious, IMHO. If we want several different ways to open the CCT, I would prefer to control this behavior through an enum value that propagates from the renderer to the browser. https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/re... File chrome/android/java/res/anim/slide_in_up.xml (right): https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/re... chrome/android/java/res/anim/slide_in_up.xml:2: <!-- Copyright (C) 2009 The Android Open Source Project Where is this file coming from? https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:370: mTopLevelOrigin = mWebContents.getLastCommittedUrl(); That's a top level URL, because it contains the full path. Origins do not have a path. Therefore, this variable should be called mTopLevelUrl. https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:371: mTopLevelOriginForDisplay = UrlFormatter.formatUrlForSecurityDisplay(mTopLevelOrigin, true); This should remain mTopLevelOrigin. https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java (right): https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:106: * @param redirectUrl The url of the background tab showing. I feel that this should be called sourceUrl. Otherwise I am getting this confused with HTTP redirect codes 3xx. https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:110: // The activity contains the background tab must be active, otherwise do not open the new The activity *that* contains... https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:119: if (!redirectUrl.equalsIgnoreCase(webcontents.getLastCommittedUrl())) return false; Let's be more restrictive and require the URLs to match case. https://codereview.chromium.org/2893823004/diff/140001/chrome/browser/android... File chrome/browser/android/service_tab_launcher.cc (right): https://codereview.chromium.org/2893823004/diff/140001/chrome/browser/android... chrome/browser/android/service_tab_launcher.cc:74: static_cast<int>(disposition), referrer_url, params.referrer.policy, How is referrer_url different from redirect_url? https://codereview.chromium.org/2893823004/diff/140001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2893823004/diff/140001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:107: const blink::WebURL&, Please provide the names for these parameters. The names are usually omitted only when it's clear what the parameters are. Here, however, it's not immediately clear which URL should be opened and which URL is the "redirect" url. https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl (right): https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl:21: [CallWith=ScriptState] Promise<WindowClient?> openWindow(USVString url); The spec does not have a question mark next to WindowClient, meaning that null cannot be returned, I believe. Can you explain the need for the question mark here? If it's an implementation requirement, then please file a bug on https://github.io/w3c/payment-handler/issues. https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.h (right): https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.h:32: ScriptPromise openWindow(ScriptState*, Perhaps a better name for this method would be a more explicit "openWindowForPaymentHandler". That would clarify that this openWindow() is not part of the Clients.idl. https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h (right): https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:73: const WebURL&, Need parameter names here. https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.h (right): https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.h:60: const WebURL&, Need parameter names. https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right): https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:276: const WebURL&, Need parameter names.
rouslan@chromium.org changed reviewers: + yusufo@chromium.org
+Yusuf for CCT review.
Ganggui, please add a note to the CL description that this feature is behind the chrome://flags/#enable-experimental-web-platform-features flag.
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the tab showing merchant website is not active. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, This feature is disabled by chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the tab showing merchant website is not active. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, This feature is disabled by chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the tab showing merchant website is not active. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 4, openWindow opens the url in custom tab with sliding up and down animation. 5, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 6, Do not open the url if the tab showing merchant website is not active. 7, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ==========
Patchset #3 (id:160001) has been deleted
Addressed rouslan@'s comments, Currently, The redirect_url is used to identify which activity the new tab should be opened within so the new tab is not displayed as a separate app in recent apps. The redirect_url is also used to check whether webContents triggering the payment request is still alive in the foreground, if not, we do not open the new tab. https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/re... File chrome/android/java/res/anim/slide_in_up.xml (right): https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/re... chrome/android/java/res/anim/slide_in_up.xml:2: <!-- Copyright (C) 2009 The Android Open Source Project On 2017/05/25 14:53:26, ಠ_ಠwrote: > Where is this file coming from? It's from here https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/platforms/... unfortunately the original file is not accessible through android.R.anim https://developer.android.com/reference/android/R.anim.html Updated the comments to "Copyright 2017 The Chromium Authors. All rights reserved." https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:370: mTopLevelOrigin = mWebContents.getLastCommittedUrl(); On 2017/05/25 14:53:26, ಠ_ಠwrote: > That's a top level URL, because it contains the full path. Origins do not have a > path. Therefore, this variable should be called mTopLevelUrl. Done. https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:371: mTopLevelOriginForDisplay = UrlFormatter.formatUrlForSecurityDisplay(mTopLevelOrigin, true); On 2017/05/25 14:53:26, ಠ_ಠwrote: > This should remain mTopLevelOrigin. Done. https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java (right): https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:106: * @param redirectUrl The url of the background tab showing. On 2017/05/25 14:53:26, ಠ_ಠwrote: > I feel that this should be called sourceUrl. Otherwise I am getting this > confused with HTTP redirect codes 3xx. I named it according to the redirect_chain in OpenURLParams https://cs.chromium.org/chromium/src/content/public/browser/page_navigator.h?... This way the name is consistent in java and c++ side, make sense? https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:110: // The activity contains the background tab must be active, otherwise do not open the new On 2017/05/25 14:53:26, ಠ_ಠwrote: > The activity *that* contains... Done. https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:119: if (!redirectUrl.equalsIgnoreCase(webcontents.getLastCommittedUrl())) return false; On 2017/05/25 14:53:26, ಠ_ಠwrote: > Let's be more restrictive and require the URLs to match case. Done. https://codereview.chromium.org/2893823004/diff/140001/chrome/browser/android... File chrome/browser/android/service_tab_launcher.cc (right): https://codereview.chromium.org/2893823004/diff/140001/chrome/browser/android... chrome/browser/android/service_tab_launcher.cc:74: static_cast<int>(disposition), referrer_url, params.referrer.policy, On 2017/05/25 14:53:27, ಠ_ಠwrote: > How is referrer_url different from redirect_url? refer it here https://cs.chromium.org/chromium/src/content/public/browser/page_navigator.h?... It has referrer and and redirect_chain https://codereview.chromium.org/2893823004/diff/140001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2893823004/diff/140001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:107: const blink::WebURL&, On 2017/05/25 14:53:27, ಠ_ಠwrote: > Please provide the names for these parameters. The names are usually omitted > only when it's clear what the parameters are. Here, however, it's not > immediately clear which URL should be opened and which URL is the "redirect" > url. Done. Add redirect_url name. check-webkit-style reports "The parameter name "callbacks"/"url" adds no information, so it should be removed". https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl (right): https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl:21: [CallWith=ScriptState] Promise<WindowClient?> openWindow(USVString url); On 2017/05/25 14:53:27, ಠ_ಠwrote: > The spec does not have a question mark next to WindowClient, meaning that null > cannot be returned, I believe. Can you explain the need for the question mark > here? If it's an implementation requirement, then please file a bug on > https://github.io/w3c/payment-handler/issues. It must have question mark as for service worker, the openWindow algorithm in the spec also mentioned windowClient could be null. reported this issue, https://github.com/w3c/payment-handler/issues/163 https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.h (right): https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.h:32: ScriptPromise openWindow(ScriptState*, On 2017/05/25 14:53:27, ಠ_ಠwrote: > Perhaps a better name for this method would be a more explicit > "openWindowForPaymentHandler". That would clarify that this openWindow() is not > part of the Clients.idl. This interface is also used by service worker, so I add a comment to explain it. https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h (right): https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:73: const WebURL&, On 2017/05/25 14:53:27, ಠ_ಠwrote: > Need parameter names here. Done. https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.h (right): https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.h:60: const WebURL&, On 2017/05/25 14:53:27, ಠ_ಠwrote: > Need parameter names. Done. https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right): https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:276: const WebURL&, On 2017/05/25 14:53:27, ಠ_ಠwrote: > Need parameter names. Done.
Great work! I left some comments. Thank you. https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl (right): https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl:21: [CallWith=ScriptState] Promise<WindowClient?> openWindow(USVString url); On 2017/05/25 14:53:27, ಠ_ಠwrote: > The spec does not have a question mark next to WindowClient, meaning that null > cannot be returned, I believe. Can you explain the need for the question mark > here? If it's an implementation requirement, then please file a bug on > https://github.io/w3c/payment-handler/issues. I think it is correct that this function does not return null. Unlike clients.openWindow(), this function will create controlled window client only. According to the spec[1] 8-4 section, if the origin is not the same as the service worker client origin associated with the payment handler, then, reject promise with SecurityError. [1] https://w3c.github.io/payment-handler/#dfn-open-window-algorithm https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.h (right): https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.h:32: ScriptPromise openWindow(ScriptState*, On 2017/05/25 14:53:27, ಠ_ಠwrote: > Perhaps a better name for this method would be a more explicit > "openWindowForPaymentHandler". That would clarify that this openWindow() is not > part of the Clients.idl. +1 I agree with rouslan@'s opinion. https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right): https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:60: return ToServiceWorkerGlobalScope(ExecutionContext::From(script_state)) If the origin of the url is not the same as the SW's origin, then we probably should reject with SecurityError. Please see 8-4 section: https://w3c.github.io/payment-handler/#dfn-open-window-algorithm Also, we will have to add a test for it. https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl (right): https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl:21: [CallWith=ScriptState] Promise<WindowClient?> openWindow(USVString url); This should be Promise<WindowClient> according to the spec. Please see my another comment: https://codereview.chromium.org/2893823004/patch/140001/150020 https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h (right): https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:72: virtual void OpenWindow(const WebURL& redirect_url, It might be better to add a new method (e.g. OpenWindowForPaymentHandler) and share the implementation with clients.openWindow. (If the redirect_url is not used in clients.openWindow)
https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/re... File chrome/android/java/res/anim/slide_in_up.xml (right): https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/re... chrome/android/java/res/anim/slide_in_up.xml:2: <!-- Copyright (C) 2009 The Android Open Source Project On 2017/05/25 16:53:32, gogerald1 wrote: > On 2017/05/25 14:53:26, ಠ_ಠwrote: > > Where is this file coming from? > > It's from here > https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/platforms/... > > unfortunately the original file is not accessible through android.R.anim > https://developer.android.com/reference/android/R.anim.html > > > Updated the comments to "Copyright 2017 The Chromium Authors. All rights > reserved." I'm not sure we can change this license, but we should definitely state "This file is copied from third_party/..." Make sure you get approval from Ted to copy this file in here. https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java (right): https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:106: * @param redirectUrl The url of the background tab showing. On 2017/05/25 16:53:32, gogerald1 wrote: > On 2017/05/25 14:53:26, ಠ_ಠwrote: > > I feel that this should be called sourceUrl. Otherwise I am getting this > > confused with HTTP redirect codes 3xx. > > I named it according to the redirect_chain in OpenURLParams > https://cs.chromium.org/chromium/src/content/public/browser/page_navigator.h?... > > This way the name is consistent in java and c++ side, make sense? Acknowledged. https://codereview.chromium.org/2893823004/diff/140001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2893823004/diff/140001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:107: const blink::WebURL&, On 2017/05/25 16:53:32, gogerald1 wrote: > On 2017/05/25 14:53:27, ಠ_ಠwrote: > > Please provide the names for these parameters. The names are usually omitted > > only when it's clear what the parameters are. Here, however, it's not > > immediately clear which URL should be opened and which URL is the "redirect" > > url. > > Done. Add redirect_url name. check-webkit-style reports "The parameter name > "callbacks"/"url" adds no information, so it should be removed". You can remove "callbacks" parameter name and rename "url" parameter into "url_to_open". https://codereview.chromium.org/2893823004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2893823004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:716: CanMakePaymentQuery query = sCanMakePaymentQueries.get(mPaymentRequestUrl); canMakePayment() queries should be throttled based on the origin of the iframe, not based on the URL. That way, https://evil.com cannot abuse the queries by opening sub-iframes from different URLS on the same origin, e.g., https://evil.com/check-visa.html and https://evil.com/check-mastercard.html. Therefore, you need to keep around mPaymentRequestOrigin variable for use with sCanMakePaymentQueries. https://codereview.chromium.org/2893823004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:728: q.getKey().getInstruments(q.getValue(), mTopLevelUrl, mPaymentRequestUrl, Please rename these parameters in PaymentApp.java and update their comments. That way, whoever is working on PaymentApp.java will know to expect the full URL instead of only origin. https://codereview.chromium.org/2893823004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1300: instrument.invokePaymentApp(mId, mMerchantName, mTopLevelUrl, mPaymentRequestUrl, Please update the names and comments of these parameters in PaymentInstrument.java. https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl (right): https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl:21: [CallWith=ScriptState] Promise<WindowClient?> openWindow(USVString url); What zino said. https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h (right): https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:72: virtual void OpenWindow(const WebURL& redirect_url, On 2017/05/25 17:02:42, zino wrote: > It might be better to add a new method (e.g. OpenWindowForPaymentHandler) and > share the implementation with clients.openWindow. (If the redirect_url is not > used in clients.openWindow) +1 https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.h (right): https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.h:60: const WebURL&, url_to_open https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp (right): https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp:62: void OpenWindow(const WebURL& redirect_url, Here the parameter name does not really matter, IMHO, because none of the parameters are used. https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right): https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:276: const WebURL&, url_to_open
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:200001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Patchset #4 (id:220001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:280001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:260001) has been deleted
Patchset #5 (id:300001) has been deleted
Patchset #5 (id:320001) has been deleted
Patchset #5 (id:340001) has been deleted
Patchset #4 (id:240001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rouslan@ and zino@, another look? https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/re... File chrome/android/java/res/anim/slide_in_up.xml (right): https://codereview.chromium.org/2893823004/diff/140001/chrome/android/java/re... chrome/android/java/res/anim/slide_in_up.xml:2: <!-- Copyright (C) 2009 The Android Open Source Project On 2017/05/25 17:56:12, ooo till 2017.6.5 wrote: > On 2017/05/25 16:53:32, gogerald1 wrote: > > On 2017/05/25 14:53:26, ಠ_ಠwrote: > > > Where is this file coming from? > > > > It's from here > > > https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/platforms/... > > > > unfortunately the original file is not accessible through android.R.anim > > https://developer.android.com/reference/android/R.anim.html > > > > > > Updated the comments to "Copyright 2017 The Chromium Authors. All rights > > reserved." > > I'm not sure we can change this license, but we should definitely state "This > file is copied from third_party/..." > > Make sure you get approval from Ted to copy this file in here. Changed to chromium property, this is a very normal definition of animation, https://codereview.chromium.org/2893823004/diff/140001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2893823004/diff/140001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:107: const blink::WebURL&, On 2017/05/25 17:56:12, ooo till 2017.6.5 wrote: > On 2017/05/25 16:53:32, gogerald1 wrote: > > On 2017/05/25 14:53:27, ಠ_ಠwrote: > > > Please provide the names for these parameters. The names are usually omitted > > > only when it's clear what the parameters are. Here, however, it's not > > > immediately clear which URL should be opened and which URL is the "redirect" > > > url. > > > > Done. Add redirect_url name. check-webkit-style reports "The parameter name > > "callbacks"/"url" adds no information, so it should be removed". > > You can remove "callbacks" parameter name and rename "url" parameter into > "url_to_open". Done. https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl (right): https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl:21: [CallWith=ScriptState] Promise<WindowClient?> openWindow(USVString url); On 2017/05/25 17:02:42, zino wrote: > On 2017/05/25 14:53:27, ಠ_ಠwrote: > > The spec does not have a question mark next to WindowClient, meaning that null > > cannot be returned, I believe. Can you explain the need for the question mark > > here? If it's an implementation requirement, then please file a bug on > > https://github.io/w3c/payment-handler/issues. > > I think it is correct that this function does not return null. > Unlike clients.openWindow(), this function will create controlled window client > only. > According to the spec[1] 8-4 section, if the origin is not the same as the > service worker client origin associated with the payment handler, then, > reject promise with SecurityError. > > [1] https://w3c.github.io/payment-handler/#dfn-open-window-algorithm Done. https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.h (right): https://codereview.chromium.org/2893823004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.h:32: ScriptPromise openWindow(ScriptState*, On 2017/05/25 17:02:42, zino wrote: > On 2017/05/25 14:53:27, ಠ_ಠwrote: > > Perhaps a better name for this method would be a more explicit > > "openWindowForPaymentHandler". That would clarify that this openWindow() is > not > > part of the Clients.idl. > > +1 I agree with rouslan@'s opinion. Done. https://codereview.chromium.org/2893823004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2893823004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:716: CanMakePaymentQuery query = sCanMakePaymentQueries.get(mPaymentRequestUrl); On 2017/05/25 17:56:13, ooo till 2017.6.5 wrote: > canMakePayment() queries should be throttled based on the origin of the iframe, > not based on the URL. That way, https://evil.com cannot abuse the queries by > opening sub-iframes from different URLS on the same origin, e.g., > https://evil.com/check-visa.html and https://evil.com/check-mastercard.html. > > Therefore, you need to keep around mPaymentRequestOrigin variable for use with > sCanMakePaymentQueries. my bad, payment request handler do require origin instead of url, so I will keep origin here. However our current format of origin is not standard, we could look it later. https://codereview.chromium.org/2893823004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:728: q.getKey().getInstruments(q.getValue(), mTopLevelUrl, mPaymentRequestUrl, On 2017/05/25 17:56:13, ಠ_ಠwrote: > Please rename these parameters in PaymentApp.java and update their comments. > That way, whoever is working on PaymentApp.java will know to expect the full URL > instead of only origin. Done. https://codereview.chromium.org/2893823004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1300: instrument.invokePaymentApp(mId, mMerchantName, mTopLevelUrl, mPaymentRequestUrl, On 2017/05/25 17:56:13, ಠ_ಠwrote: > Please update the names and comments of these parameters in > PaymentInstrument.java. Done. https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right): https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:60: return ToServiceWorkerGlobalScope(ExecutionContext::From(script_state)) On 2017/05/25 17:02:42, zino wrote: > If the origin of the url is not the same as the SW's origin, then we probably > should reject with SecurityError. > Please see 8-4 section: > https://w3c.github.io/payment-handler/#dfn-open-window-algorithm > > Also, we will have to add a test for it. Done. https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl (right): https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl:21: [CallWith=ScriptState] Promise<WindowClient?> openWindow(USVString url); On 2017/05/25 17:56:13, ಠ_ಠwrote: > What zino said. Done. https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h (right): https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:72: virtual void OpenWindow(const WebURL& redirect_url, On 2017/05/25 17:02:42, zino wrote: > It might be better to add a new method (e.g. OpenWindowForPaymentHandler) and > share the implementation with clients.openWindow. (If the redirect_url is not > used in clients.openWindow) Done. https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.h (right): https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.h:60: const WebURL&, On 2017/05/25 17:56:13, ಠ_ಠwrote: > url_to_open Done. https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp (right): https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp:62: void OpenWindow(const WebURL& redirect_url, On 2017/05/25 17:56:13, ಠ_ಠwrote: > Here the parameter name does not really matter, IMHO, because none of the > parameters are used. Done. https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right): https://codereview.chromium.org/2893823004/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:276: const WebURL&, On 2017/05/25 17:56:13, ಠ_ಠwrote: > url_to_open Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nit https://codereview.chromium.org/2893823004/diff/380001/content/browser/paymen... File content/browser/payments/payment_app_browsertest.cc (right): https://codereview.chromium.org/2893823004/diff/380001/content/browser/paymen... content/browser/payments/payment_app_browsertest.cc:216: ASSERT_EQ("", response->method_name); nit: It would be better to leave a comment why this variable is empty string. (or we can send some information from worker to tester when openWindow() is rejected.)
falken@chromium.org changed reviewers: + falken@chromium.org
The linked bug doesn't give sufficient context about this change. The bug should probably be a blocking bug of https://bugs.chromium.org/p/chromium/issues/detail?id=587995. But the spec link there is broken. Was there an intent to implement for this API? https://codereview.chromium.org/2893823004/diff/380001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2893823004/diff/380001/content/browser/servic... content/browser/service_worker/service_worker_version.h:587: void OnOpenWindow(int request_id, GURL redirect_url, GURL url_to_open); redirect_url sounds like an HTTP redirect which doesn't seem to be the case here. Is redirect_url a term in the spec being implemented?
On 2017/05/31 01:24:50, falken wrote: > The linked bug doesn't give sufficient context about this change. > > The bug should probably be a blocking bug of > https://bugs.chromium.org/p/chromium/issues/detail?id=587995. This is Payment Handler API (Payment App API): http://crbug.com/661608 > > But the spec link there is broken. > > Was there an intent to implement for this API? The spec link: https://github.com/w3c/payment-handler Intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2ojnMk_T9_c/h7... FYI, this feature is still behind a runtime flag. gogerald1@, it's better to update a commit description contains above links.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. https://w3c.github.io/payment-handler/#dom-paymentrequestevent-openwindow. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 4, openWindow opens the url in custom tab with sliding up and down animation. 5, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 6, Do not open the url if the tab showing merchant website is not active. 7, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. The spec https://github.com/w3c/payment-handler. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 4, openWindow opens the url in custom tab with sliding up and down animation. 5, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 6, Do not open the url if the tab showing merchant website is not active. 7, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. The spec https://github.com/w3c/payment-handler. 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 4, openWindow opens the url in custom tab with sliding up and down animation. 5, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 6, Do not open the url if the tab showing merchant website is not active. 7, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. The spec: https://github.com/w3c/payment-handler. The intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2ojnMk_T9_c/h7... 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 4, openWindow opens the url in custom tab with sliding up and down animation. 5, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 6, Do not open the url if the tab showing merchant website is not active. 7, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ==========
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. The spec: https://github.com/w3c/payment-handler. The intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2ojnMk_T9_c/h7... 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 4, openWindow opens the url in custom tab with sliding up and down animation. 5, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 6, Do not open the url if the tab showing merchant website is not active. 7, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. The spec: https://github.com/w3c/payment-handler. The intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2ojnMk_T9_c/h7... 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 4, openWindow opens the url in custom tab with sliding up and down animation. 5, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 6, Do not open the url if the tab showing merchant website is not active. 7, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027,661608 ==========
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. The spec: https://github.com/w3c/payment-handler. The intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2ojnMk_T9_c/h7... 2, It is based on the CL https://codereview.chromium.org/2887713002/. 3, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 4, openWindow opens the url in custom tab with sliding up and down animation. 5, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 6, Do not open the url if the tab showing merchant website is not active. 7, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027,661608 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. The spec: https://github.com/w3c/payment-handler. The intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2ojnMk_T9_c/h7... 2, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the tab showing merchant website is not active. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027,661608 ==========
Thanks zino@, Hi falken@, I updated the CL description and replied your comments, let me know if it is still not clear. https://codereview.chromium.org/2893823004/diff/380001/content/browser/paymen... File content/browser/payments/payment_app_browsertest.cc (right): https://codereview.chromium.org/2893823004/diff/380001/content/browser/paymen... content/browser/payments/payment_app_browsertest.cc:216: ASSERT_EQ("", response->method_name); On 2017/05/30 19:08:40, zino wrote: > nit: It would be better to leave a comment why this variable is empty string. > (or we can send some information from worker to tester when openWindow() is > rejected.) Done. https://codereview.chromium.org/2893823004/diff/380001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2893823004/diff/380001/content/browser/servic... content/browser/service_worker/service_worker_version.h:587: void OnOpenWindow(int request_id, GURL redirect_url, GURL url_to_open); On 2017/05/31 01:24:50, falken wrote: > redirect_url sounds like an HTTP redirect which doesn't seem to be the case > here. Is redirect_url a term in the spec being implemented? It's not a term in the spec, but named according to the parameter 'redirect_chain' in LoadURLParams (https://cs.chromium.org/chromium/src/content/public/browser/navigation_contro...). It is used to identify the web page triggering the payment request so as to caused the openWindow operation, the opened window be shown on top of the web page.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:756: tab.getWebContents()); should this be in finishNativeInitialization? Doesn't seem very related with loading, but looks more like a post tab initialization task that is needed only once per custom tab. https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java (right): https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:106: * @param requestId Id of the request for launching this tab. Does requestId have any context in other contexts? This is a very generic class and while adding new public calls to this, we should at least have more info on the params. What is this requestId used? and in what context? https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:107: * @param redirectUrl The url of the background tab showing. I do understand how these params represent your use case, but would be better to have more generic names here for this class. sourceUrl? https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:124: webcontents.getLastCommittedUrl(), true))) { how does this differ from getUrl in Tab? Can we not use that? Much more common to use than webContents in our codebase. https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:133: customTabsIntent.intent.setData(Uri.parse(url)); you can use CustomTabsIntent#launchUrl for this at the end. https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:136: customTabsIntent.intent.putExtra(IntentHandler.EXTRA_OPEN_NEW_INCOGNITO_TAB, mIsIncognito); do we open incognito tabs in this CL's context?
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Yusuf@, address your comments, another look? https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:756: tab.getWebContents()); On 2017/05/31 18:28:59, Yusuf wrote: > should this be in finishNativeInitialization? Doesn't seem very related with > loading, but looks more like a post tab initialization task that is needed only > once per custom tab. Done. https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java (right): https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:106: * @param requestId Id of the request for launching this tab. On 2017/05/31 18:28:59, Yusuf wrote: > Does requestId have any context in other contexts? This is a very generic class > and while adding new public calls to this, we should at least have more info on > the params. What is this requestId used? and in what context? Done. This requestId is the same as the request Id in AsyncTabCreationParams. So I copied the comments to here. https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:107: * @param redirectUrl The url of the background tab showing. On 2017/05/31 18:28:59, Yusuf wrote: > I do understand how these params represent your use case, but would be better to > have more generic names here for this class. > > sourceUrl? I named this parameter according to 'redirect_chain' definition in LoadURLParams (https://cs.chromium.org/chromium/src/content/public/browser/navigation_contro...), but looks two of the reviewer preferred 'sourceUrl'. Let wait for service worker reviewer's feedback, if 'sourceUrl' is preferred, I can change the variable name bottom up. https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:124: webcontents.getLastCommittedUrl(), true))) { On 2017/05/31 18:28:59, Yusuf wrote: > how does this differ from getUrl in Tab? Can we not use that? Much more common > to use than webContents in our codebase. The counterpart of getUrl in C++ is deprecated (https://cs.chromium.org/chromium/src/content/public/browser/web_contents.h?rc...) and it returns the same result as getVirtualUrl which returns the url being displayed in the url bar, but the navigation is not necessary complete, so the current webcontent may not correspond to that url. https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:133: customTabsIntent.intent.setData(Uri.parse(url)); On 2017/05/31 18:28:59, Yusuf wrote: > you can use CustomTabsIntent#launchUrl for this at the end. Done. https://codereview.chromium.org/2893823004/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:136: customTabsIntent.intent.putExtra(IntentHandler.EXTRA_OPEN_NEW_INCOGNITO_TAB, mIsIncognito); On 2017/05/31 18:28:59, Yusuf wrote: > do we open incognito tabs in this CL's context? Yes, payment request is allowed in incognito tabs as service worker,
Thanks Yusuf@, addressed your comments, another look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks for the additional context. https://codereview.chromium.org/2893823004/diff/440001/content/browser/servic... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2893823004/diff/440001/content/browser/servic... content/browser/service_worker/service_worker_client_utils.cc:214: params.redirect_chain.emplace_back(redirect_url); Is there a specific reason you must add to the redirect_chain? If I understand correctly, the flow is: 1) Navigate to site A. Do some action that triggers a payment request. 2) Payment request is dispatched to service worker B. The service worker is possibly from a different origin. 3) Service worker B wants to open site C. It calls event.openWindow(url) In this case I think we're creating a new "browsing context" (window) which is essentially a fresh navigation to C. The redirect chain as used in the code base is (AFAIK) about HTTP redirects, which isn't really what's happening here. I'm not sure what OpenURL does with the redirect chain, but it seems safest to just not touch it. Relatedly (it's not from this CL) setting the SW script url to the referrer seems reasonable but I'm not sure it matches the spec. zino@ do you remember this? It was added in https://codereview.chromium.org/1202453002
gogerald@google.com changed reviewers: + gogerald@google.com
https://codereview.chromium.org/2893823004/diff/440001/content/browser/servic... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2893823004/diff/440001/content/browser/servic... content/browser/service_worker/service_worker_client_utils.cc:214: params.redirect_chain.emplace_back(redirect_url); On 2017/06/01 07:24:07, falken wrote: > Is there a specific reason you must add to the redirect_chain? > > If I understand correctly, the flow is: > 1) Navigate to site A. Do some action that triggers a payment request. > 2) Payment request is dispatched to service worker B. The service worker is > possibly from a different origin. > 3) Service worker B wants to open site C. It calls event.openWindow(url) > > In this case I think we're creating a new "browsing context" (window) which is > essentially a fresh navigation to C. The redirect chain as used in the code base > is (AFAIK) about HTTP redirects, which isn't really what's happening here. I'm > not sure what OpenURL does with the redirect chain, but it seems safest to just > not touch it. > > Relatedly (it's not from this CL) setting the SW script url to the referrer > seems reasonable but I'm not sure it matches the spec. zino@ do you remember > this? It was added in https://codereview.chromium.org/1202453002 Your understanding is correct. Didn't know redirect_chain is only for http redirects. Thought our usage is kind of user triggered redirection. If that's the case, then I would like to add a new field "GURL source_url" in OpenURLParams.
On 2017/06/01 07:24:07, falken wrote: > Thanks for the additional context. > > https://codereview.chromium.org/2893823004/diff/440001/content/browser/servic... > File content/browser/service_worker/service_worker_client_utils.cc (right): > > https://codereview.chromium.org/2893823004/diff/440001/content/browser/servic... > content/browser/service_worker/service_worker_client_utils.cc:214: > params.redirect_chain.emplace_back(redirect_url); > Is there a specific reason you must add to the redirect_chain? > > If I understand correctly, the flow is: > 1) Navigate to site A. Do some action that triggers a payment request. > 2) Payment request is dispatched to service worker B. The service worker is > possibly from a different origin. > 3) Service worker B wants to open site C. It calls event.openWindow(url) > > In this case I think we're creating a new "browsing context" (window) which is > essentially a fresh navigation to C. The redirect chain as used in the code base > is (AFAIK) about HTTP redirects, which isn't really what's happening here. I'm > not sure what OpenURL does with the redirect chain, but it seems safest to just > not touch it. > > Relatedly (it's not from this CL) setting the SW script url to the referrer > seems reasonable but I'm not sure it matches the spec. zino@ do you remember > this? It was added in https://codereview.chromium.org/1202453002 The code was added in this CL: https://codereview.chromium.org/843583005 BTW, Is the current implementation (openWindow and navigate) correct? I'm not sure but shouldn't we set the referrer url to empty url instead of script url when calling openWindow()? Also, shouldn't we set the referrer url to previous url instead of script url when calling navigate()? The window.openWindow() and location.href are working like that.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ganggui, thank for continuing to work on this patch. It's very important that we get this done and I'm happy to see the video of this patch in action. It's working as expected. My only concern is with the usage of "redirect_url" throughout the code. I don't believe that comparison of "redirect_url" to the URL of the page in the currently displayed web contents brings us any advantage. On the other hand, using the "redirect_url" to select the PaymentHandler-specific code path seems suboptimal. I would much rather prefer either different function names for PaymentHandler-specific code or an enum that's passed from renderer to the browser. If I'm missing something crucial in your logic, let's meet up and discuss your design. Keep up the good work! https://codereview.chromium.org/2893823004/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java (right): https://codereview.chromium.org/2893823004/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:64: if (disposition == WindowOpenDisposition.NEW_POPUP && !TextUtils.isEmpty(redirectUrl)) { I recommend to use an enum for the reason why the tab is launching. That would be more clear than checking !TextUtils.isEmpty(redirectUrl). Example enum values: final static int LAUNCH_TAB_FOR_PAYMENT_HANDLER = 0; final static int LAUNCH_TAB_FOR_OTHER = 1; https://codereview.chromium.org/2893823004/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java (right): https://codereview.chromium.org/2893823004/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:110: public boolean createPopupCustomTab(int requestId, String redirectUrl, String url) { Let's be clear when this method would be called: in response to PaymentRequestEvent.openWindow() call in the payment handler API. Let's put that in the comment and rename the method to mention "payment" explicitly. Example method name: "openWindowForPaymentHandler()". https://codereview.chromium.org/2893823004/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:123: if (!redirectOrigin.equals(UrlFormatter.formatUrlForSecurityDisplay( I don't think this check is useful, because the user might have two tabs open with the same URL, both of which would pass this check anyway. Since this is the only place where "redirectUrl" is being used, I suggest that we do not pass the "redirectUrl" from the renderer to the browser. https://codereview.chromium.org/2893823004/diff/440001/content/browser/servic... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2893823004/diff/440001/content/browser/servic... content/browser/service_worker/service_worker_client_utils.cc:214: params.redirect_chain.emplace_back(redirect_url); On 2017/06/01 13:47:36, gogerald wrote: > On 2017/06/01 07:24:07, falken wrote: > > Is there a specific reason you must add to the redirect_chain? > > > > If I understand correctly, the flow is: > > 1) Navigate to site A. Do some action that triggers a payment request. > > 2) Payment request is dispatched to service worker B. The service worker is > > possibly from a different origin. > > 3) Service worker B wants to open site C. It calls event.openWindow(url) > > > > In this case I think we're creating a new "browsing context" (window) which is > > essentially a fresh navigation to C. The redirect chain as used in the code > base > > is (AFAIK) about HTTP redirects, which isn't really what's happening here. I'm > > not sure what OpenURL does with the redirect chain, but it seems safest to > just > > not touch it. > > > > Relatedly (it's not from this CL) setting the SW script url to the referrer > > seems reasonable but I'm not sure it matches the spec. zino@ do you remember > > this? It was added in https://codereview.chromium.org/1202453002 > > Your understanding is correct. Didn't know redirect_chain is only for http > redirects. Thought our usage is kind of user triggered redirection. > > If that's the case, then I would like to add a new field "GURL source_url" in > OpenURLParams. > > I would recommend to add "source_type" parameter to the OpenURLParams instead. That would be an enum with two values: CLIENTS_OPEN_WINDOW and PAYMENT_REQUEST_EVENT_OPEN_WINDOW. (The latter could also be named PAYMENT_HANDLER_OPEN_WINDOW, if you prefer.) https://codereview.chromium.org/2893823004/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right): https://codereview.chromium.org/2893823004/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:90: } Which one of these checks prevents a SW from https://a.com to open a window with https://b.com URL? https://codereview.chromium.org/2893823004/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:102: ServiceWorkerGlobalScopeClient::From(context)->OpenWindowForPaymentHandle( OpenWindowForPaymentHandler -- need "r" at the end of the method name.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Patchset #10 (id:480001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
As talked with rouslan@ offline, I simplified the CL by removing URL check. PTAL, https://codereview.chromium.org/2893823004/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java (right): https://codereview.chromium.org/2893823004/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:64: if (disposition == WindowOpenDisposition.NEW_POPUP && !TextUtils.isEmpty(redirectUrl)) { On 2017/06/01 17:50:37, rouslan wrote: > I recommend to use an enum for the reason why the tab is launching. That would > be more clear than checking !TextUtils.isEmpty(redirectUrl). > > Example enum values: > > final static int LAUNCH_TAB_FOR_PAYMENT_HANDLER = 0; > final static int LAUNCH_TAB_FOR_OTHER = 1; Acknowledged. https://codereview.chromium.org/2893823004/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java (right): https://codereview.chromium.org/2893823004/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:110: public boolean createPopupCustomTab(int requestId, String redirectUrl, String url) { On 2017/06/01 17:50:37, rouslan wrote: > Let's be clear when this method would be called: in response to > PaymentRequestEvent.openWindow() call in the payment handler API. Let's put that > in the comment and rename the method to mention "payment" explicitly. Example > method name: "openWindowForPaymentHandler()". Acknowledged. https://codereview.chromium.org/2893823004/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:123: if (!redirectOrigin.equals(UrlFormatter.formatUrlForSecurityDisplay( On 2017/06/01 17:50:37, rouslan wrote: > I don't think this check is useful, because the user might have two tabs open > with the same URL, both of which would pass this check anyway. Since this is the > only place where "redirectUrl" is being used, I suggest that we do not pass the > "redirectUrl" from the renderer to the browser. Done. https://codereview.chromium.org/2893823004/diff/440001/content/browser/servic... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2893823004/diff/440001/content/browser/servic... content/browser/service_worker/service_worker_client_utils.cc:214: params.redirect_chain.emplace_back(redirect_url); On 2017/06/01 17:50:37, rouslan wrote: > On 2017/06/01 13:47:36, gogerald wrote: > > On 2017/06/01 07:24:07, falken wrote: > > > Is there a specific reason you must add to the redirect_chain? > > > > > > If I understand correctly, the flow is: > > > 1) Navigate to site A. Do some action that triggers a payment request. > > > 2) Payment request is dispatched to service worker B. The service worker is > > > possibly from a different origin. > > > 3) Service worker B wants to open site C. It calls event.openWindow(url) > > > > > > In this case I think we're creating a new "browsing context" (window) which > is > > > essentially a fresh navigation to C. The redirect chain as used in the code > > base > > > is (AFAIK) about HTTP redirects, which isn't really what's happening here. > I'm > > > not sure what OpenURL does with the redirect chain, but it seems safest to > > just > > > not touch it. > > > > > > Relatedly (it's not from this CL) setting the SW script url to the referrer > > > seems reasonable but I'm not sure it matches the spec. zino@ do you remember > > > this? It was added in https://codereview.chromium.org/1202453002 > > > > Your understanding is correct. Didn't know redirect_chain is only for http > > redirects. Thought our usage is kind of user triggered redirection. > > > > If that's the case, then I would like to add a new field "GURL source_url" in > > OpenURLParams. > > > > > > I would recommend to add "source_type" parameter to the OpenURLParams instead. > That would be an enum with two values: CLIENTS_OPEN_WINDOW and > PAYMENT_REQUEST_EVENT_OPEN_WINDOW. (The latter could also be named > PAYMENT_HANDLER_OPEN_WINDOW, if you prefer.) Acknowledged. https://codereview.chromium.org/2893823004/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right): https://codereview.chromium.org/2893823004/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:90: } On 2017/06/01 17:50:37, rouslan wrote: > Which one of these checks prevents a SW from https://a.com to open a window with > https://b.com URL? CanAccess https://codereview.chromium.org/2893823004/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:102: ServiceWorkerGlobalScopeClient::From(context)->OpenWindowForPaymentHandle( On 2017/06/01 17:50:37, rouslan wrote: > OpenWindowForPaymentHandler -- need "r" at the end of the method name. Done.
Excellent work! Thank you for following up. LGTM % comments/nits. Please make sure the CL description is up-to-date. https://codereview.chromium.org/2893823004/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java (right): https://codereview.chromium.org/2893823004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:60: // Open popup window in custom tab. Add a comment that this is used by PaymentRequestEvent.openWindow(). https://codereview.chromium.org/2893823004/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java (right): https://codereview.chromium.org/2893823004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:103: * and out from top to bottom. Add a comment that this is used by PaymentRequestEvent.openWindow(). https://codereview.chromium.org/2893823004/diff/500001/content/browser/paymen... File content/browser/payments/payment_app_browsertest.cc (right): https://codereview.chromium.org/2893823004/diff/500001/content/browser/paymen... content/browser/payments/payment_app_browsertest.cc:110: std::string instrument_key) { const-ref? https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:931: IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_OpenWindowForPaymentHandler, Excellent! https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... content/browser/service_worker/service_worker_version.h:590: void OnOpenWindow(int request_id, Make this private, please. https://codereview.chromium.org/2893823004/diff/500001/content/common/service... File content/common/service_worker/service_worker_messages.h (right): https://codereview.chromium.org/2893823004/diff/500001/content/common/service... content/common/service_worker/service_worker_messages.h:274: // Ask the browser to open a tab/window (renderer->browser) for clients. ... for clients.openWindow(). https://codereview.chromium.org/2893823004/diff/500001/content/common/service... content/common/service_worker/service_worker_messages.h:279: // Ask the browser to open a tab/window (renderer->browser) for payment handler. ... for payment handler, i.e., PaymentRequestEvent.openWindow().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
service worker lgtm. thanks for the changes. "BTW, Is the current implementation (openWindow and navigate) correct? I'm not sure but shouldn't we set the referrer url to empty url instead of script url when calling openWindow()? Also, shouldn't we set the referrer url to previous url instead of script url when calling navigate()? The window.openWindow() and location.href are working like that." Yes, I couldn't see anything in the spec about setting the referrer to the script url. Not sure. https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... content/browser/service_worker/service_worker_version.h:587: // For Clients.openWindow() https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... content/browser/service_worker/service_worker_version.h:588: void OnOpenWindowForClients(int request_id, GURL url); // For PaymentRequestEvent.openWindow() https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... content/browser/service_worker/service_worker_version.h:590: void OnOpenWindow(int request_id, On 2017/06/02 03:43:17, rouslan wrote: > Make this private, please. +1
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. The spec: https://github.com/w3c/payment-handler. The intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2ojnMk_T9_c/h7... 2, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the activity showing merchant website so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the tab showing merchant website is not active. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027,661608 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. The spec: https://github.com/w3c/payment-handler. The intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2ojnMk_T9_c/h7... 2, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the foreground ChromeActivity so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the foreground activity is not ChromeActivity. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027,661608 ==========
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. The spec: https://github.com/w3c/payment-handler. The intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2ojnMk_T9_c/h7... 2, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the foreground ChromeActivity so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the foreground activity is not ChromeActivity. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027,661608 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. The spec: https://github.com/w3c/payment-handler. The intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2ojnMk_T9_c/h7... 2, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the foreground ChromeActivity so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the foreground activity is not ChromeActivity. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027,661608 ==========
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gogerald@chromium.org changed reviewers: + dcheng@chromium.org
Thanks for reviewing, dcheng@, ptal of the changes in content/common/service_worker/service_worker_messages.h. yusufo@, ptal of the changes in java. https://codereview.chromium.org/2893823004/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java (right): https://codereview.chromium.org/2893823004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:60: // Open popup window in custom tab. On 2017/06/02 03:43:16, rouslan wrote: > Add a comment that this is used by PaymentRequestEvent.openWindow(). Done. https://codereview.chromium.org/2893823004/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java (right): https://codereview.chromium.org/2893823004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:103: * and out from top to bottom. On 2017/06/02 03:43:16, rouslan wrote: > Add a comment that this is used by PaymentRequestEvent.openWindow(). Done. https://codereview.chromium.org/2893823004/diff/500001/content/browser/paymen... File content/browser/payments/payment_app_browsertest.cc (right): https://codereview.chromium.org/2893823004/diff/500001/content/browser/paymen... content/browser/payments/payment_app_browsertest.cc:110: std::string instrument_key) { On 2017/06/02 03:43:16, rouslan wrote: > const-ref? Done. https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:931: IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_OpenWindowForPaymentHandler, On 2017/06/02 03:43:17, rouslan wrote: > Excellent! Acknowledged. https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... content/browser/service_worker/service_worker_version.h:587: On 2017/06/02 07:58:19, falken wrote: > // For Clients.openWindow() Done. https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... content/browser/service_worker/service_worker_version.h:588: void OnOpenWindowForClients(int request_id, GURL url); On 2017/06/02 07:58:19, falken wrote: > // For PaymentRequestEvent.openWindow() Done. https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... content/browser/service_worker/service_worker_version.h:590: void OnOpenWindow(int request_id, On 2017/06/02 07:58:19, falken wrote: > On 2017/06/02 03:43:17, rouslan wrote: > > Make this private, please. > > +1 These interfaces are already in private scope, https://codereview.chromium.org/2893823004/diff/500001/content/browser/servic... content/browser/service_worker/service_worker_version.h:590: void OnOpenWindow(int request_id, On 2017/06/02 07:58:19, falken wrote: > On 2017/06/02 03:43:17, rouslan wrote: > > Make this private, please. > > +1 Done. https://codereview.chromium.org/2893823004/diff/500001/content/common/service... File content/common/service_worker/service_worker_messages.h (right): https://codereview.chromium.org/2893823004/diff/500001/content/common/service... content/common/service_worker/service_worker_messages.h:274: // Ask the browser to open a tab/window (renderer->browser) for clients. On 2017/06/02 03:43:17, rouslan wrote: > ... for clients.openWindow(). Done. https://codereview.chromium.org/2893823004/diff/500001/content/common/service... content/common/service_worker/service_worker_messages.h:279: // Ask the browser to open a tab/window (renderer->browser) for payment handler. On 2017/06/02 03:43:17, rouslan wrote: > ... for payment handler, i.e., PaymentRequestEvent.openWindow(). Done.
https://codereview.chromium.org/2893823004/diff/520001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2893823004/diff/520001/content/browser/servic... content/browser/service_worker/service_worker_version.h:592: void OnOpenWindowForPaymentHandler(int request_id, GURL url); add blank line here to make the comment scope more clear
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2893823004/diff/520001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/2893823004/diff/520001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:1055: // Open popup window for payment handler. nit: that comment really doesn't add anything that isn't already obvious from the code; it's pretty much just repeating the name of the method https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right): https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:63: ScriptPromise PaymentRequestEvent::openWindow(ScriptState* script_state, The logic in this method doesn't seem to match what the spec defines in https://w3c.github.io/payment-handler/#dfn-open-window-algorithm Not sure if that is because the spec is wrong or because the implementation is wrong, but some differences: - In the spec, the returned promise is rejected with InvalidStateError if the payment request is not in the "interactive" state, before any origin or other security checks are done on the URL. I don't see any state check in this code? - The various security checks you do here (CanAccess, CanDisplay) don't seem to match what the spec is actually describing. - The spec doesn't have a "triggered by user activation" check, but you seem to have a check like that in the implementation. https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:69: KURL parsed_url_to_open = Isn't this just context->CompleteURL(url) ?
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2893823004/diff/520001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/2893823004/diff/520001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:1055: // Open popup window for payment handler. On 2017/06/02 18:19:02, Marijn Kruisselbrink wrote: > nit: that comment really doesn't add anything that isn't already obvious from > the code; it's pretty much just repeating the name of the method Done. https://codereview.chromium.org/2893823004/diff/520001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2893823004/diff/520001/content/browser/servic... content/browser/service_worker/service_worker_version.h:592: void OnOpenWindowForPaymentHandler(int request_id, GURL url); On 2017/06/02 13:25:38, falken wrote: > add blank line here to make the comment scope more clear Done. https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right): https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:63: ScriptPromise PaymentRequestEvent::openWindow(ScriptState* script_state, On 2017/06/02 18:19:02, Marijn Kruisselbrink wrote: > The logic in this method doesn't seem to match what the spec defines in > https://w3c.github.io/payment-handler/#dfn-open-window-algorithm > > Not sure if that is because the spec is wrong or because the implementation is > wrong, but some differences: > - In the spec, the returned promise is rejected with InvalidStateError if the > payment request is not in the "interactive" state, before any origin or other > security checks are done on the URL. I don't see any state check in this code? > > - The various security checks you do here (CanAccess, CanDisplay) don't seem to > match what the spec is actually describing. > > - The spec doesn't have a "triggered by user activation" check, but you seem to > have a check like that in the implementation. Add a TODO for checking payment request state. CanAccess is used to check whether the url is from the same origin. CanDisplay is not in the spec, but I think it is the right thing to do as service worker. Note that spec is not fixed and completed in details. Check IsWindowInteractionAllowed is used to limit one window per payment request which is part of the spec, https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic.... https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:69: KURL parsed_url_to_open = On 2017/06/02 18:19:02, Marijn Kruisselbrink wrote: > Isn't this just context->CompleteURL(url) ? The url could be relative url
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #13 (id:560001) has been deleted
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Used ExecutionContext::CompleteURL in PaymentRequestEvent::openWindow for simplicity https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right): https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:69: KURL parsed_url_to_open = On 2017/06/02 18:19:02, Marijn Kruisselbrink wrote: > Isn't this just context->CompleteURL(url) ? Done.
one last comment, once we resolve this, we should be good to go. https://codereview.chromium.org/2893823004/diff/580001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java (right): https://codereview.chromium.org/2893823004/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:109: public boolean createPopupCustomTab(int requestId, String url) { I still kind of get stuck to the fact that, this is not really a generic call, but Service Worker specific (which is fine, but it is out of place here). Looking at what it does, I only see mIncognito as a class level field, can't we pass that as param and have this be static and move it to ServiceTabLauncher?
https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right): https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:63: ScriptPromise PaymentRequestEvent::openWindow(ScriptState* script_state, On 2017/06/02 at 19:33:59, gogerald1 wrote: > Add a TODO for checking payment request state. > > CanAccess is used to check whether the url is from the same origin. CanDisplay is not in the spec, but I think it is the right thing to do as service worker. Note that spec is not fixed and completed in details. CanAccess is not exactly the same as checking for same origin though. If you want a same origin check just call IsSameSchemeHostPort (or IsSameSchemeHostPortAndSuborigin). CanAccess for example takes document.domain into account (and yes, for service workers there isn't such a thing, so in this particular case it might be okay, but you're still calling a method with different semantics than what you're looking for). I'm not sure what you mean with "CanDisplay [..] is the right thing to do as service worker"? If it adds any meaningful checks more than "is same origin", those checks should be in the spec. If it doesn't add anything, than what's the point of calling it? Looking at the spec again I don't even see a same-origin requirement for the URL passed to openWindow. The only check the spec seems to have is for the origin of the eventually navigated document (which with redirections might be different from the origin of the URL passed in). But yeah, as you (and the spec) point out that entire algorithm in the spec is pretty terrible and impossibly underdefined. > Check IsWindowInteractionAllowed is used to limit one window per payment request which is part of the spec, https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic.... It doesn't seem to be part of the spec though. The ServiceWorker Clients.openWindow algorithm has a "is triggered by user activation" check, but the modified copy of this algorithm that exists in the payment-handler spec has no such check. I don't know if that's intentional, but you should somehow make sure that the spec and implementation are aligned. https://codereview.chromium.org/2893823004/diff/580001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right): https://codereview.chromium.org/2893823004/diff/580001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:69: // TODO(gogerad): Check payment request state so as to through nit: s/through/throw/ (although you're not technically throwing, but rejecting a promise)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right): https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:63: ScriptPromise PaymentRequestEvent::openWindow(ScriptState* script_state, On 2017/06/02 20:49:38, Marijn Kruisselbrink wrote: > On 2017/06/02 at 19:33:59, gogerald1 wrote: > > Add a TODO for checking payment request state. > > > > CanAccess is used to check whether the url is from the same origin. CanDisplay > is not in the spec, but I think it is the right thing to do as service worker. > Note that spec is not fixed and completed in details. > CanAccess is not exactly the same as checking for same origin though. If you > want a same origin check just call IsSameSchemeHostPort (or > IsSameSchemeHostPortAndSuborigin). CanAccess for example takes document.domain > into account (and yes, for service workers there isn't such a thing, so in this > particular case it might be okay, but you're still calling a method with > different semantics than what you're looking for). > > I'm not sure what you mean with "CanDisplay [..] is the right thing to do as > service worker"? If it adds any meaningful checks more than "is same origin", > those checks should be in the spec. If it doesn't add anything, than what's the > point of calling it? > > Looking at the spec again I don't even see a same-origin requirement for the URL > passed to openWindow. The only check the spec seems to have is for the origin of > the eventually navigated document (which with redirections might be different > from the origin of the URL passed in). But yeah, as you (and the spec) point out > that entire algorithm in the spec is pretty terrible and impossibly > underdefined. > > > Check IsWindowInteractionAllowed is used to limit one window per payment > request which is part of the spec, > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic.... > > It doesn't seem to be part of the spec though. The ServiceWorker > Clients.openWindow algorithm has a "is triggered by user activation" check, but > the modified copy of this algorithm that exists in the payment-handler spec has > no such check. I don't know if that's intentional, but you should somehow make > sure that the spec and implementation are aligned. CanDisplay From the comments "For example, web sites generally cannot display content from the user's files system". I think this is also the case for PaymentRequestEvent.openWindow. Clients.openWindow also did this check, not sure whether it is explicitly written in the spec though. Anyway, I removed it for now. same-origin requirement Refer zino@'s first comment in this CL, one window check You can find there is a text note above the openWindow algorithm in the spec. Note that this feature is still behind flag, we may update it in the future https://codereview.chromium.org/2893823004/diff/580001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java (right): https://codereview.chromium.org/2893823004/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java:109: public boolean createPopupCustomTab(int requestId, String url) { On 2017/06/02 20:12:54, Yusuf wrote: > I still kind of get stuck to the fact that, this is not really a generic call, > but Service Worker specific (which is fine, but it is out of place here). > > Looking at what it does, I only see mIncognito as a class level field, can't we > pass that as param and have this be static and move it to ServiceTabLauncher? Done. https://codereview.chromium.org/2893823004/diff/580001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right): https://codereview.chromium.org/2893823004/diff/580001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:69: // TODO(gogerad): Check payment request state so as to through On 2017/06/02 20:49:39, Marijn Kruisselbrink wrote: > nit: s/through/throw/ (although you're not technically throwing, but rejecting a > promise) Done.
custom tabs/java lgtm
https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right): https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:63: ScriptPromise PaymentRequestEvent::openWindow(ScriptState* script_state, On 2017/06/02 at 21:46:03, gogerald1 wrote: > On 2017/06/02 20:49:38, Marijn Kruisselbrink wrote: > > On 2017/06/02 at 19:33:59, gogerald1 wrote: > > > Add a TODO for checking payment request state. > > > > > > CanAccess is used to check whether the url is from the same origin. CanDisplay > > is not in the spec, but I think it is the right thing to do as service worker. > > Note that spec is not fixed and completed in details. > > CanAccess is not exactly the same as checking for same origin though. If you > > want a same origin check just call IsSameSchemeHostPort (or > > IsSameSchemeHostPortAndSuborigin). CanAccess for example takes document.domain > > into account (and yes, for service workers there isn't such a thing, so in this > > particular case it might be okay, but you're still calling a method with > > different semantics than what you're looking for). > > > > I'm not sure what you mean with "CanDisplay [..] is the right thing to do as > > service worker"? If it adds any meaningful checks more than "is same origin", > > those checks should be in the spec. If it doesn't add anything, than what's the > > point of calling it? > > > > Looking at the spec again I don't even see a same-origin requirement for the URL > > passed to openWindow. The only check the spec seems to have is for the origin of > > the eventually navigated document (which with redirections might be different > > from the origin of the URL passed in). But yeah, as you (and the spec) point out > > that entire algorithm in the spec is pretty terrible and impossibly > > underdefined. > > > > > Check IsWindowInteractionAllowed is used to limit one window per payment > > request which is part of the spec, > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic.... > > > > It doesn't seem to be part of the spec though. The ServiceWorker > > Clients.openWindow algorithm has a "is triggered by user activation" check, but > > the modified copy of this algorithm that exists in the payment-handler spec has > > no such check. I don't know if that's intentional, but you should somehow make > > sure that the spec and implementation are aligned. > > CanDisplay > From the comments "For example, web sites generally cannot display content from the user's files system". I think this is also the case for PaymentRequestEvent.openWindow. Clients.openWindow also did this check, not sure whether it is explicitly written in the spec though. Anyway, I removed it for now. Well, it depends... Clients.openWindow doesn't have a same-origin requirement for the URL being passed in (as far as I can tell), so the CanDisplay check there makes sense (but good question, not sure if it's entirely spec compliant). Assuming PaymentRequestEvent.openWindow actually does have a same-origin requirement, doing the CanDisplay check in addition is pretty pointless as it'll always return true for a same origin URL. So this comes down to the question if the spec should require the URL to be same origin or not (currently the spec doesn't require it, but your code does). > same-origin requirement > Refer zino@'s first comment in this CL, I don't see any comments by any zino@ account on this CL? But assuming you're referring to the comment referring to step 8.4 in the open window algorithm, that step seems to check the origin of the document being loaded as a result of the navigation (after redirects etc), not the origin of the URL being passed in to openWindow. So no, currently the spec does not seem to require a same origin URL, it merely requires a URL that eventually ends up with a same origin document. I don't know which is intended, but imho there should be either a spec bug or a chromium bug (or at the very least a TODO) for any place where the spec and the implementation disagree. > one window check > You can find there is a text note above the openWindow algorithm in the spec. Hmm, seemingly normative text in a non-normative note in the spec... That either shouldn't be a non-normative note, or the text shouldn't be using RFC2119 should/should not language. Either way, if that is supposed to be normative, I'm not sure if the check for IsWindowInteractionAllowed is the right check. For example should calling `new PaymentRequestEvent().openWindow(...)` in response to a notification click event or something like that (which sets the window interaction is allowed flag) be allowed? Or what about the reverse: should a PaymentRequestEvent handler be allowed to call Clients.openWindow? And if it does call Clients.openWindow, should it still be allowed to also call this openWindow method? Possibly figuring out these details is more spec work than you're willing to block on, but you should at least file a spec bug to more normatively specify what that non-normative not is trying to say, and link to the spec bug with a comment in the code here, as currently it's far from obvious that the behavior as implemented is the behavior the spec is trying to describe. > > Note that this feature is still behind flag, we may update it in the future Understood. But where there are differences between spec and implementation I'd still expect spec issues and/or chromium issues with code comments to make it clear which parts will need to be revised in the future.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi mek@, addressed your comments, please take another look, https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right): https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:63: ScriptPromise PaymentRequestEvent::openWindow(ScriptState* script_state, On 2017/06/02 22:18:05, Marijn Kruisselbrink wrote: > On 2017/06/02 at 21:46:03, gogerald1 wrote: > > On 2017/06/02 20:49:38, Marijn Kruisselbrink wrote: > > > On 2017/06/02 at 19:33:59, gogerald1 wrote: > > > > Add a TODO for checking payment request state. > > > > > > > > CanAccess is used to check whether the url is from the same origin. > CanDisplay > > > is not in the spec, but I think it is the right thing to do as service > worker. > > > Note that spec is not fixed and completed in details. > > > CanAccess is not exactly the same as checking for same origin though. If you > > > want a same origin check just call IsSameSchemeHostPort (or > > > IsSameSchemeHostPortAndSuborigin). CanAccess for example takes > document.domain > > > into account (and yes, for service workers there isn't such a thing, so in > this > > > particular case it might be okay, but you're still calling a method with > > > different semantics than what you're looking for). > > > > > > I'm not sure what you mean with "CanDisplay [..] is the right thing to do as > > > service worker"? If it adds any meaningful checks more than "is same > origin", > > > those checks should be in the spec. If it doesn't add anything, than what's > the > > > point of calling it? > > > > > > Looking at the spec again I don't even see a same-origin requirement for the > URL > > > passed to openWindow. The only check the spec seems to have is for the > origin of > > > the eventually navigated document (which with redirections might be > different > > > from the origin of the URL passed in). But yeah, as you (and the spec) point > out > > > that entire algorithm in the spec is pretty terrible and impossibly > > > underdefined. > > > > > > > Check IsWindowInteractionAllowed is used to limit one window per payment > > > request which is part of the spec, > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic.... > > > > > > It doesn't seem to be part of the spec though. The ServiceWorker > > > Clients.openWindow algorithm has a "is triggered by user activation" check, > but > > > the modified copy of this algorithm that exists in the payment-handler spec > has > > > no such check. I don't know if that's intentional, but you should somehow > make > > > sure that the spec and implementation are aligned. > > > > CanDisplay > > From the comments "For example, web sites generally cannot display content > from the user's files system". I think this is also the case for > PaymentRequestEvent.openWindow. Clients.openWindow also did this check, not sure > whether it is explicitly written in the spec though. Anyway, I removed it for > now. > > Well, it depends... Clients.openWindow doesn't have a same-origin requirement > for the URL being passed in (as far as I can tell), so the CanDisplay check > there makes sense (but good question, not sure if it's entirely spec compliant). > Assuming PaymentRequestEvent.openWindow actually does have a same-origin > requirement, doing the CanDisplay check in addition is pretty pointless as it'll > always return true for a same origin URL. So this comes down to the question if > the spec should require the URL to be same origin or not (currently the spec > doesn't require it, but your code does). > > > same-origin requirement > > Refer zino@'s first comment in this CL, > I don't see any comments by any zino@ account on this CL? But assuming you're > referring to the comment referring to step 8.4 in the open window algorithm, > that step seems to check the origin of the document being loaded as a result of > the navigation (after redirects etc), not the origin of the URL being passed in > to openWindow. So no, currently the spec does not seem to require a same origin > URL, it merely requires a URL that eventually ends up with a same origin > document. I don't know which is intended, but imho there should be either a spec > bug or a chromium bug (or at the very least a TODO) for any place where the spec > and the implementation disagree. > > > one window check > > You can find there is a text note above the openWindow algorithm in the spec. > > Hmm, seemingly normative text in a non-normative note in the spec... That either > shouldn't be a non-normative note, or the text shouldn't be using RFC2119 > should/should not language. Either way, if that is supposed to be normative, I'm > not sure if the check for IsWindowInteractionAllowed is the right check. For > example should calling `new PaymentRequestEvent().openWindow(...)` in response > to a notification click event or something like that (which sets the window > interaction is allowed flag) be allowed? Or what about the reverse: should a > PaymentRequestEvent handler be allowed to call Clients.openWindow? And if it > does call Clients.openWindow, should it still be allowed to also call this > openWindow method? > > Possibly figuring out these details is more spec work than you're willing to > block on, but you should at least file a spec bug to more normatively specify > what that non-normative not is trying to say, and link to the spec bug with a > comment in the code here, as currently it's far from obvious that the behavior > as implemented is the behavior the spec is trying to describe. > > > > > Note that this feature is still behind flag, we may update it in the future > > Understood. But where there are differences between spec and implementation I'd > still expect spec issues and/or chromium issues with code comments to make it > clear which parts will need to be revised in the future. Done. https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:63: ScriptPromise PaymentRequestEvent::openWindow(ScriptState* script_state, On 2017/06/02 22:18:05, Marijn Kruisselbrink wrote: > On 2017/06/02 at 21:46:03, gogerald1 wrote: > > On 2017/06/02 20:49:38, Marijn Kruisselbrink wrote: > > > On 2017/06/02 at 19:33:59, gogerald1 wrote: > > > > Add a TODO for checking payment request state. > > > > > > > > CanAccess is used to check whether the url is from the same origin. > CanDisplay > > > is not in the spec, but I think it is the right thing to do as service > worker. > > > Note that spec is not fixed and completed in details. > > > CanAccess is not exactly the same as checking for same origin though. If you > > > want a same origin check just call IsSameSchemeHostPort (or > > > IsSameSchemeHostPortAndSuborigin). CanAccess for example takes > document.domain > > > into account (and yes, for service workers there isn't such a thing, so in > this > > > particular case it might be okay, but you're still calling a method with > > > different semantics than what you're looking for). > > > > > > I'm not sure what you mean with "CanDisplay [..] is the right thing to do as > > > service worker"? If it adds any meaningful checks more than "is same > origin", > > > those checks should be in the spec. If it doesn't add anything, than what's > the > > > point of calling it? > > > > > > Looking at the spec again I don't even see a same-origin requirement for the > URL > > > passed to openWindow. The only check the spec seems to have is for the > origin of > > > the eventually navigated document (which with redirections might be > different > > > from the origin of the URL passed in). But yeah, as you (and the spec) point > out > > > that entire algorithm in the spec is pretty terrible and impossibly > > > underdefined. > > > > > > > Check IsWindowInteractionAllowed is used to limit one window per payment > > > request which is part of the spec, > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic.... > > > > > > It doesn't seem to be part of the spec though. The ServiceWorker > > > Clients.openWindow algorithm has a "is triggered by user activation" check, > but > > > the modified copy of this algorithm that exists in the payment-handler spec > has > > > no such check. I don't know if that's intentional, but you should somehow > make > > > sure that the spec and implementation are aligned. > > > > CanDisplay > > From the comments "For example, web sites generally cannot display content > from the user's files system". I think this is also the case for > PaymentRequestEvent.openWindow. Clients.openWindow also did this check, not sure > whether it is explicitly written in the spec though. Anyway, I removed it for > now. > > Well, it depends... Clients.openWindow doesn't have a same-origin requirement > for the URL being passed in (as far as I can tell), so the CanDisplay check > there makes sense (but good question, not sure if it's entirely spec compliant). > Assuming PaymentRequestEvent.openWindow actually does have a same-origin > requirement, doing the CanDisplay check in addition is pretty pointless as it'll > always return true for a same origin URL. So this comes down to the question if > the spec should require the URL to be same origin or not (currently the spec > doesn't require it, but your code does). > > > same-origin requirement > > Refer zino@'s first comment in this CL, > I don't see any comments by any zino@ account on this CL? But assuming you're > referring to the comment referring to step 8.4 in the open window algorithm, > that step seems to check the origin of the document being loaded as a result of > the navigation (after redirects etc), not the origin of the URL being passed in > to openWindow. So no, currently the spec does not seem to require a same origin > URL, it merely requires a URL that eventually ends up with a same origin > document. I don't know which is intended, but imho there should be either a spec > bug or a chromium bug (or at the very least a TODO) for any place where the spec > and the implementation disagree. > > > one window check > > You can find there is a text note above the openWindow algorithm in the spec. > > Hmm, seemingly normative text in a non-normative note in the spec... That either > shouldn't be a non-normative note, or the text shouldn't be using RFC2119 > should/should not language. Either way, if that is supposed to be normative, I'm > not sure if the check for IsWindowInteractionAllowed is the right check. For > example should calling `new PaymentRequestEvent().openWindow(...)` in response > to a notification click event or something like that (which sets the window > interaction is allowed flag) be allowed? Or what about the reverse: should a > PaymentRequestEvent handler be allowed to call Clients.openWindow? And if it > does call Clients.openWindow, should it still be allowed to also call this > openWindow method? > > Possibly figuring out these details is more spec work than you're willing to > block on, but you should at least file a spec bug to more normatively specify > what that non-normative not is trying to say, and link to the spec bug with a > comment in the code here, as currently it's far from obvious that the behavior > as implemented is the behavior the spec is trying to describe. > > > > > Note that this feature is still behind flag, we may update it in the future > > Understood. But where there are differences between spec and implementation I'd > still expect spec issues and/or chromium issues with code comments to make it > clear which parts will need to be revised in the future. Filed two bugs against the spec and added two todos in code for them, The IsWindowInteractionAllowed should work since we already set allowed flag when dispatching payment request event here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with comments addressed https://codereview.chromium.org/2893823004/diff/640001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2893823004/diff/640001/content/browser/servic... content/browser/service_worker/service_worker_version.h:592: void OnOpenWindowForPaymentHandler(int request_id, GURL url); Please pass GURL by const ref (same for above and below) https://codereview.chromium.org/2893823004/diff/640001/content/common/service... File content/common/service_worker/service_worker_messages.h (right): https://codereview.chromium.org/2893823004/diff/640001/content/common/service... content/common/service_worker/service_worker_messages.h:282: IPC_MESSAGE_ROUTED2(ServiceWorkerHostMsg_OpenWindowForPaymentHandler, Nit: I think it would be clearer if the messages just encoded what they did, rather than what they're used for. e.g. ServiceWorkerHostMsg_OpenNewTab ServiceWorkerHostMsg_OpenNewPopup
thanks, lgtm. Just make sure that before actually shipping any of that all the possible inconsistencies between spec and implementation get resolved. https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right): https://codereview.chromium.org/2893823004/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:63: ScriptPromise PaymentRequestEvent::openWindow(ScriptState* script_state, On 2017/06/05 at 13:49:46, gogerald1 wrote: > The IsWindowInteractionAllowed should work since we already set allowed flag when dispatching payment request event here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic... Yes, if the desired behavior is that during a payment request event you can call both Clients.openWindow and event.openWindow, but only one call in total to either of those methods, then things "work". The question is if that is indeed what is the desired behavior. Hopefully formalizing the spec better will help answer that question (to be fair the whole area around user gestures and is window interaction allowed like behavior isn't particularly rigorously specified in general, as this is an area where browser historically don't all agree).
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, Definitely, we will check spec compliance before shipping it. https://codereview.chromium.org/2893823004/diff/640001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2893823004/diff/640001/content/browser/servic... content/browser/service_worker/service_worker_version.h:592: void OnOpenWindowForPaymentHandler(int request_id, GURL url); On 2017/06/05 21:04:46, dcheng wrote: > Please pass GURL by const ref (same for above and below) Done. Kept below OnOpenWindow since the url is canonicalized inside the interface. https://codereview.chromium.org/2893823004/diff/640001/content/common/service... File content/common/service_worker/service_worker_messages.h (right): https://codereview.chromium.org/2893823004/diff/640001/content/common/service... content/common/service_worker/service_worker_messages.h:282: IPC_MESSAGE_ROUTED2(ServiceWorkerHostMsg_OpenWindowForPaymentHandler, On 2017/06/05 21:04:47, dcheng wrote: > Nit: I think it would be clearer if the messages just encoded what they did, > rather than what they're used for. > > e.g. > > ServiceWorkerHostMsg_OpenNewTab > ServiceWorkerHostMsg_OpenNewPopup The sender and receiver of these messages both specified usage in their name, and these messages are only been used in service worker, it looks a little easier to read with usage in name, so I'd like to keep them if you don't mind.
On 2017/06/05 22:26:44, gogerald1 wrote: > Thanks, > > Definitely, we will check spec compliance before shipping it. > > https://codereview.chromium.org/2893823004/diff/640001/content/browser/servic... > File content/browser/service_worker/service_worker_version.h (right): > > https://codereview.chromium.org/2893823004/diff/640001/content/browser/servic... > content/browser/service_worker/service_worker_version.h:592: void > OnOpenWindowForPaymentHandler(int request_id, GURL url); > On 2017/06/05 21:04:46, dcheng wrote: > > Please pass GURL by const ref (same for above and below) > > Done. > > Kept below OnOpenWindow since the url is canonicalized inside the interface. > > https://codereview.chromium.org/2893823004/diff/640001/content/common/service... > File content/common/service_worker/service_worker_messages.h (right): > > https://codereview.chromium.org/2893823004/diff/640001/content/common/service... > content/common/service_worker/service_worker_messages.h:282: > IPC_MESSAGE_ROUTED2(ServiceWorkerHostMsg_OpenWindowForPaymentHandler, > On 2017/06/05 21:04:47, dcheng wrote: > > Nit: I think it would be clearer if the messages just encoded what they did, > > rather than what they're used for. > > > > e.g. > > > > ServiceWorkerHostMsg_OpenNewTab > > ServiceWorkerHostMsg_OpenNewPopup > > The sender and receiver of these messages both specified usage in their name, > and these messages are only been used in service worker, it looks a little > easier to read with usage in name, so I'd like to keep them if you don't mind. ServiceTabLauncher.java - lgtm
https://codereview.chromium.org/2893823004/diff/640001/content/common/service... File content/common/service_worker/service_worker_messages.h (right): https://codereview.chromium.org/2893823004/diff/640001/content/common/service... content/common/service_worker/service_worker_messages.h:282: IPC_MESSAGE_ROUTED2(ServiceWorkerHostMsg_OpenWindowForPaymentHandler, On 2017/06/05 22:26:44, gogerald1 wrote: > On 2017/06/05 21:04:47, dcheng wrote: > > Nit: I think it would be clearer if the messages just encoded what they did, > > rather than what they're used for. > > > > e.g. > > > > ServiceWorkerHostMsg_OpenNewTab > > ServiceWorkerHostMsg_OpenNewPopup > > The sender and receiver of these messages both specified usage in their name, > and these messages are only been used in service worker, it looks a little > easier to read with usage in name, so I'd like to keep them if you don't mind. There's nothing payments specific about this though. The only distinction here is the window open disposition--without that in the name, I have to go look at the IPC handler to figure that nothing particularly interesting is happening. It also makes the sending side clearer--it's immediately obvious what kind of window is being opened, etc. In fact, I would say that the new methods in content/renderer should be named to reflect the IPC messages (and their actual result). I'm pretty sure there's a style tip for this somewhere as well, but I can't find it atm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #18 (id:680001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Okay, I updated the message and listener names, https://codereview.chromium.org/2893823004/diff/640001/content/common/service... File content/common/service_worker/service_worker_messages.h (right): https://codereview.chromium.org/2893823004/diff/640001/content/common/service... content/common/service_worker/service_worker_messages.h:282: IPC_MESSAGE_ROUTED2(ServiceWorkerHostMsg_OpenWindowForPaymentHandler, On 2017/06/06 00:25:48, dcheng wrote: > On 2017/06/05 22:26:44, gogerald1 wrote: > > On 2017/06/05 21:04:47, dcheng wrote: > > > Nit: I think it would be clearer if the messages just encoded what they did, > > > rather than what they're used for. > > > > > > e.g. > > > > > > ServiceWorkerHostMsg_OpenNewTab > > > ServiceWorkerHostMsg_OpenNewPopup > > > > The sender and receiver of these messages both specified usage in their name, > > and these messages are only been used in service worker, it looks a little > > easier to read with usage in name, so I'd like to keep them if you don't mind. > > There's nothing payments specific about this though. The only distinction here > is the window open disposition--without that in the name, I have to go look at > the IPC handler to figure that nothing particularly interesting is happening. > > It also makes the sending side clearer--it's immediately obvious what kind of > window is being opened, etc. > > In fact, I would say that the new methods in content/renderer should be named to > reflect the IPC messages (and their actual result). I'm pretty sure there's a > style tip for this somewhere as well, but I can't find it atm. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gogerald@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, jinho.bang@samsung.com, rouslan@chromium.org, falken@chromium.org, yusufo@chromium.org, dcheng@chromium.org, mek@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2893823004/#ps700001 (title: "rename ipc messages")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 700001, "attempt_start_ts": 1496719929832650,
"parent_rev": "3b24c4dbe118451b3e3740ae7176c56eef87f0a0", "commit_rev":
"1a4b4420868253582467b228f5df4bf167b3ecff"}
Message was sent while issue was closed.
Description was changed from ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. The spec: https://github.com/w3c/payment-handler. The intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2ojnMk_T9_c/h7... 2, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the foreground ChromeActivity so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the foreground activity is not ChromeActivity. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027,661608 ========== to ========== Implement openWindow for web based payment handler 1, This CL implements openWindow in PaymentRequestEvent.idl for service worker based payment handler. The spec: https://github.com/w3c/payment-handler. The intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2ojnMk_T9_c/h7... 2, This feature is behind the chrome://flags/#enable-experimental-web-platform-features and chrome://flags/#service_worker_payment_apps flags 3, openWindow opens the url in custom tab with sliding up and down animation. 4, The custom tab is opened within the foreground ChromeActivity so as to not list in recent apps (Intent.FLAG_ACTIVITY_NEW_TASK is not set). 5, Do not open the url if the foreground activity is not ChromeActivity. 6, Test video record https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNVUtXbmx5cE1XWndyLWh... BUG=720027,661608 Review-Url: https://codereview.chromium.org/2893823004 Cr-Commit-Position: refs/heads/master@{#477180} Committed: https://chromium.googlesource.com/chromium/src/+/1a4b4420868253582467b228f5df... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:700001) as https://chromium.googlesource.com/chromium/src/+/1a4b4420868253582467b228f5df...
Message was sent while issue was closed.
https://codereview.chromium.org/2893823004/diff/700001/third_party/WebKit/pub... File third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right): https://codereview.chromium.org/2893823004/diff/700001/third_party/WebKit/pub... third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:280: virtual void OpenWindowForPaymentHandler( Btw, my point was that we should use a consistent name for this throughout, including in the Blink public API. Do you mind fixing this in a followup? |
