Refactor getInstalledRelatedApps code and add manifest logic and tests.

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

Index: third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
diff --git a/third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp b/third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
index 2d06c5f3fdbdb7d656ee19d6344cb50835d32393..7e2420d5a960e348c0e9969641f5e632f7162b28 100644
--- a/third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
+++ b/third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
@@ -7,55 +7,117 @@
 #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) {