Move creation of plugin placeholder DOM from JS to C++.

Move creation of plugin placeholder DOM from JS to C++.

The JavaScript code remains responsible for the hiding logic, but this
gives C++ control over the creation of the constituent elements, and
lets it freely bind its own event listeners to them without introducing
private script APIs.

There is no functionality change in this CL, and it is pretty much a
direct translation of the JavaScript code back into corresponding C++.

BUG=364716
TEST=fast/plugins, webkit_unit_tests:PluginPlaceholderImplTest.*

[jbroman, 2014-11-19 23:31:21]

jbroman@chromium.org changed reviewers:
+ esprehn@chromium.org, mkwst@chromium.org

[jbroman, 2014-11-19 23:31:22]

There's a lot of concern about dispatching from event listeners in private
script (Blink-in-JS) back to C++ code, so I'm moving the DOM ownership over to
C++, where we can handle these events without introducing private script APIs.

This CL should functionality-neutral (and the tests continue to pass), but
handles DOM creation and event handling in a way that we can do entirely from
Blink C++ code. This paves the way for handling events for "reload plugin" etc.

[Mike West, 2014-11-20 08:35:07]

mkwst@chromium.org changed reviewers:
+ haraken@chromium.org, jochen@chromium.org

[Mike West, 2014-11-20 08:35:08]

:(

I'd like to be able to do this in JS; it seems like a pretty clear example of
something that doesn't need to be in C++. I'd suggest that we not move this back
out of JS.

Adding Kentaro-san and Jochen for opinions.

[haraken, 2014-11-20 08:37:16]

On 2014/11/19 23:31:22, jbroman wrote:
> There's a lot of concern about dispatching from event listeners in private
> script (Blink-in-JS) back to C++ code, so I'm moving the DOM ownership over to
> C++, where we can handle these events without introducing private script APIs.
> 
> This CL should functionality-neutral (and the tests continue to pass), but
> handles DOM creation and event handling in a way that we can do entirely from
> Blink C++ code. This paves the way for handling events for "reload plugin"
etc.

What's feedback of PlaceHolderElement experts (Elliott? Dimitri?) about the idea
of dispatching custom events?

[jochen (gone - plz use gerrit), 2014-11-20 15:25:38]

On 2014/11/20 at 08:37:16, haraken wrote:
> On 2014/11/19 23:31:22, jbroman wrote:
> > There's a lot of concern about dispatching from event listeners in private
> > script (Blink-in-JS) back to C++ code, so I'm moving the DOM ownership over
to
> > C++, where we can handle these events without introducing private script
APIs.
> > 
> > This CL should functionality-neutral (and the tests continue to pass), but
> > handles DOM creation and event handling in a way that we can do entirely
from
> > Blink C++ code. This paves the way for handling events for "reload plugin"
etc.
> 
> What's feedback of PlaceHolderElement experts (Elliott? Dimitri?) about the
idea of dispatching custom events?

I'm still unconvinced this is the way to go.

The old plugin placeholder was already using javascript. It was also on a layer
where it could talk to the rest of the plugin loading system (like getting
updates for changed content settings).

I understand that you already invested a lot of energy into this approach, but I
think if the right thing might be something else, we should not stick with a
solution that is having lots of difficulties.

Should we have a discussion on blink-dev@ about this feature?

[jbroman, 2014-11-20 17:54:18]

On 2014/11/20 15:25:38, jochen (slow) wrote:
> On 2014/11/20 at 08:37:16, haraken wrote:
> > On 2014/11/19 23:31:22, jbroman wrote:
> > > There's a lot of concern about dispatching from event listeners in private
> > > script (Blink-in-JS) back to C++ code, so I'm moving the DOM ownership
over
> to
> > > C++, where we can handle these events without introducing private script
> APIs.
> > > 
> > > This CL should functionality-neutral (and the tests continue to pass), but
> > > handles DOM creation and event handling in a way that we can do entirely
> from
> > > Blink C++ code. This paves the way for handling events for "reload plugin"
> etc.
> > 
> > What's feedback of PlaceHolderElement experts (Elliott? Dimitri?) about the
> idea of dispatching custom events?
> 
> I'm still unconvinced this is the way to go.
> 
> The old plugin placeholder was already using javascript. It was also on a
layer
> where it could talk to the rest of the plugin loading system (like getting
> updates for changed content settings).

It also relies on hosting a WebView inside a plugin using an old code path
(it has a null layer tree host, IIUC).

Here's a summary of the past five months of history leading to my current
approach:
 
https://docs.google.com/a/google.com/document/d/1xGuqIewYfuzgk8gNgXrx0C09-75s...

> I understand that you already invested a lot of energy into this approach, but
I
> think if the right thing might be something else, we should not stick with a
> solution that is having lots of difficulties.
> 
> Should we have a discussion on blink-dev@ about this feature?

Do you think such a thread would be productive? I'm concerned it would consist
of rehashing various previous discussions.

[esprehn, 2014-11-20 18:07:10]

Can you explain the event issue? It seems like we should expose a very targeted
API on the <object>/<embed> elements for private script like loadPlugin() and
when there's a click you can call it and trigger the actual load.

[jbroman, 2014-11-20 18:17:19]

On 2014/11/20 18:07:10, esprehn wrote:
> Can you explain the event issue? It seems like we should expose a very
targeted
> API on the <object>/<embed> elements for private script like loadPlugin() and
> when there's a click you can call it and trigger the actual load.

The Blink-in-JS folk have told me that this sort of private script API is not
okay because it is neither to-be-deprecated nor to-be-exposed. (I've heard this
object even if it is defined on PluginPlaceholderElement, which is an object
that shouldn't be reachable at all from author script.)

The second case that I expect to require invoking C++ code is opening the
plugin settings (the current placeholders have a link to chrome://plugins in the
message for disabled plugins, though it doesn't seem to currently work). We
don't ordinarily permit navigation to WebUI URLs, so that might require the
help of C++. But I could imagine a similarly small private API to tell the
embedder
that its link was clicked, or whatever.

[jochen (gone - plz use gerrit), 2014-11-20 18:40:45]

On 2014/11/20 at 17:54:18, jbroman wrote:
> On 2014/11/20 15:25:38, jochen (slow) wrote:
> > On 2014/11/20 at 08:37:16, haraken wrote:
> > > On 2014/11/19 23:31:22, jbroman wrote:
> > > > There's a lot of concern about dispatching from event listeners in
private
> > > > script (Blink-in-JS) back to C++ code, so I'm moving the DOM ownership
over
> > to
> > > > C++, where we can handle these events without introducing private script
> > APIs.
> > > > 
> > > > This CL should functionality-neutral (and the tests continue to pass),
but
> > > > handles DOM creation and event handling in a way that we can do entirely
> > from
> > > > Blink C++ code. This paves the way for handling events for "reload
plugin"
> > etc.
> > > 
> > > What's feedback of PlaceHolderElement experts (Elliott? Dimitri?) about
the
> > idea of dispatching custom events?
> > 
> > I'm still unconvinced this is the way to go.
> > 
> > The old plugin placeholder was already using javascript. It was also on a
layer
> > where it could talk to the rest of the plugin loading system (like getting
> > updates for changed content settings).
> 
> It also relies on hosting a WebView inside a plugin using an old code path
> (it has a null layer tree host, IIUC).
> 
> Here's a summary of the past five months of history leading to my current
approach:
>  
https://docs.google.com/a/google.com/document/d/1xGuqIewYfuzgk8gNgXrx0C09-75s...
> 
> > I understand that you already invested a lot of energy into this approach,
but I
> > think if the right thing might be something else, we should not stick with a
> > solution that is having lots of difficulties.
> > 
> > Should we have a discussion on blink-dev@ about this feature?
> 
> Do you think such a thread would be productive? I'm concerned it would consist
of rehashing various previous discussions.

Can you point me at the previous discussions so I can read up on this?

[jbroman, 2014-11-20 18:50:25]

On 2014/11/20 18:40:45, jochen (slow) wrote:
> On 2014/11/20 at 17:54:18, jbroman wrote:
> > On 2014/11/20 15:25:38, jochen (slow) wrote:
> > > On 2014/11/20 at 08:37:16, haraken wrote:
> > > > On 2014/11/19 23:31:22, jbroman wrote:
> > > > > There's a lot of concern about dispatching from event listeners in
> private
> > > > > script (Blink-in-JS) back to C++ code, so I'm moving the DOM ownership
> over
> > > to
> > > > > C++, where we can handle these events without introducing private
script
> > > APIs.
> > > > > 
> > > > > This CL should functionality-neutral (and the tests continue to pass),
> but
> > > > > handles DOM creation and event handling in a way that we can do
entirely
> > > from
> > > > > Blink C++ code. This paves the way for handling events for "reload
> plugin"
> > > etc.
> > > > 
> > > > What's feedback of PlaceHolderElement experts (Elliott? Dimitri?) about
> the
> > > idea of dispatching custom events?
> > > 
> > > I'm still unconvinced this is the way to go.
> > > 
> > > The old plugin placeholder was already using javascript. It was also on a
> layer
> > > where it could talk to the rest of the plugin loading system (like getting
> > > updates for changed content settings).
> > 
> > It also relies on hosting a WebView inside a plugin using an old code path
> > (it has a null layer tree host, IIUC).
> > 
> > Here's a summary of the past five months of history leading to my current
> approach:
> >  
>
https://docs.google.com/a/google.com/document/d/1xGuqIewYfuzgk8gNgXrx0C09-75s...
> > 
> > > I understand that you already invested a lot of energy into this approach,
> but I
> > > think if the right thing might be something else, we should not stick with
a
> > > solution that is having lots of difficulties.
> > > 
> > > Should we have a discussion on blink-dev@ about this feature?
> > 
> > Do you think such a thread would be productive? I'm concerned it would
consist
> of rehashing various previous discussions.
> 
> Can you point me at the previous discussions so I can read up on this?

The discussions were off-list, but I've forwarded them to you. There's also
earlier
discussion on the bug itself
(https://code.google.com/p/chromium/issues/detail?id=364716).

I've also had shorter conversations on the topic in various CLs hanging off that
bug,
and via instant messaging (which, due to corp IM settings, I mostly don't have
logs of).

[jbroman, 2014-11-20 18:50:25]

On 2014/11/20 18:40:45, jochen (slow) wrote:
> On 2014/11/20 at 17:54:18, jbroman wrote:
> > On 2014/11/20 15:25:38, jochen (slow) wrote:
> > > On 2014/11/20 at 08:37:16, haraken wrote:
> > > > On 2014/11/19 23:31:22, jbroman wrote:
> > > > > There's a lot of concern about dispatching from event listeners in
> private
> > > > > script (Blink-in-JS) back to C++ code, so I'm moving the DOM ownership
> over
> > > to
> > > > > C++, where we can handle these events without introducing private
script
> > > APIs.
> > > > > 
> > > > > This CL should functionality-neutral (and the tests continue to pass),
> but
> > > > > handles DOM creation and event handling in a way that we can do
entirely
> > > from
> > > > > Blink C++ code. This paves the way for handling events for "reload
> plugin"
> > > etc.
> > > > 
> > > > What's feedback of PlaceHolderElement experts (Elliott? Dimitri?) about
> the
> > > idea of dispatching custom events?
> > > 
> > > I'm still unconvinced this is the way to go.
> > > 
> > > The old plugin placeholder was already using javascript. It was also on a
> layer
> > > where it could talk to the rest of the plugin loading system (like getting
> > > updates for changed content settings).
> > 
> > It also relies on hosting a WebView inside a plugin using an old code path
> > (it has a null layer tree host, IIUC).
> > 
> > Here's a summary of the past five months of history leading to my current
> approach:
> >  
>
https://docs.google.com/a/google.com/document/d/1xGuqIewYfuzgk8gNgXrx0C09-75s...
> > 
> > > I understand that you already invested a lot of energy into this approach,
> but I
> > > think if the right thing might be something else, we should not stick with
a
> > > solution that is having lots of difficulties.
> > > 
> > > Should we have a discussion on blink-dev@ about this feature?
> > 
> > Do you think such a thread would be productive? I'm concerned it would
consist
> of rehashing various previous discussions.
> 
> Can you point me at the previous discussions so I can read up on this?

The discussions were off-list, but I've forwarded them to you. There's also
earlier
discussion on the bug itself
(https://code.google.com/p/chromium/issues/detail?id=364716).

I've also had shorter conversations on the topic in various CLs hanging off that
bug,
and via instant messaging (which, due to corp IM settings, I mostly don't have
logs of).

[esprehn, 2014-11-20 19:01:44]

Adding a void loadPlugin() API seems reasonable even though it doesn't fit those
two criteria. As for the navigation, we should allow links in UA ShadowRoot's to
navigate to chrome internal urls. That can be handled in the C++ without any new
API in JS.

[jochen (gone - plz use gerrit), 2014-11-20 19:10:58]

On 2014/11/20 at 18:50:25, jbroman wrote:
> On 2014/11/20 18:40:45, jochen (slow) wrote:
> > On 2014/11/20 at 17:54:18, jbroman wrote:
> > > On 2014/11/20 15:25:38, jochen (slow) wrote:
> > > > On 2014/11/20 at 08:37:16, haraken wrote:
> > > > > On 2014/11/19 23:31:22, jbroman wrote:
> > > > > > There's a lot of concern about dispatching from event listeners in
> > private
> > > > > > script (Blink-in-JS) back to C++ code, so I'm moving the DOM
ownership
> > over
> > > > to
> > > > > > C++, where we can handle these events without introducing private
script
> > > > APIs.
> > > > > > 
> > > > > > This CL should functionality-neutral (and the tests continue to
pass),
> > but
> > > > > > handles DOM creation and event handling in a way that we can do
entirely
> > > > from
> > > > > > Blink C++ code. This paves the way for handling events for "reload
> > plugin"
> > > > etc.
> > > > > 
> > > > > What's feedback of PlaceHolderElement experts (Elliott? Dimitri?)
about
> > the
> > > > idea of dispatching custom events?
> > > > 
> > > > I'm still unconvinced this is the way to go.
> > > > 
> > > > The old plugin placeholder was already using javascript. It was also on
a
> > layer
> > > > where it could talk to the rest of the plugin loading system (like
getting
> > > > updates for changed content settings).
> > > 
> > > It also relies on hosting a WebView inside a plugin using an old code path
> > > (it has a null layer tree host, IIUC).
> > > 
> > > Here's a summary of the past five months of history leading to my current
> > approach:
> > >  
> >
https://docs.google.com/a/google.com/document/d/1xGuqIewYfuzgk8gNgXrx0C09-75s...
> > > 
> > > > I understand that you already invested a lot of energy into this
approach,
> > but I
> > > > think if the right thing might be something else, we should not stick
with a
> > > > solution that is having lots of difficulties.
> > > > 
> > > > Should we have a discussion on blink-dev@ about this feature?
> > > 
> > > Do you think such a thread would be productive? I'm concerned it would
consist
> > of rehashing various previous discussions.
> > 
> > Can you point me at the previous discussions so I can read up on this?
> 
> The discussions were off-list, but I've forwarded them to you. There's also
earlier
> discussion on the bug itself
(https://code.google.com/p/chromium/issues/detail?id=364716).
> 
> I've also had shorter conversations on the topic in various CLs hanging off
that bug,
> and via instant messaging (which, due to corp IM settings, I mostly don't have
logs of).

it's unfortunate that the discussion didn't take place in public.

In any case, we've agreed that adding private script APIs should be discussed on
blink-dev. I don't think it makes sense to continue to work on this in a bit by
bit fashion.

If we decide to stick with this approach, we should decide all APIs that this
will require at once (I'd like to understand how messages about changed content
settings will be handled, and where the actual js/html content comes from).

[esprehn, 2016-03-18 05:08:57]

Can we close this?

[jbroman, 2016-03-18 14:15:44]

On 2016/03/18 at 05:08:57, esprehn wrote:
> Can we close this?

Yup, closed.