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..eb90f02e051013e6c2cfdd3cc82f4387fc45c4ce 100644
--- a/third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
+++ b/third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
@@ -7,55 +7,118 @@
 #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 {
 
+namespace {
+
+// Wrapper around an AppInstalledCallbacks, applying a filter to the successful
+// result before passing it to the inner callback's onSuccess method.
+class CallbackFilter : public AppInstalledCallbacks {
+ public:
+  // The filter function takes a vector of related applications, applies some
+  // operation to it, and then forwards the result to the given callback.
+  using FilterFunction =
+      WTF::Function<void(const blink::WebVector<blink::WebRelatedApplication>&,
+                         std::unique_ptr<blink::AppInstalledCallbacks>)>;
+
+  CallbackFilter(std::unique_ptr<FilterFunction>,
+                 std::unique_ptr<AppInstalledCallbacks>);
+
+  // AppInstalledCallbacks overrides:
+  void onSuccess(const WebVector<WebRelatedApplication>& relatedApps) override;
+  void onError() override;
+
+ private:
+  std::unique_ptr<FilterFunction> m_filter;
+  std::unique_ptr<AppInstalledCallbacks> m_callbacks;
+};
+
+CallbackFilter::CallbackFilter(std::unique_ptr<FilterFunction> filter,
+                               std::unique_ptr<AppInstalledCallbacks> callbacks)
+    : m_filter(std::move(filter)), m_callbacks(std::move(callbacks)) {}
+
+void CallbackFilter::onSuccess(

[esprehn, 2017/02/08 04:40:33]

lets just make this virtual and call the super method or override it. No need to
have callbacks and binding.

[Matt Giuca, 2017/02/08 08:27:11]

Sorry, I don't understand what you're asking for here. This method is virtual,
and there is no subclass to CallbackFilter.

This is essentially a way of being able to give an AppInstalledCallbacks object
to the content/renderer code, that will 1. do the filter, then 2. call the
callback supplied from NavigatorInstalledApp. This class used to have the
filtering function in it, but then it had to have heaps of state variables too
so I refactored it by moving the state into InstalledAppController and just
having it take a bound filter function (so essentially this class is just a
stateless/logic-less composer now).

Long term, this won't be needed because the code in content/renderer will come
into Blink. But shorter term, I plan to fold InstalledAppController into
NavigatorInstalledApp and then I think this will be significantly simplified as
well. But I tried to do it and ran into a huge rats nest and backed out for the
time being.

So is this OK? (If not, please state concretely what you want me to change.)

[Matt Giuca, 2017/02/13 06:34:50]

sammc@ helped me to remove some of the abstraction here (removing the bind and
filter function, and just directly calling filterByInstalledApps). Is this more
what you wanted?

+    const WebVector<WebRelatedApplication>& relatedApps) {
+  (*m_filter)(relatedApps, std::move(m_callbacks));
+}
+
+void CallbackFilter::onError() {
+  m_callbacks->onError();
+}
+
+}  // namespace
+
 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
+    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<CallbackFilter>(
+          WTF::bind(&InstalledAppController::filterByInstalledApps,
+                    wrapPersistent(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) {