Bug 98322 - Add render object that paints plugin snapshots
Summary: Add render object that paints plugin snapshots
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.8
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on:
Blocks: 98318
  Show dependency treegraph
 
Reported: 2012-10-03 16:35 PDT by Jon Lee
Modified: 2012-10-08 14:27 PDT (History)
8 users (show)

See Also:


Attachments
Patch (30.72 KB, patch)
2012-10-08 10:35 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (38.88 KB, patch)
2012-10-08 13:19 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Removed layout() call, renamed setSnapshot to updateSnapshot, and moved setting the snapshot into RenderSnapshottedPlugin to make cleaner code (37.99 KB, patch)
2012-10-08 14:05 PDT, Jon Lee
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2012-10-03 16:35:49 PDT
Add a new render class that knows how to paint poster images when the plugin is inactive.
Comment 1 Radar WebKit Bug Importer 2012-10-03 16:36:09 PDT
<rdar://problem/12426546>
Comment 2 Jon Lee 2012-10-08 10:35:30 PDT
Created attachment 167559 [details]
Patch
Comment 3 Simon Fraser (smfr) 2012-10-08 10:45:34 PDT
Comment on attachment 167559 [details]
Patch

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

> Source/WebCore/html/HTMLPlugInImageElement.cpp:55
>      , m_needsDocumentActivationCallbacks(false)
>  {
>      setHasCustomCallbacks();
> +
> +    if (document->page() && document->page()->settings()->plugInSnapshottingEnabled())
> +        setDisplayState(WaitingForSnapshot);
>  }

Are you missing initialization of m_displayState here in other cases?

> Source/WebCore/html/HTMLPlugInImageElement.cpp:273
> +    if (image) {
> +        toRenderSnapshottedPlugin(renderer())->snapshotResource()->setCachedImage(new CachedImage(image));
> +        renderer()->repaint();
> +    }

What happens if the plugin is display:none (or became display:none during the waiting phase)? renderer() could be null.

> Source/WebCore/rendering/RenderSnapshottedPlugin.cpp:77
> +void RenderSnapshottedPlugin::layout()
> +{
> +    ASSERT(needsLayout());
> +
> +    updateLogicalWidth();
> +    updateLogicalHeight();
> +
> +    if (plugInImageElement()->displayState() < HTMLPlugInElement::Playing)
> +        RenderBox::layout();
> +    else
> +        RenderPart::layout();
> +
> +    m_overflow.clear();
> +    addVisualEffectOverflow();
> +
> +    updateLayerTransform();
> +
> +    if (!widget() && frameView())
> +        frameView()->addWidgetToUpdate(this);
> +
> +    setNeedsLayout(false);
> +}

It's a bit odd that you need special layout behavior. Are there cases where the layout of the element changes when it goes from live to snapshot, or vice versa?
Comment 4 Build Bot 2012-10-08 10:59:58 PDT
Comment on attachment 167559 [details]
Patch

Attachment 167559 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14214391
Comment 5 Jon Lee 2012-10-08 11:05:35 PDT
Comment on attachment 167559 [details]
Patch

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

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:55
>>  }
> 
> Are you missing initialization of m_displayState here in other cases?

No. In HTMLPlugInElement it sets the default state to Playing. Only this subclass overrides that setting.

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:273
>> +    }
> 
> What happens if the plugin is display:none (or became display:none during the waiting phase)? renderer() could be null.

Hmm. With display:none we could still obtain the snapshot from the plugin. Does that mean I should hold the snapshotResource in the HTML element instead of the renderer?

>> Source/WebCore/rendering/RenderSnapshottedPlugin.cpp:77
>> +}
> 
> It's a bit odd that you need special layout behavior. Are there cases where the layout of the element changes when it goes from live to snapshot, or vice versa?

No. This code is exactly the same as RenderEmbeddedObject::layout(), except that if we are snapshotting I use RenderBox::layout() instead of RenderPart::layout(). I didn't know how to factor that one part out without making this function needlessly complicated. Any suggestion?

> Source/WebCore/rendering/RenderSnapshottedPlugin.cpp:130
> +void RenderSnapshottedPlugin::paintReplacedOverlay(PaintInfo& paintInfo, const LayoutPoint& paintOffset)

This needs to be moved to RenderTheme. Will post a new patch.
Comment 6 Jon Lee 2012-10-08 13:19:01 PDT
Created attachment 167595 [details]
Patch
Comment 7 Jon Lee 2012-10-08 13:20:43 PDT
Comment on attachment 167595 [details]
Patch

Moved the overlay rendering code out to RenderTheme. Fixed, hopefully, Windows build error.
Comment 8 Jon Lee 2012-10-08 14:05:21 PDT
Created attachment 167603 [details]
Removed layout() call, renamed setSnapshot to updateSnapshot, and moved setting the snapshot into RenderSnapshottedPlugin to make cleaner code
Comment 9 Jon Lee 2012-10-08 14:07:05 PDT
Comment on attachment 167603 [details]
Removed layout() call, renamed setSnapshot to updateSnapshot, and moved setting the snapshot into RenderSnapshottedPlugin to make cleaner code

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:7823
> +		2981CAA5131822EC00D12F2A /* AccessibilityObject.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; lineEnding = 0; path = AccessibilityObject.cpp; sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.cpp; };

Not sure what happened here. Will remove this when checked in.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:11265
> +		A81872140977D3C0005826D9 /* ContainerNode.cpp */ = {isa = PBXFileReference; fileEncoding = 30; indentWidth = 4; lastKnownFileType = sourcecode.cpp.cpp; lineEnding = 0; path = ContainerNode.cpp; sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.cpp; };

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:12812
> +		BC772B370C4EA91E0083285F /* CSSParser.cpp */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.cpp; lineEnding = 0; path = CSSParser.cpp; sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.cpp; };

And here too.
Comment 10 Jon Lee 2012-10-08 14:27:44 PDT
Committed r130688: <http://trac.webkit.org/changeset/130688>