Bug 33927 – Hit testing on composited plugins is broken

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

[Simon Fraser (smfr)]

Hit testing on composited plugins is broken

[Simon Fraser (smfr)]

<rdar://problem/7559069>

[Simon Fraser (smfr)]

Created attachment 47080 [details]
Patch

Comment on attachment 47080 [details]
Patch

> +/*    border: 1px solid black;*/

Please remove the commented-out property.

> [phone]  Simon Fraser  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Hit testing on composited plugins is broken
> +        https://bugs.webkit.org/show_bug.cgi?id=33927
> +        <rdar://problem/7559069>
> +
> +        
> +        Test: plugins/mouse-events-fixedpos.html

Extra empty line.

> +        * page/FrameView.cpp:
> +        (WebCore::FrameView::scrollPositionChanged):
> +        * platform/graphics/GraphicsContext.h:
> +        (WebCore::GraphicsContext::translate):
> +        * platform/graphics/IntSize.h:
> +        (WebCore::IntSize::isZeroSize):
> +        * platform/mac/WidgetMac.mm:
> +        (WebCore::Widget::paint):
> +        * rendering/RenderWidget.cpp:
> +        (WebCore::RenderWidget::paint):

I think you can provide more detail about the changes to each function.

> @@ -950,6 +950,10 @@ void FrameView::scrollPositionChanged()
>      if (layer)
>          layer->updateLayerPositions(RenderLayer::UpdateCompositingLayers);
>  #endif
> +
> +    // Update widget positions to take care of widgets inside fixed position elements.
> +    if (RenderView* root = m_frame->contentRenderer())
> +        root->updateWidgetPositions();

updateWidgetPositions() can potentially call layout() or call into plug-in code. Is it safe to do here? Can scrollPositionChanged() ever be called during layout?

[Darin Adler]

Comment on attachment 47080 [details]
Patch

> +    // Update widget positions to take care of widgets inside fixed position elements.
> +    if (RenderView* root = m_frame->contentRenderer())
> +        root->updateWidgetPositions();

Does this do unneeded expensive work on pages that have no fixed position elements? Will it make scrolling noticeably slower in such cases?

> +        void translate(const FloatSize& size) { translate(size.width(), size.height()); }
>          void translate(float x, float y);

Can we get rid of the separate x, y version of this function at some point?

> +    bool isZeroSize() const { return !m_width && !m_height; }

Why not just isZero()? The class's name already has the word size in it.

> +            if (!paintOffset.isZeroSize()) {
> +                paintInfo.context->translate(paintOffset);
> +                paintRect.move(-paintOffset);
> +            }

> +            if (!paintOffset.isZeroSize())
> +                paintInfo.context->translate(-paintOffset);

Is the zero check here a performance optimization? Could that optimization go into GraphicsContext::translate instead of the call site?

r=me

[Simon Fraser (smfr)]

(In reply to comment #4)
> (From update of attachment 47080 [details])
> > +    // Update widget positions to take care of widgets inside fixed position elements.
> > +    if (RenderView* root = m_frame->contentRenderer())
> > +        root->updateWidgetPositions();
> 
> Does this do unneeded expensive work on pages that have no fixed position
> elements? Will it make scrolling noticeably slower in such cases?

It's pretty fast; it uses a map of widgets, rather than crawling the tree.

> > +        void translate(const FloatSize& size) { translate(size.width(), size.height()); }
> >          void translate(float x, float y);
> 
> Can we get rid of the separate x, y version of this function at some point?

Eventually!

> 
> > +    bool isZeroSize() const { return !m_width && !m_height; }
> 
> Why not just isZero()? The class's name already has the word size in it.

Right.

> 
> > +            if (!paintOffset.isZeroSize()) {
> > +                paintInfo.context->translate(paintOffset);
> > +                paintRect.move(-paintOffset);
> > +            }
> 
> > +            if (!paintOffset.isZeroSize())
> > +                paintInfo.context->translate(-paintOffset);
> 
> Is the zero check here a performance optimization? Could that optimization go
> into GraphicsContext::translate instead of the call site?

GraphicsContext would need to avoid setting m_userToDeviceTransformKnownToBeIdentity for zero translations, rotations etc. This should be done as a separate patch.

[Simon Fraser (smfr)]

Created attachment 47132 [details]
Patch

[Dave Hyatt]

Comment on attachment 47132 [details]
Patch

r=me.  You still need to fix scrollbars, though, since they have the same problem.

[Simon Fraser (smfr)]

I filed bug 33965 to fix scrollbar updating later.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/53637

[Eric Seidel (no email)]

This caused the Gtk bots to fail. :( Please fix.

[Eric Seidel (no email)]

Nevermind.  looks like you already fixed.  Thank you!  I guess I was looking at an old copy of /console. :)