Extract AppBannerDataFetcher into an InstallableManager.

chrome/browser/installable/installable_checker.h

Index: chrome/browser/installable/installable_checker.h
diff --git a/chrome/browser/installable/installable_checker.h b/chrome/browser/installable/installable_checker.h
new file mode 100644
index 0000000000000000000000000000000000000000..e0f9ac5b4d0e791a700014df59050dc4570f6dbb
--- /dev/null
+++ b/chrome/browser/installable/installable_checker.h
@@ -0,0 +1,270 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_INSTALLABLE_INSTALLABLE_CHECKER_H_
+#define CHROME_BROWSER_INSTALLABLE_INSTALLABLE_CHECKER_H_
+
+#include <utility>
+
+#include "base/callback_forward.h"
+#include "base/gtest_prod_util.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/installable/installable_logging.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/web_contents_user_data.h"
+#include "content/public/common/manifest.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "url/gurl.h"
+
+namespace installable {

[benwells, 2016/07/21 06:52:47]

Do we need the namespace? All the classes in this file are prefixed with
Installable anyway.

[dominickn, 2016/07/21 09:13:38]

Removed.

+
+// This struct specifies the work to be done by the InstallableChecker on the
+// site loaded in the main frame of the WebContents to which it is attached.
+// Results are cached and evaluated the order specified in this struct; if any
+// previous step is requested and fails, no further steps will be performed.
+// For example, if |check_valid_icon| and |check_service_worker| are both true,
+// then the site's icon will only be fetched if a service worker was found.
+// The checker will *always* look for a web app manifest first as this is
+// required for all other resources.

[benwells, 2016/07/21 06:52:47]

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'.

[dominickn, 2016/07/21 09:13:38]

Thinned.

+struct InstallableParams {
+  // The ideal icon size that should be fetched. Used only if |check_valid_icon|
+  // is true.
+  int ideal_icon_size_in_dp;
+
+  // The minimum icon size that should be fetched. Used only if
+  // |check_valid_icon| is true.
+  int minimum_icon_size_in_dp;
+
+  // Check whether the site's manifest is valid for a web app. That is, the
+  // manifest:
+  //  - is non-empty
+  //  - has a valid start URL
+  //  - has a non-null name OR a non-null short_name
+  //  - has a display field set to "standalone" or "fullscreen"
+  //  - contains a PNG icon of at least 144x144px
+  bool check_valid_webapp_manifest;
+
+  // Check whether the site has an active service worker controlling it and the
+  // manifest start URL.
+  bool check_service_worker;
+
+  // Check whether there is an icon in the manifest conforming to the icon size
+  // parameters, and that the icon can be fetched and isn't an empty bitmap.

[benwells, 2016/07/21 06:52:47]

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)?

[dominickn, 2016/07/21 09:13:38]

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.

+  bool check_valid_icon;
+
+  // The default constructor sets all bool parameters are set to false.

[benwells, 2016/07/21 06:52:47]

Wanna just use defaults on the parameters instead of bothering with a default
constructor?

[dominickn, 2016/07/21 09:13:38]

Done.

+  InstallableParams();
+};
+
+// This struct is passed to an InstallableCallback when the InstallableChecker

[benwells, 2016/07/21 06:52:47]

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.

+// has finished working. Each pointer/reference member is a reference to the
+// data in InstallableChecker; the checker owns these resources, and callers
+// should copy any which they wish to use later.
+// Fields which were not requested by callers will be set to null, empty, or
+// false objects when the callback is invoked.
+struct InstallableResult {
+  // NO_ERROR if there were no issues.
+  const ErrorCode error_code;
+
+  // The manifest URL. If non-empty, the site specified a <link rel="manifest">
+  // tag.
+  const GURL& manifest_url;
+
+  // The manifest object. Empty if a manifest was not specified, or could not be
+  // parsed.
+  const content::Manifest& manifest;
+
+  // The URL of the most appropriate icon from the manifest. Empty if no icon
+  // was requested.
+  const GURL& icon_url;
+
+  // The most appropriate icon from the manifest. nullptr if no icon was
+  // requested.
+  const SkBitmap* icon;
+
+  // True if the site has a manifest which is valid for a webapp. False if the
+  // manifest isn't valid for a webapp, or this check wasn't requested.
+  const bool has_valid_webapp_manifest;
+
+  // True if the site has a service worker controlling the manifest start URL.
+  // False if there is no such service worker, or this check wasn't requested.
+  const bool has_service_worker;
+};
+
+// The callback type passed to InstallableChecker::Start. Invoked on the UI
+// thread once the checker has performed all of the operations specified in the
+// InstallableParams passed to Start.
+using InstallableCallback = base::Callback<void(const InstallableResult&)>;

[benwells, 2016/07/21 06:52:47]

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?

[dominickn, 2016/07/21 09:13:38]

I think this comment is redundant based on the redesign.

+
+// This class is responsible for fetching the resources required to check and
+// install a site. Minimally, this is a web app manifest for the document in the
+// main frame; this manifest is all that is required for a site offering native
+// app banners. Progressive web apps must have specific fields in the manifest
+// set, a service worker controlling the manifest's start URL, and a valid,
+// fetchable icon in the manifest. Sites which do not have a service worker may
+// still specify a valid, fetchable icon in the manifest, and this will be used
+// when the site is added to homescreen.
+class InstallableChecker
+    : public content::WebContentsObserver,
+      public content::WebContentsUserData<InstallableChecker> {
+ public:
+  explicit InstallableChecker(content::WebContents* web_contents);
+  ~InstallableChecker() override;
+
+  // Returns true if the InstallableChecker is currently running.
+  bool IsActive() const { return HasFlag(STARTED); }
+
+  // Returns true if |manifest| meets the requirements for being installable.
+  // Sets |valid_webapp_manifest_error_| if any errors are encountered.
+  bool IsManifestValidForWebApp(const content::Manifest& manifest);
+
+  // Returns the minimum icon size in pixels for a site to be installable.
+  // TODO(dominickn): consolidate this concept with minimum_icon_size_in_dp
+  // across all platforms.
+  static int GetMinimumIconSizeInPx();
+
+  // Start the installable check, fetching the resources specified in |params|.
+  // |callback| is invoked on the UI thread when the checks are complete,
+  // passing an InstallableResult struct containing the requested resources.
+  //
+  // Separate calls to Start are processed serially; each call is guaranteed to
+  // invoke |callback| before the next one is processed. Resources are cached on
+  // this object: a previous call for the same page and with the same icon sizes
+  // will not perform duplicate work and will run the callback immediately with
+  // the cached resources. When |callback| runs, it should copy any resources on
+  // the result that are necessary for future work. It should also avoid running
+  // expensive tasks directly and defer them to later as the callback is
+  // directly invoked by the InstallableChecker to guarantee the consistency of
+  // the check results (i.e. the icon).
+  // This method is marked virtual so clients may mock this object in tests.
+  virtual void Start(const InstallableParams& params,
+                     const InstallableCallback& callback);
+
+ private:
+  friend class InstallableCheckerUnitTest;
+  FRIEND_TEST_ALL_PREFIXES(InstallableCheckerBrowserTest,
+                           CheckerBeginsInEmptyState);
+  FRIEND_TEST_ALL_PREFIXES(InstallableCheckerBrowserTest,
+                           CheckWebapp);
+
+  // This pair holds the parameters for a request to Start, along with the
+  // callback to be invoked when the request parameters are fulfilled. This is
+  // cheaply copyable and will be held in a std::vector.
+  using Task = std::pair<InstallableParams, InstallableCallback>;
+
+  // Bit flags used to maintain internal state regarding what has and hasn't yet
+  // been fetched/computed.
+  enum StatusFlag : uint32_t {
+    DORMANT = 0,
+    STARTED = 1 << 0,
+    RUNNING_CALLBACKS = 1 << 1,
+    MANIFEST_FETCHED = 1 << 2,
+    MANIFEST_VALIDATED = 1 << 3,
+    ICON_FETCHED = 1 << 4,
+    SERVICE_WORKER_CHECKED = 1 << 5,
+  };
+
+  // Stops the checking pipeline for the current task. Does not cancel any other
+  // tasks which may be queued.
+  void Cancel();
+
+  // Returns true if the icon size in |params| match those currently being
+  // requested.
+  bool DoesIconSizeMatch(const InstallableParams& params) const;
+
+  // Returns the error code associated with the resources requested in |params|,
+  // or NO_ERROR if there is no error.
+  ErrorCode GetErrorCode(const InstallableParams& params);
+
+  // Returns the WebContents to which this object is attached, or nullptr if the
+  // WebContents doesn't exist or is currently being destroyed.
+  content::WebContents* GetWebContents();
+
+  // Returns true if |params| requires no more work to be done (i.e. all the
+  // resources originally requested by |params| have been retrieved).
+  bool IsComplete(const InstallableParams& params) const;
+
+  // Returns true if |web_contents| is non-null and this checker remains active.
+  // If false is returned, |processing_error_| will be set.
+  bool IsRunning(content::WebContents* web_contents);
+
+  // Resets this object and removes all queued tasks. Called when navigating to
+  // a new page.
+  void Reset();
+
+  // Runs the callback associated with any task that is now complete.
+  void RunCallbacks();
+
+  // Runs the callbacks for any completed tasks, and starts work on the next.
+  void StartTask();
+
+  // Sets |flag| bit in |status_| to 0 and 1 respectively.
+  void ClearFlag(StatusFlag flag) { status_ &= ~flag; }
+  void SetFlag(StatusFlag flag) { status_ |= flag; }
+
+  // Returns true if the current status has the |flag| bit set to 1.
+  bool HasFlag(StatusFlag flag) const { return (status_ & flag) != 0; }
+
+  // Fetches the next required resource.
+  void FetchResource();
+
+  // Data retrieval methods.
+  void FetchManifest();
+  void OnDidGetManifest(const GURL& manifest_url,
+                        const content::Manifest& manifest);
+
+  void CheckValidWebappManifest();
+
+  void CheckServiceWorker();
+  void OnDidCheckHasServiceWorker(bool has_service_worker);
+
+  void ExtractAndFetchBestIcon();
+  void OnAppIconFetched(const GURL icon_url, const SkBitmap& bitmap);
+
+  // content::WebContentsObserver overrides
+  void DidFinishNavigation(content::NavigationHandle* handle) override;
+  void WebContentsDestroyed() override;
+
+  // Internal status field to keep track of what resources have been retrieved.
+  uint32_t status_;
+
+  // Error codes for each resource retrieval operation.
+  ErrorCode processing_error_;
+  ErrorCode manifest_error_;
+  ErrorCode valid_webapp_manifest_error_;
+  ErrorCode service_worker_error_;
+  ErrorCode icon_error_;
+
+  // The list of params + callback pairs that have come from a call to Start.
+  std::vector<Task> tasks_;
+
+  // Tasks received while we are currently working. These are held separately to
+  // avoid contaminating the tasks_ vector (e.g. if a new Task is received
+  // during RunCallbacks, e.g. an InstallableCallback is invoked which calls
+  // Start), we cannot add it to tasks_ as the vector may be modified).
+  std::vector<Task> pending_tasks_;
+
+  // The icon size parameters used if a valid icon was requested.
+  int ideal_icon_size_in_dp_;
+  int minimum_icon_size_in_dp_;
+
+  // Installable properties cached on this object.
+  GURL manifest_url_;
+  content::Manifest manifest_;
+  GURL icon_url_;
+  std::unique_ptr<SkBitmap> icon_;
+  bool has_valid_webapp_manifest_;
+  bool has_service_worker_;
+
+  // This object has a lifetime scoped to a WebContents, so WeakPtrs can be used
+  // for binding callbacks.
+  base::WeakPtrFactory<InstallableChecker> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(InstallableChecker);
+};
+
+}  // namespace installable
+
+#endif  // CHROME_BROWSER_INSTALLABLE_INSTALLABLE_CHECKER_H_