Refactor getInstalledRelatedApps code and add manifest logic and tests.

third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp

 // 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 "modules/installedapp/InstalledAppController.h"
 
 #include "core/dom/Document.h"
 #include "core/frame/LocalFrame.h"
 #include "platform/RuntimeEnabledFeatures.h"
+#include "public/platform/InterfaceProvider.h"
 #include "public/platform/WebSecurityOrigin.h"
+#include "wtf/Functional.h"
 
 #include <utility>
 
 namespace blink {
 
+// Callbacks for the result of
+// WebRelatedAppsFetcher::getManifestRelatedApplications. Calls
+// filterByInstalledApps upon receiving the list of related applications.
+// TODO(mgiuca): Inline this. DO NOT SUBMIT.

[esprehn, 2017/02/15 01:56:50]

???

[Matt Giuca, 2017/02/15 07:55:35]

I wrote this as a comment to myself to do before landing the CL. (Basically to
inline all the methods into the class declaration since I don't think they
warrant being outside.)

I write DO NOT SUBMIT so the presubmit will pick up on it if I forget to do it
before landing (you don't need to ask me to act on a DO NOT SUBMIT because I
will be forced to).

+class InstalledAppController::GetRelatedAppsCallbacks
+    : public AppInstalledCallbacks {
+ public:
+  GetRelatedAppsCallbacks(InstalledAppController*,
+                          const WebSecurityOrigin& url,
+                          std::unique_ptr<AppInstalledCallbacks>);
+
+  // AppInstalledCallbacks overrides:
+  void onSuccess(const WebVector<WebRelatedApplication>& relatedApps) override;
+  void onError() override;
+
+ private:
+  WeakPersistent<InstalledAppController> m_controller;
+  const WebSecurityOrigin m_url;
+  std::unique_ptr<AppInstalledCallbacks> m_callbacks;
+};
+
+InstalledAppController::GetRelatedAppsCallbacks::GetRelatedAppsCallbacks(
+    InstalledAppController* controller,
+    const WebSecurityOrigin& url,
+    std::unique_ptr<AppInstalledCallbacks> callbacks)
+    : m_controller(controller), m_url(url), m_callbacks(std::move(callbacks)) {}
+
+void InstalledAppController::GetRelatedAppsCallbacks::onSuccess(
+    const WebVector<WebRelatedApplication>& relatedApps) {
+  if (!m_controller)
+    return;
+
+  m_controller->filterByInstalledApps(m_url, relatedApps,
+                                      std::move(m_callbacks));
+}
+
+void InstalledAppController::GetRelatedAppsCallbacks::onError() {
+  m_callbacks->onError();
+}
+
 InstalledAppController::~InstalledAppController() {}
 
-void InstalledAppController::provideTo(LocalFrame& frame,
-                                       WebInstalledAppClient* client) {
-  ASSERT(RuntimeEnabledFeatures::installedAppEnabled());
+void InstalledAppController::getInstalledRelatedApps(
+    const WebSecurityOrigin& url,
+    std::unique_ptr<AppInstalledCallbacks> callbacks) {
+  // When detached, the fetcher is no longer valid.
+  if (!m_relatedAppsFetcher) {
+    // TODO(mgiuca): AbortError rather than simply undefined.
+    // https://crbug.com/687846

[esprehn, 2017/02/15 01:56:50]

Do we have a test for this behavior?

[Matt Giuca, 2017/02/15 07:55:34]

Hmmmmmm..

This is a "not supposed to happen" case (but it can't be a DCHECK because
theoretically contextDestroyed could be called). I'm not familiar enough with
the lifetime of these objects to know exactly how to trigger it in a test.

I think this will require some major changes to the testing infrastructure (I
had a long chat with sammc@; I don't understand this fully but his suggestion
was to have a layout test with an iframe, triggering a navigation on the iframe
while still holding onto the context in the outer frame, then call
getInstalledRelatedApps on the outer frame).

I think this gets a lot simpler in my plan to combine InstalledAppController
with NavigatorInstalledApp -- I think then this case will actually disappear.

Given the difficulty of testing this, plus the fact that this is technically a
problem with the existing code (I'm just moving this from Line 49, not
introducing it), plus the fact that this isn't launched yet, plus the fact that
this issue might go away when I refactor it, I'm going to save testing this edge
case for later on when this code is simpler.

+    callbacks->onError();
+    return;
+  }
+
+  // Get the list of related applications from the manifest. This requires a
+  // request to the content layer (because the manifest is not a Blink concept).
+  // Upon returning, filter the result list to those apps that are installed.
+  // TODO(mgiuca): This roundtrip to content could be eliminated if the Manifest
+  // class was moved from content into Blink.
+  m_relatedAppsFetcher->getManifestRelatedApplications(
+      WTF::makeUnique<GetRelatedAppsCallbacks>(this, url,
+                                               std::move(callbacks)));
+}
+
+void InstalledAppController::provideTo(
+    LocalFrame& frame,
+    WebRelatedAppsFetcher* relatedAppsFetcher) {
+  DCHECK(RuntimeEnabledFeatures::installedAppEnabled());
 
   InstalledAppController* controller =
-      new InstalledAppController(frame, client);
+      new InstalledAppController(frame, relatedAppsFetcher);
   Supplement<LocalFrame>::provideTo(frame, supplementName(), controller);
 }
 
 InstalledAppController* InstalledAppController::from(LocalFrame& frame) {
   InstalledAppController* controller = static_cast<InstalledAppController*>(
       Supplement<LocalFrame>::from(frame, supplementName()));
-  ASSERT(controller);
+  DCHECK(controller);
   return controller;
 }
 
-InstalledAppController::InstalledAppController(LocalFrame& frame,
-                                               WebInstalledAppClient* client)
-    : Supplement<LocalFrame>(frame),
-      ContextLifecycleObserver(frame.document()),
-      m_client(client) {}
-
 const char* InstalledAppController::supplementName() {
   return "InstalledAppController";
 }
 
-void InstalledAppController::getInstalledApps(
-    const WebSecurityOrigin& url,
-    std::unique_ptr<AppInstalledCallbacks> callback) {
-  // When detached, the client is no longer valid.
-  if (!m_client) {
-    callback.release()->onError();
-    return;
-  }
+InstalledAppController::InstalledAppController(
+    LocalFrame& frame,
+    WebRelatedAppsFetcher* relatedAppsFetcher)
+    : Supplement<LocalFrame>(frame),
+      ContextLifecycleObserver(frame.document()),
+      m_relatedAppsFetcher(relatedAppsFetcher) {}
 
-  // Client is expected to take ownership of the callback
-  m_client->getInstalledRelatedApps(url, std::move(callback));
+void InstalledAppController::contextDestroyed(ExecutionContext*) {
+  m_relatedAppsFetcher = nullptr;
 }
 
-void InstalledAppController::contextDestroyed(ExecutionContext*) {
-  m_client = nullptr;
+void InstalledAppController::filterByInstalledApps(
+    const blink::WebSecurityOrigin& /*origin*/,
+    const blink::WebVector<blink::WebRelatedApplication>& /*relatedApps*/,
+    std::unique_ptr<blink::AppInstalledCallbacks> callbacks) {
+  // TODO(mgiuca): Call through to the browser to filter the list of
+  // applications down to just those that are installed on the device.
+  // For now, just return the empty list (no applications are installed).
+  callbacks->onSuccess(blink::WebVector<blink::WebRelatedApplication>());
 }
 
 DEFINE_TRACE(InstalledAppController) {
   Supplement<LocalFrame>::trace(visitor);
   ContextLifecycleObserver::trace(visitor);
 }
 
 }  // namespace blink