Bug 110811 – Draw intermediate snapshots if possible

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Dean Jackson]

After http://webkit.org/b/110495 we delayed snapshotting until we've received a nice image, but this makes the page look like it is broken. We should draw any intermediate snapshots that we find, such as progress bars.

This is actually a regression on previous behaviour (say from about r139000)

[Dean Jackson]

<rdar://problem/13246641>

[Radar WebKit Bug Importer]

<rdar://problem/13289301>

[Dean Jackson]

Created attachment 190171 [details]
Patch

[Dean Jackson]

Created attachment 190176 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 190176 [details]
Patch

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

> Source/WebCore/html/HTMLPlugInImageElement.h:69
> +    Image* snapshotImage() { return m_snapshotImage.get(); }

Can this be const?

> Source/WebCore/rendering/RenderEmbeddedObject.cpp:176
> +    if (element && element->isPluginElement()) {
> +        HTMLPlugInImageElement* plugInImageElement = static_cast<HTMLPlugInImageElement*>(element);

This cast looks unsafe; you're only testing isPluginElement(), not if it's a HTMLPlugInImageElement. Or are the names confusing?

I also think this should be one of those SECURITY_ASSERT-checked casts.

> Source/WebCore/rendering/RenderEmbeddedObject.cpp:180
> +                if (paintInfo.phase == PaintPhaseForeground)

You should bail earlier with this check, since it's cheap.

[Dean Jackson]

Created attachment 190182 [details]
Patch

[Dean Jackson]

New patch addresses all of Simon's comments. Main change is that paintContents is called from the typical painting path in RenderWidget, but overridden sometimes by RenderEmbeddedObject.

[Jon Lee]

Comment on attachment 190182 [details]
Patch

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

> Source/WebCore/ChangeLog:25
> +        that in RenderSnapshottedPlugIn.

I think it would be useful to also mention that if we are not snapshotting we fall back to the default widget painting code, although we should not reach it since we swap renderers.

> Source/WebCore/ChangeLog:34
> +        of paint() that can be overridden by RenderEmbeddedObject.

Mentioning that this was just moved out of paint() would be useful, since in the snapshotting case we don't want to provide the widget an opportunity to paint.

> Source/WebCore/html/HTMLPlugInElement.h:77
> +    virtual bool isPlugInImageElement() { return false; }

const please!

> Source/WebCore/html/HTMLPlugInImageElement.h:69
> +    Image* snapshotImage() const { return m_snapshotImage.get(); }

const Image* would be better.

> Source/WebCore/html/HTMLPlugInImageElement.h:115
> +    virtual bool isPlugInImageElement() OVERRIDE { return true; }

... and const here.

[Dean Jackson]

Created attachment 190184 [details]
Patch

[Dean Jackson]

Created attachment 190186 [details]
Patch

[Jon Lee]

Comment on attachment 190186 [details]
Patch

Looks good. Unofficial r=me.

[Simon Fraser (smfr)]

Comment on attachment 190186 [details]
Patch

I'm wondering why overriding paintReplaced() isn't more appropriate.

[Simon Fraser (smfr)]

Comment on attachment 190186 [details]
Patch

OK I guess paintReplaced() is never called for RenderWidget subclasses since RenderWidget has its own paint().

[Dean Jackson]

Committed r144067: <http://trac.webkit.org/changeset/144067>