Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2018113002:
Upstream: Do not show the add-to-homescreen/install-native-app infobar for WebAPKs (Closed)
DescriptionUpstream: Do not show the add-to-homescreen/install-native-app infobar for WebAPKs
This CL also moves WebappActivity's TabDelegateFactory - WebappDelegateFactory
- to its own file so that WebApkActivity can extend WebappDelegateFactory.
BUG=609122, 614336
Committed: https://crrev.com/59fbc602e64fe0999df7d3c428c0bfd656930932
Cr-Commit-Position: refs/heads/master@{#397439}
Patch Set 1 : #
Total comments: 17
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Messages
Total messages: 37 (18 generated)
Description was changed from ========== Upstream: Do not should the add-to-homescreen/install-native-app infobar for WebAPKs BUG=609122 ========== to ========== Upstream: Do not show the add-to-homescreen/install-native-app infobar for WebAPKs BUG=609122 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Upstream: Do not show the add-to-homescreen/install-native-app infobar for WebAPKs BUG=609122 ========== to ========== Upstream: Do not show the add-to-homescreen/install-native-app infobar for WebAPKs This CL also moves WebappActivity's TabDelegateFactory - WebappDelegateFactory - to its own file so that WebApkActivity can extend WebappDelegateFactory. BUG=609122 ==========
pkotwicz@chromium.org changed reviewers: + hanxi@chromium.org
Xi can you please take a look?
https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenDelegateFactory.java (right): https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenDelegateFactory.java:26: public TopControlsVisibilityDelegate createTopControlsVisibilityDelegate(Tab tab) { Do we still need to override this function in FullScreenDelegateFactory? https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java (right): https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java:25: private static class WebContentsDelegate extends TabWebContentsDelegateAndroid { Rename WebContentsDelegate to WebappContentsDelegate? It is more precise for what you have here.
Xi can you please take another look? https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenDelegateFactory.java (right): https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenDelegateFactory.java:26: public TopControlsVisibilityDelegate createTopControlsVisibilityDelegate(Tab tab) { I think so. The implementation here is different than the default implementation in TopControlsVisibilityDelegate https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java (right): https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java:25: private static class WebContentsDelegate extends TabWebContentsDelegateAndroid { I named the class WebContentsDelegate as a shortening of TabWebContentsDelegateAndroid which extends WebContentsDelegateAndroid
lgtm https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenDelegateFactory.java (right): https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenDelegateFactory.java:26: public TopControlsVisibilityDelegate createTopControlsVisibilityDelegate(Tab tab) { On 2016/05/31 02:32:01, pkotwicz wrote: > I think so. The implementation here is different than the default implementation > in TopControlsVisibilityDelegate I missed the class EmbedContentViewActivity which also extends the FullscreenActivity. Yes, we need to keep the implementation for EmbedContentViewActivity. https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java (right): https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java:25: private static class WebContentsDelegate extends TabWebContentsDelegateAndroid { On 2016/05/31 02:32:01, pkotwicz wrote: > I named the class WebContentsDelegate as a shortening of > TabWebContentsDelegateAndroid which extends WebContentsDelegateAndroid Usually the subclass name is more specific than the base class. Since this is a private class inside the WebappDelegateFactory, it might be fine. We could leave it as it is now.
pkotwicz@chromium.org changed reviewers: + dfalcantara@chromium.org
dfalcantara@ PTAL
https://chromiumcodereview.appspot.com/2018113002/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://chromiumcodereview.appspot.com/2018113002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:50: public AppBannerManager createAppBannerManager(Tab tab) { Do you not want to show _any_ app banners in WebApks? https://chromiumcodereview.appspot.com/2018113002/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java (right): https://chromiumcodereview.appspot.com/2018113002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. It's 2016. https://chromiumcodereview.appspot.com/2018113002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java:21: * A {@link TabDelegateFactory} class to be used in all {@link Tab} instances owned Wrap at 100 everywhere. https://chromiumcodereview.appspot.com/2018113002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java:25: private static class WebContentsDelegate extends TabWebContentsDelegateAndroid { On 2016/05/31 14:18:27, Xi Han wrote: > On 2016/05/31 02:32:01, pkotwicz wrote: > > I named the class WebContentsDelegate as a shortening of > > TabWebContentsDelegateAndroid which extends WebContentsDelegateAndroid > > Usually the subclass name is more specific than the base class. Since this is a > private class inside the WebappDelegateFactory, it might be fine. We could leave > it as it is now. Naming this WebContentsDelegate goes against both the (admittedly horrible) existing convention and duplicates the name of the base class on the native side. Go with WebappWebContentsDelegateAndroid. https://chromiumcodereview.appspot.com/2018113002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDelegateFactory.java:80: * @param url Fill in these parameters. https://chromiumcodereview.appspot.com/2018113002/diff/20001/chrome/android/j... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappDelegateFactoryTest.java (right): https://chromiumcodereview.appspot.com/2018113002/diff/20001/chrome/android/j... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappDelegateFactoryTest.java:25: @MediumTest This test is disabled on trunk, which you didn't copy over from line #36 in WebappVisibilityTest. Copy it from scratch to make sure you get the latest file.
Patchset #2 (id:40001) has been deleted
Description was changed from ========== Upstream: Do not show the add-to-homescreen/install-native-app infobar for WebAPKs This CL also moves WebappActivity's TabDelegateFactory - WebappDelegateFactory - to its own file so that WebApkActivity can extend WebappDelegateFactory. BUG=609122 ========== to ========== Upstream: Do not show the add-to-homescreen/install-native-app infobar for WebAPKs This CL also moves WebappActivity's TabDelegateFactory - WebappDelegateFactory - to its own file so that WebApkActivity can extend WebappDelegateFactory. BUG=609122, 614336 ==========
dfalcantara@ can you please take another look? https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:50: public AppBannerManager createAppBannerManager(Tab tab) { I am aware of only two types of app banners: - App banners which show an info bar to add an app shortcut to the homescreen - App banners which show an info bar to install a native Android app We do not want to show either. Are there other types of app banners that I do not know about? https://codereview.chromium.org/2018113002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappDelegateFactoryTest.java (right): https://codereview.chromium.org/2018113002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappDelegateFactoryTest.java:25: @MediumTest I changed the test to be more of a unit test than it used to be. crbug.com/614336 was with respect to WebappActivity not starting. This test no longer needs to start a WebappActivity. I have renamed the test class back to WebappVisibilityTest.java to make code review easier
Hrm... lgtm then. For some reason Rietveld is saying that the test file is being straight up deleted, though I'm hoping that's just a mistake on review site. https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:50: public AppBannerManager createAppBannerManager(Tab tab) { On 2016/05/31 20:14:27, pkotwicz wrote: > I am aware of only two types of app banners: > - App banners which show an info bar to add an app shortcut to the homescreen > - App banners which show an info bar to install a native Android app > > We do not want to show either. Are there other types of app banners that I do > not know about? You're assuming the only app banners shown are for the current web app. What about users who end up on other sites that trigger banners? Are those unimportant?
https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:50: public AppBannerManager createAppBannerManager(Tab tab) { That's a sticky point. Ideally when a user navigates out of the scope of the WebAPK they would be punted out of Chrome. That's the intent of https://codereview.chromium.org/2005053002/ It is easy for a web page to stay within the WebAPK app but navigate out of the WebAPK scope. An example on how this can occur is if the WebAPK web page navigates itself to a URL outside of the WebApk scope while the WebAPK is in the background (via JavaScript) I am fine if the app banner is not shown in this scenario. What do you think?
https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2018113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:50: public AppBannerManager createAppBannerManager(Tab tab) { On 2016/05/31 21:59:30, pkotwicz wrote: > That's a sticky point. Ideally when a user navigates out of the scope of the > WebAPK they would be punted out of Chrome. That's the intent of > https://codereview.chromium.org/2005053002/ It is easy for a web page to stay > within the WebAPK app but navigate out of the WebAPK scope. An example on how > this can occur is if the WebAPK web page navigates itself to a URL outside of > the WebApk scope while the WebAPK is in the background (via JavaScript) > > I am fine if the app banner is not shown in this scenario. What do you think? Yeah, I'm fine with that, too. That needs to be documented somewhere to prevent it from being filed as a bug, though. Add a comment here to make it obvious that it was discussed and that app banners are being explicitly not shown in WebApks.
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2018113002/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018113002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018113002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2018113002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018113002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018113002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018113002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018113002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018113002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018113002/100001
Message was sent while issue was closed.
Description was changed from ========== Upstream: Do not show the add-to-homescreen/install-native-app infobar for WebAPKs This CL also moves WebappActivity's TabDelegateFactory - WebappDelegateFactory - to its own file so that WebApkActivity can extend WebappDelegateFactory. BUG=609122, 614336 ========== to ========== Upstream: Do not show the add-to-homescreen/install-native-app infobar for WebAPKs This CL also moves WebappActivity's TabDelegateFactory - WebappDelegateFactory - to its own file so that WebApkActivity can extend WebappDelegateFactory. BUG=609122, 614336 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Upstream: Do not show the add-to-homescreen/install-native-app infobar for WebAPKs This CL also moves WebappActivity's TabDelegateFactory - WebappDelegateFactory - to its own file so that WebApkActivity can extend WebappDelegateFactory. BUG=609122, 614336 ========== to ========== Upstream: Do not show the add-to-homescreen/install-native-app infobar for WebAPKs This CL also moves WebappActivity's TabDelegateFactory - WebappDelegateFactory - to its own file so that WebApkActivity can extend WebappDelegateFactory. BUG=609122, 614336 Committed: https://crrev.com/59fbc602e64fe0999df7d3c428c0bfd656930932 Cr-Commit-Position: refs/heads/master@{#397439} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/59fbc602e64fe0999df7d3c428c0bfd656930932 Cr-Commit-Position: refs/heads/master@{#397439} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
