[webkit-reviews] review granted: [Bug 33927] Hit testing on composited plugins is broken : [Attachment 47080] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 33927: Hit testing on composited plugins is broken
https://bugs.webkit.org/show_bug.cgi?id=33927

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // 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