Extract AppBannerDataFetcher into an InstallableManager.

chrome/browser/installable/installable_checker.h

+// 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"
+
+// This struct specifies the work to be done by the InstallableChecker.
+// Results are cached and fetched in the order specified in this struct. A web
+// app manifest will always be fetched first.
+struct InstallableParams {
+  // The ideal icon size to fetch. Used only if |check_valid_icon| is true.
+  int ideal_icon_size_in_dp = -1;
+
+  // The minimum icon size to fetch. Used only if |check_valid_icon| is true.
+  int minimum_icon_size_in_dp = -1;
+
+  // Check whether the site's manifest specifies a web app. i.e. it:
+  //  - 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 = false;
+
+  // Check for a service worker controlling the site and the manifest start URL.
+  bool check_service_worker = false;
+
+  // 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.
+  bool check_valid_icon = false;

[benwells, 2016/07/26 03:28:46]

Does this also fetch the icon? From the name alone you'd think not, but then I'm
not sure what else would.

[dominickn, 2016/07/28 00:36:28]

Renamed to fetch.

+};
+
+// This struct is passed to an InstallableCallback when the InstallableChecker
+// has finished with a task. Each reference is owned by InstallableChecker, and
+// callers should copy any objects which they wish to use later.
+// Non-requested fields will be set to null, empty, or false.
+struct InstallableResult {
+  // NO_ERROR_DETECTED if there were no issues.
+  const InstallableErrorCode error_code;
+
+  // Empty if the site has no <link rel="manifest"> tag.
+  const GURL& manifest_url;
+
+  // Empty if the site has an unparseable manifest.
+  const content::Manifest& manifest;
+
+  // Empty if no icon was requested.
+  const GURL& icon_url;
+
+  // nullptr if the most appropriate icon couldn't be determined or downloaded.
+  const SkBitmap* icon;
+
+  const bool has_valid_webapp_manifest;
+
+  const bool has_service_worker;
+};
+
+using InstallableCallback = base::Callback<void(const InstallableResult&)>;
+
+// This class is responsible for fetching the resources required to check and
+// install a site. A call to Start() will always fetch a site's web app manifest
+// if it exists. Other resources may be requested in the InstallableParams
+// passed to Start. Results are provided in a struct sent to an
+// InstallableCallback; previously fetched resources are cached for efficiency.
+class InstallableChecker

[benwells, 2016/07/26 03:28:46]

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.

[dominickn, 2016/07/28 00:36:27]

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?

[benwells, 2016/07/28 08:10:58]

While I don't like calling things Manager, I think it might be the best of the
bunch.

+    : 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); }

[benwells, 2016/07/26 07:27:14]

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.

[dominickn, 2016/07/28 00:36:28]

It's gone.

+
+  // 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. |callback| should defer rexpensive tasks to later as

[benwells, 2016/07/26 03:28:46]

s/rexpensive/expensive

[dominickn, 2016/07/28 00:36:27]

Done.

+  // it will be directly called.

[benwells, 2016/07/26 03:28:45]

Why is important not to do expensive work?

[dominickn, 2016/07/28 00:36:27]

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.

+  // 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);
+
+  using Task = std::pair<InstallableParams, InstallableCallback>;
+
+  // Bit flags used to maintain internal state.
+  enum StatusFlag : uint32_t {

[benwells, 2016/07/26 03:28:45]

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.

[dominickn, 2016/07/28 00:36:28]

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.

+    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| matches the current request.

[benwells, 2016/07/26 03:28:46]

What is the current request in this context? Does it mean the icon that has been
fetched already, or the current task being run?

[dominickn, 2016/07/28 00:36:27]

Clarified.

+  bool DoesIconSizeMatch(const InstallableParams& params) const;
+
+  // Returns the error code associated with the resources requested in |params|,
+  // or NO_ERROR_DETECTED if there is no error.
+  InstallableErrorCode 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.
+  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);

[benwells, 2016/07/26 03:28:46]

This seems weird. Why would looking if something is running cause a processing
error?

[dominickn, 2016/07/28 00:36:27]

processing_error is gone. I've renamed the method IsContentsValid for clarity.

+
+  // Resets members to empty 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_;
+
+  InstallableErrorCode processing_error_;
+  InstallableErrorCode manifest_error_;
+  InstallableErrorCode valid_webapp_manifest_error_;
+  InstallableErrorCode service_worker_error_;
+  InstallableErrorCode 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 working. These are held separately to avoid
+  // contaminating the tasks_ vector (e.g. if a new Task is received during
+  // callback invocation in RunCallbacks).
+  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. We use a factory so we can invalidate pointers.

[benwells, 2016/07/26 03:28:46]

I don't understand the first sentence of this comment.

[dominickn, 2016/07/28 00:36:28]

Removed.

+  base::WeakPtrFactory<InstallableChecker> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(InstallableChecker);
+};
+
+#endif  // CHROME_BROWSER_INSTALLABLE_INSTALLABLE_CHECKER_H_