Bug 112432 - RenderSnapshottedPlugIn can't be a RenderBlock (what if it's display: inline?)
Summary: RenderSnapshottedPlugIn can't be a RenderBlock (what if it's display: inline?)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-03-15 04:42 PDT by Tim Horton
Modified: 2013-03-15 13:36 PDT (History)
8 users (show)

See Also:

Attachments
patch (12.81 KB, patch)
2013-03-15 04:57 PDT, Tim Horton 		simon.fraser: review-
Details | Formatted Diff | Diff
patch (13.36 KB, patch)
2013-03-15 13:12 PDT, Tim Horton 		simon.fraser: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2013-03-15 04:42:15 PDT 
RenderSnapshottedPlugIn can break the layout of pages, because it replaces a RenderReplaced (the plugin's RenderEmbeddedObject) but is a RenderBlock. If the element is inline, madness breaks loose and the page layout goes downhill quickly.

We can make RenderSnapshottedPlugIn a RenderEmbeddedObject subclass with a little bit of care, and rescue ourselves from layout peril.

<rdar://problem/13187211>
Comment 1 Tim Horton 2013-03-15 04:57:23 PDT 
Created attachment 193282 [details]
patch
Comment 2 Simon Fraser (smfr) 2013-03-15 09:38:02 PDT 
Comment on attachment 193282 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193282&action=review

I like it but think it would benefit from some minor cleanup.

> Source/WebCore/rendering/RenderEmbeddedObject.cpp:295
> +    if (!widget() && frameView() && !isSnapshottedPlugIn())
>          frameView()->addWidgetToUpdate(this);

It's not clear why a snapshotted plugin should not have to addWidgetToUpdate(). Is it because it will never have a widget? Perhaps we should add a member function that describes this more clearly?

> Source/WebCore/rendering/RenderEmbeddedObject.cpp:302
> +#if !ENABLE(PLUGIN_PROXY_FOR_VIDEO)
> +    if (!isSnapshottedPlugIn())
> +        return;
> +#endif

Why not just call canHaveChildren() here?
Comment 3 Tim Horton 2013-03-15 13:12:16 PDT 
Created attachment 193361 [details]
patch
Comment 4 Simon Fraser (smfr) 2013-03-15 13:23:04 PDT 
Comment on attachment 193361 [details]
patch

Nice!
Comment 5 Tim Horton 2013-03-15 13:26:53 PDT 
Comment on attachment 193361 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193361&action=review

> Source/WebCore/page/FrameView.cpp:2532
> +                HTMLPlugInImageElement* pluginElement = static_cast<HTMLPlugInImageElement*>(ownerElement);

I’ll make sure this uses the to* helper.
Comment 6 Dean Jackson 2013-03-15 13:29:06 PDT 
Comment on attachment 193361 [details]
patch

I love this. But I wonder why we're still tearing down and swapping renderers now. We should go back to the original idea of using RSPI all the time (until we un-snapshot). That could be a followup patch I guess.

PS. I think we can remove some InlineBox exports too now.
Comment 7 Tim Horton 2013-03-15 13:36:37 PDT 
http://trac.webkit.org/changeset/145934