Extract AppBannerDataFetcher into an InstallableManager.

chrome/browser/installable/installable_checker.cc

+// 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";
+
+// This constant is the icon size on Android (48dp) multiplied by the scale
+// factor of a Nexus 5 device (3). For mobile and desktop platforms, a 144px
+// icon is an approximate, appropriate lower bound.
+// TODO(dominickn): consolidate with minimum_icon_size_in_dp across platforms.
+const int kIconMinimumSizeInPx = 144;
+
+// 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(InstallableChecker);
+
+InstallableChecker::InstallableChecker(content::WebContents* web_contents)
+    : content::WebContentsObserver(web_contents),
+      status_(DORMANT),
+      processing_error_(NO_ERROR_DETECTED),
+      manifest_error_(NO_ERROR_DETECTED),
+      valid_webapp_manifest_error_(NO_ERROR_DETECTED),
+      service_worker_error_(NO_ERROR_DETECTED),
+      icon_error_(NO_ERROR_DETECTED),
+      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_ = MANIFEST_EMPTY;
+    return false;
+  }
+
+  if (!manifest.start_url.is_valid()) {
+    valid_webapp_manifest_error_ = START_URL_NOT_VALID;
+    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_ = MANIFEST_MISSING_NAME_OR_SHORT_NAME;
+    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_ = MANIFEST_DISPLAY_NOT_SUPPORTED;
+    return false;
+  }
+
+  if (!DoesManifestContainRequiredIcon(manifest)) {
+    valid_webapp_manifest_error_ = MANIFEST_MISSING_SUITABLE_ICON;
+    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_ = NO_ERROR_DETECTED;
+
+  // 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.

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

Reading this it feels like STARTED could be named better. Maybe FETCHING?
CHECKING? DOING_STUFF?

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

It's now a separate boolean flag is_active_.

+  // Callers of this method should immediately call back to FetchResource() or
+  // Start(), which will terminate the current task and run callbacks.
+  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);
+}
+
+InstallableErrorCode InstallableChecker::GetErrorCode(
+    const InstallableParams& params) {
+  if (processing_error_ != NO_ERROR_DETECTED)
+    return processing_error_;
+  if (manifest_error_ != NO_ERROR_DETECTED)
+    return manifest_error_;
+  if (params.check_valid_webapp_manifest &&
+      valid_webapp_manifest_error_ != NO_ERROR_DETECTED) {
+    return valid_webapp_manifest_error_;
+  }
+  if (params.check_service_worker && service_worker_error_ != NO_ERROR_DETECTED)
+    return service_worker_error_;
+  if (params.check_valid_icon && icon_error_ != NO_ERROR_DETECTED)
+    return icon_error_;
+
+  return NO_ERROR_DETECTED;
+}
+
+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.check_valid_webapp_manifest || HasFlag(MANIFEST_VALIDATED)) &&
+         (!params.check_service_worker || HasFlag(SERVICE_WORKER_CHECKED)) &&
+         (!params.check_valid_icon ||
+          (HasFlag(ICON_FETCHED) && DoesIconSizeMatch(params)));
+}
+
+bool InstallableChecker::IsRunning(content::WebContents* web_contents) {
+  if (!web_contents) {
+    processing_error_ = RENDERER_EXITING;
+    return false;
+  }
+
+  if (!IsActive()) {
+    processing_error_ = USER_NAVIGATED;
+    return false;
+  }
+
+  return true;
+}
+
+void InstallableChecker::Reset() {
+  status_ = DORMANT;
+  processing_error_ = NO_ERROR_DETECTED;
+  manifest_error_ = NO_ERROR_DETECTED;
+  valid_webapp_manifest_error_ = NO_ERROR_DETECTED;
+  service_worker_error_ = NO_ERROR_DETECTED;
+  icon_error_ = NO_ERROR_DETECTED;

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

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.

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

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?

+
+  ideal_icon_size_in_dp_ = kIconSizeUnset;
+  minimum_icon_size_in_dp_ = kIconSizeUnset;
+
+  // Prevent any outstanding callbacks to or from this object from being called.
+  weak_factory_.InvalidateWeakPtrs();
+  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.
+  // We run through the entire tasks_ vector here since we may have queued
+  // requests which have also been completed while working on the active task.
+  SetFlag(RUNNING_CALLBACKS);

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

Why does this all have to be wrapped in this?

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

Clarified.

+  for (auto it = tasks_.begin(); it != tasks_.end();) {
+    const InstallableParams& params = it->first;
+    if (!IsActive() || IsComplete(params)) {

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

Why check IsActive for each task? Can it change in this loop?

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

Removed.

+      InstallableResult result = {
+          GetErrorCode(params), manifest_url_, manifest_,
+          params.check_valid_icon ? icon_url_ : GURL::EmptyGURL(),
+          params.check_valid_icon ? icon_.get() : nullptr,
+          params.check_valid_webapp_manifest ? has_valid_webapp_manifest_
+                                             : false,
+          params.check_service_worker ? has_service_worker_ : false};
+
+      // 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.

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

This wouldn't be a problem if you kept a map of icon sizes to icons. Did you
consider that?

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

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.

+      it->second.Run(result);
+      it = tasks_.erase(it);
+    } else {
+      ++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.check_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_ = NO_ERROR_DETECTED;
+    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) != NO_ERROR_DETECTED)
+    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

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

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

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

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?

+  // 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.check_valid_webapp_manifest && !HasFlag(MANIFEST_VALIDATED))
+    CheckValidWebappManifest();
+  else if (params.check_service_worker && !HasFlag(SERVICE_WORKER_CHECKED))
+    CheckServiceWorker();
+  else if (params.check_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_ = NO_MANIFEST;
+  else if (manifest.IsEmpty())
+    manifest_error_ = MANIFEST_EMPTY;
+
+  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_ = NO_MATCHING_SERVICE_WORKER;
+
+  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_);
+
+  if (icon_url.is_empty()) {
+    icon_error_ = NO_ACCEPTABLE_ICON;
+  } 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_ = CANNOT_DOWNLOAD_ICON;
+  }
+
+  SetFlag(ICON_FETCHED);
+  FetchResource();
+}
+
+void InstallableChecker::OnAppIconFetched(const GURL icon_url,
+                                          const SkBitmap& bitmap) {
+  if (!IsRunning(GetWebContents())) {
+    Cancel();
+  } else if (bitmap.drawsNothing()) {
+    icon_error_ = NO_ICON_AVAILABLE;
+  } 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->IsSamePage()) {
+    Reset();
+  }
+}
+
+void InstallableChecker::WebContentsDestroyed() {
+  Reset();
+  Observe(nullptr);
+}
+
+// static
+int InstallableChecker::GetMinimumIconSizeInPx() {
+  return kIconMinimumSizeInPx;
+}