Bug 110811 - Draw intermediate snapshots if possible
Summary: Draw intermediate snapshots if possible
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
Keywords: InRadar
Depends on:
Reported: 2013-02-25 15:11 PST by Dean Jackson
Modified: 2013-02-26 09:18 PST (History)
7 users (show)

See Also:

Patch (6.81 KB, patch)
2013-02-25 18:18 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (7.97 KB, patch)
2013-02-25 18:23 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (14.31 KB, patch)
2013-02-25 19:16 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (14.32 KB, patch)
2013-02-25 19:39 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (14.52 KB, patch)
2013-02-25 19:42 PST, Dean Jackson
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 Dean Jackson 2013-02-25 15:11:28 PST
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)
Comment 1 Dean Jackson 2013-02-25 15:12:12 PST
Comment 2 Radar WebKit Bug Importer 2013-02-25 15:12:57 PST
Comment 3 Dean Jackson 2013-02-25 18:18:04 PST
Created attachment 190171 [details]
Comment 4 Dean Jackson 2013-02-25 18:23:34 PST
Created attachment 190176 [details]
Comment 5 Simon Fraser (smfr) 2013-02-25 18:27:50 PST
Comment on attachment 190176 [details]

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.
Comment 6 Dean Jackson 2013-02-25 19:16:34 PST
Created attachment 190182 [details]
Comment 7 Dean Jackson 2013-02-25 19:26:48 PST
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.
Comment 8 Jon Lee 2013-02-25 19:31:05 PST
Comment on attachment 190182 [details]

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.
Comment 9 Dean Jackson 2013-02-25 19:39:00 PST
Created attachment 190184 [details]
Comment 10 Dean Jackson 2013-02-25 19:42:01 PST
Created attachment 190186 [details]
Comment 11 Jon Lee 2013-02-25 19:46:11 PST
Comment on attachment 190186 [details]

Looks good. Unofficial r=me.
Comment 12 Simon Fraser (smfr) 2013-02-25 21:36:04 PST
Comment on attachment 190186 [details]

I'm wondering why overriding paintReplaced() isn't more appropriate.
Comment 13 Simon Fraser (smfr) 2013-02-25 21:46:35 PST
Comment on attachment 190186 [details]

OK I guess paintReplaced() is never called for RenderWidget subclasses since RenderWidget has its own paint().
Comment 14 Dean Jackson 2013-02-26 09:18:49 PST
Committed r144067: <http://trac.webkit.org/changeset/144067>