Extract AppBannerDataFetcher into an InstallableManager.

chrome/browser/installable/installable_checker.cc

Index: chrome/browser/installable/installable_checker.cc
diff --git a/chrome/browser/installable/installable_checker.cc b/chrome/browser/installable/installable_checker.cc
new file mode 100644
index 0000000000000000000000000000000000000000..157ae517159c34035b02367ecae43bfc67822756
--- /dev/null
+++ b/chrome/browser/installable/installable_checker.cc
@@ -0,0 +1,449 @@
+// 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.
+
+#include "chrome/browser/installable/installable_checker.h"
+
+#include "base/bind.h"
+#include "base/strings/string_util.h"
+#include "chrome/browser/manifest/manifest_icon_downloader.h"
+#include "chrome/browser/manifest/manifest_icon_selector.h"
+#include "chrome/browser/profiles/profile.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/service_worker_context.h"
+#include "content/public/browser/storage_partition.h"
+#include "third_party/WebKit/public/platform/WebDisplayMode.h"
+
+namespace {
+
+const int kIconSizeUnset = -1;
+
+const char kPngExtension[] = ".png";
+
+// TODO(dominickn): consolidate this constant with minimum_icon_size_in_dp
+// across platforms.
+const int kIconMinimumSizeInPx = 144;

[gone, 2016/07/20 22:21:35]

Always weird to see this number in C++ land... Completely disconnects it from
the reason why it's defined as 48 * 3, too.

[dominickn, 2016/07/21 00:24:47]

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.

[gone, 2016/07/21 00:49:27]

Acknowledged.

+
+// Returns true if |manifest| specifies a PNG icon that is at least 144x144px
+// (or has size "any").
+bool DoesManifestContainRequiredIcon(const content::Manifest& manifest) {
+  for (const auto& icon : manifest.icons) {
+    // The type field is optional. If it isn't present, fall back on checking
+    // the src extension, and allow the icon if the extension ends with png.
+    if (!base::EqualsASCII(icon.type.string(), "image/png") &&
+        !(icon.type.is_null() &&
+          base::EndsWith(icon.src.ExtractFileName(), kPngExtension,
+                         base::CompareCase::INSENSITIVE_ASCII)))
+      continue;
+
+    for (const auto& size : icon.sizes) {
+      if (size.IsEmpty())  // "any"
+        return true;
+      if (size.width() >= kIconMinimumSizeInPx &&
+          size.height() >= kIconMinimumSizeInPx) {
+        return true;
+      }
+    }
+  }
+
+  return false;
+}
+
+}  // anonymous namespace
+
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(installable::InstallableChecker);
+
+namespace installable {
+
+InstallableParams::InstallableParams()
+    : ideal_icon_size_in_dp(kIconSizeUnset),
+      minimum_icon_size_in_dp(kIconSizeUnset),
+      has_valid_webapp_manifest(false),
+      has_service_worker(false),
+      has_valid_icon(false) { }
+
+InstallableChecker::InstallableChecker(content::WebContents* web_contents)
+    : content::WebContentsObserver(web_contents),
+      status_(DORMANT),
+      processing_error_(NoErrorDetected),
+      manifest_error_(NoErrorDetected),
+      valid_webapp_manifest_error_(NoErrorDetected),
+      service_worker_error_(NoErrorDetected),
+      icon_error_(NoErrorDetected),
+      ideal_icon_size_in_dp_(kIconSizeUnset),
+      minimum_icon_size_in_dp_(kIconSizeUnset),
+      has_valid_webapp_manifest_(false),
+      has_service_worker_(false),
+      weak_factory_(this) {}
+
+InstallableChecker::~InstallableChecker() { }
+
+bool InstallableChecker::IsManifestValidForWebApp(
+    const content::Manifest& manifest) {
+  if (manifest.IsEmpty()) {
+    valid_webapp_manifest_error_ = ManifestEmpty;
+    return false;
+  }
+
+  if (!manifest.start_url.is_valid()) {
+    valid_webapp_manifest_error_ = StartUrlNotValid;
+    return false;
+  }
+
+  if ((manifest.name.is_null() || manifest.name.string().empty()) &&
+      (manifest.short_name.is_null() || manifest.short_name.string().empty())) {
+    valid_webapp_manifest_error_ = ManifestMissingNameOrShortName;
+    return false;
+  }
+
+  // TODO(dominickn,mlamouri): when Chrome supports "minimal-ui", it should be
+  // accepted. If we accept it today, it would fallback to "browser" and make
+  // this check moot. See https://crbug.com/604390.
+  if (manifest.display != blink::WebDisplayModeStandalone &&
+      manifest.display != blink::WebDisplayModeFullscreen) {
+    valid_webapp_manifest_error_ = ManifestDisplayIsNotStandaloneOrFullscreen;
+    return false;
+  }
+
+  if (!DoesManifestContainRequiredIcon(manifest)) {
+    valid_webapp_manifest_error_ = ManifestMissingSuitableIcon;
+    return false;
+  }
+
+  return true;
+}
+
+void InstallableChecker::Start(const InstallableParams& params,
+                               const InstallableCallback& callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // Always reset processing_error_, as it records events like navigation which
+  // are outside of fetching/validating resources.
+  processing_error_ = NoErrorDetected;
+
+  // If we've already working on a task, or running callbacks, add the new task
+  // to the pending list and return. It will be dealt with once the current work
+  // completes. We keep the pending list separate in case this call was
+  // initiated during an InstallableCallback invocation, which modifies the
+  // tasks_ list.
+  if (IsActive() || HasFlag(RUNNING_CALLBACKS)) {
+    pending_tasks_.push_back({params, callback});
+    return;
+  }
+
+  tasks_.push_back({params, callback});
+  SetFlag(STARTED);
+  StartTask();
+}
+
+void InstallableChecker::Cancel() {
+  // Clear the STARTED flag to signal that we should stop work immediately.
+  // Callers of this method should immediately call back to FetchResource(),
+  // which will terminate the current task and run callbacks.

[gone, 2016/07/20 22:21:35]

Is there any way to enforce that FetchResource() is called?

[dominickn, 2016/07/21 00:24:47]

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

[gone, 2016/07/21 00:49:27]

Acknowledged.

+  ClearFlag(STARTED);
+}
+
+bool InstallableChecker::DoesIconSizeMatch(
+    const InstallableParams& params) const {
+  return (ideal_icon_size_in_dp_ == params.ideal_icon_size_in_dp) &&
+         (minimum_icon_size_in_dp_ == params.minimum_icon_size_in_dp);
+}
+
+ErrorCode InstallableChecker::GetErrorCode(const InstallableParams& params) {
+  if (processing_error_ != NoErrorDetected)
+    return processing_error_;
+  if (manifest_error_ != NoErrorDetected)

[gone, 2016/07/20 22:21:35]

should we do else if all the way down?

[dominickn, 2016/07/21 00:24:47]

I didn't because every branch returns (saves a word each line!). Do you mind?

[gone, 2016/07/21 00:49:27]

Nah, keep it the way you like.

+    return manifest_error_;
+  if (params.has_valid_webapp_manifest &&
+      valid_webapp_manifest_error_ != NoErrorDetected) {
+    return valid_webapp_manifest_error_;
+  }
+  if (params.has_service_worker && service_worker_error_ != NoErrorDetected)
+    return service_worker_error_;
+  if (params.has_valid_icon && icon_error_ != NoErrorDetected)
+    return icon_error_;
+
+  return NoErrorDetected;
+}
+
+content::WebContents* InstallableChecker::GetWebContents() {
+  content::WebContents* contents = web_contents();
+  if (!contents || contents->IsBeingDestroyed())
+    return nullptr;
+  return contents;
+}
+
+bool InstallableChecker::IsComplete(const InstallableParams& params) const {
+  // Returns true if for all resources:
+  //  a. the params did not request it, OR
+  //  b. the resource has been retrieved.
+  return (HasFlag(MANIFEST_FETCHED)) &&
+         (!params.has_valid_webapp_manifest || HasFlag(MANIFEST_VALIDATED)) &&

[gone, 2016/07/20 22:21:35]

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?

[dominickn, 2016/07/21 00:24:47]

Changed to params.check_*

+         (!params.has_service_worker || HasFlag(SERVICE_WORKER_CHECKED)) &&
+         (!params.has_valid_icon ||
+          (HasFlag(ICON_FETCHED) && DoesIconSizeMatch(params)));
+}
+
+bool InstallableChecker::IsRunning(content::WebContents* web_contents) {
+  if (!web_contents) {
+    processing_error_ = RendererExiting;
+    return false;
+  }
+
+  if (!IsActive()) {
+    processing_error_ = UserNavigatedBeforeBannerShown;
+    return false;
+  }
+
+  return true;
+}
+
+void InstallableChecker::Reset() {
+  status_ = DORMANT;
+  processing_error_ = NoErrorDetected;
+  manifest_error_ = NoErrorDetected;
+  valid_webapp_manifest_error_ = NoErrorDetected;
+  service_worker_error_ = NoErrorDetected;
+  icon_error_ = NoErrorDetected;
+
+  ideal_icon_size_in_dp_ = kIconSizeUnset;
+  minimum_icon_size_in_dp_ = kIconSizeUnset;
+
+  tasks_.clear();
+  pending_tasks_.clear();
+
+  manifest_url_ = GURL();
+  manifest_ = content::Manifest();
+  icon_url_ = GURL();
+  icon_.reset(nullptr);
+  has_valid_webapp_manifest_ = false;
+  has_service_worker_ = false;
+}
+
+void InstallableChecker::RunCallbacks() {
+  // Post a callback and delete it from the list of tasks if:
+  //  - the STARTED status bit is missing. This means that we've either finished
+  //    all possible checks, or we have canceled the pipeline
+  //  - the params that the callback was started with have been satisfied.
+  SetFlag(RUNNING_CALLBACKS);
+  for (auto it = tasks_.begin(); it != tasks_.end();) {
+    if (!IsActive() || IsComplete(it->first)) {

[gone, 2016/07/20 22:21:35]

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.

[dominickn, 2016/07/21 00:24:47]

Done.

+      InstallableResult result = {GetErrorCode(it->first),
+                                  manifest_url_,
+                                  manifest_,
+                                  icon_url_,
+                                  icon_.get(),
+                                  has_valid_webapp_manifest_,
+                                  has_service_worker_};
+      // We must run this directly to guarantee the callback gets consistent
+      // results. If we PostTask, a second Task may begin that invalidates the
+      // icon object before the callback gets a chance to use it.
+      it->second.Run(result);
+      it = tasks_.erase(it);
+     } else {

[gone, 2016/07/20 22:21:35]

indentation here is wonky

[dominickn, 2016/07/21 00:24:47]

Done.

+       ++it;
+     }
+  }
+  ClearFlag(RUNNING_CALLBACKS);
+}
+
+void InstallableChecker::StartTask() {
+  RunCallbacks();
+
+  if (!pending_tasks_.empty()) {
+    // Shift any pending tasks to the end of tasks_.
+    tasks_.insert(tasks_.end(), pending_tasks_.begin(), pending_tasks_.end());
+    pending_tasks_.clear();
+  }
+
+  // If there's nothing to do, exit. Resources remain cached so any future calls
+  // won't re-fetch anything that has already been retrieved.
+  if (tasks_.empty()) {
+    ClearFlag(STARTED);
+    return;
+  }
+
+  // If we are requesting an icon, and the requested size differs from any
+  // previously fetched (or if we haven't yet fetched an icon), reset the icon
+  // state. This has no impact on the other resources, so don't reset them.
+  const InstallableParams& params = tasks_[0].first;
+  if (params.has_valid_icon && !DoesIconSizeMatch(params)) {
+    ideal_icon_size_in_dp_ = params.ideal_icon_size_in_dp;
+    minimum_icon_size_in_dp_ = params.minimum_icon_size_in_dp;
+    icon_url_ = GURL();
+    icon_.reset(nullptr);
+    icon_error_ = NoErrorDetected;
+    ClearFlag(ICON_FETCHED);
+  }
+
+  FetchResource();
+}
+
+void InstallableChecker::FetchResource() {
+  DCHECK(!tasks_.empty());
+  const InstallableParams& params = tasks_[0].first;
+
+  // Cancel if there is an error code for any resource requested by params.
+  if (GetErrorCode(params) != NoErrorDetected)
+    Cancel();
+
+  // If not active, then Cancel() has been called. Go straight back to
+  // StartTask, which will clear the current task. Otherwise, if we fall through
+  // to the else case, we've fetched everything necessary for this task, so
+  // call StartTask to run its callback and start the next task.
+  if (!IsActive())
+    StartTask();
+  else if (!HasFlag(MANIFEST_FETCHED))
+    FetchManifest();
+  else if (params.has_valid_webapp_manifest && !HasFlag(MANIFEST_VALIDATED))
+    CheckValidWebappManifest();
+  else if (params.has_service_worker && !HasFlag(SERVICE_WORKER_CHECKED))
+    CheckServiceWorker();
+  else if (params.has_valid_icon && !HasFlag(ICON_FETCHED))
+    ExtractAndFetchBestIcon();
+  else
+    StartTask();
+}
+
+void InstallableChecker::FetchManifest() {
+  DCHECK(!HasFlag(MANIFEST_FETCHED));
+  DCHECK(manifest_.IsEmpty());
+  DCHECK(manifest_url_.is_empty());
+
+  content::WebContents* web_contents = GetWebContents();
+  DCHECK(web_contents);
+
+  web_contents->GetManifest(base::Bind(&InstallableChecker::OnDidGetManifest,
+                                       weak_factory_.GetWeakPtr()));
+}
+
+void InstallableChecker::OnDidGetManifest(const GURL& manifest_url,
+                                          const content::Manifest& manifest) {
+  if (!IsRunning(GetWebContents()))
+    Cancel();
+  else if (manifest_url.is_empty())
+    manifest_error_ = NoManifest;
+  else if (manifest.IsEmpty())
+    manifest_error_ = ManifestEmpty;
+
+  manifest_url_ = manifest_url;
+  manifest_ = manifest;
+
+  SetFlag(MANIFEST_FETCHED);
+  FetchResource();
+}
+
+void InstallableChecker::CheckValidWebappManifest() {
+  DCHECK(!HasFlag(MANIFEST_VALIDATED));
+  DCHECK(!manifest_.IsEmpty());
+
+  has_valid_webapp_manifest_ = IsManifestValidForWebApp(manifest_);
+  if (!has_valid_webapp_manifest_)
+    Cancel();
+
+  SetFlag(MANIFEST_VALIDATED);
+  FetchResource();
+}
+
+void InstallableChecker::CheckServiceWorker() {
+  DCHECK(!HasFlag(SERVICE_WORKER_CHECKED));
+  DCHECK(!manifest_.IsEmpty());
+
+  content::WebContents* web_contents = GetWebContents();
+
+  // Check to see if there is a single service worker controlling this page
+  // and the manifest's start url.
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents->GetBrowserContext());
+  content::StoragePartition* storage_partition =
+      content::BrowserContext::GetStoragePartition(
+          profile, web_contents->GetSiteInstance());
+  DCHECK(storage_partition);
+
+  storage_partition->GetServiceWorkerContext()->CheckHasServiceWorker(
+      web_contents->GetLastCommittedURL(), manifest_.start_url,
+      base::Bind(&InstallableChecker::OnDidCheckHasServiceWorker,
+                 weak_factory_.GetWeakPtr()));
+}
+
+void InstallableChecker::OnDidCheckHasServiceWorker(bool has_service_worker) {
+  if (!IsRunning(GetWebContents()))
+    Cancel();
+
+  has_service_worker_ = has_service_worker;
+  if (!has_service_worker)
+    service_worker_error_ = NoMatchingServiceWorker;
+
+  SetFlag(SERVICE_WORKER_CHECKED);
+  FetchResource();
+}
+
+void InstallableChecker::ExtractAndFetchBestIcon() {
+  // icon_url_ and icon_ should have both been reset if this method is called.
+  DCHECK(!HasFlag(ICON_FETCHED));
+  DCHECK(!manifest_.IsEmpty());
+  DCHECK(icon_url_.is_empty());
+  DCHECK(icon_.get() == nullptr);
+  DCHECK_GT(ideal_icon_size_in_dp_, 0);
+  DCHECK_GT(minimum_icon_size_in_dp_, 0);
+
+  GURL icon_url = ManifestIconSelector::FindBestMatchingIcon(
+      manifest_.icons, ideal_icon_size_in_dp_, minimum_icon_size_in_dp_);
+
+  // First, check the icon URL. If it exists, see if the ManifestIconDownloader
+  // is able to download it. If it can, it will call OnAppIconFetched. Any
+  // conditional failures will set the ICON_FETCHED bit and fall through back to
+  // FetchResource.
+  if (icon_url.is_empty()) {
+    icon_error_ = NoIconMatchingRequirements;
+  } else {
+    bool can_download_icon = ManifestIconDownloader::Download(
+        GetWebContents(), icon_url, ideal_icon_size_in_dp_,
+        minimum_icon_size_in_dp_,
+        base::Bind(&InstallableChecker::OnAppIconFetched,
+                   weak_factory_.GetWeakPtr(), icon_url));
+    if (can_download_icon)
+      return;
+    icon_error_ = CannotDownloadIcon;
+  }
+
+  SetFlag(ICON_FETCHED);
+  FetchResource();
+}
+
+void InstallableChecker::OnAppIconFetched(const GURL icon_url,
+                                          const SkBitmap& bitmap) {
+  if (!IsRunning(GetWebContents())) {
+    Cancel();
+  } else if (bitmap.drawsNothing()) {
+    icon_error_ = NoIconAvailable;
+  } else {
+    icon_url_ = icon_url;
+    icon_.reset(new SkBitmap(bitmap));
+  }
+
+  SetFlag(ICON_FETCHED);
+  FetchResource();
+}
+
+void InstallableChecker::DidFinishNavigation(
+    content::NavigationHandle* handle) {
+  if (handle->IsInMainFrame() && handle->HasCommitted() &&
+      !handle->IsErrorPage() && !handle->IsSamePage()) {
+    Reset();
+  }
+}
+
+void InstallableChecker::WebContentsDestroyed() {
+  Reset();
+  Observe(nullptr);
+}
+
+// static
+int InstallableChecker::GetMinimumIconSizeInPx() {
+  return kIconMinimumSizeInPx;
+}
+
+}  // namespace installable