Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(22)

Issue 2829943002: Redirecting off-origin navigations in PWAs to CCT. (Closed)

Created:
3 years, 8 months ago by piotrs
Modified:
3 years, 6 months ago
Reviewers:
dominickn, Ted C, Yusuf, pkotwicz
CC:
chromium-reviews, dominickn+watch_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org, agrieve+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Redirects off-origin navigations in an installed PWAs to a CustomTab. This is done in order to improve UX by getting rid of a "minibar" toolbar (to be done in a follow-up patch) and bring more app-like feel to PWAs. In a long run this will help bring implementation of Minimal-UI, which requires replacing "minibar" with a new "minimal-ui" toolbar. BUG=709889 Review-Url: https://codereview.chromium.org/2829943002 Cr-Original-Original-Commit-Position: refs/heads/master@{#474625} Committed: https://chromium.googlesource.com/chromium/src/+/7c7e9e600d256af4d2af1dbf68b59b030be0ab25 Review-Url: https://codereview.chromium.org/2829943002 Cr-Original-Commit-Position: refs/heads/master@{#474953} Committed: https://chromium.googlesource.com/chromium/src/+/4c271c0deaf103d5612078b3d0d81847819c5d52 Review-Url: https://codereview.chromium.org/2829943002 Cr-Commit-Position: refs/heads/master@{#475274} Committed: https://chromium.googlesource.com/chromium/src/+/49753ec593d5cd5e2faab80397058b15bdfb2089

Patch Set 1 : Initial Patch #

Total comments: 10

Patch Set 2 : Fixes for comments. #

Total comments: 2

Patch Set 3 : Updates WebApkActivity #

Total comments: 11

Patch Set 4 : Improvements to WebappInterceptNavigationDelegate #

Patch Set 5 : No longer maintains a session to CustomTab #

Patch Set 6 : Tiny cleanup #

Total comments: 3

Patch Set 7 : Fixes the rule for navigating to CCT for WebAPKs #

Patch Set 8 : Show page title in the CCT #

Patch Set 9 : Moving tests to JUnit4 #

Patch Set 10 : Fixes: WebAPK navigation block, 'Open in Chrome' from context menu more tests #

Patch Set 11 : Merge #

Total comments: 4

Patch Set 12 : Fixed comments #

Patch Set 13 : Merge adds launchMode="singleTop" back (removed by pkotwicz@ last week) to avoid kiling WebApkActivity. #

Total comments: 2

Patch Set 14 : Added singleTop to launch mode of the WebappActivity #

Patch Set 15 : Merge #

Patch Set 16 : Fix for Context menu issues #

Total comments: 2

Patch Set 17 : Comment cleanup #

Patch Set 18 : Relaxed the WebApkIntegrationTest #

Unified diffs Side-by-side diffs Delta from patch set Stats (+362 lines, -27 lines) Patch
M chrome/android/java/AndroidManifest.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +7 lines, -1 line 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java View 1 2 3 4 5 6 7 8 9 2 chunks +6 lines, -0 lines 0 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java View 1 2 3 4 5 6 7 1 chunk +53 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +11 lines, -13 lines 0 comments Download
M chrome/android/java_sources.gni View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +3 lines, -0 lines 0 comments Download
A chrome/android/javatests/src/org/chromium/chrome/browser/webapps/TopActivityListener.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +64 lines, -0 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkIntegrationTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +25 lines, -3 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappActivityTestRule.java View 1 2 3 4 5 6 7 8 2 chunks +11 lines, -4 lines 0 comments Download
A chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +166 lines, -0 lines 0 comments Download
M chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/contextmenu/ContextMenuUtils.java View 1 2 3 4 5 6 7 8 9 3 chunks +11 lines, -6 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 165 (115 generated)
piotrs
Can you please have a first look at this?
3 years, 7 months ago (2017-04-28 06:25:24 UTC) #43
dominickn
Looks pretty good! I mainly have nits at this stage - not much of a ...
3 years, 7 months ago (2017-05-01 00:41:41 UTC) #46
piotrs
Thanks Dom for the review! https://codereview.chromium.org/2829943002/diff/180001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/180001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java#newcode1 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:1: // Copyright 2015 The ...
3 years, 7 months ago (2017-05-01 02:55:07 UTC) #47
piotrs
Hi Yusuf, can you please take a look at this change? We're redirecting so called ...
3 years, 7 months ago (2017-05-01 02:58:07 UTC) #49
pkotwicz
https://codereview.chromium.org/2829943002/diff/200001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java (right): https://codereview.chromium.org/2829943002/diff/200001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java#newcode77 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java:77: public InterceptNavigationDelegateImpl createInterceptNavigationDelegate(Tab tab) { Drive by: You'll need ...
3 years, 7 months ago (2017-05-02 21:41:41 UTC) #51
piotrs
Thanks Peter! https://codereview.chromium.org/2829943002/diff/200001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java (right): https://codereview.chromium.org/2829943002/diff/200001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java#newcode77 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java:77: public InterceptNavigationDelegateImpl createInterceptNavigationDelegate(Tab tab) { On 2017/05/02 ...
3 years, 7 months ago (2017-05-03 01:05:42 UTC) #52
pkotwicz
Adding a dedicated test for WebAPKs sounds good. We already have some WebAPK specific tests ...
3 years, 7 months ago (2017-05-03 01:23:28 UTC) #53
piotrs
On 2017/05/03 01:23:28, pkotwicz wrote: > Adding a dedicated test for WebAPKs sounds good. We ...
3 years, 7 months ago (2017-05-03 01:56:23 UTC) #54
pkotwicz
Adding a test to WebApkIntegrationTest sounds reasonable
3 years, 7 months ago (2017-05-03 17:39:57 UTC) #55
Yusuf
https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java#newcode35 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:35: CustomTabsServiceConnection connection = new CustomTabsServiceConnection() { I dont think ...
3 years, 7 months ago (2017-05-03 17:56:06 UTC) #56
piotrs
Thanks for the comments Yusuf! https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java#newcode35 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:35: CustomTabsServiceConnection connection = new ...
3 years, 7 months ago (2017-05-03 22:58:18 UTC) #57
piotrs
On 2017/05/03 17:39:57, pkotwicz wrote: > Adding a test to WebApkIntegrationTest sounds reasonable FYI I ...
3 years, 7 months ago (2017-05-04 07:15:06 UTC) #58
Yusuf
https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java#newcode35 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:35: CustomTabsServiceConnection connection = new CustomTabsServiceConnection() { On 2017/05/03 22:58:18, ...
3 years, 7 months ago (2017-05-04 19:14:25 UTC) #59
piotrs
Hi Yusuf, Can you take a look and maybe LGTM the CCT part before your ...
3 years, 7 months ago (2017-05-09 23:43:14 UTC) #64
Yusuf
custom tabs bits lgtm
3 years, 7 months ago (2017-05-09 23:47:18 UTC) #65
dominickn
webapps lgtm % fixing the subject: "Regirecting" -> "Redirect"
3 years, 7 months ago (2017-05-10 03:02:30 UTC) #66
pkotwicz
https://codereview.chromium.org/2829943002/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java#newcode34 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:34: && !UrlUtilities.sameDomainOrHost(webappUrl, navigationParams.url, true)) { The logic for whether ...
3 years, 7 months ago (2017-05-10 04:52:21 UTC) #67
dominickn
not lgtm (sorry!) https://codereview.chromium.org/2829943002/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java#newcode34 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:34: && !UrlUtilities.sameDomainOrHost(webappUrl, navigationParams.url, true)) { On ...
3 years, 7 months ago (2017-05-10 05:15:11 UTC) #68
piotrs
Thank you Peter! https://codereview.chromium.org/2829943002/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java#newcode34 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:34: && !UrlUtilities.sameDomainOrHost(webappUrl, navigationParams.url, true)) { On ...
3 years, 7 months ago (2017-05-10 05:40:26 UTC) #69
pkotwicz
WebApkActivity.java LGTM
3 years, 7 months ago (2017-05-10 14:39:32 UTC) #70
pkotwicz
Actually there is still an issue with WebAPKs (but we are making progress!) Sorry for ...
3 years, 7 months ago (2017-05-10 18:58:15 UTC) #71
piotrs
On 2017/05/10 18:58:15, pkotwicz wrote: > Actually there is still an issue with WebAPKs (but ...
3 years, 7 months ago (2017-05-11 02:53:02 UTC) #72
dominickn
Having a case where we just don't navigate is a little sad. Is there a ...
3 years, 7 months ago (2017-05-11 03:10:51 UTC) #73
piotrs
On 2017/05/11 03:10:51, dominickn (OOO till 19th May) wrote: > Having a case where we ...
3 years, 7 months ago (2017-05-11 03:58:09 UTC) #74
dominickn
lgtm % sorting out the CCT back to WebAPK navigation story (I'm OOO for the ...
3 years, 7 months ago (2017-05-11 05:05:25 UTC) #75
pkotwicz
piotrs@ any progress?
3 years, 7 months ago (2017-05-17 19:07:30 UTC) #76
pkotwicz
piotrs@ any progress on the WebAPK stuff?
3 years, 7 months ago (2017-05-17 19:07:47 UTC) #77
piotrs
On 2017/05/17 19:07:47, pkotwicz wrote: > piotrs@ any progress on the WebAPK stuff? Yes, I ...
3 years, 7 months ago (2017-05-17 22:49:22 UTC) #78
piotrs
Number of things added: - Fixed WebAPK navigation being blocked by adding FLAG_ACTIVITY_CLEAR_TOP, which will ...
3 years, 7 months ago (2017-05-19 01:33:33 UTC) #102
piotrs
On 2017/05/19 01:33:33, piotrs wrote: > Number of things added: > - Fixed WebAPK navigation ...
3 years, 7 months ago (2017-05-23 01:39:36 UTC) #105
dominickn
lgtm % nit, thanks! https://codereview.chromium.org/2829943002/diff/480001/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2829943002/diff/480001/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java#newcode261 chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:261: // Limit context menu to ...
3 years, 7 months ago (2017-05-23 03:20:13 UTC) #106
piotrs
Thanks Dom, done. https://codereview.chromium.org/2829943002/diff/480001/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2829943002/diff/480001/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java#newcode261 chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:261: // Limit context menu to copying ...
3 years, 7 months ago (2017-05-23 04:11:11 UTC) #107
piotrs
Hi Ted, Can you please take a look at changes I'm introducing in the following ...
3 years, 7 months ago (2017-05-24 01:03:24 UTC) #109
Ted C
On 2017/05/24 01:03:24, piotrs wrote: > Hi Ted, > > Can you please take a ...
3 years, 7 months ago (2017-05-24 16:33:07 UTC) #115
pkotwicz
LGTM. I will start a thread with askatte@ to investigate whether android:launchMode="singleTop" causes issues on ...
3 years, 7 months ago (2017-05-25 04:59:33 UTC) #121
pkotwicz
Also worth mentioning that the CCT does not close when a user taps a link ...
3 years, 7 months ago (2017-05-25 05:47:30 UTC) #122
pkotwicz
https://codereview.chromium.org/2829943002/diff/560001/chrome/android/java/AndroidManifest.xml File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2829943002/diff/560001/chrome/android/java/AndroidManifest.xml#newcode486 chrome/android/java/AndroidManifest.xml:486: <activity android:name="org.chromium.chrome.browser.webapps.WebappActivity" Don't you need to make WebappActivity also ...
3 years, 7 months ago (2017-05-25 06:01:17 UTC) #123
piotrs
On 2017/05/25 05:47:30, pkotwicz wrote: > Also worth mentioning that the CCT does not close ...
3 years, 7 months ago (2017-05-25 07:04:52 UTC) #124
piotrs
https://codereview.chromium.org/2829943002/diff/560001/chrome/android/java/AndroidManifest.xml File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2829943002/diff/560001/chrome/android/java/AndroidManifest.xml#newcode486 chrome/android/java/AndroidManifest.xml:486: <activity android:name="org.chromium.chrome.browser.webapps.WebappActivity" On 2017/05/25 06:01:17, pkotwicz wrote: > Don't ...
3 years, 7 months ago (2017-05-25 07:05:11 UTC) #125
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2829943002/580001
3 years, 7 months ago (2017-05-25 11:23:00 UTC) #132
commit-bot: I haz the power
Committed patchset #14 (id:580001) as https://chromium.googlesource.com/chromium/src/+/7c7e9e600d256af4d2af1dbf68b59b030be0ab25
3 years, 7 months ago (2017-05-25 11:27:38 UTC) #135
Moe
A revert of this CL (patchset #14 id:580001) has been created in https://codereview.chromium.org/2905073002/ by mahmadi@chromium.org. ...
3 years, 7 months ago (2017-05-25 14:31:48 UTC) #136
pkotwicz
https://codereview.chromium.org/2829943002/diff/620001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (left): https://codereview.chromium.org/2829943002/diff/620001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java#oldcode208 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:208: if (source == ShortcutSource.NOTIFICATION) { Given that this CL ...
3 years, 7 months ago (2017-05-26 05:13:01 UTC) #142
piotrs
Thank Peter! https://codereview.chromium.org/2829943002/diff/620001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (left): https://codereview.chromium.org/2829943002/diff/620001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java#oldcode208 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:208: if (source == ShortcutSource.NOTIFICATION) { On 2017/05/26 ...
3 years, 7 months ago (2017-05-26 05:39:31 UTC) #143
piotrs
Thanks Peter!
3 years, 7 months ago (2017-05-26 05:39:38 UTC) #144
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2829943002/640001
3 years, 7 months ago (2017-05-26 05:56:35 UTC) #150
commit-bot: I haz the power
Committed patchset #17 (id:640001) as https://chromium.googlesource.com/chromium/src/+/4c271c0deaf103d5612078b3d0d81847819c5d52
3 years, 7 months ago (2017-05-26 07:26:18 UTC) #153
Guido Urdaneta
A revert of this CL (patchset #17 id:640001) has been created in https://codereview.chromium.org/2906043002/ by guidou@chromium.org. ...
3 years, 7 months ago (2017-05-26 09:51:17 UTC) #154
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2829943002/700001
3 years, 6 months ago (2017-05-29 00:46:18 UTC) #162
commit-bot: I haz the power
3 years, 6 months ago (2017-05-29 01:55:04 UTC) #165
Message was sent while issue was closed.
Committed patchset #18 (id:700001) as
https://chromium.googlesource.com/chromium/src/+/49753ec593d5cd5e2faab8039705...

Powered by Google App Engine
This is Rietveld 408576698