Bug 98322

Summary: Add render object that paints plugin snapshots
Product: WebKit Reporter: Jon Lee <jonlee>
Component: WebKit Misc.Assignee: Jon Lee <jonlee>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, eric, gyuyoung.kim, mitz, rakuco, simon.fraser, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.8   
Bug Depends on:    
Bug Blocks: 98318    
Attachments:
Description Flags
Patch
none
Patch
none
Removed layout() call, renamed setSnapshot to updateSnapshot, and moved setting the snapshot into RenderSnapshottedPlugin to make cleaner code simon.fraser: review+

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>