[webkit-reviews] review granted: [Bug 70821] PDF SUBFRAMES: Incomplete repaint after pinch to zoom : [Attachment 112354] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 70821: PDF SUBFRAMES: Incomplete repaint after pinch to zoom
https://bugs.webkit.org/show_bug.cgi?id=70821

Attachment 112354: Patch
https://bugs.webkit.org/attachment.cgi?id=112354&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=112354&action=review


Can we add some tests pls?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFView.cpp:307
> +	   graphicsContext->translate(-pluginView()->frameRect().x(),
-pluginView()->frameRect().y());

Hmm, is frameRect() OK to use under transforms?

>>> Source/WebKit2/WebProcess/Plugins/Plugin.h:212
>>> +	 virtual bool wantsWindowRelativeCoordinates() = 0;
>> 
>> Should this be a const member function?
> 
> For abstract classes I prefer having pure virtual member functions not be
const since the derived classes might want to do things that are non-const.

I think the name is a bit too generic. Which coordinates, passed to which
function?

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:578
> +    if (!m_plugin->wantsWindowRelativeCoordinates()) {
> +	   // FIXME: We should try to intersect the dirty rect with the
plug-in's clip rect here.
> +	   paintRect = IntRect(IntPoint(), frameRect().size());
> +    } else {
> +	   IntRect dirtyRectInWindowCoordinates =
parent()->contentsToWindow(dirtyRect);
> +	   paintRect = intersection(dirtyRectInWindowCoordinates,
clipRectInWindowCoordinates());
> +    }

I'd flip the clauses so you can write if
(m_plugin->wantsWindowRelativeCoordinates())

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:593
> +    if (!m_plugin->wantsWindowRelativeCoordinates()) {
> +	   // Translate the coordinate system so that the origin is in the
top-left corner of the plug-in.
> +	   context->translate(frameRect().location().x(),
frameRect().location().y());
> +    } else {

And here

> Source/WebKit2/WebProcess/Plugins/PluginView.h:71
> +    // FIXME: Remove this.
> +    WebCore::RenderBoxModelObject* renderer() const;

The FIXME needs to say a bit more about why it should be removed (possibly even
linking to a bug).