Add a custom element to own structure of shadow DOM plugin placeholders.

Add a custom element to own structure of shadow DOM plugin placeholders.

This creates PluginPlaceholderElement, which manages a subtree of
elements which represent plugin placeholder content and exposes an
interface to manipulate this content. It is expected that this interface
will grow to encompass more than a single string in the future.

This change also adds an Internals method which uses this, so that its
appearance can be covered by layout tests, and adds one such layout test.

Ultimately, this class will be manipulated by a glue class which controls
this content on behalf of the embedder.

BUG=364716
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182014

[jbroman, 2014-08-29 15:30:07]

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

[dglazkov, 2014-08-29 16:01:11]

On 2014/08/29 at 15:30:07, jbroman wrote:
> 

Is this a good fit for Blink-in-JS?

[esprehn, 2014-08-29 21:13:26]

I think this just wants to be blink-in-js, but we can do this if you want.

https://codereview.chromium.org/522783002/diff/1/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/522783002/diff/1/LayoutTests/TestExpectations...
LayoutTests/TestExpectations:1093: crbug.com/364716
fast/plugins/plugin-placeholder-structured.html [ NeedsRebaseline ]
You should write a ref test, you don't need to write a pixel test.

https://codereview.chromium.org/522783002/diff/1/Source/core/html/shadow/Plug...
File Source/core/html/shadow/PluginPlaceholderElement.cpp (right):

https://codereview.chromium.org/522783002/diff/1/Source/core/html/shadow/Plug...
Source/core/html/shadow/PluginPlaceholderElement.cpp:25: if
(element->initializePlaceholderElements())
This shouldn't be allowed to return nullptr, create() should never fail.

Also this should use Shadow DOM, you want to do ensureUserAgentShadowRoot()
here.

https://codereview.chromium.org/522783002/diff/1/Source/core/html/shadow/Plug...
Source/core/html/shadow/PluginPlaceholderElement.cpp:42: bool
PluginPlaceholderElement::initializePlaceholderElements()
Just use didAddUserAgentShadowRoot(ShadowRoot& root)

and append these elements to the shadow root.

https://codereview.chromium.org/522783002/diff/1/Source/core/html/shadow/Plug...
Source/core/html/shadow/PluginPlaceholderElement.cpp:77: if
(exceptionState.hadException())
ASSERT_NO_EXCEPTION

https://codereview.chromium.org/522783002/diff/1/Source/core/html/shadow/Plug...
Source/core/html/shadow/PluginPlaceholderElement.cpp:80:
appendChild(contentElement.release(), exceptionState);
ASSERT_NO_EXCEPTION, don't check it. Internal code should never throw.

https://codereview.chromium.org/522783002/diff/1/Source/core/html/shadow/Plug...
Source/core/html/shadow/PluginPlaceholderElement.cpp:84: return true;
no return value, just use the Shadow DOM framework.

https://codereview.chromium.org/522783002/diff/1/Source/core/testing/Internal...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/522783002/diff/1/Source/core/testing/Internal...
Source/core/testing/Internals.cpp:2235: void
Internals::forcePluginPlaceholder(HTMLElement* element, const Dictionary&
options, ExceptionState& exceptionState)
HTMLObjectElement*

[jbroman, 2014-08-30 13:55:31]

On 2014/08/29 16:01:11, dglazkov wrote:
> Is this a good fit for Blink-in-JS?

On 2014/08/29 21:13:26, esprehn wrote:
> I think this just wants to be blink-in-js, but we can do this if you want.

I'd forgotten about Blink-in-JS when writing this CL. I'll have a look at how
this looks with Blink-in-JS and upload another patchset.

[jbroman, 2014-08-30 13:55:49]

Replying to comments on this patchset nonetheless.

https://codereview.chromium.org/522783002/diff/1/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/522783002/diff/1/LayoutTests/TestExpectations...
LayoutTests/TestExpectations:1093: crbug.com/364716
fast/plugins/plugin-placeholder-structured.html [ NeedsRebaseline ]
On 2014/08/29 21:13:25, esprehn wrote:
> You should write a ref test, you don't need to write a pixel test.

OK. I suppose I can, though the ref test will need to be changed each time the
plugin placeholder appearance changes (effectively it'll have a copy of the CSS
for plugin placeholders, etc.).

https://codereview.chromium.org/522783002/diff/1/Source/core/html/shadow/Plug...
File Source/core/html/shadow/PluginPlaceholderElement.cpp (right):

https://codereview.chromium.org/522783002/diff/1/Source/core/html/shadow/Plug...
Source/core/html/shadow/PluginPlaceholderElement.cpp:25: if
(element->initializePlaceholderElements())
On 2014/08/29 21:13:26, esprehn wrote:
> This shouldn't be allowed to return nullptr, create() should never fail.
> 
> Also this should use Shadow DOM, you want to do ensureUserAgentShadowRoot()
> here.

The shadow root host is the HTMLPlugInElement (object, embed or applet). This
element is intended to be attached as a child of that shadow root, so it doesn't
itself need a UA shadow root.

While it could implicitly attach itself to the shadow root of an
HTMLPlugInElement it took as a construction argument, that behaviour does not
seem expected.

See the new Internals::forcePluginPlaceholder overload for an example; the
production call site will probably be in the Source/web/ glue class
(WebPluginPlaceholderContainerImpl, or whatever other monstrous name it ends up
getting).

https://codereview.chromium.org/522783002/diff/1/Source/core/html/shadow/Plug...
Source/core/html/shadow/PluginPlaceholderElement.cpp:42: bool
PluginPlaceholderElement::initializePlaceholderElements()
On 2014/08/29 21:13:26, esprehn wrote:
> Just use didAddUserAgentShadowRoot(ShadowRoot& root)
> 
> and append these elements to the shadow root.

See above.

https://codereview.chromium.org/522783002/diff/1/Source/core/html/shadow/Plug...
Source/core/html/shadow/PluginPlaceholderElement.cpp:77: if
(exceptionState.hadException())
On 2014/08/29 21:13:26, esprehn wrote:
> ASSERT_NO_EXCEPTION

Okay. I assumed that tracking the exception and possibly returning null was
preferred in this pattern, as that is what, for example, MediaControls::create
does.

If I rewrite this with Blink-in-JS, this point will be moot anyhow.

https://codereview.chromium.org/522783002/diff/1/Source/core/html/shadow/Plug...
Source/core/html/shadow/PluginPlaceholderElement.cpp:80:
appendChild(contentElement.release(), exceptionState);
On 2014/08/29 21:13:26, esprehn wrote:
> ASSERT_NO_EXCEPTION, don't check it. Internal code should never throw.

Ditto.

https://codereview.chromium.org/522783002/diff/1/Source/core/html/shadow/Plug...
Source/core/html/shadow/PluginPlaceholderElement.cpp:84: return true;
On 2014/08/29 21:13:26, esprehn wrote:
> no return value, just use the Shadow DOM framework.

I understand the "no return value", though I'm not sure what you mean by "use
the Shadow DOM frameowrk".

https://codereview.chromium.org/522783002/diff/1/Source/core/testing/Internal...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/522783002/diff/1/Source/core/testing/Internal...
Source/core/testing/Internals.cpp:2235: void
Internals::forcePluginPlaceholder(HTMLElement* element, const Dictionary&
options, ExceptionState& exceptionState)
On 2014/08/29 21:13:26, esprehn wrote:
> HTMLObjectElement*

I'd expect this to work on HTMLEmbedElement and HTMLAppletElement as well (and
their nearest common IDL type is HTMLElement). It so happens that I only have
one test case so far that uses this, but I'm not sure what is gained by limiting
it to HTMLObjectElement.

[jbroman, 2014-08-31 12:51:09]

Re-implemented on top of Blink-in-JS; PTAL. (It's a pretty similar amount of
code, but I do expect that event handling will be cleaner in JS.)

I'm a little concerned about the warning in one of the .gyp files about using
Blink-in-JS, but it seems to be working.

[haraken, 2014-08-31 14:53:33]

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

[haraken, 2014-08-31 14:53:34]

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/PluginPlaceholderElement.cpp (right):

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/PluginPlaceholderElement.cpp:32: void
PluginPlaceholderElement::initializePlaceholderElements()

This helper function looks redundant.

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/PluginPlaceholderElement.idl (right):

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/PluginPlaceholderElement.idl:9:
[ImplementedInPrivateScript, OnlyExposedToPrivateScript] void
initializePlaceholderElements();

Can we rename this to 'createCallback', since this should be replaced with the
create callback of the custom elements in the future?

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/PluginPlaceholderElement.js (right):

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/PluginPlaceholderElement.js:17: // FIXME: Move style out
of script and into CSS.

This is something Blink-in-JS needs to support. Do you have any preference about
how Blink-in-JS supports CSS?

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/PluginPlaceholderElement.js:44: set: function(html) {
this.messageElement.innerHTML = html; },

What happens if |html| contains <script> tags? Isn't there any risk of XSS?

[jbroman, 2014-08-31 16:50:04]

jbroman@chromium.org changed reviewers:
+ bauerb@chromium.org

[jbroman, 2014-08-31 16:53:50]

[+bauerb since one of the comments concerns the embedder interface this will
ultimately plug into]

Replied to comments; I haven't yet applied haraken@'s concrete suggestions, but
I'll do so next time I revise this patch.

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/PluginPlaceholderElement.cpp (right):

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/PluginPlaceholderElement.cpp:32: void
PluginPlaceholderElement::initializePlaceholderElements()
On 2014/08/31 14:53:33, haraken wrote:
> 
> This helper function looks redundant.

Yes, this could be inlined into create, if that's what you're suggesting.

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/PluginPlaceholderElement.idl (right):

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/PluginPlaceholderElement.idl:9:
[ImplementedInPrivateScript, OnlyExposedToPrivateScript] void
initializePlaceholderElements();
On 2014/08/31 14:53:34, haraken wrote:
> 
> Can we rename this to 'createCallback', since this should be replaced with the
> create callback of the custom elements in the future?

I don't have any objections to this. I did notice some methods with this
convention in HTMLMarqueeElement, but it sounded to me like the plumbing to deal
with them automatically does not exist yet. Is that correct?

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/PluginPlaceholderElement.js (right):

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/PluginPlaceholderElement.js:17: // FIXME: Move style out
of script and into CSS.
On 2014/08/31 14:53:34, haraken wrote:
> 
> This is something Blink-in-JS needs to support. Do you have any preference
about
> how Blink-in-JS supports CSS?

The simplest approach for my needs so far (and PhistucK has already discovered)
is prototyped here:
https://codereview.chromium.org/516273002/

I'm not certain what the best approach is, but it seems to me that it's
necessary for Blink-in-JS to have some capacity for loading style (and possibly
image resources, though -webkit-appearance could be used), regardless of the
document's content security policy.

I think it's convenient for this to be stored in a separate .css file, either
next to the .js file or in core/css/.

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/PluginPlaceholderElement.js:44: set: function(html) {
this.messageElement.innerHTML = html; },
On 2014/08/31 14:53:34, haraken wrote:
> 
> What happens if |html| contains <script> tags? Isn't there any risk of XSS?

It should not be possible for author script to reach here; this should only be
supplied by the user agent (in this case, that will ultimately be the embedder,
though that's not plumbed through yet). My initial intention had been to only
accept plain text, but the current plugin placeholders also allow the user to
click on a link, so my current goal is for the embedder to supply plain text
only with simple links. For example:

  This plugin is disabled. Open <a href="#"
id="chrome-plugin-settings">chrome://plugins</a> to re-enable it.

I would then have a click event listener which would prevent the default action
on click, and dispatch that through to the embedder, which is empowered to take
that action.

It is true that this will require the embedder to escape any untrusted strings
that it may insert into the HTML, or for this private script to locally apply a
more restrictive CSP (that prevents inline style/scripts).

[jbroman, 2014-08-31 16:56:21]

On 2014/08/31 16:53:50, jbroman wrote:
> On 2014/08/31 14:53:34, haraken wrote:
> > 
> > This is something Blink-in-JS needs to support. Do you have any preference
> about
> > how Blink-in-JS supports CSS?
> 
> The simplest approach for my needs so far (and PhistucK has already
discovered)
> is prototyped here:
> https://codereview.chromium.org/516273002/

Oh, note that this CL was written before I ported this one to Blink-in-JS, but
you can easily imagine the adding-a-style-element bit being a C++ method invoked
from the private script.

[jbroman, 2014-08-31 19:00:51]

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/PluginPlaceholderElement.js (right):

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/PluginPlaceholderElement.js:44: set: function(html) {
this.messageElement.innerHTML = html; },
On 2014/08/31 16:53:50, jbroman wrote:
> On 2014/08/31 14:53:34, haraken wrote:
> > 
> > What happens if |html| contains <script> tags? Isn't there any risk of XSS?
> 
> It should not be possible for author script to reach here; this should only be
> supplied by the user agent (in this case, that will ultimately be the
embedder,
> though that's not plumbed through yet). My initial intention had been to only
> accept plain text, but the current plugin placeholders also allow the user to
> click on a link, so my current goal is for the embedder to supply plain text
> only with simple links. For example:
> 
>   This plugin is disabled. Open <a href="#"
> id="chrome-plugin-settings">chrome://plugins</a> to re-enable it.
> 
> I would then have a click event listener which would prevent the default
action
> on click, and dispatch that through to the embedder, which is empowered to
take
> that action.
> 
> It is true that this will require the embedder to escape any untrusted strings
> that it may insert into the HTML, or for this private script to locally apply
a
> more restrictive CSP (that prevents inline style/scripts).

Though now that this is in JavaScript, the possibility of accepting some very
limited templating language rather than raw HTML is probably easier than it
would be in C++. So the option of taking syntax like "Open
[chrome://plugins][chrome-plugin-settings] to re-enable it." and then turning
that into DOM in private script also exists.

[haraken, 2014-09-01 00:47:38]

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/PluginPlaceholderElement.idl (right):

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/PluginPlaceholderElement.idl:9:
[ImplementedInPrivateScript, OnlyExposedToPrivateScript] void
initializePlaceholderElements();
On 2014/08/31 16:53:49, jbroman wrote:
> On 2014/08/31 14:53:34, haraken wrote:
> > 
> > Can we rename this to 'createCallback', since this should be replaced with
the
> > create callback of the custom elements in the future?
> 
> I don't have any objections to this. I did notice some methods with this
> convention in HTMLMarqueeElement, but it sounded to me like the plumbing to
deal
> with them automatically does not exist yet. Is that correct?

Yes. Ideally both HTMLMarqueeElement and PluginPlaceholderElement should be
implemented using lifecycle callbacks of custom elements, but we cannot do it at
the moment due to the reason described here
(https://codereview.chromium.org/394773003/#msg14).

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/PluginPlaceholderElement.js (right):

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/PluginPlaceholderElement.js:17: // FIXME: Move style out
of script and into CSS.
On 2014/08/31 16:53:50, jbroman wrote:
> On 2014/08/31 14:53:34, haraken wrote:
> > 
> > This is something Blink-in-JS needs to support. Do you have any preference
> about
> > how Blink-in-JS supports CSS?
> 
> The simplest approach for my needs so far (and PhistucK has already
discovered)
> is prototyped here:
> https://codereview.chromium.org/516273002/
> 
> I'm not certain what the best approach is, but it seems to me that it's
> necessary for Blink-in-JS to have some capacity for loading style (and
possibly
> image resources, though -webkit-appearance could be used), regardless of the
> document's content security policy.
> 
> I think it's convenient for this to be stored in a separate .css file, either
> next to the .js file or in core/css/.

Yes, I believe this is a right way to go. I'll comment on the CSS CL for more
details.

https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/PluginPlaceholderElement.js:44: set: function(html) {
this.messageElement.innerHTML = html; },
On 2014/08/31 19:00:51, jbroman wrote:
> On 2014/08/31 16:53:50, jbroman wrote:
> > On 2014/08/31 14:53:34, haraken wrote:
> > > 
> > > What happens if |html| contains <script> tags? Isn't there any risk of
XSS?
> > 
> > It should not be possible for author script to reach here; this should only
be
> > supplied by the user agent (in this case, that will ultimately be the
> embedder,
> > though that's not plumbed through yet). My initial intention had been to
only
> > accept plain text, but the current plugin placeholders also allow the user
to
> > click on a link, so my current goal is for the embedder to supply plain text
> > only with simple links. For example:
> > 
> >   This plugin is disabled. Open <a href="#"
> > id="chrome-plugin-settings">chrome://plugins</a> to re-enable it.
> > 
> > I would then have a click event listener which would prevent the default
> action
> > on click, and dispatch that through to the embedder, which is empowered to
> take
> > that action.
> > 
> > It is true that this will require the embedder to escape any untrusted
strings
> > that it may insert into the HTML, or for this private script to locally
apply
> a
> > more restrictive CSP (that prevents inline style/scripts).
> 
> Though now that this is in JavaScript, the possibility of accepting some very
> limited templating language rather than raw HTML is probably easier than it
> would be in C++. So the option of taking syntax like "Open
> [chrome://plugins][chrome-plugin-settings] to re-enable it." and then turning
> that into DOM in private script also exists.

I'm not familiar with how the plugin placeholder element should be implemented,
so I'll delegate the decision to experts.

[jbroman, 2014-09-02 21:44:40]

Patchset #4 (id:60001) has been deleted

[jbroman, 2014-09-02 21:47:52]

Rebased and discovered some changes needed due to bindings changes.

1. DEFINE_WRAPPERTYPEINFO() is now needed.
2. Since HTMLElement::wrap will create the wrong wrapper (V8HTMLDivElement), an
override of ScriptWrappable::wrap is needed that will actually create a
V8PluginPlaceholderElement.

Reviewers: LGTY?

[esprehn, 2014-09-02 23:12:00]

Don't use innerHTML, please build a proper API. We should never stuff html in
the UA code.

https://codereview.chromium.org/522783002/diff/100001/Source/core/html/shadow...
File Source/core/html/shadow/PluginPlaceholderElement.js (right):

https://codereview.chromium.org/522783002/diff/100001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.js:43: get: function() { return
this.messageElement.innerHTML; },
no innerHTML should be exposed like this.

[haraken, 2014-09-03 02:40:37]

On 2014/09/02 23:12:00, esprehn wrote:
> Don't use innerHTML, please build a proper API. We should never stuff html in
> the UA code.
> 
>
https://codereview.chromium.org/522783002/diff/100001/Source/core/html/shadow...
> File Source/core/html/shadow/PluginPlaceholderElement.js (right):
> 
>
https://codereview.chromium.org/522783002/diff/100001/Source/core/html/shadow...
> Source/core/html/shadow/PluginPlaceholderElement.js:43: get: function() {
return
> this.messageElement.innerHTML; },
> no innerHTML should be exposed like this.

FYI, discussed with Elliott, I'm planning to add [ForbiddenInPrivateScript] IDL
attribute, which will be added to innerHTML etc.

I'm happy to take another close look at this CL once Elliott is happy with it.

[jbroman, 2014-09-03 14:26:06]

On 2014/09/03 02:40:37, haraken wrote:
> On 2014/09/02 23:12:00, esprehn wrote:
> > Don't use innerHTML, please build a proper API. We should never stuff html
in
> > the UA code.
> > 
> >
>
https://codereview.chromium.org/522783002/diff/100001/Source/core/html/shadow...
> > File Source/core/html/shadow/PluginPlaceholderElement.js (right):
> > 
> >
>
https://codereview.chromium.org/522783002/diff/100001/Source/core/html/shadow...
> > Source/core/html/shadow/PluginPlaceholderElement.js:43: get: function() {
> return
> > this.messageElement.innerHTML; },
> > no innerHTML should be exposed like this.
> 
> FYI, discussed with Elliott, I'm planning to add [ForbiddenInPrivateScript]
IDL
> attribute, which will be added to innerHTML etc.
> 
> I'm happy to take another close look at this CL once Elliott is happy with it.

I've changed it to use only a plaintext message for now. I'll come back and
replace it with something more sophisticated in a subsequent CL.

[jbroman, 2014-09-05 14:16:02]

On 2014/09/03 14:26:06, jbroman wrote:
> On 2014/09/03 02:40:37, haraken wrote:
> > On 2014/09/02 23:12:00, esprehn wrote:
> > > Don't use innerHTML, please build a proper API. We should never stuff html
> in
> > > the UA code.
> > > 
> > >
> >
>
https://codereview.chromium.org/522783002/diff/100001/Source/core/html/shadow...
> > > File Source/core/html/shadow/PluginPlaceholderElement.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/522783002/diff/100001/Source/core/html/shadow...
> > > Source/core/html/shadow/PluginPlaceholderElement.js:43: get: function() {
> > return
> > > this.messageElement.innerHTML; },
> > > no innerHTML should be exposed like this.
> > 
> > FYI, discussed with Elliott, I'm planning to add [ForbiddenInPrivateScript]
> IDL
> > attribute, which will be added to innerHTML etc.
> > 
> > I'm happy to take another close look at this CL once Elliott is happy with
it.
> 
> I've changed it to use only a plaintext message for now. I'll come back and
> replace it with something more sophisticated in a subsequent CL.

I don't think there are currently any unresolved comments on this CL, so

esprehn: ping

[esprehn, 2014-09-09 04:13:30]

Looks much better.

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
File Source/core/html/shadow/PluginPlaceholderElement.cpp (right):

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.cpp:15: :
HTMLDivElement(document)
We really need to stop having exotic <div>'s :/ I guess this is okay for this
patch, but we need to come up with a better design.

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.cpp:23: bool success =
V8PluginPlaceholderElement::PrivateScript::createCallbackMethod(document.frame(),
element.get());
createdCallback, this should be just like a custom element.

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
File Source/core/html/shadow/PluginPlaceholderElement.h (right):

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.h:16: // Returns a null pointer
if the placeholder DOM could not be built.
Remove comment, this should never fail.

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
File Source/core/html/shadow/PluginPlaceholderElement.js (right):

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.js:8:
PluginPlaceholderElementPrototype.createCallback = function() {
createdCallback, this should be like a custom element.

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.js:15: // </div>
I'd remove this comment, the test shows the structure.

[jbroman, 2014-09-09 13:18:48]

Patchset #8 (id:160001) has been deleted

[jbroman, 2014-09-09 14:05:23]

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
File Source/core/html/shadow/PluginPlaceholderElement.cpp (right):

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.cpp:15: :
HTMLDivElement(document)
On 2014/09/09 04:13:30, esprehn wrote:
> We really need to stop having exotic <div>'s :/ I guess this is okay for this
> patch, but we need to come up with a better design.

Acknowledged.

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.cpp:23: bool success =
V8PluginPlaceholderElement::PrivateScript::createCallbackMethod(document.frame(),
element.get());
On 2014/09/09 04:13:30, esprehn wrote:
> createdCallback, this should be just like a custom element.

Done.

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
File Source/core/html/shadow/PluginPlaceholderElement.h (right):

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.h:16: // Returns a null pointer
if the placeholder DOM could not be built.
On 2014/09/09 04:13:30, esprehn wrote:
> Remove comment, this should never fail.

Done.

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
File Source/core/html/shadow/PluginPlaceholderElement.js (right):

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.js:8:
PluginPlaceholderElementPrototype.createCallback = function() {
On 2014/09/09 04:13:30, esprehn wrote:
> createdCallback, this should be like a custom element.

Done.

https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.js:15: // </div>
On 2014/09/09 04:13:30, esprehn wrote:
> I'd remove this comment, the test shows the structure.

Done.

[esprehn, 2014-09-12 19:36:35]

Lgtm

[jbroman, 2014-09-13 00:40:31]

On 2014/09/12 19:36:35, esprehn wrote:
> Lgtm

tl;dr: Will try again to land this shortly; now needs additional patches before
it can land.

Apparently a change to move private script files to grit has just landed, which
I think means this CL needs to become a 3-sided CL (a Blink CL to create an
empty resource with an identifier, a Chromium CL to map it to a resource name in
the blink::Platform implementation, and a Blink CL to actually land most of
this). Those first two should be pretty simple, but they are going to block this
from landing. I'll send those out shortly (but it would be nice if we didn't
need a 3-sided patch to add a new resource that is both supplied and consumed by
Blink).

[jbroman, 2014-09-15 21:16:31]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2014-09-15 21:17:21]

On 2014/09/13 00:40:31, jbroman wrote:
> On 2014/09/12 19:36:35, esprehn wrote:
> > Lgtm
> 
> tl;dr: Will try again to land this shortly; now needs additional patches
before
> it can land.
> 
> Apparently a change to move private script files to grit has just landed,
which
> I think means this CL needs to become a 3-sided CL (a Blink CL to create an
> empty resource with an identifier, a Chromium CL to map it to a resource name
in
> the blink::Platform implementation, and a Blink CL to actually land most of
> this). Those first two should be pretty simple, but they are going to block
this
> from landing. I'll send those out shortly (but it would be nice if we didn't
> need a 3-sided patch to add a new resource that is both supplied and consumed
by
> Blink).

First two sides have landed; throwing this in the CQ after a rebase.

[commit-bot: I haz the power, 2014-09-15 21:17:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/522783002/220001

[commit-bot: I haz the power, 2014-09-15 22:48:16]

Committed patchset #10 (id:220001) as 182014