Bug 60783

Summary: Switch addFocusRingRects to use IntPoint
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eae, eric, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60318, 60789    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Levi Weintraub 2011-05-13 11:52:05 PDT
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.
Comment 1 Levi Weintraub 2011-05-13 11:54:32 PDT
Erm, I mean IntSize!
Comment 2 Levi Weintraub 2011-05-13 12:29:14 PDT
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?
Comment 3 Levi Weintraub 2011-05-13 14:32:53 PDT
Created attachment 93511 [details]
Patch
Comment 4 Eric Seidel (no email) 2011-05-13 14:42:55 PDT
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"?
Comment 5 Levi Weintraub 2011-05-13 14:45:58 PDT
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 :-/
Comment 6 Darin Adler 2011-05-13 17:18:22 PDT
Certainly if tx,ty,width,height is an IntRect, we want tx,ty to be an IntPoint, not an IntSize.
Comment 7 Levi Weintraub 2011-05-23 20:53:28 PDT
Created attachment 94554 [details]
Patch
Comment 8 Eric Seidel (no email) 2011-05-23 21:11:14 PDT
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?
Comment 9 Levi Weintraub 2011-05-23 21:49:22 PDT
(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!
Comment 10 Eric Seidel (no email) 2011-05-23 21:52:54 PDT
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.
Comment 11 Simon Fraser (smfr) 2011-05-24 08:29:09 PDT
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.
Comment 12 Eric Seidel (no email) 2011-05-24 08:37:26 PDT
I feel like "offset" is as meaningless as "size" or "o" would be.  What's it an offset from?
Comment 13 Darin Adler 2011-05-24 09:15:04 PDT
(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.
Comment 14 Levi Weintraub 2011-05-24 09:57:56 PDT
(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.
Comment 15 Darin Adler 2011-05-24 10:00:46 PDT
(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.
Comment 16 Levi Weintraub 2011-05-24 10:08:54 PDT
Created attachment 94634 [details]
Patch
Comment 17 Darin Adler 2011-05-24 11:33:11 PDT
Perhaps “additionalOffset” would be a good name.
Comment 18 Levi Weintraub 2011-05-24 11:34:18 PDT
(In reply to comment #17)
> Perhaps “additionalOffset” would be a good name.

Sounds reasonable.
Comment 19 Levi Weintraub 2011-05-24 12:27:40 PDT
Created attachment 94663 [details]
Patch
Comment 20 Levi Weintraub 2011-05-24 17:12:37 PDT
Created attachment 94719 [details]
Patch

Re-uploading without unintentional xcodeproj change.
Comment 21 Eric Seidel (no email) 2011-05-24 18:52:48 PDT
Comment on attachment 94719 [details]
Patch

OK.  Darin convinced me of the "offset" case.  Thanks for all your work on this.
Comment 22 Levi Weintraub 2011-05-25 10:04:51 PDT
Comment on attachment 94719 [details]
Patch

Clearing flags on attachment: 94719

Committed r87302: <http://trac.webkit.org/changeset/87302>
Comment 23 Levi Weintraub 2011-05-25 10:04:57 PDT
All reviewed patches have been landed.  Closing bug.