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

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

Adds the InstalledAppProvider Mojo service for getInstalledRelatedApps,
which takes a list of related apps (from the app manifest) and filters
out those apps that are not installed.

The stub implementation always returns the empty list (Android-specific
real implementation to follow).

Updated layout tests to mock out the Mojo service and therefore fully
test the IDL bindings code.

Based on a CL by dhnishi:
https://codereview.chromium.org/1756793004/

BUG=587623
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2671683002
Cr-Commit-Position: refs/heads/master@{#456652}
Committed: https://chromium.googlesource.com/chromium/src/+/35f4c715eb3bb5735cf8148f24076a65fdfe7cbb

[Matt Giuca, 2017-02-02 07:16:05]

Description was changed from

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

Adds the InstalledAppProvider Mojo service for getInstalledRelatedApps,
which takes a list of related apps (from the app manifest) and filters
out those apps that are not installed.

The stub implementation always returns the empty list (Android-specific
real implementation to follow).

Updated layout tests to mock out the Mojo service and therefore fully
test the IDL bindings code.

Based on a CL by dhnishi:
https://codereview.chromium.org/1756793004/

BUG=587623
==========

to

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

Adds the InstalledAppProvider Mojo service for getInstalledRelatedApps,
which takes a list of related apps (from the app manifest) and filters
out those apps that are not installed.

The stub implementation always returns the empty list (Android-specific
real implementation to follow).

Updated layout tests to mock out the Mojo service and therefore fully
test the IDL bindings code.

Based on a CL by dhnishi:
https://codereview.chromium.org/1756793004/

BUG=587623
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Matt Giuca, 2017-02-23 07:25:12]

mgiuca@chromium.org changed reviewers:
+ jochen@chromium.org, mbarbella@chromium.org

[Matt Giuca, 2017-02-23 07:25:13]

Hi Martin: I randomly chose you from IPC security owners for adding this new
Mojo service. (Also for chrome_content_browser_manifest_overlay.json). There
will be a follow-up implementation on Android.

Hi Jochen: For content/browser owners (hopefully you won't mind me becoming an
owner of the new content/browser/installedapp directory).

Architecture diagram:
https://docs.google.com/drawings/d/1D9zDpCXe3GWVYO2XS4w0mIDY21CHbGhjQVOOJtcWW...

[dcheng, 2017-02-23 08:15:00]

drive-by: let's not check in stubs for mojo/ipc, since it's difficult to do an
ipc security review without any actual implementation. thanks.

[haraken, 2017-02-23 08:35:50]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-02-23 08:35:51]

Drive-by

https://codereview.chromium.org/2671683002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/installedapp/InstalledAppController.h
(right):

https://codereview.chromium.org/2671683002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.h:66:
WeakMember<LocalFrame> m_frame;

You can use supplementable(), which will return a frame for you.

[Matt Giuca, 2017-02-24 01:04:38]

On 2017/02/23 08:15:00, dcheng wrote:
> drive-by: let's not check in stubs for mojo/ipc, since it's difficult to do an
> ipc security review without any actual implementation. thanks.

Here is the follow-up Android CL:
https://codereview.chromium.org/2706403014
It isn't ready for review yet.

Is it really necessary to combine these into a single CL? The follow-up would
also have an IPC security owner on it. Otherwise I'm going to have to block this
on getting the Android CL just perfect and end up with a 650-line CL that spans
Blink, content and Android (many reviewers, much harder to land). This isn't how
I've done things in the past, e.g.:
1. https://codereview.chromium.org/1806253002 (adds Web Share mojo interface)
2. https://codereview.chromium.org/1814133002 (adds Web Share Android mojo
service)

[dcheng, 2017-02-25 08:02:31]

On 2017/02/24 01:04:38, Matt Giuca wrote:
> On 2017/02/23 08:15:00, dcheng wrote:
> > drive-by: let's not check in stubs for mojo/ipc, since it's difficult to do
an
> > ipc security review without any actual implementation. thanks.
> 
> Here is the follow-up Android CL:
> https://codereview.chromium.org/2706403014
> It isn't ready for review yet.
> 
> Is it really necessary to combine these into a single CL? The follow-up would
> also have an IPC security owner on it. Otherwise I'm going to have to block
this
> on getting the Android CL just perfect and end up with a 650-line CL that
spans
> Blink, content and Android (many reviewers, much harder to land). This isn't
how
> I've done things in the past, e.g.:
> 1. https://codereview.chromium.org/1806253002 (adds Web Share mojo interface)
> 2. https://codereview.chromium.org/1814133002 (adds Web Share Android mojo
> service)

I would say it's not desirable. I've seen this happen in the past, where the
followup implementation CLs didn't go through any security review.

[Matt Giuca, 2017-02-26 22:44:09]

On 2017/02/25 08:02:31, dcheng wrote:
> On 2017/02/24 01:04:38, Matt Giuca wrote:
> > On 2017/02/23 08:15:00, dcheng wrote:
> > > drive-by: let's not check in stubs for mojo/ipc, since it's difficult to
do
> an
> > > ipc security review without any actual implementation. thanks.
> > 
> > Here is the follow-up Android CL:
> > https://codereview.chromium.org/2706403014
> > It isn't ready for review yet.
> > 
> > Is it really necessary to combine these into a single CL? The follow-up
would
> > also have an IPC security owner on it. Otherwise I'm going to have to block
> this
> > on getting the Android CL just perfect and end up with a 650-line CL that
> spans
> > Blink, content and Android (many reviewers, much harder to land). This isn't
> how
> > I've done things in the past, e.g.:
> > 1. https://codereview.chromium.org/1806253002 (adds Web Share mojo
interface)
> > 2. https://codereview.chromium.org/1814133002 (adds Web Share Android mojo
> > service)
> 
> I would say it's not desirable. I've seen this happen in the past, where the
> followup implementation CLs didn't go through any security review.

If that's your only concern about splitting it up, then I'll keep it separate
and be sure to add an IPC security reviewer on the follow-up.

(This problem still exists anyway, for example if I was to add a separate
implementation on Windows then I would certainly do that in a separate CL, and
you would expect a security reviewer on that CL as well.)

[Matt Giuca, 2017-02-26 22:47:41]

mgiuca@chromium.org changed reviewers:
+ clamy@chromium.org
- jochen@chromium.org

[Matt Giuca, 2017-02-26 22:47:42]

-jochen (OOO), +clamy: Please review content/browser.

Martin: friendly ping? (IPC security owners).

[clamy, 2017-02-27 14:49:27]

Thanks! One question below.

https://codereview.chromium.org/2671683002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2671683002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2314:
GetInterfaceRegistry()->AddInterface(
Do we envision the InstalledAppProvider to be used on desktop or will it only be
used on Android? If the later, does it make sense to register an interface
that's doing nothing on desktop? Why not register it just on Android?

[Matt Giuca, 2017-02-27 22:51:15]

https://codereview.chromium.org/2671683002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2671683002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2314:
GetInterfaceRegistry()->AddInterface(
On 2017/02/27 14:49:27, clamy wrote:
> Do we envision the InstalledAppProvider to be used on desktop or will it only
be
> used on Android? If the later, does it make sense to register an interface
> that's doing nothing on desktop? Why not register it just on Android?

We might do it on desktop in the future but no immediate plans (and it isn't
clear what that would mean).

We still want it to work on desktop, and simply return the empty list. I think
that is the best answer to "what native apps are installed?" on a platform that
doesn't have a concept of related native apps. (The web platform team does not
want APIs to be undefined on one platform or another if possible.)

If there was no Mojo service, then we would just fail to get a connection and it
would never resolve the promise. The alternative, I guess, would be to
platform-detect in the Blink code and only try to call the Mojo service on
Android. But I think it's cleaner to have Blink be unaware of the platform, and
have the Chrome-side implementation diverge by registering different Mojo
services.

[Matt Giuca, 2017-02-28 23:07:30]

mgiuca@chromium.org changed reviewers:
+ dcheng@chromium.org

[Matt Giuca, 2017-02-28 23:07:31]

+dcheng: Martin hasn't responded. Are you able to be the IPC security owner
since you've already looked at this CL?

[Matt Giuca, 2017-03-02 05:45:34]

Oops, forgot to address haraken@'s comment.

https://codereview.chromium.org/2671683002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/installedapp/InstalledAppController.h
(right):

https://codereview.chromium.org/2671683002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.h:66:
WeakMember<LocalFrame> m_frame;
On 2017/02/23 08:35:51, haraken wrote:
> 
> You can use supplementable(), which will return a frame for you.

Done. Thanks for the suggestion.

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

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
(right):

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:135:
WTF::Vector<mojom::blink::RelatedApplicationPtr> result) {

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]

Sorry for the delay, I missed the reply where you asked me to take a look.

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
(right):

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:119:
mojo::MakeRequest(&m_provider));
We should close m_provider in contextDestroyed(), IMO.

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

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:130:
wrapPersistent(this), WTF::passed(std::move(callbacks)))));
Nit: WTF::passed(&callbacks)

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:135:
WTF::Vector<mojom::blink::RelatedApplicationPtr> result) {
On 2017/03/02 07:33:28, haraken wrote:
> 
> 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?

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

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/installedapp/installed_app_provider.mojom
(right):

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/installedapp/installed_app_provider.mojom:11:
// Filters |relatedApps|, keeping only those which are both installed on the
hacker_style_naming is the convention in most mojoms, even in Blink.

[clamy, 2017-03-02 13:31:54]

Thanks! content/ lgtm with a nit.

https://codereview.chromium.org/2671683002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2671683002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2314:
GetInterfaceRegistry()->AddInterface(
On 2017/02/27 22:51:15, Matt Giuca wrote:
> On 2017/02/27 14:49:27, clamy wrote:
> > Do we envision the InstalledAppProvider to be used on desktop or will it
only
> be
> > used on Android? If the later, does it make sense to register an interface
> > that's doing nothing on desktop? Why not register it just on Android?
> 
> We might do it on desktop in the future but no immediate plans (and it isn't
> clear what that would mean).
> 
> We still want it to work on desktop, and simply return the empty list. I think
> that is the best answer to "what native apps are installed?" on a platform
that
> doesn't have a concept of related native apps. (The web platform team does not
> want APIs to be undefined on one platform or another if possible.)
> 
> If there was no Mojo service, then we would just fail to get a connection and
it
> would never resolve the promise. The alternative, I guess, would be to
> platform-detect in the Blink code and only try to call the Mojo service on
> Android. But I think it's cleaner to have Blink be unaware of the platform,
and
> have the Chrome-side implementation diverge by registering different Mojo
> services.

Acknowledged.

https://codereview.chromium.org/2671683002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2671683002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2283: // The default
(no-op) implementation of InstalledAppProvider.
nit: add an empty line above.

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

Done those things (mostly). Please note Patch Set 8, where I changed the Mojo
interface.

It turns out that passing |origin| from the renderer to browser was a bad idea
(because the browser would trust that the page is coming from that origin -- not
good if the renderer is compromised). I've simply removed that argument; the
browser needs to figure out which origin it is talking to out of band.

https://codereview.chromium.org/2671683002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2671683002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2283: // The default
(no-op) implementation of InstalledAppProvider.
On 2017/03/02 13:31:53, clamy wrote:
> nit: add an empty line above.

Done.

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
(right):

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:119:
mojo::MakeRequest(&m_provider));
On 2017/03/02 07:41:51, dcheng wrote:
> We should close m_provider in contextDestroyed(), IMO.
> 
> (We want to have some abstractions for automatically doing this, but they
don't
> yet exist...)

Done.

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:130:
wrapPersistent(this), WTF::passed(std::move(callbacks)))));
On 2017/03/02 07:41:51, dcheng wrote:
> Nit: WTF::passed(&callbacks)

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

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:135:
WTF::Vector<mojom::blink::RelatedApplicationPtr> result) {
On 2017/03/02 07:41:51, dcheng wrote:
> On 2017/03/02 07:33:28, haraken wrote:
> > 
> > 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?
> 
> Yeah, this should be pretty easy to typemap, and +1 to doing that here.

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

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/installedapp/installed_app_provider.mojom
(right):

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/installedapp/installed_app_provider.mojom:11:
// Filters |relatedApps|, keeping only those which are both installed on the
On 2017/03/02 07:41:51, dcheng wrote:
> hacker_style_naming is the convention in most mojoms, even in Blink.

Done.

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

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
(right):

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:135:
WTF::Vector<mojom::blink::RelatedApplicationPtr> result) {
On 2017/03/10 03:35:47, Matt Giuca wrote:
> On 2017/03/02 07:41:51, dcheng wrote:
> > On 2017/03/02 07:33:28, haraken wrote:
> > > 
> > > 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?
> > 
> > Yeah, this should be pretty easy to typemap, and +1 to doing that here.
> 
> 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...

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.

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installedapp/InstalledAppController.h
(right):

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.h:63:
mojom::blink::InstalledAppProviderPtr m_provider = nullptr;
Aside: = nullptr is unnecessary here (this is the same as the default
constructor)

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom
(right):

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom:11:
string? url;
Nit: please make this a mojo.common.mojom.Url.

[dcheng, 2017-03-13 07:15:08]

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom
(right):

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom:8:
// See: https://www.w3.org/TR/appmanifest/#related_applications-member
https://www.w3.org/TR/appmanifest/#dfn-application-object might be a more
helpful link (or in addition... I read the link here and had to click through
once or twice to find the application object definition, which I found more
useful)

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

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
(right):

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:135:
WTF::Vector<mojom::blink::RelatedApplicationPtr> result) {
On 2017/03/13 06:16:51, dcheng wrote:
> On 2017/03/10 03:35:47, Matt Giuca wrote:
> > On 2017/03/02 07:41:51, dcheng wrote:
> > > On 2017/03/02 07:33:28, haraken wrote:
> > > > 
> > > > 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?
> > > 
> > > Yeah, this should be pretty easy to typemap, and +1 to doing that here.
> > 
> > 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...
> 
> 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.

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

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installedapp/InstalledAppController.h
(right):

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.h:63:
mojom::blink::InstalledAppProviderPtr m_provider = nullptr;
On 2017/03/13 06:16:51, dcheng wrote:
> Aside: = nullptr is unnecessary here (this is the same as the default
> constructor)

Done.

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom
(right):

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom:8:
// See: https://www.w3.org/TR/appmanifest/#related_applications-member
On 2017/03/13 07:15:08, dcheng wrote:
> https://www.w3.org/TR/appmanifest/#dfn-application-object might be a more
> helpful link (or in addition... I read the link here and had to click through
> once or twice to find the application object definition, which I found more
> useful)

Better! Thanks.

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom:11:
string? url;
On 2017/03/13 06:16:51, dcheng wrote:
> Nit: please make this a mojo.common.mojom.Url.

OK, you're right, this should be a Url. But this is a pretty big refactor which
requires also converting the existing WebRelatedApplication's url field from
WebString to WebUrl (or else the URL gets re-parsed several times).

I did this change: https://codereview.chromium.org/2748023002

It touches too many unrelated things to do it in this CL, and also it broke my
Android implementation CL in a possibly-critical way ("class file for
org.chromium.url.mojom.Url not found" -- this might indicate a problem with
Mojo). I'd rather save this change for a follow-up and not try to do it now.

[dcheng, 2017-03-14 06:41:54]

LGTM with nits

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
(right):

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:135:
WTF::Vector<mojom::blink::RelatedApplicationPtr> result) {
On 2017/03/14 06:38:11, Matt Giuca wrote:
> On 2017/03/13 06:16:51, dcheng wrote:
> > On 2017/03/10 03:35:47, Matt Giuca wrote:
> > > On 2017/03/02 07:41:51, dcheng wrote:
> > > > On 2017/03/02 07:33:28, haraken wrote:
> > > > > 
> > > > > 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?
> > > > 
> > > > Yeah, this should be pretty easy to typemap, and +1 to doing that here.
> > > 
> > > 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...
> > 
> > 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.
> 
> 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).

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.

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom
(right):

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom:11:
string? url;
On 2017/03/14 06:38:11, Matt Giuca wrote:
> On 2017/03/13 06:16:51, dcheng wrote:
> > Nit: please make this a mojo.common.mojom.Url.
> 
> OK, you're right, this should be a Url. But this is a pretty big refactor
which
> requires also converting the existing WebRelatedApplication's url field from
> WebString to WebUrl (or else the URL gets re-parsed several times).
> 
> I did this change: https://codereview.chromium.org/2748023002
> 
> It touches too many unrelated things to do it in this CL, and also it broke my
> Android implementation CL in a possibly-critical way ("class file for
> org.chromium.url.mojom.Url not found" -- this might indicate a problem with
> Mojo). I'd rather save this change for a follow-up and not try to do it now.

Please add a TODO

[dcheng, 2017-03-14 06:43:53]

(I'll start a thread about IDL typemapping later as well, so I guess we can
wait to see what people have to say there)

Daniel

On Mon, Mar 13, 2017 at 11:41 PM <dcheng@chromium.org> wrote:

> LGTM with nits
>
>
>
>
>
https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
> (right):
>
>
>
https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
>
>
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:135:
> WTF::Vector<mojom::blink::RelatedApplicationPtr> result) {
> On 2017/03/14 06:38:11, Matt Giuca wrote:
> > On 2017/03/13 06:16:51, dcheng wrote:
> > > On 2017/03/10 03:35:47, Matt Giuca wrote:
> > > > On 2017/03/02 07:41:51, dcheng wrote:
> > > > > On 2017/03/02 07:33:28, haraken wrote:
> > > > > >
> > > > > > 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?
> > > > >
> > > > > Yeah, this should be pretty easy to typemap, and +1 to doing
> that here.
> > > >
> > > > 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...
> > >
> > > 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.
> >
> > 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).
>
> 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.
>
>
>
>
https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
> File
>
>
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom
> (right):
>
>
>
https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
>
>
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom:11:
> string? url;
> On 2017/03/14 06:38:11, Matt Giuca wrote:
> > On 2017/03/13 06:16:51, dcheng wrote:
> > > Nit: please make this a mojo.common.mojom.Url.
> >
> > OK, you're right, this should be a Url. But this is a pretty big
> refactor which
> > requires also converting the existing WebRelatedApplication's url
> field from
> > WebString to WebUrl (or else the URL gets re-parsed several times).
> >
> > I did this change: https://codereview.chromium.org/2748023002
> >
> > It touches too many unrelated things to do it in this CL, and also it
> broke my
> > Android implementation CL in a possibly-critical way ("class file for
> > org.chromium.url.mojom.Url not found" -- this might indicate a problem
> with
> > Mojo). I'd rather save this change for a follow-up and not try to do
> it now.
>
> Please add a TODO
>
> https://codereview.chromium.org/2671683002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "blink-reviews-api" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-reviews-api+unsubscribe@chromium.org.
> To post to this group, send email to blink-reviews-api@chromium.org.
> To view this discussion on the web visit
>
https://groups.google.com/a/chromium.org/d/msgid/blink-reviews-api/001a113ebf...
>
<https://groups.google.com/a/chromium.org/d/msgid/blink-reviews-api/001a113ebf...>
> .
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dcheng, 2017-03-14 06:43:53]

(I'll start a thread about IDL typemapping later as well, so I guess we can
wait to see what people have to say there)

Daniel

On Mon, Mar 13, 2017 at 11:41 PM <dcheng@chromium.org> wrote:

> LGTM with nits
>
>
>
>
>
https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
> (right):
>
>
>
https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
>
>
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:135:
> WTF::Vector<mojom::blink::RelatedApplicationPtr> result) {
> On 2017/03/14 06:38:11, Matt Giuca wrote:
> > On 2017/03/13 06:16:51, dcheng wrote:
> > > On 2017/03/10 03:35:47, Matt Giuca wrote:
> > > > On 2017/03/02 07:41:51, dcheng wrote:
> > > > > On 2017/03/02 07:33:28, haraken wrote:
> > > > > >
> > > > > > 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?
> > > > >
> > > > > Yeah, this should be pretty easy to typemap, and +1 to doing
> that here.
> > > >
> > > > 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...
> > >
> > > 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.
> >
> > 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).
>
> 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.
>
>
>
>
https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
> File
>
>
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom
> (right):
>
>
>
https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
>
>
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom:11:
> string? url;
> On 2017/03/14 06:38:11, Matt Giuca wrote:
> > On 2017/03/13 06:16:51, dcheng wrote:
> > > Nit: please make this a mojo.common.mojom.Url.
> >
> > OK, you're right, this should be a Url. But this is a pretty big
> refactor which
> > requires also converting the existing WebRelatedApplication's url
> field from
> > WebString to WebUrl (or else the URL gets re-parsed several times).
> >
> > I did this change: https://codereview.chromium.org/2748023002
> >
> > It touches too many unrelated things to do it in this CL, and also it
> broke my
> > Android implementation CL in a possibly-critical way ("class file for
> > org.chromium.url.mojom.Url not found" -- this might indicate a problem
> with
> > Mojo). I'd rather save this change for a follow-up and not try to do
> it now.
>
> Please add a TODO
>
> https://codereview.chromium.org/2671683002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "blink-reviews-api" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-reviews-api+unsubscribe@chromium.org.
> To post to this group, send email to blink-reviews-api@chromium.org.
> To view this discussion on the web visit
>
https://groups.google.com/a/chromium.org/d/msgid/blink-reviews-api/001a113ebf...
>
<https://groups.google.com/a/chromium.org/d/msgid/blink-reviews-api/001a113ebf...>
> .
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

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

Done.

I literally can't see whether I have enough reviewers so going to check the
commit box and see what the story is.

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
(right):

https://codereview.chromium.org/2671683002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:135:
WTF::Vector<mojom::blink::RelatedApplicationPtr> result) {
On 2017/03/14 06:41:53, dcheng wrote:
> On 2017/03/14 06:38:11, Matt Giuca wrote:
> > On 2017/03/13 06:16:51, dcheng wrote:
> > > On 2017/03/10 03:35:47, Matt Giuca wrote:
> > > > On 2017/03/02 07:41:51, dcheng wrote:
> > > > > On 2017/03/02 07:33:28, haraken wrote:
> > > > > > 
> > > > > > 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?
> > > > > 
> > > > > Yeah, this should be pretty easy to typemap, and +1 to doing that
here.
> > > > 
> > > > 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...
> > > 
> > > 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.
> > 
> > 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).
> 
> 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.

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

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom
(right):

https://codereview.chromium.org/2671683002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/installedapp/related_application.mojom:11:
string? url;
On 2017/03/14 06:41:53, dcheng wrote:
> On 2017/03/14 06:38:11, Matt Giuca wrote:
> > On 2017/03/13 06:16:51, dcheng wrote:
> > > Nit: please make this a mojo.common.mojom.Url.
> > 
> > OK, you're right, this should be a Url. But this is a pretty big refactor
> which
> > requires also converting the existing WebRelatedApplication's url field from
> > WebString to WebUrl (or else the URL gets re-parsed several times).
> > 
> > I did this change: https://codereview.chromium.org/2748023002
> > 
> > It touches too many unrelated things to do it in this CL, and also it broke
my
> > Android implementation CL in a possibly-critical way ("class file for
> > org.chromium.url.mojom.Url not found" -- this might indicate a problem with
> > Mojo). I'd rather save this change for a follow-up and not try to do it now.
> 
> Please add a TODO

Done.

[Matt Giuca, 2017-03-14 06:54:02]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2017-03-14 06:54:03]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2671683002/#ps200001
(title: "Added TODO.")

[commit-bot: I haz the power, 2017-03-14 06:54:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 08:30:37]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1489474442642240,
"parent_rev": "cd45ce00fc6cf88a65b370c1a574e5b26319a40b", "commit_rev":
"35f4c715eb3bb5735cf8148f24076a65fdfe7cbb"}

[commit-bot: I haz the power, 2017-03-14 08:31:41]

Description was changed from

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

Adds the InstalledAppProvider Mojo service for getInstalledRelatedApps,
which takes a list of related apps (from the app manifest) and filters
out those apps that are not installed.

The stub implementation always returns the empty list (Android-specific
real implementation to follow).

Updated layout tests to mock out the Mojo service and therefore fully
test the IDL bindings code.

Based on a CL by dhnishi:
https://codereview.chromium.org/1756793004/

BUG=587623
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

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

Adds the InstalledAppProvider Mojo service for getInstalledRelatedApps,
which takes a list of related apps (from the app manifest) and filters
out those apps that are not installed.

The stub implementation always returns the empty list (Android-specific
real implementation to follow).

Updated layout tests to mock out the Mojo service and therefore fully
test the IDL bindings code.

Based on a CL by dhnishi:
https://codereview.chromium.org/1756793004/

BUG=587623
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2671683002
Cr-Commit-Position: refs/heads/master@{#456652}
Committed:
https://chromium.googlesource.com/chromium/src/+/35f4c715eb3bb5735cf8148f2407...
==========

[commit-bot: I haz the power, 2017-03-14 08:31:42]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/35f4c715eb3bb5735cf8148f2407...

[haraken, 2017-03-14 08:58:55]

https://codereview.chromium.org/2671683002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
(right):

https://codereview.chromium.org/2671683002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:106:
convertedApplication->url = relatedApplication.url;

Would it be possible to type-map between RelatedApplicationPtr and
WebRelatedApplication?

https://codereview.chromium.org/2671683002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:134:
app.id = res->id;

Ditto.

[Matt Giuca, 2017-03-14 23:21:04]

https://codereview.chromium.org/2671683002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
(right):

https://codereview.chromium.org/2671683002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:106:
convertedApplication->url = relatedApplication.url;
On 2017/03/14 08:58:55, haraken wrote:
> 
> Would it be possible to type-map between RelatedApplicationPtr and
> WebRelatedApplication?

This was discussed above (see #17 to #23). I think the typemap there is
unnecessary as I'll remove this intermediate step of converting RAP to WRA (I'm
not sure why it was there in the first place; before my time).

But some typemaps are probably a good idea... we have FOUR different structures
for representing a RelatedApplication (content::Manifest::RelatedApplication,
blink::mojom::RelatedApplication, blink::RelatedApplication [the JavaScript
type], and blink::WebRelatedApplication). This whole process converts back and
forth between all four of these types, so it would be good to have some type
mapping. But I think the first step is to simplify the logic to avoid so many
conversions in the first place.

[haraken, 2017-03-15 07:41:14]

On 2017/03/14 23:21:04, Matt Giuca wrote:
>
https://codereview.chromium.org/2671683002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp
> (right):
> 
>
https://codereview.chromium.org/2671683002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/installedapp/InstalledAppController.cpp:106:
> convertedApplication->url = relatedApplication.url;
> On 2017/03/14 08:58:55, haraken wrote:
> > 
> > Would it be possible to type-map between RelatedApplicationPtr and
> > WebRelatedApplication?
> 
> This was discussed above (see #17 to #23). I think the typemap there is
> unnecessary as I'll remove this intermediate step of converting RAP to WRA
(I'm
> not sure why it was there in the first place; before my time).
> 
> But some typemaps are probably a good idea... we have FOUR different
structures
> for representing a RelatedApplication (content::Manifest::RelatedApplication,
> blink::mojom::RelatedApplication, blink::RelatedApplication [the JavaScript
> type], and blink::WebRelatedApplication). This whole process converts back and
> forth between all four of these types, so it would be good to have some type
> mapping. But I think the first step is to simplify the logic to avoid so many
> conversions in the first place.

Ah, sorry I missed the discussion. LGTM.