|
|
DescriptionRedirects 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 #Dependent Patchsets: Messages
Total messages: 165 (115 generated)
The CQ bit was checked by piotrs@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: This issue passed the CQ dry run.
The CQ bit was checked by piotrs@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by piotrs@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by piotrs@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by piotrs@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by piotrs@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by piotrs@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by piotrs@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 #8 (id:140001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== Regirecting off-origin navigations in PWAs to CCT. CL uploaded to run trybots and see what it breaks. BUG= ========== to ========== 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 ==========
Description was changed from ========== 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 ========== to ========== 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 ==========
piotrs@chromium.org changed reviewers: + dominickn@chromium.org
Can you please have a first look at this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks pretty good! I mainly have nits at this stage - not much of a CustomTabs expert so I'm not sure how much detail I've covered there. https://codereview.chromium.org/2829943002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. Nit: new file, use 2017. https://codereview.chromium.org/2829943002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:35: CustomTabsServiceConnection mConnection = new CustomTabsServiceConnection() { Nit: the m prefix is just for member variables. https://codereview.chromium.org/2829943002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java (right): https://codereview.chromium.org/2829943002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. Nit: new file, use 2017 https://codereview.chromium.org/2829943002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java:75: CustomTabActivity cta = assertCustomTabActivityLaunchedForOffOriginUrl(); Nit: the variable name here should probably be customTab (and I see you've used custo,mTab later) https://codereview.chromium.org/2829943002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java:90: CustomTabActivity cta = assertCustomTabActivityLaunchedForOffOriginUrl(); Nit: variable name
Thanks Dom for the review! https://codereview.chromium.org/2829943002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/05/01 00:41:41, dominickn wrote: > Nit: new file, use 2017. Done. https://codereview.chromium.org/2829943002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:35: CustomTabsServiceConnection mConnection = new CustomTabsServiceConnection() { On 2017/05/01 00:41:41, dominickn wrote: > Nit: the m prefix is just for member variables. Done. https://codereview.chromium.org/2829943002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java (right): https://codereview.chromium.org/2829943002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/05/01 00:41:41, dominickn wrote: > Nit: new file, use 2017 Done. https://codereview.chromium.org/2829943002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java:75: CustomTabActivity cta = assertCustomTabActivityLaunchedForOffOriginUrl(); On 2017/05/01 00:41:41, dominickn wrote: > Nit: the variable name here should probably be customTab (and I see you've used > custo,mTab later) Oh, you got me... changed, but now I have to add one more line break below and it's no longer as nice ;) https://codereview.chromium.org/2829943002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java:90: CustomTabActivity cta = assertCustomTabActivityLaunchedForOffOriginUrl(); On 2017/05/01 00:41:41, dominickn wrote: > Nit: variable name Done.
piotrs@chromium.org changed reviewers: + yusufo@chromium.org
Hi Yusuf, can you please take a look at this change? We're redirecting so called "off-origin" navigations from an installed PWA to CCT. Thanks!
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
https://codereview.chromium.org/2829943002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java (right): https://codereview.chromium.org/2829943002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java:77: public InterceptNavigationDelegateImpl createInterceptNavigationDelegate(Tab tab) { Drive by: You'll need to modify WebApkActivity#createTabDelegateFactory() as well. In particular, WebApkActivity#createTabDelegateFactory() creates a subclass of WebappDelegateFactory which overwrites createInterceptNavigationDelegate()
Thanks Peter! https://codereview.chromium.org/2829943002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java (right): https://codereview.chromium.org/2829943002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java:77: public InterceptNavigationDelegateImpl createInterceptNavigationDelegate(Tab tab) { On 2017/05/02 21:41:41, pkotwicz wrote: > Drive by: You'll need to modify WebApkActivity#createTabDelegateFactory() as > well. In particular, WebApkActivity#createTabDelegateFactory() creates a > subclass of WebappDelegateFactory which overwrites > createInterceptNavigationDelegate() Thanks! I've missed this. Done. WDYT about adding a dedicated test for WebApks?
Adding a dedicated test for WebAPKs sounds good. We already have some WebAPK specific tests in ExternalNavigationHandlerTest.java
On 2017/05/03 01:23:28, pkotwicz wrote: > Adding a dedicated test for WebAPKs sounds good. We already have some WebAPK > specific tests in ExternalNavigationHandlerTest.java In this case ExternalNavigationHandlerTest won't be enough, as it doesn't start WebApkActivity. The only test I found that does that is WebApkIntegrationTest, do you recommend adding it there? So far there's only one test in there. There will be a fair bit of "test duplication", but I think it's the right thing to do. WDYT?
Adding a test to WebApkIntegrationTest sounds reasonable
https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:35: CustomTabsServiceConnection connection = new CustomTabsServiceConnection() { I dont think you actually need to maintain this at this point. The only benefit you are getting is having a service connection, which could have been used to warmup Chrome by other apps. But since we dont have to worry about that for WebApps, we can just call the CustomTabsIntent related bits and not worry about maintaining a connection and client. https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:56: return mClient.newSession(new CustomTabsCallback() { At some point, we might do more with each session and may need to use CustomTabsSession etc, but at this point we dont. https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:59: super.onNavigationEvent(navigationEvent, extras); super does nothing. This part is not needed. https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:70: if (isHttpOrHttps(navigationParams.url) Is this strictly http or https only? Data? File? About? Javascript not included? Might be good to check with UrlUtilities.isAcceptedScheme or UrlUtilities.isValidForIntentFallbackNavigation to see if any of them are what you want rather than doing a one off here (things might diverge without someone remembering the scheme restriction here). I think your case actually do look like isValidForIntentFallbackNavigation
Thanks for the comments Yusuf! https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:35: CustomTabsServiceConnection connection = new CustomTabsServiceConnection() { On 2017/05/03 17:56:06, Yusuf wrote: > I dont think you actually need to maintain this at this point. The only benefit > you are getting is having a service connection, which could have been used to > warmup Chrome by other apps. But since we dont have to worry about that for > WebApps, we can just call the CustomTabsIntent related bits and not worry about > maintaining a connection and client. There are 2 reasons I went this way: 1) This allows me to bindCustomTabsService to the current implementation. We can stay in the scope of the same implementation, so no point bothering the user with a browser picker I think. Also it makes it much easier to test, as system UI doesn't interrupt. 2) When I was trying out the code without CustomTabsClient on try bots, I was always getting strict mode violations (policy=2655 violation=2) on linux_android_rel_ng, and even wrapping these calls in StrictMode.allowThreadDiskReads() was not helping. Can I pick CCT implementation if I only use the intent? There's nothing in CustomTabsIntent.Builder that seems to allow that. https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:56: return mClient.newSession(new CustomTabsCallback() { On 2017/05/03 17:56:05, Yusuf wrote: > At some point, we might do more with each session and may need to use > CustomTabsSession etc, but at this point we dont. See reply to the comment above. https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:59: super.onNavigationEvent(navigationEvent, extras); On 2017/05/03 17:56:05, Yusuf wrote: > super does nothing. This part is not needed. Done, thanks for noticing. https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:70: if (isHttpOrHttps(navigationParams.url) On 2017/05/03 17:56:05, Yusuf wrote: > Is this strictly http or https only? Data? File? About? Javascript not included? > > Might be good to check with UrlUtilities.isAcceptedScheme or > UrlUtilities.isValidForIntentFallbackNavigation to see if any of them are what > you want rather than doing a one off here (things might diverge without someone > remembering the scheme restriction here). > > I think your case actually do look like isValidForIntentFallbackNavigation I know this intent would crash if I pass data: URL (android cannot find suitable component), it's what made me narrow down the protocol. I don't see why I would want to pass file, about, or JS URLs to CCT, so I think HTTP+HTTPS is indeed the only thing I want to allow. And you are correct that isValidForIntentFallbackNavigation expresses exactly what I need! FYI added some more tests for isValidForIntentFallbackNavigation to specify its behavior better in https://codereview.chromium.org/2855293002/.
On 2017/05/03 17:39:57, pkotwicz wrote: > Adding a test to WebApkIntegrationTest sounds reasonable FYI I will add the tests to this CL when I convert all the related tests to JUnit4 (already started). It will be much cleaner with a custom @Rule to capture most recent activity.
https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:35: CustomTabsServiceConnection connection = new CustomTabsServiceConnection() { On 2017/05/03 22:58:18, piotrs wrote: > On 2017/05/03 17:56:06, Yusuf wrote: > > I dont think you actually need to maintain this at this point. The only > benefit > > you are getting is having a service connection, which could have been used to > > warmup Chrome by other apps. But since we dont have to worry about that for > > WebApps, we can just call the CustomTabsIntent related bits and not worry > about > > maintaining a connection and client. > > There are 2 reasons I went this way: > 1) This allows me to bindCustomTabsService to the current implementation. We can > stay in the scope of the same implementation, so no point bothering the user > with a browser picker I think. Also it makes it much easier to test, as system > UI doesn't interrupt. > 2) When I was trying out the code without CustomTabsClient on try bots, I was > always getting strict mode violations (policy=2655 violation=2) on > linux_android_rel_ng, and even wrapping these calls in > StrictMode.allowThreadDiskReads() was not helping. > > Can I pick CCT implementation if I only use the intent? There's nothing in > CustomTabsIntent.Builder that seems to allow that. once you build your CustomTabsIntent, CustomTabsIntent.intent is public and you can call setPackageName on that one to our own package name. Not really sure what is happening around StringModeViolations but using the service definitely won't make you less susceptible to them. I would just have the intent related bits here. https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:70: if (isHttpOrHttps(navigationParams.url) On 2017/05/03 22:58:18, piotrs wrote: > On 2017/05/03 17:56:05, Yusuf wrote: > > Is this strictly http or https only? Data? File? About? Javascript not > included? > > > > Might be good to check with UrlUtilities.isAcceptedScheme or > > UrlUtilities.isValidForIntentFallbackNavigation to see if any of them are what > > you want rather than doing a one off here (things might diverge without > someone > > remembering the scheme restriction here). > > > > I think your case actually do look like isValidForIntentFallbackNavigation > > I know this intent would crash if I pass data: URL (android cannot find suitable > component), it's what made me narrow down the protocol. I don't see why I would > want to pass file, about, or JS URLs to CCT, so I think HTTP+HTTPS is indeed the > only thing I want to allow. > > And you are correct that isValidForIntentFallbackNavigation expresses exactly > what I need! > > FYI added some more tests for isValidForIntentFallbackNavigation to specify its > behavior better in https://codereview.chromium.org/2855293002/. Thanks.
The CQ bit was checked by piotrs@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: This issue passed the CQ dry run.
Hi Yusuf, Can you take a look and maybe LGTM the CCT part before your vacations? I'm planning to add one more test for WebAPKs before landing this, but this requires landing a refactoring to JUnit4 which I'm waiting for. With your LGTM it will be much easier to land this week, Thanks, Piotr https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:35: CustomTabsServiceConnection connection = new CustomTabsServiceConnection() { On 2017/05/04 19:14:25, Yusuf wrote: > On 2017/05/03 22:58:18, piotrs wrote: > > On 2017/05/03 17:56:06, Yusuf wrote: > > > I dont think you actually need to maintain this at this point. The only > > benefit > > > you are getting is having a service connection, which could have been used > to > > > warmup Chrome by other apps. But since we dont have to worry about that for > > > WebApps, we can just call the CustomTabsIntent related bits and not worry > > about > > > maintaining a connection and client. > > > > There are 2 reasons I went this way: > > 1) This allows me to bindCustomTabsService to the current implementation. We > can > > stay in the scope of the same implementation, so no point bothering the user > > with a browser picker I think. Also it makes it much easier to test, as system > > UI doesn't interrupt. > > 2) When I was trying out the code without CustomTabsClient on try bots, I was > > always getting strict mode violations (policy=2655 violation=2) on > > linux_android_rel_ng, and even wrapping these calls in > > StrictMode.allowThreadDiskReads() was not helping. > > > > Can I pick CCT implementation if I only use the intent? There's nothing in > > CustomTabsIntent.Builder that seems to allow that. > > once you build your CustomTabsIntent, CustomTabsIntent.intent is public and you > can call setPackageName on that one to our own package name. > > Not really sure what is happening around StringModeViolations but using the > service definitely won't make you less susceptible to them. I would just have > the intent related bits here. Done as suggested.
custom tabs bits lgtm
webapps lgtm % fixing the subject: "Regirecting" -> "Redirect"
https://codereview.chromium.org/2829943002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:34: && !UrlUtilities.sameDomainOrHost(webappUrl, navigationParams.url, true)) { The logic for whether to open the CCT is different for WebAPKs and Webapps. Notice WebApkBrowserControlsDelegate.java
not lgtm (sorry!) https://codereview.chromium.org/2829943002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:34: && !UrlUtilities.sameDomainOrHost(webappUrl, navigationParams.url, true)) { On 2017/05/10 04:52:21, pkotwicz wrote: > The logic for whether to open the CCT is different for WebAPKs and Webapps. > Notice WebApkBrowserControlsDelegate.java Oh, that's a good catch Peter, thanks. This is twice today I've forgotten about the scope logic for WebAPKs. The sooner we consolidate all this the better.
Thank you Peter! https://codereview.chromium.org/2829943002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java (right): https://codereview.chromium.org/2829943002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java:34: && !UrlUtilities.sameDomainOrHost(webappUrl, navigationParams.url, true)) { On 2017/05/10 05:15:11, dominickn wrote: > On 2017/05/10 04:52:21, pkotwicz wrote: > > The logic for whether to open the CCT is different for WebAPKs and Webapps. > > Notice WebApkBrowserControlsDelegate.java > > Oh, that's a good catch Peter, thanks. This is twice today I've forgotten about > the scope logic for WebAPKs. The sooner we consolidate all this the better. Thanks a lot and fixed now. We definitely need separate tests for WebAPKs now. I'm still waiting for owner LGTM on codereview.chromium.org/2863583002 before I can write them nicely.
WebApkActivity.java LGTM
Actually there is still an issue with WebAPKs (but we are making progress!) Sorry for not noticing it on the first pass In particular for the following scenario: - A WebAPK is launched for https://tests.peter.sh/notification-generator/ - A user taps on a link which opens the CCT (e.g. "public domain" link) - A user taps a link in the CCT which navigates back to https://tests.peter.sh/notification-generator/ (I used dev tools for this) I would expect the CCT to close and the WebAPK to be navigated. Instead the CCT stays open and does not navigate because InterceptNavigationDelegate#shouldIgnoreNavigation() returns true
On 2017/05/10 18:58:15, pkotwicz wrote: > Actually there is still an issue with WebAPKs (but we are making progress!) > Sorry for not noticing it on the first pass > > In particular for the following scenario: > - A WebAPK is launched for https://tests.peter.sh/notification-generator/ > - A user taps on a link which opens the CCT (e.g. "public domain" link) > - A user taps a link in the CCT which navigates back to > https://tests.peter.sh/notification-generator/ (I used dev tools for this) > > I would expect the CCT to close and the WebAPK to be navigated. Instead the CCT > stays open and does not navigate because > InterceptNavigationDelegate#shouldIgnoreNavigation() returns true Good catch, forgot about this case! I do however have mixed feelings about it. We can add special code to CustomTabActivity to handle this edge case, however I think the right thing to do is for WebAPKs to define Intent Filters and become handlers of PWA URLs regardless where they are clicked. I'm concerned that we will add a lot of complexity for a rare and rather harmless case.
Having a case where we just don't navigate is a little sad. Is there a way that we can know in shouldIgnoreNavigation that the navigation is coming from the CustomTabActivity on top of the WebApkActivity?
On 2017/05/11 03:10:51, dominickn (OOO till 19th May) wrote: > Having a case where we just don't navigate is a little sad. Is there a way that > we can know in shouldIgnoreNavigation that the navigation is coming from the > CustomTabActivity on top of the WebApkActivity? Sorry, I forgot to reply to the "shouldIgnoreNavigation" bit. I cannot replicate it. I have 2 PWAs pointing at each other A and B. When I add A to the home screen, open it and navigate to B, it will open in a CCT. If I click on a link to A while being in a CCT for B it will stay in CCT and navigate to A. shouldIgnoreNavigation which we are discussing here is defined by WebappInterceptNavigationDelegate (or the anonymous subclass in WebApkActivity) and this code is not executed when we move to CustomTabActivity. Peter, can you check again, maybe there was something else that made CCT not react to your clicks?
lgtm % sorting out the CCT back to WebAPK navigation story (I'm OOO for the next week, as is dfalcantara. Trusting the two of you to work out an acceptable solution here)
piotrs@ any progress?
piotrs@ any progress on the WebAPK stuff?
On 2017/05/17 19:07:47, pkotwicz wrote: > piotrs@ any progress on the WebAPK stuff? Yes, I figured that out. Writing a good test for it, that's where the challenge is! I also realized "Open in Chrome" from the context menu should go straight to Chrome bypassing CCT, so adding that as well. Hopefully will get it done today.
The CQ bit was checked by piotrs@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_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_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 piotrs@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by piotrs@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by piotrs@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...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 piotrs@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 #11 (id:380001) has been deleted
Patchset #11 (id:400001) has been deleted
Patchset #11 (id:420001) has been deleted
Patchset #11 (id:440001) has been deleted
Patchset #8 (id:320001) has been deleted
Number of things added: - Fixed WebAPK navigation being blocked by adding FLAG_ACTIVITY_CLEAR_TOP, which will close CCT on top of WebApkActivity to which we navigate. I didn't manage to add a test in WebApkIntegrationTest, as test setup is very simple and Chromium under test is not aware of the WebAPK that we start under test. It seems that creating robust test setup for WebAPKs is a big task in itself. - Added a test for WebAPK -> CCT navigation that was missing. This required introducing ugly setScopeForTest to override the scope of launched app, as test setup doesn't allow any customization of web manifest. Without it every URL is outside scope and opens in CCT. Suggestions welcome. - Fixed the "Open in Chrome" from context menu in WebappActivity to go straight to TabbedChrome and not be redirected to CCT. Added tests for it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/19 01:33:33, piotrs wrote: > Number of things added: > - Fixed WebAPK navigation being blocked by adding FLAG_ACTIVITY_CLEAR_TOP, which > will close CCT on top of WebApkActivity to which we navigate. I didn't manage to > add a test in WebApkIntegrationTest, as test setup is very simple and Chromium > under test is not aware of the WebAPK that we start under test. It seems that > creating robust test setup for WebAPKs is a big task in itself. > - Added a test for WebAPK -> CCT navigation that was missing. This required > introducing ugly setScopeForTest to override the scope of launched app, as test > setup doesn't allow any customization of web manifest. Without it every URL is > outside scope and opens in CCT. Suggestions welcome. > - Fixed the "Open in Chrome" from context menu in WebappActivity to go straight > to TabbedChrome and not be redirected to CCT. Added tests for it. Hey Dominick and Peter, I consider this ready for taking a look again when you have time. Cheers, Piotr
lgtm % nit, thanks! https://codereview.chromium.org/2829943002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2829943002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:261: // Limit context menu to copying until FRE has been completed, Amend this comment as discussed. https://codereview.chromium.org/2829943002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2829943002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:170: // - If a plain PWA is launching from a notification, we want to ensure that the URL being Nit: "legacy" PWA, not "plain" PWA.
Thanks Dom, done. https://codereview.chromium.org/2829943002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2829943002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:261: // Limit context menu to copying until FRE has been completed, On 2017/05/23 03:20:13, dominickn wrote: > Amend this comment as discussed. Done. https://codereview.chromium.org/2829943002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2829943002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:170: // - If a plain PWA is launching from a notification, we want to ensure that the URL being On 2017/05/23 03:20:13, dominickn wrote: > Nit: "legacy" PWA, not "plain" PWA. Done.
piotrs@chromium.org changed reviewers: + tedchoc@chromium.org
Hi Ted, Can you please take a look at changes I'm introducing in the following files: - ChromeContextMenuPopulator.java - TabContextMenuItemDelegate.java - ContextMenuUtils.java I would hugely appreciate this, Thanks, Piotr
Patchset #13 (id:520001) has been deleted
The CQ bit was checked by piotrs@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: This issue passed the CQ dry run.
On 2017/05/24 01:03:24, piotrs wrote: > Hi Ted, > > Can you please take a look at changes I'm introducing in the following files: > - ChromeContextMenuPopulator.java > - TabContextMenuItemDelegate.java > - ContextMenuUtils.java > > I would hugely appreciate this, Thanks, > Piotr lgtm
The CQ bit was checked by piotrs@chromium.org to run a CQ dry run
Patchset #13 (id:540001) 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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
LGTM. I will start a thread with askatte@ to investigate whether android:launchMode="singleTop" causes issues on Samsung Galaxy Grand Prime. I think that it is safe to land as is. We can do more investigation based on what askatte@ finds There is an ugly transition when tapping a link inside of the CCT causes the CCT to close. We can probably fix that in a follow up CL
Also worth mentioning that the CCT does not close when a user taps a link in the CCT which is within the webapp's scope (This is for non WebAPK mode). I think it is ok to fix this as a follow up CL
https://codereview.chromium.org/2829943002/diff/560001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2829943002/diff/560001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:486: <activity android:name="org.chromium.chrome.browser.webapps.WebappActivity" Don't you need to make WebappActivity also singleTop? From my testing it looks like with your change tapping the "homescreen icon" relaunches the webapp even if it is already running
On 2017/05/25 05:47:30, pkotwicz wrote: > Also worth mentioning that the CCT does not close when a user taps a link in the > CCT which is within the webapp's scope (This is for non WebAPK mode). I think it > is ok to fix this as a follow up CL This is expected. Will be fixed when WebAPKs take over the world ;-)
https://codereview.chromium.org/2829943002/diff/560001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2829943002/diff/560001/chrome/android/java/An... 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 you need to make WebappActivity also singleTop? From my testing it looks > like with your change tapping the "homescreen icon" relaunches the webapp even > if it is already running You're right, added!
The CQ bit was checked by piotrs@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: This issue passed the CQ dry run.
The CQ bit was checked by piotrs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, dominickn@chromium.org, tedchoc@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2829943002/#ps580001 (title: "Added singleTop to launch mode of the WebappActivity")
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": 580001, "attempt_start_ts": 1495711368685410, "parent_rev": "83d0b9c95c500b60b8f53416d17027cdbf96e955", "commit_rev": "7c7e9e600d256af4d2af1dbf68b59b030be0ab25"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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-Commit-Position: refs/heads/master@{#474625} Committed: https://chromium.googlesource.com/chromium/src/+/7c7e9e600d256af4d2af1dbf68b5... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:580001) as https://chromium.googlesource.com/chromium/src/+/7c7e9e600d256af4d2af1dbf68b5...
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:580001) has been created in https://codereview.chromium.org/2905073002/ by mahmadi@chromium.org. The reason for reverting is: This CL is likely the culprit for test failures: https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... .
Message was sent while issue was closed.
Description was changed from ========== 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-Commit-Position: refs/heads/master@{#474625} Committed: https://chromium.googlesource.com/chromium/src/+/7c7e9e600d256af4d2af1dbf68b5... ========== to ========== 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-Commit-Position: refs/heads/master@{#474625} Committed: https://chromium.googlesource.com/chromium/src/+/7c7e9e600d256af4d2af1dbf68b5... ==========
The CQ bit was checked by piotrs@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2829943002/diff/620001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (left): https://codereview.chromium.org/2829943002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:208: if (source == ShortcutSource.NOTIFICATION) { Given that this CL was reverted (not by me!) it is worth mentioning that with singleTop in AndroidManifest.xml for WebappActivity, this no longer causes the webapp to be relaunched for Intent.FLAG_ACTIVITY_CLEAR_TOP for notifications.
Thank Peter! https://codereview.chromium.org/2829943002/diff/620001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (left): https://codereview.chromium.org/2829943002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:208: if (source == ShortcutSource.NOTIFICATION) { On 2017/05/26 05:13:01, pkotwicz wrote: > Given that this CL was reverted (not by me!) it is worth mentioning that with > singleTop in AndroidManifest.xml for WebappActivity, this no longer causes the > webapp to be relaunched for Intent.FLAG_ACTIVITY_CLEAR_TOP for notifications. Done.
Thanks Peter!
The CQ bit was checked by piotrs@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 piotrs@chromium.org
The CQ bit was checked by piotrs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, tedchoc@chromium.org, yusufo@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2829943002/#ps640001 (title: "Comment cleanup")
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": 640001, "attempt_start_ts": 1495778168150310, "parent_rev": "69dabee977f04d3166b0a42bdc335818567a762e", "commit_rev": "4c271c0deaf103d5612078b3d0d81847819c5d52"}
Message was sent while issue was closed.
Description was changed from ========== 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-Commit-Position: refs/heads/master@{#474625} Committed: https://chromium.googlesource.com/chromium/src/+/7c7e9e600d256af4d2af1dbf68b5... ========== to ========== 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-Commit-Position: refs/heads/master@{#474625} Committed: https://chromium.googlesource.com/chromium/src/+/7c7e9e600d256af4d2af1dbf68b5... Review-Url: https://codereview.chromium.org/2829943002 Cr-Commit-Position: refs/heads/master@{#474953} Committed: https://chromium.googlesource.com/chromium/src/+/4c271c0deaf103d5612078b3d0d8... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:640001) as https://chromium.googlesource.com/chromium/src/+/4c271c0deaf103d5612078b3d0d8...
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:640001) has been created in https://codereview.chromium.org/2906043002/ by guidou@chromium.org. The reason for reverting is: Suspect of breaking Android Tests bot. First failure: https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... Will reland if the revert does not fix the bot..
Message was sent while issue was closed.
Description was changed from ========== 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-Commit-Position: refs/heads/master@{#474625} Committed: https://chromium.googlesource.com/chromium/src/+/7c7e9e600d256af4d2af1dbf68b5... Review-Url: https://codereview.chromium.org/2829943002 Cr-Commit-Position: refs/heads/master@{#474953} Committed: https://chromium.googlesource.com/chromium/src/+/4c271c0deaf103d5612078b3d0d8... ========== to ========== 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-Commit-Position: refs/heads/master@{#474625} Committed: https://chromium.googlesource.com/chromium/src/+/7c7e9e600d256af4d2af1dbf68b5... Review-Url: https://codereview.chromium.org/2829943002 Cr-Commit-Position: refs/heads/master@{#474953} Committed: https://chromium.googlesource.com/chromium/src/+/4c271c0deaf103d5612078b3d0d8... ==========
The CQ bit was checked by piotrs@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:660001) has been deleted
Patchset #18 (id:680001) has been deleted
The CQ bit was checked by piotrs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, tedchoc@chromium.org, yusufo@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2829943002/#ps700001 (title: "Relaxed the WebApkIntegrationTest")
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": 1496018762666010, "parent_rev": "a57b15774cb1599333fa6e3c9cb57095d847b02a", "commit_rev": "49753ec593d5cd5e2faab80397058b15bdfb2089"}
Message was sent while issue was closed.
Description was changed from ========== 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-Commit-Position: refs/heads/master@{#474625} Committed: https://chromium.googlesource.com/chromium/src/+/7c7e9e600d256af4d2af1dbf68b5... Review-Url: https://codereview.chromium.org/2829943002 Cr-Commit-Position: refs/heads/master@{#474953} Committed: https://chromium.googlesource.com/chromium/src/+/4c271c0deaf103d5612078b3d0d8... ========== to ========== 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/+/7c7e9e600d256af4d2af1dbf68b5... Review-Url: https://codereview.chromium.org/2829943002 Cr-Original-Commit-Position: refs/heads/master@{#474953} Committed: https://chromium.googlesource.com/chromium/src/+/4c271c0deaf103d5612078b3d0d8... Review-Url: https://codereview.chromium.org/2829943002 Cr-Commit-Position: refs/heads/master@{#475274} Committed: https://chromium.googlesource.com/chromium/src/+/49753ec593d5cd5e2faab8039705... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:700001) as https://chromium.googlesource.com/chromium/src/+/49753ec593d5cd5e2faab8039705... |