[webkit-reviews] review denied: [Bug 110895] Focus ring for a child layer is incorrectly offset by ancestor composited layer's position : [Attachment 190636] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Feb 27 19:49:54 PST 2013


Simon Fraser (smfr) <simon.fraser at apple.com> has denied Xianzhu Wang
<wangxianzhu at chromium.org>'s request for review:
Bug 110895: Focus ring for a child layer is incorrectly offset by ancestor
composited layer's position
https://bugs.webkit.org/show_bug.cgi?id=110895

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=190636&action=review


The fix is good, just some naming quibbles.

> Source/WebCore/ChangeLog:31
> +	   (RenderBox):

You should remove useless lines like this.

> Source/WebCore/rendering/PaintInfo.h:58
> +	   OverlapTestRequestMap* overlapTestRequests = 0, const
RenderLayerModelObject* newRepaintContainer = 0)

The name is a bit confusing, since this isn't about "repaint". Let's call it
"paintContainer" here. Or we could rename "paintingRoot", and re-use that name
for this.

> Source/WebCore/rendering/RenderBlock.h:538
> +    virtual void addFocusRingRects(Vector<IntRect>&, const LayoutPoint&,
const RenderLayerModelObject*);

This should name the parameter so it's easy to tell what it's for. Perhaps
"rootForRects" or "paintingRoot".

> Source/WebCore/rendering/RenderBox.h:165
> +    virtual void addFocusRingRects(Vector<IntRect>&, const LayoutPoint&,
const RenderLayerModelObject*);

Ditto.

> Source/WebCore/rendering/RenderInline.cpp:1368
> +void RenderInline::addFocusRingRects(Vector<IntRect>& rects, const
LayoutPoint& additionalOffset, const RenderLayerModelObject* repaintContainer)

repaintContainer->paintingRoot.

> Source/WebCore/rendering/RenderInline.h:84
> +    virtual void addFocusRingRects(Vector<IntRect>&, const LayoutPoint&,
const RenderLayerModelObject*);

Same here. If you're touching these functions, you might as well add OVERRIDE.


More information about the webkit-reviews mailing list