Bug 60783 – Switch addFocusRingRects to use IntPoint

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Levi Weintraub]

This is a low-level function taking tx/ty offsets that can be switched to IntPoint without forcing too many other areas to follow suit.

[Levi Weintraub]

Erm, I mean IntSize!

[Levi Weintraub]

This isn't explicitly a paint call, though it's called from paint functions and ultimately results in coordinates for the paint system, so I'm not sure paintOffset is quite right here. Perhaps offsetForPainting?

[Levi Weintraub]

Created attachment 93511 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 93511 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93511&action=review

> Source/WebCore/rendering/RenderBlock.cpp:5852
> +void RenderBlock::addFocusRingRects(Vector<IntRect>& rects, const IntSize& offset)

Seems like this is an IntPoint?  originInParent?  Or maybe we should compute originInParent from offset?  In any case, "offset" needs a better name.  offset to what?  offset in what?  what is "offset"?

[Levi Weintraub]

I brought this up in IRC too and got nowhere. There will be a similar issue with hit testing. Perhaps this is where we typedef something like RootOffset to IntSize and include a comment about what it is and how it's used? Otherwise naming this is thoroughly non-trivial :-/

[Darin Adler]

Certainly if tx,ty,width,height is an IntRect, we want tx,ty to be an IntPoint, not an IntSize.

[Levi Weintraub]

Created attachment 94554 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 94554 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94554&action=review

> Source/WebCore/rendering/RenderBlock.h:332
> +    virtual void addFocusRingRects(Vector<IntRect>&, const IntPoint&);

I think the IntPoint may need a name here.  I'm not sure "paintOffset" is descriptive enough though. originInContainer?

> Source/WebCore/rendering/RenderInline.cpp:1351
> +            FloatPoint pos(paintOffset);

Why do we use a FloatPoint here if we just floor it later?

[Levi Weintraub]

(In reply to comment #8)
> (From update of attachment 94554 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94554&action=review
> 
> > Source/WebCore/rendering/RenderBlock.h:332
> > +    virtual void addFocusRingRects(Vector<IntRect>&, const IntPoint&);
> 
> I think the IntPoint may need a name here.  I'm not sure "paintOffset" is descriptive enough though. originInContainer?
> 

Again, this is the real rub... I'm happy to call it that if we're convinced that isn't misleading in some cases. Container is kind of an overloaded term. Simon's kind of my go-to guy for naming this, but last run through in IRC only yielded "offset."

> > Source/WebCore/rendering/RenderInline.cpp:1351
> > +            FloatPoint pos(paintOffset);
> 
> Why do we use a FloatPoint here if we just floor it later?

Sheesh, good question. Sorry I missed that!

[Eric Seidel (no email)]

Comment on attachment 94554 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94554&action=review

>>> Source/WebCore/rendering/RenderBlock.h:332
>>> +    virtual void addFocusRingRects(Vector<IntRect>&, const IntPoint&);
>> 
>> I think the IntPoint may need a name here.  I'm not sure "paintOffset" is descriptive enough though. originInContainer?
> 
> Again, this is the real rub... I'm happy to call it that if we're convinced that isn't misleading in some cases. Container is kind of an overloaded term. Simon's kind of my go-to guy for naming this, but last run through in IRC only yielded "offset."

My understanding is that tx,ty mean differnet things in differnet plaes.  We need to enumerate those places and come up with consistent naming for each.  It seems here it's used as the origin of the RenderObject for which focus rings are being painted?  I assume that a normal focus ring is something like -5, -5, width + 10, height + 10? and tx, ty are then the x, y (in the containing block?) of the render object?

>>> Source/WebCore/rendering/RenderInline.cpp:1351
>>> +            FloatPoint pos(paintOffset);
>> 
>> Why do we use a FloatPoint here if we just floor it later?
> 
> Sheesh, good question. Sorry I missed that!

I mean, it's possible we're using intermediate float values (adding floats, etc), I didn't look at the code closely enough.

[Simon Fraser (smfr)]

Comment on attachment 94554 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94554&action=review

>>>> Source/WebCore/rendering/RenderBlock.h:332
>>>> +    virtual void addFocusRingRects(Vector<IntRect>&, const IntPoint&);
>>> 
>>> I think the IntPoint may need a name here.  I'm not sure "paintOffset" is descriptive enough though. originInContainer?
>> 
>> Again, this is the real rub... I'm happy to call it that if we're convinced that isn't misleading in some cases. Container is kind of an overloaded term. Simon's kind of my go-to guy for naming this, but last run through in IRC only yielded "offset."
> 
> My understanding is that tx,ty mean differnet things in differnet plaes.  We need to enumerate those places and come up with consistent naming for each.  It seems here it's used as the origin of the RenderObject for which focus rings are being painted?  I assume that a normal focus ring is something like -5, -5, width + 10, height + 10? and tx, ty are then the x, y (in the containing block?) of the render object?

IIRC addFocusRingRects() is called at times other than painting (you can ask for a renderer's focus ring rects at any time), so paintOffset doesn't work so well. I'm OK with 'offset', since we still don't have a good term for all the things that tx,ty can mean.

[Eric Seidel (no email)]

I feel like "offset" is as meaningless as "size" or "o" would be.  What's it an offset from?

[Darin Adler]

(In reply to comment #12)
> I feel like "offset" is as meaningless as "size" or "o" would be.  What's it an offset from?

That’s not a meaningful question for this function.

The function adds rectangles to a vector and also adds an offset to each rectangle while doing so. The meaning of the offset is known to the caller but not to this function. As far as this function is concerned, it is just an offset to add to the rectangles while adding them to the vector.

Another design would be to remove the offset and apply the offset to the rectangles separately after constructing the vector.

[Levi Weintraub]

(In reply to comment #13)
> Another design would be to remove the offset and apply the offset to the rectangles separately after constructing the vector.

I think we're better off keeping the offset since some overloaded versions of this function adjust this offset differently, and I believe it makes the most sense to keep that logic in its respective class.

[Darin Adler]

(In reply to comment #14)
> I think we're better off keeping the offset since some overloaded versions of this function adjust this offset differently, and I believe it makes the most sense to keep that logic in its respective class.

I’m not sure that’s right. Adjusting the offset is simply applying another offset. That could be done inside the overloaded functions regardless of whether an offset is passed in to the function.

But my main point is that giving a meaningful name explaining what the offset is “relative to” would not be correct for this function.

[Levi Weintraub]

Created attachment 94634 [details]
Patch

[Darin Adler]

Perhaps “additionalOffset” would be a good name.

[Levi Weintraub]

(In reply to comment #17)
> Perhaps “additionalOffset” would be a good name.

Sounds reasonable.

[Levi Weintraub]

Created attachment 94663 [details]
Patch

[Levi Weintraub]

Created attachment 94719 [details]
Patch

Re-uploading without unintentional xcodeproj change.

[Eric Seidel (no email)]

Comment on attachment 94719 [details]
Patch

OK.  Darin convinced me of the "offset" case.  Thanks for all your work on this.

[Levi Weintraub]

Comment on attachment 94719 [details]
Patch

Clearing flags on attachment: 94719

Committed r87302: <http://trac.webkit.org/changeset/87302>

[Levi Weintraub]

All reviewed patches have been landed.  Closing bug.