Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2160513002:
Extract AppBannerDataFetcher into an InstallableManager. (Closed)
DescriptionExtract 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
Committed: https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254
Cr-Commit-Position: refs/heads/master@{#409478}
Patch Set 1 #Patch Set 2 : Add explicit default constructor for InstallableParams #Patch Set 3 : Uninitialized pointer #Patch Set 4 : Fix uint32_t coerce to bool #Patch Set 5 : Better DCHECKs #Patch Set 6 : Use pending_tasks_ vector and invoke callbacks directly #
Total comments: 17
Patch Set 7 : Addressing reviewer comments #
Total comments: 11
Patch Set 8 : NO_ERROR is a macro on Windows >.< #Patch Set 9 : Extract refcounted InstallableDataFetcher #Patch Set 10 : Revert to a non-refcounted implementation #
Total comments: 31
Patch Set 11 : Addressing comments #Patch Set 12 : Remove RunCallbacks loop, remove pending_tasks_, clean up #Patch Set 13 : Remove running_callback_, add nested test #Patch Set 14 : s/checker/manager; collapse valid manifest + SW into one #
Total comments: 34
Patch Set 15 : Addressing comments #
Total comments: 4
Patch Set 16 : Addressing nits #
Total comments: 11
Patch Set 17 : Addressing comments #Dependent Patchsets: Messages
Total messages: 104 (75 generated)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Extract AppBannerDataFetcher into an InstallableChecker. 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. This CL introduces an InstallableChecker, 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 InstallableChecker, but each resource will only be retrieved once before being cached on the object in the browser process. This will reduce code duplication and reduce the number of IPCs required for checking installability. Future CLs will: - replace AppBannerDataFetchers with the InstallableChecker, and consolidate platform-dependent app banner features in Manager code - replace manifest/icon fetching in add to homescreen code with a call to the InstallableChecker - introduce new UMA metrics for banners using the new installable::ErrorCode enum - improve app banner testing by mocking out the InstallableChecker (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ========== to ========== Extract AppBannerDataFetcher into an InstallableChecker. 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. This CL introduces an InstallableChecker, 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 InstallableChecker, but each resource will only be retrieved once before being cached on the object in the browser process. This will reduce code duplication and reduce the number of IPCs required for checking installability. Future CLs will: - replace AppBannerDataFetchers with the InstallableChecker, and consolidate platform-dependent app banner features in Manager code - replace manifest/icon fetching in add to homescreen code with a call to the InstallableChecker - introduce new UMA metrics for banners using the new installable::ErrorCode enum - improve app banner testing by mocking out the InstallableChecker (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ==========
dominickn@chromium.org changed reviewers: + benwells@chromium.org, dfalcantara@chromium.org
Description was changed from ========== Extract AppBannerDataFetcher into an InstallableChecker. 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. This CL introduces an InstallableChecker, 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 InstallableChecker, but each resource will only be retrieved once before being cached on the object in the browser process. This will reduce code duplication and reduce the number of IPCs required for checking installability. Future CLs will: - replace AppBannerDataFetchers with the InstallableChecker, and consolidate platform-dependent app banner features in Manager code - replace manifest/icon fetching in add to homescreen code with a call to the InstallableChecker - introduce new UMA metrics for banners using the new installable::ErrorCode enum - improve app banner testing by mocking out the InstallableChecker (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ========== to ========== Extract AppBannerDataFetcher into an InstallableChecker. 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. This CL introduces an InstallableChecker, 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 InstallableChecker, 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. Future CLs will: - replace AppBannerDataFetchers with the InstallableChecker, and consolidate platform-dependent app banner features in Manager code - replace manifest/icon fetching in add to homescreen code with a call to the InstallableChecker - introduce new UMA metrics for banners using the new installable::ErrorCode enum - improve app banner testing by mocking out the InstallableChecker (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ==========
Description was changed from ========== Extract AppBannerDataFetcher into an InstallableChecker. 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. This CL introduces an InstallableChecker, 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 InstallableChecker, 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. Future CLs will: - replace AppBannerDataFetchers with the InstallableChecker, and consolidate platform-dependent app banner features in Manager code - replace manifest/icon fetching in add to homescreen code with a call to the InstallableChecker - introduce new UMA metrics for banners using the new installable::ErrorCode enum - improve app banner testing by mocking out the InstallableChecker (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ========== to ========== Extract AppBannerDataFetcher into an InstallableChecker. 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 InstallableChecker, 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 InstallableChecker, 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. Future CLs will: - replace AppBannerDataFetchers with the InstallableChecker, and consolidate platform-dependent app banner features in Manager code - replace manifest/icon fetching in add to homescreen code with a call to the InstallableChecker - introduce new UMA metrics for banners using the new installable::ErrorCode enum - improve app banner testing by mocking out the InstallableChecker (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dominickn@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_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Extract AppBannerDataFetcher into an InstallableChecker. 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 InstallableChecker, 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 InstallableChecker, 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. Future CLs will: - replace AppBannerDataFetchers with the InstallableChecker, and consolidate platform-dependent app banner features in Manager code - replace manifest/icon fetching in add to homescreen code with a call to the InstallableChecker - introduce new UMA metrics for banners using the new installable::ErrorCode enum - improve app banner testing by mocking out the InstallableChecker (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ========== to ========== Extract AppBannerDataFetcher into an InstallableChecker. 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 InstallableChecker, 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 InstallableChecker, 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. Future CLs will: - replace AppBannerDataFetchers with the InstallableChecker, and consolidate platform-dependent app banner features in Manager code (https://crrev.com/2156113002) - replace manifest/icon fetching in add to homescreen code with a call to the InstallableChecker - introduce new UMA metrics for banners using the new installable::ErrorCode enum - improve app banner testing by mocking out the InstallableChecker (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ==========
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Extract AppBannerDataFetcher into an InstallableChecker. 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 InstallableChecker, 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 InstallableChecker, 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. Future CLs will: - replace AppBannerDataFetchers with the InstallableChecker, and consolidate platform-dependent app banner features in Manager code (https://crrev.com/2156113002) - replace manifest/icon fetching in add to homescreen code with a call to the InstallableChecker - introduce new UMA metrics for banners using the new installable::ErrorCode enum - improve app banner testing by mocking out the InstallableChecker (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ========== to ========== Extract AppBannerDataFetcher into an InstallableChecker. 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 InstallableChecker, 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 InstallableChecker, 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. Future CLs will: - replace AppBannerDataFetchers with the InstallableChecker, and consolidate platform-dependent app banner features in Manager code (https://crrev.com/2156113002) - replace manifest/icon fetching in add to homescreen code with a call to the InstallableChecker - introduce new UMA metrics for banners using the new installable::ErrorCode enum - improve app banner testing by mocking out the InstallableChecker (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ==========
The CQ bit was checked by dominickn@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...
PTAL, thanks! (Also I'm sorry) This is the first of two very large patches which extract AppBannerDataFetcher into an InstallableChecker, and then remove AppBannerDataFetcher. CC list: FYI. See the browser test or crrev.com/2156113002 for examples of how to use the InstallableChecker.
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 dominickn@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...
@dfalcantara: let me know if you'd also like to be an OWNER of installable/
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_...)
Haven't gone through the test yet, but I figure I should send out this bit since I've been poking you about things over chat. https://chromiumcodereview.appspot.com/2160513002/diff/100001/chrome/browser/... File chrome/browser/installable/installable_checker.cc (right): https://chromiumcodereview.appspot.com/2160513002/diff/100001/chrome/browser/... chrome/browser/installable/installable_checker.cc:27: const int kIconMinimumSizeInPx = 144; Always weird to see this number in C++ land... Completely disconnects it from the reason why it's defined as 48 * 3, too. https://chromiumcodereview.appspot.com/2160513002/diff/100001/chrome/browser/... chrome/browser/installable/installable_checker.cc:144: // which will terminate the current task and run callbacks. Is there any way to enforce that FetchResource() is called? https://chromiumcodereview.appspot.com/2160513002/diff/100001/chrome/browser/... chrome/browser/installable/installable_checker.cc:157: if (manifest_error_ != NoErrorDetected) should we do else if all the way down? https://chromiumcodereview.appspot.com/2160513002/diff/100001/chrome/browser/... chrome/browser/installable/installable_checker.cc:183: (!params.has_valid_webapp_manifest || HasFlag(MANIFEST_VALIDATED)) && just reading the line straight makes it seem like !has_valid_webapp_manifest needs to be true for IsComplete to pass. maybe it should be check_has_valid_webapp_manifest, or params_to_check.has_valid_webapp_manifest or something? https://chromiumcodereview.appspot.com/2160513002/diff/100001/chrome/browser/... chrome/browser/installable/installable_checker.cc:232: if (!IsActive() || IsComplete(it->first)) { Can you add a comment here describing the scenario we talked about offline? It's a little confusing to see that the same icon_.get() and such can be assigned to multiple InstallableResults when really only one task will be using it. Might be less confusing if you check the InstallableParams before stuffing the result with things it didn't ask for. https://chromiumcodereview.appspot.com/2160513002/diff/100001/chrome/browser/... chrome/browser/installable/installable_checker.cc:245: } else { indentation here is wonky https://chromiumcodereview.appspot.com/2160513002/diff/100001/chrome/browser/... File chrome/browser/installable/installable_logging.h (right): https://chromiumcodereview.appspot.com/2160513002/diff/100001/chrome/browser/... chrome/browser/installable/installable_logging.h:22: enum ErrorCode { I think enums are generally easier to read when defined as CAPS_WITH_UNDERSCORES, or as kEnumName. We don't seem to be consistent on which one gets used in chromium, but it makes it easier to see that they're constants if you follow one convention or the other.
Overall looks a lot better than before. The test does seem like it's checking some very specific bits, though. Thought tests were encouraged to do black-box testing, but I don't know how you'd do that for a big state machine.
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Thanks! Regarding the test, it's a bit painful, but I think it's pretty important to test the internal state here since this is a big state machine. https://codereview.chromium.org/2160513002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_checker.cc (right): https://codereview.chromium.org/2160513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:27: const int kIconMinimumSizeInPx = 144; On 2016/07/20 22:21:35, dfalcantara wrote: > Always weird to see this number in C++ land... Completely disconnects it from > the reason why it's defined as 48 * 3, too. So this was set at 128 for desktop platforms, and I figured that 128 is so close to 144 that they can all share the same constant for now. It drastically simplifies some of the plumbing and what numbers have to be passed around. I'll add an explanatory comment on this. https://codereview.chromium.org/2160513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:144: // which will terminate the current task and run callbacks. On 2016/07/20 22:21:35, dfalcantara wrote: > Is there any way to enforce that FetchResource() is called? This is tricky, because it should call FetchResource() or Start() (clarified comment). I'm not sure if it'll be any clearer to enforce the call though... https://codereview.chromium.org/2160513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:157: if (manifest_error_ != NoErrorDetected) On 2016/07/20 22:21:35, dfalcantara wrote: > should we do else if all the way down? I didn't because every branch returns (saves a word each line!). Do you mind? https://codereview.chromium.org/2160513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:183: (!params.has_valid_webapp_manifest || HasFlag(MANIFEST_VALIDATED)) && On 2016/07/20 22:21:35, dfalcantara wrote: > just reading the line straight makes it seem like !has_valid_webapp_manifest > needs to be true for IsComplete to pass. maybe it should be > check_has_valid_webapp_manifest, or params_to_check.has_valid_webapp_manifest or > something? Changed to params.check_* https://codereview.chromium.org/2160513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:232: if (!IsActive() || IsComplete(it->first)) { On 2016/07/20 22:21:35, dfalcantara wrote: > Can you add a comment here describing the scenario we talked about offline? > It's a little confusing to see that the same icon_.get() and such can be > assigned to multiple InstallableResults when really only one task will be using > it. Might be less confusing if you check the InstallableParams before stuffing > the result with things it didn't ask for. Done. https://codereview.chromium.org/2160513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:245: } else { On 2016/07/20 22:21:35, dfalcantara wrote: > indentation here is wonky Done. https://codereview.chromium.org/2160513002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_logging.h (right): https://codereview.chromium.org/2160513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_logging.h:22: enum ErrorCode { On 2016/07/20 22:21:35, dfalcantara wrote: > I think enums are generally easier to read when defined as > CAPS_WITH_UNDERSCORES, or as kEnumName. We don't seem to be consistent on which > one gets used in chromium, but it makes it easier to see that they're constants > if you follow one convention or the other. Made CAPS_WITH_UNDERSCORES.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
lgtm, thanks! https://chromiumcodereview.appspot.com/2160513002/diff/100001/chrome/browser/... File chrome/browser/installable/installable_checker.cc (right): https://chromiumcodereview.appspot.com/2160513002/diff/100001/chrome/browser/... chrome/browser/installable/installable_checker.cc:27: const int kIconMinimumSizeInPx = 144; On 2016/07/21 00:24:47, dominickn wrote: > On 2016/07/20 22:21:35, dfalcantara wrote: > > Always weird to see this number in C++ land... Completely disconnects it from > > the reason why it's defined as 48 * 3, too. > > So this was set at 128 for desktop platforms, and I figured that 128 is so close > to 144 that they can all share the same constant for now. It drastically > simplifies some of the plumbing and what numbers have to be passed around. I'll > add an explanatory comment on this. Acknowledged. https://chromiumcodereview.appspot.com/2160513002/diff/100001/chrome/browser/... chrome/browser/installable/installable_checker.cc:144: // which will terminate the current task and run callbacks. On 2016/07/21 00:24:47, dominickn wrote: > On 2016/07/20 22:21:35, dfalcantara wrote: > > Is there any way to enforce that FetchResource() is called? > > This is tricky, because it should call FetchResource() or Start() (clarified > comment). I'm not sure if it'll be any clearer to enforce the call though... Acknowledged. https://chromiumcodereview.appspot.com/2160513002/diff/100001/chrome/browser/... chrome/browser/installable/installable_checker.cc:157: if (manifest_error_ != NoErrorDetected) On 2016/07/21 00:24:47, dominickn wrote: > On 2016/07/20 22:21:35, dfalcantara wrote: > > should we do else if all the way down? > > I didn't because every branch returns (saves a word each line!). Do you mind? Nah, keep it the way you like.
The CQ bit was checked by dominickn@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...)
Initial comments, mainly on comments :) I want to talk to you about the structure of the main checker, I'm finding it a bit monolithic / state machiney, and am wondering if we can break it up a bit somehow. https://codereview.chromium.org/2160513002/diff/120001/chrome/browser/install... File chrome/browser/installable/installable_checker.h (right): https://codereview.chromium.org/2160513002/diff/120001/chrome/browser/install... chrome/browser/installable/installable_checker.h:21: namespace installable { Do we need the namespace? All the classes in this file are prefixed with Installable anyway. https://codereview.chromium.org/2160513002/diff/120001/chrome/browser/install... chrome/browser/installable/installable_checker.h:30: // required for all other resources. This confused me. Maybe instead of putting this comment here, put it in the comment for |check_valid_webapp_manifest| and rephrase as something like 'the existence of the manifest is always checked regardless of this boolean'. https://codereview.chromium.org/2160513002/diff/120001/chrome/browser/install... chrome/browser/installable/installable_checker.h:54: // parameters, and that the icon can be fetched and isn't an empty bitmap. The icon size checking seems a bit confusing. Why is it checked here (using the icon size parameters) and in check_valid_webapp_manifest (using a constant 144x144)? https://codereview.chromium.org/2160513002/diff/120001/chrome/browser/install... chrome/browser/installable/installable_checker.h:57: // The default constructor sets all bool parameters are set to false. Wanna just use defaults on the parameters instead of bothering with a default constructor? https://codereview.chromium.org/2160513002/diff/120001/chrome/browser/install... chrome/browser/installable/installable_checker.h:61: // This struct is passed to an InstallableCallback when the InstallableChecker So the InstallableChecker is also an InstallableInfoFetcher? From all the naming (e.g. the check_* names in the Params struct above) that isn't obvious. https://codereview.chromium.org/2160513002/diff/120001/chrome/browser/install... chrome/browser/installable/installable_checker.h:99: using InstallableCallback = base::Callback<void(const InstallableResult&)>; Can we split this out into two callbacks / functions? One to find out if the thing is installable, and one to get the stuff you're interested in?
The CQ bit was checked by dominickn@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...
dfalcantara, benwells - please take another look. This has shifted right around now based on a discussion with benwells. The bulk of the InstallableChecker is now an InstallableDataFetcher, which is refcounted like AppBannerDataFetcher. The idea is that clients get a refptr to the fetcher, and query that. On navigation, clients simply reset the refptr and it disappears, taking care of subtle race conditions. The bulk of the logic remains the same. https://codereview.chromium.org/2160513002/diff/120001/chrome/browser/install... File chrome/browser/installable/installable_checker.h (right): https://codereview.chromium.org/2160513002/diff/120001/chrome/browser/install... chrome/browser/installable/installable_checker.h:21: namespace installable { On 2016/07/21 06:52:47, benwells wrote: > Do we need the namespace? All the classes in this file are prefixed with > Installable anyway. Removed. https://codereview.chromium.org/2160513002/diff/120001/chrome/browser/install... chrome/browser/installable/installable_checker.h:30: // required for all other resources. On 2016/07/21 06:52:47, benwells wrote: > This confused me. Maybe instead of putting this comment here, put it in the > comment for |check_valid_webapp_manifest| and rephrase as something like 'the > existence of the manifest is always checked regardless of this boolean'. Thinned. https://codereview.chromium.org/2160513002/diff/120001/chrome/browser/install... chrome/browser/installable/installable_checker.h:54: // parameters, and that the icon can be fetched and isn't an empty bitmap. On 2016/07/21 06:52:47, benwells wrote: > The icon size checking seems a bit confusing. Why is it checked here (using the > icon size parameters) and in check_valid_webapp_manifest (using a constant > 144x144)? The 144px is a minimum, but platforms may want a higher resolution icon depending on scale factors and the like. There's a TODO to try and fix this mess up. https://codereview.chromium.org/2160513002/diff/120001/chrome/browser/install... chrome/browser/installable/installable_checker.h:57: // The default constructor sets all bool parameters are set to false. On 2016/07/21 06:52:47, benwells wrote: > Wanna just use defaults on the parameters instead of bothering with a default > constructor? Done. https://codereview.chromium.org/2160513002/diff/120001/chrome/browser/install... chrome/browser/installable/installable_checker.h:99: using InstallableCallback = base::Callback<void(const InstallableResult&)>; On 2016/07/21 06:52:47, benwells wrote: > Can we split this out into two callbacks / functions? One to find out if the > thing is installable, and one to get the stuff you're interested in? I think this comment is redundant based on the redesign.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #9 (id:150001) has been deleted
Still LGTM on my end.
Er, any reason why you keep flipping back and forth?
On 2016/07/22 17:29:32, dfalcantara wrote: > Er, any reason why you keep flipping back and forth? benwells@ and I are going back and forth on the best way to do this. I prefer the way I originally submitted and I think I've almost convinced him. :)
On 2016/07/24 23:44:44, dominickn wrote: > On 2016/07/22 17:29:32, dfalcantara wrote: > > Er, any reason why you keep flipping back and forth? > > benwells@ and I are going back and forth on the best way to do this. I prefer > the way I originally submitted and I think I've almost convinced him. :) You don't have to convince me. I'll review it as is.
Sorry I'm so slow with this. This is a first round, as I'm sheriffing I'll do the review in bits and pieces. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... File chrome/browser/installable/installable_checker.h (right): https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:44: bool check_valid_icon = false; Does this also fetch the icon? From the name alone you'd think not, but then I'm not sure what else would. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:79: class InstallableChecker This name still doesn't seem great, as it is doing more than just checking. It's fetching as well. The name InstallableResult above also feels kinda incomplete, it also seems to miss out the fact that it has real data. Result just feels like the result of the check. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:106: // the cached resources. |callback| should defer rexpensive tasks to later as s/rexpensive/expensive https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:107: // it will be directly called. Why is important not to do expensive work? https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:122: enum StatusFlag : uint32_t { I'm guessing DORMANT and STARTED are mutually exclusive. What about RUNNING_CALLBACKS? It's a bit confusing to have these mixed in with the other things ... some are about what the Checker is doing, and some are about what the Checker has done, if that makes sense. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:136: // Returns true if the icon size in |params| matches the current request. What is the current request in this context? Does it mean the icon that has been fetched already, or the current task being run? https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:152: bool IsRunning(content::WebContents* web_contents); This seems weird. Why would looking if something is running cause a processing error? https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:221: // for binding callbacks. We use a factory so we can invalidate pointers. I don't understand the first sentence of this comment.
next bunch of stuff https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... File chrome/browser/installable/installable_checker.cc (right): https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:135: // Clear the STARTED flag to signal that we should stop work immediately. Reading this it feels like STARTED could be named better. Maybe FETCHING? CHECKING? DOING_STUFF? https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:203: icon_error_ = NO_ERROR_DETECTED; It feels a bit kludgy to have all these error variables. One kludginess is would be easy to get into a state where icon_error_ is something like 'no service worker', which is semantically wrong. Do we really need to have all these variables? Can we just have one error and always return that? I think processing error should definitely be pulled out as it feels like a completely different class of error. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:228: SetFlag(RUNNING_CALLBACKS); Why does this all have to be wrapped in this? https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:231: if (!IsActive() || IsComplete(params)) { Why check IsActive for each task? Can it change in this loop? https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:242: // icon object before the callback gets a chance to use it. This wouldn't be a problem if you kept a map of icon sizes to icons. Did you consider that? https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:293: // StartTask, which will clear the current task. Otherwise, if we fall through The interaction of FetchResource, StartTask, and the flags is kind of confusing. As is the big if statement at the end here - StartTask is at the beginning and the end of it, which seems pretty strange. Oh ... I think I get it now: StartTask - Task here is the specific Task thing, not a generic thing FetchResource is kind of like StartNextSubTask. Is that right? Maybe we could split StartTask into StartTask and FinishTask? It seems to finish the previous thing as part of StartTask... https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... File chrome/browser/installable/installable_checker.h (right): https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:87: bool IsActive() const { return HasFlag(STARTED); } I don't think this function is worth the indirection. Reading the .cc file I had to come back to see what Active meant, as it is counterintuitive that it doesn't include RUNNING_CALLBACKS.
The CQ bit was checked by dominickn@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 dominickn@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.
PTAL - #ps11 is an intermediate step, while #ps12 is a bigger change that removes a bunch of complexity. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... File chrome/browser/installable/installable_checker.cc (right): https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:135: // Clear the STARTED flag to signal that we should stop work immediately. On 2016/07/26 07:27:13, benwells wrote: > Reading this it feels like STARTED could be named better. Maybe FETCHING? > CHECKING? DOING_STUFF? It's now a separate boolean flag is_active_. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:203: icon_error_ = NO_ERROR_DETECTED; On 2016/07/26 07:27:13, benwells wrote: > It feels a bit kludgy to have all these error variables. One kludginess is would > be easy to get into a state where icon_error_ is something like 'no service > worker', which is semantically wrong. > > Do we really need to have all these variables? Can we just have one error and > always return that? > > I think processing error should definitely be pulled out as it feels like a > completely different class of error. Having a separate error value for each type is important to make sure clients get back the right error. Imagine that you had one task asking for the manifest + icon + SW, and another asking for manifest + icon, and another asking for manifest only. Let's say there's no SW and no icon. With a single error variable, you remember that there's no SW and stop there. But when you ask for the icon, then you have to separately remember that there's no icon, because a future task could come along and ask for the SW again. And then when you ask for the manifest, you have stored this error about the SW. Preserving the single error enum is also important, as handling error logging and reporting gets really ugly otherwise. I've deleted processing_error. This is now refactored so that there is a wrapping struct around each thing which contains the error. WDYT? https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:228: SetFlag(RUNNING_CALLBACKS); On 2016/07/26 07:27:13, benwells wrote: > Why does this all have to be wrapped in this? Clarified. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:231: if (!IsActive() || IsComplete(params)) { On 2016/07/26 07:27:13, benwells wrote: > Why check IsActive for each task? Can it change in this loop? Removed. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:242: // icon object before the callback gets a chance to use it. On 2016/07/26 07:27:13, benwells wrote: > This wouldn't be a problem if you kept a map of icon sizes to icons. Did you > consider that? I thought you didn't like that idea because of the references-to-a-map problem? I started implementing this a while ago and didn't really like how it turned out, but it's here for your thoughts now. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.cc:293: // StartTask, which will clear the current task. Otherwise, if we fall through On 2016/07/26 07:27:13, benwells wrote: > The interaction of FetchResource, StartTask, and the flags is kind of confusing. > As is the big if statement at the end here - StartTask is at the beginning and > the end of it, which seems pretty strange. > > > Oh ... I think I get it now: > StartTask - Task here is the specific Task thing, not a generic thing > FetchResource is kind of like StartNextSubTask. > > Is that right? > > Maybe we could split StartTask into StartTask and FinishTask? It seems to finish > the previous thing as part of StartTask... I've renamed StartTask -> StartNextTask, renamed FetchResource to WorkOnTask, and reshuffled the if statement to make it clearer. I've also made RunCallbacks -> RunCallback, removing the loop over the entire task list. That lets me get rid of pending_tasks_ as well, simplifying it all down. WDYT? https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... File chrome/browser/installable/installable_checker.h (right): https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:44: bool check_valid_icon = false; On 2016/07/26 03:28:46, benwells wrote: > Does this also fetch the icon? From the name alone you'd think not, but then I'm > not sure what else would. Renamed to fetch. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:79: class InstallableChecker On 2016/07/26 03:28:46, benwells wrote: > This name still doesn't seem great, as it is doing more than just checking. It's > fetching as well. > > The name InstallableResult above also feels kinda incomplete, it also seems to > miss out the fact that it has real data. Result just feels like the result of > the check. On the name of the checker... I settled on checker because it seemed to best capture that this was fetching, verifying, and caching. Data fetcher felt like too much emphasis on fetching on not enough on the caching/verifying. Perhaps InstallableVendor? InstallableManager? https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:87: bool IsActive() const { return HasFlag(STARTED); } On 2016/07/26 07:27:14, benwells wrote: > I don't think this function is worth the indirection. Reading the .cc file I had > to come back to see what Active meant, as it is counterintuitive that it doesn't > include RUNNING_CALLBACKS. It's gone. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:106: // the cached resources. |callback| should defer rexpensive tasks to later as On 2016/07/26 03:28:46, benwells wrote: > s/rexpensive/expensive Done. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:107: // it will be directly called. On 2016/07/26 03:28:45, benwells wrote: > Why is important not to do expensive work? It will delay any other work that the checker is doing, since this is invoked while the checker is running as opposed to in a PostTask. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:122: enum StatusFlag : uint32_t { On 2016/07/26 03:28:45, benwells wrote: > I'm guessing DORMANT and STARTED are mutually exclusive. What about > RUNNING_CALLBACKS? It's a bit confusing to have these mixed in with the other > things ... some are about what the Checker is doing, and some are about what the > Checker has done, if that makes sense. DORMANT is meant to be the empty state. Strictly speaking, DORMANT means nothing is fetched, nothing is cached, the checker is in an empty state. Whereas you could finish a fetch, and thus not be STARTED, but you still have everything cached and ready to return immediately if something asks for it. Does that make sense? Anyway, I've removed this state enum due to some bigger refactoring below. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:136: // Returns true if the icon size in |params| matches the current request. On 2016/07/26 03:28:46, benwells wrote: > What is the current request in this context? Does it mean the icon that has been > fetched already, or the current task being run? Clarified. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:152: bool IsRunning(content::WebContents* web_contents); On 2016/07/26 03:28:46, benwells wrote: > This seems weird. Why would looking if something is running cause a processing > error? processing_error is gone. I've renamed the method IsContentsValid for clarity. https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:221: // for binding callbacks. We use a factory so we can invalidate pointers. On 2016/07/26 03:28:46, benwells wrote: > I don't understand the first sentence of this comment. Removed.
The CQ bit was checked by dominickn@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...
Update: ps13 removes running_callback_ since changes to how is_active_ is used make it unnecessary. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This is getting a lot nicer! I'm still wondering why we need to have the errors tracked separately. There are currently four classes of errors: - manifest fetching - manifest validity - SW - icon (but not manifest icon). Likewise, I'm not sure if we need two check_ fields on the params, we could probably just have check_installability. I'm thinking clients will either want to get info (e.g. adding via the menu), and if it isn't there they will find it some other way - so specifics of the error for them aren't important. Or they are checking installability as a whole, in which case there isn't a need to track the errors separately. Does that make sense? https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... File chrome/browser/installable/installable_checker.h (right): https://codereview.chromium.org/2160513002/diff/190001/chrome/browser/install... chrome/browser/installable/installable_checker.h:79: class InstallableChecker On 2016/07/28 00:36:27, dominickn wrote: > On 2016/07/26 03:28:46, benwells wrote: > > This name still doesn't seem great, as it is doing more than just checking. > It's > > fetching as well. > > > > The name InstallableResult above also feels kinda incomplete, it also seems to > > miss out the fact that it has real data. Result just feels like the result of > > the check. > > On the name of the checker... I settled on checker because it seemed to best > capture that this was fetching, verifying, and caching. Data fetcher felt like > too much emphasis on fetching on not enough on the caching/verifying. Perhaps > InstallableVendor? InstallableManager? While I don't like calling things Manager, I think it might be the best of the bunch.
The CQ bit was checked by dominickn@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...
PTAL, thanks. Checker is now manager. I still think it's important for consistency to have the error state monitored per resource; the complexity is going down as there are fewer types of things that can go wrong now. Particularly because errors with the icon downloading should be distinct from error of manifest validity/SW checking.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looking great and getting really close... https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:166: if (!web_contents || !is_active_) Why are the contents not valid if the manager isn't active? https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:200: is_active_ = true; Nit: I think you can delete this line, maybe add DCHECK(is_active_) instead. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:256: Reset(); Should you reset here, or just return? https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:305: installable_prop_.value = false; Is there an error to be set in this case? Oh, hang on - this should be NOTREACHED, if you're already checking it in IsManifestValidForWebApp. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:313: Reset(); Ditto about reset vs return. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:59: // nullptr if the most appropriate icon couldn't be determined or downloaded. Nit: add comment about ownership. Also mention under what cases the reason would be in error_code. Something like, if fetch_valid_icon is true and there was a problem with the icon, problem will be in the error_code. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:62: // true if the site has a service worker and a viable web app manifest. Nit: mention that if it is not installable the reason will be in error_code. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:70: // manifest if it exists; optionally the full installability criteria can be Nit: you can probably remove the second sentence of this comment. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:89: // passing an InstallableData struct containing the requested resources. s/Run the installable check/Get the installable data/ s/when the checks are complete/when the data is ready/ Or something like that. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:94: // will not perform duplicate work and will run the callback immediately with Will the callback ever be run synchronously / before GetData exits? If so that is worth noting. I think earlier versions of this code did not make the guarantee about |callback| being invoked before the next one is processed. Is it worth making that guarantee? It might make it harder to change the internals later. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:109: using IconMap = std::map<IconParams, IconProperty>; Nit: consider not having Tasks / IconMap but just having std::vector / std::map in the field declaration. These types are only used there so the extra indirection doesn't buy much. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:114: // most recently been requested. false if no icon has been requested yet or s/most recently/already/ I thikn that is more correct now you have the map. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... File chrome/browser/installable/installable_property.h (right): https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_property.h:17: struct ManifestProperty { Any reason to move this into its own file? Seems like they're just an implementation detail of InstallableManager. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_property.h:18: ManifestProperty(); Can you just have default values for everything, instead of a constructor? https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_property.h:19: void Reset(); You could get rid of Reset on these by using unique_ptrs on the Manager to them, and just resetting that. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_property.h:54: DISALLOW_COPY_AND_ASSIGN(IconProperty); Why have disallow copy and assign on this? Seems like you have manually added them back. Also doesn't seem much reason to make the other two Property classes non-copy / assignable.
Looking great and getting really close... https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:166: if (!web_contents || !is_active_) Why are the contents not valid if the manager isn't active? https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:200: is_active_ = true; Nit: I think you can delete this line, maybe add DCHECK(is_active_) instead. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:256: Reset(); Should you reset here, or just return? https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:305: installable_prop_.value = false; Is there an error to be set in this case? Oh, hang on - this should be NOTREACHED, if you're already checking it in IsManifestValidForWebApp. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:313: Reset(); Ditto about reset vs return. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:59: // nullptr if the most appropriate icon couldn't be determined or downloaded. Nit: add comment about ownership. Also mention under what cases the reason would be in error_code. Something like, if fetch_valid_icon is true and there was a problem with the icon, problem will be in the error_code. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:62: // true if the site has a service worker and a viable web app manifest. Nit: mention that if it is not installable the reason will be in error_code. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:70: // manifest if it exists; optionally the full installability criteria can be Nit: you can probably remove the second sentence of this comment. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:89: // passing an InstallableData struct containing the requested resources. s/Run the installable check/Get the installable data/ s/when the checks are complete/when the data is ready/ Or something like that. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:94: // will not perform duplicate work and will run the callback immediately with Will the callback ever be run synchronously / before GetData exits? If so that is worth noting. I think earlier versions of this code did not make the guarantee about |callback| being invoked before the next one is processed. Is it worth making that guarantee? It might make it harder to change the internals later. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:109: using IconMap = std::map<IconParams, IconProperty>; Nit: consider not having Tasks / IconMap but just having std::vector / std::map in the field declaration. These types are only used there so the extra indirection doesn't buy much. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:114: // most recently been requested. false if no icon has been requested yet or s/most recently/already/ I thikn that is more correct now you have the map. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... File chrome/browser/installable/installable_property.h (right): https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_property.h:17: struct ManifestProperty { Any reason to move this into its own file? Seems like they're just an implementation detail of InstallableManager. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_property.h:18: ManifestProperty(); Can you just have default values for everything, instead of a constructor? https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_property.h:19: void Reset(); You could get rid of Reset on these by using unique_ptrs on the Manager to them, and just resetting that. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_property.h:54: DISALLOW_COPY_AND_ASSIGN(IconProperty); Why have disallow copy and assign on this? Seems like you have manually added them back. Also doesn't seem much reason to make the other two Property classes non-copy / assignable.
The CQ bit was checked by dominickn@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.
PTAL, thanks. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:166: if (!web_contents || !is_active_) On 2016/07/29 04:36:54, benwells wrote: > Why are the contents not valid if the manager isn't active? Deleted the method. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:200: is_active_ = true; On 2016/07/29 04:36:53, benwells wrote: > Nit: I think you can delete this line, maybe add DCHECK(is_active_) instead. Done. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:256: Reset(); On 2016/07/29 04:36:54, benwells wrote: > Should you reset here, or just return? I'm not really sure about the guarantees of WebContents lifecycles. I suppose that WebContentsDestroyed() calls Reset(), so we can just return in these. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:305: installable_prop_.value = false; On 2016/07/29 04:36:53, benwells wrote: > Is there an error to be set in this case? Oh, hang on - this should be > NOTREACHED, if you're already checking it in IsManifestValidForWebApp. Ah yes, now that this method is only called from there this is NOTREACHED(). Updated. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:313: Reset(); On 2016/07/29 04:36:54, benwells wrote: > Ditto about reset vs return. Done. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:59: // nullptr if the most appropriate icon couldn't be determined or downloaded. On 2016/07/29 04:36:54, benwells wrote: > Nit: add comment about ownership. Also mention under what cases the reason would > be in error_code. Something like, if fetch_valid_icon is true and there was a > problem with the icon, problem will be in the error_code. Done. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:62: // true if the site has a service worker and a viable web app manifest. On 2016/07/29 04:36:54, benwells wrote: > Nit: mention that if it is not installable the reason will be in error_code. Done. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:70: // manifest if it exists; optionally the full installability criteria can be On 2016/07/29 04:36:54, benwells wrote: > Nit: you can probably remove the second sentence of this comment. Done. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:89: // passing an InstallableData struct containing the requested resources. On 2016/07/29 04:36:54, benwells wrote: > s/Run the installable check/Get the installable data/ > s/when the checks are complete/when the data is ready/ > > Or something like that. Done. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:94: // will not perform duplicate work and will run the callback immediately with On 2016/07/29 04:36:54, benwells wrote: > Will the callback ever be run synchronously / before GetData exits? If so that > is worth noting. > > I think earlier versions of this code did not make the guarantee about > |callback| being invoked before the next one is processed. Is it worth making > that guarantee? It might make it harder to change the internals later. Done. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:109: using IconMap = std::map<IconParams, IconProperty>; On 2016/07/29 04:36:54, benwells wrote: > Nit: consider not having Tasks / IconMap but just having std::vector / std::map > in the field declaration. These types are only used there so the extra > indirection doesn't buy much. Done. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:114: // most recently been requested. false if no icon has been requested yet or On 2016/07/29 04:36:54, benwells wrote: > s/most recently/already/ > > I thikn that is more correct now you have the map. Done. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... File chrome/browser/installable/installable_property.h (right): https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_property.h:17: struct ManifestProperty { On 2016/07/29 04:36:55, benwells wrote: > Any reason to move this into its own file? Seems like they're just an > implementation detail of InstallableManager. I was trying to keep the size of the manager file down, but I guess it's shrunk again. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_property.h:18: ManifestProperty(); On 2016/07/29 04:36:54, benwells wrote: > Can you just have default values for everything, instead of a constructor? Done. The destructor is forced by chromium-style. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_property.h:19: void Reset(); On 2016/07/29 04:36:54, benwells wrote: > You could get rid of Reset on these by using unique_ptrs on the Manager to them, > and just resetting that. Done. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_property.h:54: DISALLOW_COPY_AND_ASSIGN(IconProperty); On 2016/07/29 04:36:54, benwells wrote: > Why have disallow copy and assign on this? Seems like you have manually added > them back. > > Also doesn't seem much reason to make the other two Property classes non-copy / > assignable. IconProperty has a std::unique_ptr, which is non-copyable, so the struct also should be explicitly non-copyable. Since it's stored in a std::map, I also had to define all of the constructors to make sure it would be move-only and that the map wouldn't call the copy constructor.
Description was changed from ========== Extract AppBannerDataFetcher into an InstallableChecker. 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 InstallableChecker, 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 InstallableChecker, 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. Future CLs will: - replace AppBannerDataFetchers with the InstallableChecker, and consolidate platform-dependent app banner features in Manager code (https://crrev.com/2156113002) - replace manifest/icon fetching in add to homescreen code with a call to the InstallableChecker - introduce new UMA metrics for banners using the new installable::ErrorCode enum - improve app banner testing by mocking out the InstallableChecker (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ========== to ========== 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. 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 - improve app banner testing by mocking out the InstallableManager (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ==========
benwells: ping
lgtm with some nits. thanks Dom! https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:94: // will not perform duplicate work and will run the callback immediately with On 2016/07/31 23:32:04, dominickn wrote: > On 2016/07/29 04:36:54, benwells wrote: > > Will the callback ever be run synchronously / before GetData exits? If so that > > is worth noting. > > > > I think earlier versions of this code did not make the guarantee about > > |callback| being invoked before the next one is processed. Is it worth making > > that guarantee? It might make it harder to change the internals later. > > Done. Nit: I don't think it's clear that the callback can run synchronously (which I thikn it can). this is important to note as it is a bit unusual, and also can cause reentrancy problems if not dealt with properly. https://codereview.chromium.org/2160513002/diff/280001/chrome/browser/install... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2160513002/diff/280001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:57: InstallableManager::ManifestProperty::~ManifestProperty() = default; Nit: is this needed? https://codereview.chromium.org/2160513002/diff/280001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2160513002/diff/280001/chrome/browser/install... chrome/browser/installable/installable_manager.h:106: struct ManifestProperty { Nit: Can these just be fwd declared here? Edit: ah, I see not, the errors are used in tests. It might be worth just exposing the errors (via protected or private functions) instead of the different types.
Description was changed from ========== 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. 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 - improve app banner testing by mocking out the InstallableManager (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ========== to ========== 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 - improve app banner testing by mocking out the InstallableManager (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ==========
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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 - improve app banner testing by mocking out the InstallableManager (i.e. separating testing of resource fetching from testing of banner triggering conditions). BUG=628921 ========== to ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
dominickn@chromium.org changed reviewers: + thestig@chromium.org
Thanks! dfalcantara: any further comments? +thestig for chrome/browser OWNERs. This adds a new directory to chrome/browser. https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2160513002/diff/260001/chrome/browser/install... chrome/browser/installable/installable_manager.h:94: // will not perform duplicate work and will run the callback immediately with On 2016/08/03 01:17:28, benwells wrote: > On 2016/07/31 23:32:04, dominickn wrote: > > On 2016/07/29 04:36:54, benwells wrote: > > > Will the callback ever be run synchronously / before GetData exits? If so > that > > > is worth noting. > > > > > > I think earlier versions of this code did not make the guarantee about > > > |callback| being invoked before the next one is processed. Is it worth > making > > > that guarantee? It might make it harder to change the internals later. > > > > Done. > > Nit: I don't think it's clear that the callback can run synchronously (which I > thikn it can). this is important to note as it is a bit unusual, and also can > cause reentrancy problems if not dealt with properly. Done. https://codereview.chromium.org/2160513002/diff/280001/chrome/browser/install... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2160513002/diff/280001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:57: InstallableManager::ManifestProperty::~ManifestProperty() = default; On 2016/08/03 01:17:28, benwells wrote: > Nit: is this needed? It was at this time (complex structs must have a destructor, enforced by chromium-style). However, now it's inlined in the .cc file this can go. https://codereview.chromium.org/2160513002/diff/280001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2160513002/diff/280001/chrome/browser/install... chrome/browser/installable/installable_manager.h:106: struct ManifestProperty { On 2016/08/03 01:17:28, benwells wrote: > Nit: Can these just be fwd declared here? > > Edit: ah, I see not, the errors are used in tests. It might be worth just > exposing the errors (via protected or private functions) instead of the > different types. Done.
lgtm https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... File chrome/browser/installable/installable_logging.cc (right): https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... chrome/browser/installable/installable_logging.cc:14: static const std::string& GetMessagePrefix() { no need for static inside an anonymous namespace. https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... chrome/browser/installable/installable_logging.cc:63: } // anonymous namespace omit anonymous https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... chrome/browser/installable/installable_logging.cc:65: void LogErrorToConsole(content::WebContents* web_contents, Nobody is calling this yet? https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:305: } else if (manifest_url.is_empty()) { no need for else after a return. https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:404: } else if (bitmap.drawsNothing()) { Ditto
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... File chrome/browser/installable/installable_logging.cc (right): https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... chrome/browser/installable/installable_logging.cc:14: static const std::string& GetMessagePrefix() { On 2016/08/03 06:49:51, Lei Zhang wrote: > no need for static inside an anonymous namespace. Done. https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... chrome/browser/installable/installable_logging.cc:63: } // anonymous namespace On 2016/08/03 06:49:51, Lei Zhang wrote: > omit anonymous Done. https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... chrome/browser/installable/installable_logging.cc:65: void LogErrorToConsole(content::WebContents* web_contents, On 2016/08/03 06:49:51, Lei Zhang wrote: > Nobody is calling this yet? It's being called in the follow-up CL - crrev.com/2156113002. It's implemented here so that (already big) CL doesn't have to make any changes in this new directory. https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:305: } else if (manifest_url.is_empty()) { On 2016/08/03 06:49:51, Lei Zhang wrote: > no need for else after a return. Done. https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:305: } else if (manifest_url.is_empty()) { On 2016/08/03 06:49:51, Lei Zhang wrote: > no need for else after a return. Done. https://codereview.chromium.org/2160513002/diff/300001/chrome/browser/install... chrome/browser/installable/installable_manager.cc:404: } else if (bitmap.drawsNothing()) { On 2016/08/03 06:49:51, Lei Zhang wrote: > Ditto Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org, benwells@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2160513002/#ps320001 (title: "Addressing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254 Cr-Commit-Position: refs/heads/master@{#409478} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/54ca9f2b3d1220955427267319895a8e077c2254 Cr-Commit-Position: refs/heads/master@{#409478} |
