Bug 70821 – PDF SUBFRAMES: Incomplete repaint after pinch to zoom

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anders Carlsson]

PDF SUBFRAMES: Incomplete repaint after pinch to zoom

[Anders Carlsson]

Created attachment 112354 [details]
Patch

[Jeff Miller]

Comment on attachment 112354 [details]
Patch

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

> Source/WebKit2/WebProcess/Plugins/Plugin.h:212
> +    virtual bool wantsWindowRelativeCoordinates() = 0;

Should this be a const member function?

> Source/WebKit2/WebProcess/Plugins/PluginView.h:124
> +    virtual bool transformsAffectFrameRect();

Should this be a const member function?

[Anders Carlsson]

(In reply to comment #2)
> (From update of attachment 112354 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112354&action=review
> 
> > Source/WebKit2/WebProcess/Plugins/Plugin.h:212
> > +    virtual bool wantsWindowRelativeCoordinates() = 0;
> 
> Should this be a const member function?
> 
> > Source/WebKit2/WebProcess/Plugins/PluginView.h:124
> > +    virtual bool transformsAffectFrameRect();
> 
> 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.

[Simon Fraser (smfr)]

Comment on attachment 112354 [details]
Patch

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).

[Anders Carlsson]

(In reply to comment #4)
> (From update of attachment 112354 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112354&action=review
> 
> Can we add some tests pls?

I'll add more tests when I do the work necessary to make plug-ins work better with transforms.

> 
> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFView.cpp:307
> > +        graphicsContext->translate(-pluginView()->frameRect().x(), -pluginView()->frameRect().y());
> 
> Hmm, is frameRect() OK to use under transforms?

Not really, but since the scroll bars are child widgets of the enclosing scroll view, they're going to break horribly in non-trivial transforms anyway.

> 
> >>> 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?

Ideally all coordinates :) The plan is to move all plug-in subclasses to require non-window coordinates and then we can get rid of it.

> 
> > 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())

Fixed.

> 
> > 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

Fixed.

> 
> > 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).

I'll make the comment more explicit.

[Anders Carlsson]

Committed r98369: <http://trac.webkit.org/changeset/98369>