This looks like an internal design request. Let's leave the means of implementation to those providing experience. I assume installability requirements are going to be documented and set in stone so that developers could follow them. If so, if it is easier to hard-code the checks based on the documentation than plumb it through abstractions, implementer should feel free to do so.

This isn't just internal to the devtools codebase. The current implementation of the PWA check represents a maintenance burden for my team (PWAs) because every time we want to change the checking logic, we have to either update devtools ourselves, or file a bug to get your team to change it.

> I assume installability requirements are going to be documented and set in
> stone so that developers could follow them.

No, they won't be, at least for the foreseeable future. We want the flexibility to expand the scope of these checks on an ongoing basis. As an aside, I have been trying to get us to standardize the checks, but it's a complex topic and it won't be resolved in the immediate future.

Regardless, I'd like this logic to be centralized in one place, so developers know for sure whether their site will be installable (at least, in this version of Chrome; it can change in the future). Having a check that just approximates the logic is harmful because developers can't rely on it as an accurate reflection of the browser's installability check.

While we're at it, we could also look at de-duplicating the manifest parsing logic. Dom pointed out that the devtools manifest parser in third_party/blink/renderer/devtools/front_end/resources/AppManifestView.js parses the JSON itself, rather than relying on the more complex parsing logic in content/renderer/manifest/manifest_parser.cc. The latter takes into account a lot more of the rules from the Manifest standard (https://www.w3.org/TR/appmanifest/) such as default scope, which the installability check relies on. Ideally, we could fix the Manifest page in the Applications tab to accurately reflect Chrome's parsing of the manifest, while we're at it (technically a separate bug, but relevant to the fact that DevTools reimplements a lot of the application logic from Chrome).

> The current implementation of the PWA check represents a maintenance burden for my team

Could you point to the CLs from your team or the bugs filed? So far your team was only breaking DevTools, despite the existing shared C++ implementation that you ask for here.

> No, they won't be, at least for the foreseeable future.

That's unfortunate. I understand how things are changing rapidly in the initial phase of design and development, but this is not a new feature. Stability of the API maps to its credibility and adoption.

> at least, in this version of Chrome; it can change in the future

To me, that's not acceptable. Chrome is ever-green and I expect my functionality to work unless there has been an intent to break it. Changes to the contract are as harmful as approximation. We need to have clear guidance in the form of the docs and tooling that supports it.

> manifest parser in third_party/blink/renderer/devtools/front_end/resources/AppManifestView.js parses the JSON itself

You've been mislead, ManifestManagerHost::RequestManifestDebugInfo is used for that.


You assigned this to me, how do you expect me to follow up?

After further discussion offline, we have a path forward:

1. I'll take the bug first and land some form of AppBannerManager::DryRun(base::OnceCallback<void(std::vector<std::string>)> callback) method. This will asynchronously run |callback| with a vector of all installability errors on the page (or an empty vector if there were no problems and the site is installable).

2. When done I'll reassign to pfeldman who will help connect the method from (1) into DevTools.

pfeldman: let me know if taking a callback is the appropriate interface for the devtools protocol.

DryRun(base::OnceCallback) sounds good!

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

commit 72cfd665f98e6ce700e9a59430283e4a566eb669
Author: Dominick Ng <dominickn@chromium.org>
Date: Wed Jan 09 03:12:39 2019

Remove requestAppBanner from the DevTools protocol.

This CL removes the ability to request an app banner from DevTools. This
follows the removal of the explicit link to trigger this action in
https://crrev.com/c/1370371.

Follow-up CLs will implement a new AppBannerManager::DryRun method which
will be accessed by devtools in order to collect all errors from the
installability check to surface to developers.

BUG=915945

Change-Id: I2d95030965474ebb35d6f8882c1ff00eeeb6a29a
Reviewed-on: https://chromium-review.googlesource.com/c/1393126
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621025}
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/chrome/browser/android/tab_web_contents_delegate_android.cc
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/chrome/browser/android/tab_web_contents_delegate_android.h
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/chrome/browser/ui/browser.cc
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/chrome/browser/ui/browser.h
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/components/embedder_support/android/delegate/web_contents_delegate_android.cc
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/components/embedder_support/android/delegate/web_contents_delegate_android.h
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/content/browser/devtools/protocol/page_handler.cc
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/content/browser/devtools/protocol/page_handler.h
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/content/browser/devtools/protocol_config.json
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/content/public/browser/web_contents_delegate.h
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/third_party/blink/renderer/core/inspector/browser_protocol-1.2.json
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/third_party/blink/renderer/core/inspector/browser_protocol-1.3.json
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/third_party/blink/renderer/core/inspector/browser_protocol.pdl
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/third_party/blink/renderer/core/inspector/inspector_protocol_config.json
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/third_party/blink/renderer/devtools/BUILD.gn
[delete] https://crrev.com/5d914d559f58bae7399e184fb466944602ff2dad/third_party/blink/renderer/devtools/front_end/inspector_main/RequestAppBannerActionDelegate.js
[modify] https://crrev.com/72cfd665f98e6ce700e9a59430283e4a566eb669/third_party/blink/renderer/devtools/front_end/inspector_main/module.json