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)
<rdar://problem/13246641>
<rdar://problem/13289301>
Created attachment 190171 [details] Patch
Created attachment 190176 [details] Patch
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.
Created attachment 190182 [details] Patch
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 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.
Created attachment 190184 [details] Patch
Created attachment 190186 [details] Patch
Comment on attachment 190186 [details] Patch Looks good. Unofficial r=me.
Comment on attachment 190186 [details] Patch I'm wondering why overriding paintReplaced() isn't more appropriate.
Comment on attachment 190186 [details] Patch OK I guess paintReplaced() is never called for RenderWidget subclasses since RenderWidget has its own paint().
Committed r144067: <http://trac.webkit.org/changeset/144067>