New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 628921 link

Starred by 2 users
Status: Fixed
Owner:
dominickn@chromium.org
Closed: Sep 2016
Cc:
hanxi@chromium.org
pkotw...@chromium.org
█ dfalcant...@chromium.org
benwells@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Replace AppBannerDataFetcher with a reusable InstallableChecker

Project Member Reported by dominickn@chromium.org, Jul 17 2016 
AppBannerDataFetcher currently performs the asynchronous fetching and validating of a site's installability (i.e. whether the site is a PWA). This includes:

 - fetching a manifest
 - checking its properties
 - checking for a service worker
 - fetching an app icon

This runs on every page load on Android. However, the fetched resources are not easily accessible outside the banner code, and more clients wish to do similar checks, including:

 - add to homescreen (manifest + icon)
 - WebAPKs (everything)
 - etc.

The data fetcher functionality will be moved into a new InstallableChecker, which will act as a browser-process WebContents-scoped cache for app installability data. Clients will ask it for resources and provide a callback to be run when the resources are fetched/checked. Data that is requested multiple times will not need to be re-fetched, reducing the number of IPCs between the browser and renderer and reducing code duplication throughout banners, WebAPKs, add to homescreen, and future clients.

Splitting out the fetcher will improve the testability of the app banner code, and help consolidate the platform-specific functionality in the Android/Desktop managers, rather than splitting it between the managers and data fetchers.

Comment 1 by dominickn@chromium.org, Jul 18 2016

Description: Show this description
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 19 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/482eeff448654b3eec603009deee13d402941e31

commit 482eeff448654b3eec603009deee13d402941e31
Author: horo <horo@chromium.org>
Date: Tue Jul 19 23:06:58 2016

Use WebContents::GetManifest in ServiceWorker FetchTest instead of RequestAppBannerFromDevTools

This is blocking the refactoring of AppBannerDataFetcher.
https://codereview.chromium.org/2156113002/

BUG= 628921 

Review-Url: https://codereview.chromium.org/2158253002
Cr-Commit-Position: refs/heads/master@{#406408}

[modify] https://crrev.com/482eeff448654b3eec603009deee13d402941e31/chrome/browser/chrome_service_worker_browsertest.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 3 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/54ca9f2b3d1220955427267319895a8e077c2254

commit 54ca9f2b3d1220955427267319895a8e077c2254
Author: dominickn <dominickn@chromium.org>
Date: Wed Aug 03 08:48:49 2016

Extract AppBannerDataFetcher into an InstallableManager.

Checking that a webapp is installable requires several asynchronous
calls to verify a number of installable criteria, including:
 - the existence of a web app manifest and its validity
 - the existence of a service worker
 - the existence and fetchability of an appropriate icon

Currently, AppBannerDataFetcher performs the fetching and checking of
these resources for displaying app banners. However, new features, such
as WebAPKs, and existing features, such as add to homescreen, cannot
easily reuse the app banner code, leading to duplicated code and
browser->renderer IPCs to repeatedly fetch manifests and icons.
Additionally, the app banner code is complicated by the need to support
native app banners on mobile, which require a manifest but no service
worker or manifest icon. App banners have 3 manager classes and 3
fetcher classes, containing generic, desktop-specific, and
Android-specific functionality respectively.

This CL introduces an InstallableManager which is a
WebContents-scoped version of AppBannerDataFetcher. It allows the
installable criteria to be fetched and cached per WebContents.
Multiple clients can request the same data from the InstallableManager,
but each resource will only be retrieved once before being cached on
the object in the browser process. This will reduce the code
duplication required for checking installability. It will also ensure that
multiple requests for the same data do not cause additional,
extraneous IPCs and network requests.

Future CLs will:
 - replace AppBannerDataFetchers with InstallableManager, and
   consolidate platform-dependent app banner features
   (https://crrev.com/2156113002)
 - replace manifest/icon fetching in add to homescreen code with a
   call to the InstallableManager
 - introduce new UMA metrics for banners using the new
   InstallableErrorCode enum (https://crrev.com/2178833002)
 - improve app banner testing by mocking out the InstallableManager
   (i.e. separating testing of resource fetching from testing of
   banner triggering conditions).

BUG= 628921 

Review-Url: https://codereview.chromium.org/2160513002
Cr-Commit-Position: refs/heads/master@{#409478}

[add] https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254/chrome/browser/installable/OWNERS
[add] https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254/chrome/browser/installable/installable_logging.cc
[add] https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254/chrome/browser/installable/installable_logging.h
[add] https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254/chrome/browser/installable/installable_manager.cc
[add] https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254/chrome/browser/installable/installable_manager.h
[add] https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254/chrome/browser/installable/installable_manager_browsertest.cc
[add] https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254/chrome/browser/installable/installable_manager_unittest.cc
[modify] https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254/chrome/chrome_browser.gypi
[modify] https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254/chrome/chrome_tests.gypi
[modify] https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254/chrome/chrome_tests_unit.gypi
[add] https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254/chrome/test/data/banners/manifest_no_service_worker.html
[modify] https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254/chrome/test/data/banners/play_app_test_page.html
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 10 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d223e62e9459d219141c09ac6b10f983c841fa45

commit d223e62e9459d219141c09ac6b10f983c841fa45
Author: dominickn <dominickn@chromium.org>
Date: Wed Aug 10 04:54:13 2016

Replace AppBannerDataFetcher with InstallableManager.

https://crrev.com/2160513002 introduces the InstallableManager, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableManager. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. Future CLs will introduce further tests and new
metrics with the InstallableErrorCode enum that is now used to
signal error states.

BUG= 628921 
TBR=thestig@chromium.org

Review-Url: https://codereview.chromium.org/2156113002
Cr-Commit-Position: refs/heads/master@{#410959}

[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java
[delete] https://crrev.com/f11a6e6e4134f2145ca9f66e355f8a14f643c1df/chrome/browser/android/banners/app_banner_data_fetcher_android.cc
[delete] https://crrev.com/f11a6e6e4134f2145ca9f66e355f8a14f643c1df/chrome/browser/android/banners/app_banner_data_fetcher_android.h
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/browser/android/banners/app_banner_infobar_delegate_android.h
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/browser/android/banners/app_banner_manager_android.cc
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/browser/android/banners/app_banner_manager_android.h
[delete] https://crrev.com/f11a6e6e4134f2145ca9f66e355f8a14f643c1df/chrome/browser/banners/app_banner_data_fetcher.cc
[delete] https://crrev.com/f11a6e6e4134f2145ca9f66e355f8a14f643c1df/chrome/browser/banners/app_banner_data_fetcher.h
[delete] https://crrev.com/f11a6e6e4134f2145ca9f66e355f8a14f643c1df/chrome/browser/banners/app_banner_data_fetcher_browsertest.cc
[delete] https://crrev.com/f11a6e6e4134f2145ca9f66e355f8a14f643c1df/chrome/browser/banners/app_banner_data_fetcher_desktop.cc
[delete] https://crrev.com/f11a6e6e4134f2145ca9f66e355f8a14f643c1df/chrome/browser/banners/app_banner_data_fetcher_desktop.h
[delete] https://crrev.com/f11a6e6e4134f2145ca9f66e355f8a14f643c1df/chrome/browser/banners/app_banner_data_fetcher_unittest.cc
[delete] https://crrev.com/f11a6e6e4134f2145ca9f66e355f8a14f643c1df/chrome/browser/banners/app_banner_debug_log.cc
[delete] https://crrev.com/f11a6e6e4134f2145ca9f66e355f8a14f643c1df/chrome/browser/banners/app_banner_debug_log.h
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/browser/banners/app_banner_infobar_delegate_desktop.cc
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/browser/banners/app_banner_infobar_delegate_desktop.h
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/browser/banners/app_banner_manager.cc
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/browser/banners/app_banner_manager.h
[add] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/browser/banners/app_banner_manager_browsertest.cc
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/browser/banners/app_banner_manager_desktop.cc
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/browser/banners/app_banner_manager_desktop.h
[delete] https://crrev.com/f11a6e6e4134f2145ca9f66e355f8a14f643c1df/chrome/browser/banners/app_banner_manager_emulation.cc
[delete] https://crrev.com/f11a6e6e4134f2145ca9f66e355f8a14f643c1df/chrome/browser/banners/app_banner_manager_emulation.h
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/browser/banners/app_banner_settings_helper.cc
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/browser/ui/browser.cc
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/chrome_browser.gypi
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/chrome_tests.gypi
[modify] https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45/chrome/chrome_tests_unit.gypi
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 12 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ed8b475808ee068d8d453b78a46fe1d8f63f5662

commit ed8b475808ee068d8d453b78a46fe1d8f63f5662
Author: dominickn <dominickn@chromium.org>
Date: Fri Aug 12 00:36:05 2016

Migrate add to homescreen data fetcher to use InstallableManager.

This CL stops the add to homescreen data fetcher from fetching
a manifest and icon itself, and instead defer that task to the
InstallableManager. This is particularly beneficial for PWAs, which may
have already run the InstallableManager to fetch the manifest and icon
to verify whether they are banner eligible. In this case, the add to
homescreen data fetcher no longer needs to IPC to the renderer process,
as the data it needs is already cached in InstallableManager.

This also paves the way for the WebAPK install flow to be triggered from
the add to homescreen dialog. A future CL will make the add to
homescreen data fetcher check whether a site is a PWA using the
InstallableManager - kicking off the WebAPK flow if so.

This CL also tidies up includes and code style in the data fetcher and
dialog helper.

BUG= 628921 

Review-Url: https://codereview.chromium.org/2230773002
Cr-Commit-Position: refs/heads/master@{#411483}

[modify] https://crrev.com/ed8b475808ee068d8d453b78a46fe1d8f63f5662/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc
[modify] https://crrev.com/ed8b475808ee068d8d453b78a46fe1d8f63f5662/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h
[modify] https://crrev.com/ed8b475808ee068d8d453b78a46fe1d8f63f5662/chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc
[modify] https://crrev.com/ed8b475808ee068d8d453b78a46fe1d8f63f5662/chrome/browser/android/webapps/add_to_homescreen_dialog_helper.h
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 16 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df301a20d16cba20fa4cf459cf4130c1ecf4127c

commit df301a20d16cba20fa4cf459cf4130c1ecf4127c
Author: dominickn <dominickn@chromium.org>
Date: Tue Aug 16 02:32:27 2016

Add new app banner metrics using InstallableStatusCode.

The InstallableManager reports status codes to signal if the checking
pipeline failed in some way. This CL introduces a new UMA metric to
report the value of this code, and introduces new codes to ensure that
every run through the banner pipeline results in precisely one code.
This new metric is intended to be the final source of truth for banners.

Additional testing is added to ensure the correct histogram values are
recorded. The existing metrics will be removed in a future CL.

BUG= 628921 

Review-Url: https://codereview.chromium.org/2178833002
Cr-Commit-Position: refs/heads/master@{#412143}

[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/android/banners/app_banner_manager_android.cc
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/android/banners/app_banner_manager_android.h
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/banners/app_banner_manager.cc
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/banners/app_banner_manager.h
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/banners/app_banner_manager_browsertest.cc
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/banners/app_banner_manager_desktop.cc
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/banners/app_banner_metrics.cc
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/banners/app_banner_metrics.h
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/banners/app_banner_settings_helper.cc
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/banners/app_banner_settings_helper.h
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/banners/app_banner_settings_helper_unittest.cc
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/installable/installable_logging.cc
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/installable/installable_logging.h
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/installable/installable_manager.cc
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/installable/installable_manager.h
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/installable/installable_manager_browsertest.cc
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/chrome/browser/installable/installable_manager_unittest.cc
[modify] https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c/tools/metrics/histograms/histograms.xml

Comment 7 by dominickn@chromium.org, Sep 13 2016

Status: Fixed (was: Started)
►

Sign in to add a comment