Allow HTMLPlugInElement to host UA shadow DOM.

Allow HTMLPlugInElement to host UA shadow DOM.

When in "use placeholder content" mode, the element will
create an ordinary RenderBlockFlow as its renderer, and
so will be laid out as a block or inline-block.

A rule was added to StyleAdjuster to ensure that the
default width/height rules for replaced elements are
applied in this case. Since plugins have no intrinsic
ratio, this should be equivalent to mapping 'auto' to
the appropriate defaults.

Finally, a hook in Internals was added to allow layout
tests to trigger this mode, and two layout tests using
this were added.

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

[jbroman, 2014-07-04 14:05:47]

This is a little simpler than having a separate renderer, I think. This permits
UA shadow DOM to be attached to a plugin element, and be laid out in such a way
that it is confined to the size of the plugin.

This is pretty close to what's implemented in RenderMedia for the purpose of
media controls. I expect to use this to similarly show shadow DOM placeholder
content (e.g. when a plugin is blocked).

Machinery will be added later to actually supply this content when required.

[jbroman, 2014-07-04 14:07:41]

+reviewers

[esprehn, 2014-07-06 09:15:35]

https://codereview.chromium.org/369893006/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderEmbeddedObject.cpp (right):

https://codereview.chromium.org/369893006/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderEmbeddedObject.cpp:123: // (e.g. by setting
'position: relative' in its style)
You should force it to get a layer in the style adjustment I suspect, having it
magically appear/disappear based on style doesn't seem right.

https://codereview.chromium.org/369893006/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderEmbeddedObject.cpp:213: if (RenderBox* childRenderer
= toRenderBox(m_children.firstChild())) {
This is a bad cast if you have an inline child. I don't think we want more of
the terrible hacks that media elements have.

https://codereview.chromium.org/369893006/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderEmbeddedObject.cpp:220:
childRenderer->forceLayout();
I'd rather we didn't do more of this, why not just do normal box layout?

[jbroman, 2014-07-07 20:33:04]

Not quite sure what more elegant approach you had in mind; can you elaborate?

https://codereview.chromium.org/369893006/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderEmbeddedObject.cpp (right):

https://codereview.chromium.org/369893006/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderEmbeddedObject.cpp:123: // (e.g. by setting
'position: relative' in its style)
On 2014/07/06 09:15:35, esprehn wrote:
> You should force it to get a layer in the style adjustment I suspect, having
it
> magically appear/disappear based on style doesn't seem right.

As far as I can tell, this is currently done by overriding
RenderLayerModelObject::layerTypeRequired in the renderer you want to get a
layer (the usual overrides check whether it is positioned, has an overflow clip,
etc.).

It might be possible to avoid needing this by having RenderEmbeddedObject paint
its children (etc.) properly.

I did it this way because it's how RenderMedia works (though I realize that's a
hack), and I couldn't think of a substantially better way. Happy to get your
input, though.

https://codereview.chromium.org/369893006/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderEmbeddedObject.cpp:213: if (RenderBox* childRenderer
= toRenderBox(m_children.firstChild())) {
On 2014/07/06 09:15:35, esprehn wrote:
> This is a bad cast if you have an inline child. I don't think we want more of
> the terrible hacks that media elements have.

Could add a more explicit check here, but the content here is supplied by the
UA, so we should know what kind of renderer this is.

https://codereview.chromium.org/369893006/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderEmbeddedObject.cpp:220:
childRenderer->forceLayout();
On 2014/07/06 09:15:35, esprehn wrote:
> I'd rather we didn't do more of this, why not just do normal box layout?

What did you have in mind?

What's needed here is a way for a child to get a size matching the size of the
replaced renderer, so that it can present content in that rect. In this case,
it's for placeholder content; in the RenderMedia case, it's for controls.

I'm not aware of any of the normal layout paths that let you do this. Block and
flexbox layout can, but they have to be within an appropriate container to do so
(e.g. a replaced object isn't a containing block). Is there an elegant way to
achieve this?

[esprehn, 2014-07-14 08:50:39]

On 2014/07/07 at 20:33:04, jbroman wrote:
> Not quite sure what more elegant approach you had in mind; can you elaborate?
>

I'm not sure, but we shouldn't be adding more RenderMedia like hacks, especially
not the super unsafe casts. I've already fixed lots of security bugs that came
from all the hackery we added for the media code.
 
>
https://codereview.chromium.org/369893006/diff/20001/Source/core/rendering/Re...
> File Source/core/rendering/RenderEmbeddedObject.cpp (right):
> 
>
https://codereview.chromium.org/369893006/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderEmbeddedObject.cpp:123: // (e.g. by setting
'position: relative' in its style)
> On 2014/07/06 09:15:35, esprehn wrote:
> > You should force it to get a layer in the style adjustment I suspect, having
it
> > magically appear/disappear based on style doesn't seem right.
> 
> As far as I can tell, this is currently done by overriding
RenderLayerModelObject::layerTypeRequired in the renderer you want to get a
layer (the usual overrides check whether it is positioned, has an overflow clip,
etc.).

In StyleAdjuster you can force a layer on all things that are inside a
RenderEmbeddedObject. It's gross, but at least it's somewhat more sane.

> 
> It might be possible to avoid needing this by having RenderEmbeddedObject
paint its children (etc.) properly.
> 

That would be even better.

> I did it this way because it's how RenderMedia works (though I realize that's
a hack), and I couldn't think of a substantially better way. Happy to get your
input, though.
> 
>
https://codereview.chromium.org/369893006/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderEmbeddedObject.cpp:213: if (RenderBox*
childRenderer = toRenderBox(m_children.firstChild())) {
> On 2014/07/06 09:15:35, esprehn wrote:
> > This is a bad cast if you have an inline child. I don't think we want more
of
> > the terrible hacks that media elements have.
> 
> Could add a more explicit check here, but the content here is supplied by the
UA, so we should know what kind of renderer this is.
> 

That's rarely worked out in practice, I spent months fixing security bugs
introduced because of assumptions like this.

>
https://codereview.chromium.org/369893006/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderEmbeddedObject.cpp:220:
childRenderer->forceLayout();
> On 2014/07/06 09:15:35, esprehn wrote:
> > I'd rather we didn't do more of this, why not just do normal box layout?
> 
> What did you have in mind?
> 
> What's needed here is a way for a child to get a size matching the size of the
replaced renderer, so that it can present content in that rect. In this case,
it's for placeholder content; in the RenderMedia case, it's for controls.
> 
> I'm not aware of any of the normal layout paths that let you do this. Block
and flexbox layout can, but they have to be within an appropriate container to
do so (e.g. a replaced object isn't a containing block). Is there an elegant way
to achieve this?

Why don't we name RenderEmbeddedObject into a containing block? :) Since it
can't have children today that won't break anything.

[jbroman, 2014-07-24 18:48:25]

On 2014/07/14 08:50:39, esprehn wrote:
> Why don't we name RenderEmbeddedObject into a containing block? :) Since it
> can't have children today that won't break anything.

Doing that is really rough because containing blocks must be instances of
RenderBlock RenderEmbeddedObject is part of a long hierarchy:
  RenderEmbeddedObject -> RenderPart -> RenderWidget -> RenderReplaced ->
RenderBox -> ...

And making all replaced renderers, or something like that, blocks would have
large consequences.

Instead, this patch set goes back to using a different renderer for placeholder
content; reattaching is used to switch between plugin and placeholder content.
HTMLPlugInElement already knows about "fallback" content, which has a non-REO
renderer. This is similar, but not the same (unlike fallback content, it should
still respect the default plugin size, it should be hidden behind UA shadow DOM,
etc.).

After reading the CSS visual formatting model details spec in some detail, I'm
pretty sure that it's possible to lay out a block flow (i.e. block or
inline-block) in a way that is the same as an block- or inline-level replaced
object. So now a regular RenderBlockFlow is used to hold the placeholder render
tree.

For what it's worth, Firefox's plugin placeholders seem to be implemented in a
similar way (using their analogue of shadow DOM, producing a
block/inline-block), which helps my confidence that this approach doesn't break
the web.

What do you think?

[jbroman, 2014-07-28 21:00:24]

ping

[leviw_travelin_and_unemployed, 2014-07-29 20:37:44]

This seems reasonable to me, but I'll defer to esprehn for final approval.

https://codereview.chromium.org/369893006/diff/40001/LayoutTests/fast/plugins...
File LayoutTests/fast/plugins/plugin-placeholder-expected.txt (right):

https://codereview.chromium.org/369893006/diff/40001/LayoutTests/fast/plugins...
LayoutTests/fast/plugins/plugin-placeholder-expected.txt:1: layer at (0,0) size
800x600
Why not use a ref test?

https://codereview.chromium.org/369893006/diff/40001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/369893006/diff/40001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:587: void
HTMLPlugInElement::setUsePlaceholderContent(bool use)
Nit: usePlaceholder?

https://codereview.chromium.org/369893006/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderReplaced.h (right):

https://codereview.chromium.org/369893006/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderReplaced.h:43: // These values are specified in the
spec.
How about a link instead of a non-specific reference?

[jbroman, 2014-07-29 21:47:29]

https://codereview.chromium.org/369893006/diff/40001/LayoutTests/fast/plugins...
File LayoutTests/fast/plugins/plugin-placeholder-expected.txt (right):

https://codereview.chromium.org/369893006/diff/40001/LayoutTests/fast/plugins...
LayoutTests/fast/plugins/plugin-placeholder-expected.txt:1: layer at (0,0) size
800x600
On 2014/07/29 20:37:44, leviw wrote:
> Why not use a ref test?

No reason other than the evolution of this test. Changed to be a simple ref
test.

https://codereview.chromium.org/369893006/diff/40001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/369893006/diff/40001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:587: void
HTMLPlugInElement::setUsePlaceholderContent(bool use)
On 2014/07/29 20:37:44, leviw wrote:
> Nit: usePlaceholder?

I'm unsure whether you're asking for a change from "use placeholder content" to
"use placeholder" here and everywhere, or whether you're objecting to the name
"use" for the boolean parameter to this method.

I'm OK with either; just need to know which. :)

https://codereview.chromium.org/369893006/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderReplaced.h (right):

https://codereview.chromium.org/369893006/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderReplaced.h:43: // These values are specified in the
spec.
On 2014/07/29 20:37:44, leviw wrote:
> How about a link instead of a non-specific reference?

Sure. I'd just kept the comment that existed previously in RenderVideo.

[esprehn, 2014-08-01 14:01:26]

I'll make sure to take a look at this on Monday.

[jbroman, 2014-08-12 14:16:42]

On 2014/08/01 14:01:26, esprehn wrote:
> I'll make sure to take a look at this on Monday.

ping?

[jbroman, 2014-08-18 13:19:26]

On 2014/08/12 14:16:42, jbroman wrote:
> On 2014/08/01 14:01:26, esprehn wrote:
> > I'll make sure to take a look at this on Monday.
> 
> ping?

Bump again; it's been over three weeks now.

[esprehn, 2014-08-18 16:00:50]

lgtm

[jbroman, 2014-08-18 16:51:06]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2014-08-18 16:51:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbroman@chromium.org/369893006/60001

[commit-bot: I haz the power, 2014-08-18 16:51:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-18 16:51:40]

Failed to apply patch for Source/core/testing/Internals.idl:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/testing/Internals.idl
  Hunk #1 FAILED at 283.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/testing/Internals.idl.rej

Patch:       Source/core/testing/Internals.idl
Index: Source/core/testing/Internals.idl
diff --git a/Source/core/testing/Internals.idl
b/Source/core/testing/Internals.idl
index
3fa0e91dde3d28e074600765a149e2a03b40430a..0a4c54fea36e137153e15f6e11d1afd134715559
100644
--- a/Source/core/testing/Internals.idl
+++ b/Source/core/testing/Internals.idl
@@ -283,4 +283,6 @@
     unsigned long countHitRegions(CanvasRenderingContext2D context);
 
     DOMString serializeNavigationMarkup(Document document);
+
+    [RaisesException] void forcePluginPlaceholder(HTMLElement plugin, DOMString
htmlSource);
 };

[jbroman, 2014-08-18 18:29:03]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2014-08-18 18:29:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbroman@chromium.org/369893006/80001

[commit-bot: I haz the power, 2014-08-18 19:36:08]

Committed patchset #4 (80001) as 180487