Bug 33927

Summary: Hit testing on composited plugins is broken
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, gns, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch hyatt: review+

Description Simon Fraser (smfr) 2010-01-20 16:51:23 PST
Hit testing on composited plugins is broken
Comment 1 Simon Fraser (smfr) 2010-01-20 16:51:53 PST
<rdar://problem/7559069>
Comment 2 Simon Fraser (smfr) 2010-01-20 16:59:44 PST
Created attachment 47080 [details]
Patch
Comment 3 mitz 2010-01-20 19:12:57 PST
Comment on attachment 47080 [details]
Patch

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

Please remove the commented-out property.

> +2010-01-20  Simon Fraser  <simon.fraser@apple.com>
> +
> +        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?
Comment 4 Darin Adler 2010-01-21 09:53:15 PST
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
Comment 5 Simon Fraser (smfr) 2010-01-21 10:57:05 PST
(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.
Comment 6 Simon Fraser (smfr) 2010-01-21 11:24:59 PST
Created attachment 47132 [details]
Patch
Comment 7 Dave Hyatt 2010-01-21 11:27:29 PST
Comment on attachment 47132 [details]
Patch

r=me.  You still need to fix scrollbars, though, since they have the same problem.
Comment 8 Simon Fraser (smfr) 2010-01-21 12:14:47 PST
I filed bug 33965 to fix scrollbar updating later.
Comment 9 Simon Fraser (smfr) 2010-01-21 12:27:31 PST
http://trac.webkit.org/changeset/53637
Comment 10 Eric Seidel (no email) 2010-01-21 15:55:59 PST
This caused the Gtk bots to fail. :( Please fix.
Comment 11 Eric Seidel (no email) 2010-01-21 15:59:56 PST
Nevermind.  looks like you already fixed.  Thank you!  I guess I was looking at an old copy of /console. :)