PDF SUBFRAMES: Incomplete repaint after pinch to zoom
Created attachment 112354 [details] Patch
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?
(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.
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).
(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.
Committed r98369: <http://trac.webkit.org/changeset/98369>