getInstalledRelatedApps: Add browser-side Mojo service (stub).

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 66ac9ff72ff4fa788e2cbf77050c09d5d9e58cb8..3e7b235061bb01cb93b486e1b76c02e1ef166ca6 100644
--- a/third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
+++ b/third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
@@ -100,13 +100,49 @@ void InstalledAppController::contextDestroyed(ExecutionContext*) {
 }
 
 void InstalledAppController::filterByInstalledApps(
-    WTF::RefPtr<SecurityOrigin> /*origin*/,
-    const WebVector<WebRelatedApplication>& /*relatedApps*/,
-    std::unique_ptr<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(WebVector<WebRelatedApplication>());
+    WTF::RefPtr<SecurityOrigin> origin,
+    const blink::WebVector<blink::WebRelatedApplication>& relatedApps,
+    std::unique_ptr<blink::AppInstalledCallbacks> callbacks) {
+  WTF::Vector<mojom::blink::RelatedApplicationPtr> mojoRelatedApps;
+  for (const auto& relatedApplication : relatedApps) {
+    mojom::blink::RelatedApplicationPtr convertedApplication(
+        mojom::blink::RelatedApplication::New());
+    DCHECK(!relatedApplication.platform.isEmpty());
+    convertedApplication->platform = relatedApplication.platform;
+    convertedApplication->id = relatedApplication.id;
+    convertedApplication->url = relatedApplication.url;
+    mojoRelatedApps.push_back(std::move(convertedApplication));
+  }
+
+  if (!m_provider) {
+    supplementable()->interfaceProvider()->getInterface(
+        mojo::MakeRequest(&m_provider));

[dcheng, 2017/03/02 07:41:51]

We should close m_provider in contextDestroyed(), IMO.

(We want to have some abstractions for automatically doing this, but they don't
yet exist...)

[Matt Giuca, 2017/03/10 03:35:47]

Done.

+    // TODO(mgiuca): Set a connection error handler. This requires a refactor to
+    // work like NavigatorShare.cpp (retain a persistent list of clients to
+    // reject all of their promises).
+    DCHECK(m_provider);
+  }
+
+  m_provider->FilterInstalledApps(
+      std::move(mojoRelatedApps), origin->toString(),
+      convertToBaseCallback(
+          WTF::bind(&InstalledAppController::OnFilterInstalledApps,
+                    wrapPersistent(this), WTF::passed(std::move(callbacks)))));

[dcheng, 2017/03/02 07:41:51]

Nit: WTF::passed(&callbacks)

[Matt Giuca, 2017/03/10 03:35:47]

This doesn't work (no viable conversion from
'std::unique_ptr<blink::AppInstalledCallbacks>*' to
'std::unique_ptr<blink::AppInstalledCallbacks>').

An error message in WTF::passed says "Add std::move() if necessary" indicating
that my code is doing the right thing.

Here's some code that works exactly like mine:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/sensor...

+}
+
+void InstalledAppController::OnFilterInstalledApps(
+    std::unique_ptr<blink::AppInstalledCallbacks> callbacks,
+    WTF::Vector<mojom::blink::RelatedApplicationPtr> result) {

[haraken, 2017/03/02 07:33:28]

dcheng@: Is there any way to type-map mojom::blink::RelatedApplicationPtr to
blink::WebRelatedApplication so that we don't need to write the conversion
manually?

[dcheng, 2017/03/02 07:41:51]

Yeah, this should be pretty easy to typemap, and +1 to doing that here.

[Matt Giuca, 2017/03/10 03:35:47]

This is all going to change... I'm planning to refactor the interface between
NavigatorInstalledApp and InstalledAppController and simplify it to avoid
creating a WebRelatedApplication here at all (going to directly convert to an
IDL/JavaScript RelatedApplication type and resolve the promise). I don't think
it's worth doing a typemap here when it will change soon.

I am waiting until this code lands before working on the refactor. I spent
awhile working on it about a month ago and realised it was kind of big and that
every change I make to this CL would result in a merge conflict on that one, so
I gave up and decided to do it after landing this one. This sort of thing came
up in the previous CL as well:
https://codereview.chromium.org/2488573002/diff/800001/third_party/WebKit/Sou...

[dcheng, 2017/03/13 06:16:51]

Honestly, I'd prefer if the last step (mojom -> IDL type) was still
typemappable. I know we have a lot of manual code to do that today... but it
would be better if we didn't need that. Is this something you'd be willing to
experiment with in a followup? If there are deficiencies in this area, it would
be good to understand what they are so we can improve Mojo in this respect.

[Matt Giuca, 2017/03/14 06:38:11]

I'm happy to play with it after this feature is working (and after the coming
refactor to merge IAC with NavigatorInstalledApp). So the plan is:

1. Land the Android side of this (WIP).
2. Combine InstalledAppController into NavigatorInstalledApp.
3. Play with typemaps (i.e., not a high priority).

[dcheng, 2017/03/14 06:41:53]

I wouldn't really say 3 is not a high priority; manual conversion code is
something we've tried really hard to discourage, because it's hard to find and
audit.

[Matt Giuca, 2017/03/14 06:53:57]

Sure; but I'm going to prioritize it behind actually landing this code because I
want to actually get this working now (the number of yaks I've shaved on this
before and during this CL has resulted in a significant delay; I'm happy to
shave more yaks later but at some point I need to make some progress).

+  std::vector<blink::WebRelatedApplication> applications;
+  for (const auto& res : result) {
+    blink::WebRelatedApplication app;
+    app.platform = res->platform;
+    app.url = res->url;
+    app.id = res->id;
+    applications.push_back(app);
+  }
+  callbacks->onSuccess(
+      blink::WebVector<blink::WebRelatedApplication>(applications));
 }
 
 DEFINE_TRACE(InstalledAppController) {