Bug 60783

Erm, I mean IntSize!

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?

Created attachment 93511 [details] Patch

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"?

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 :-/

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

Created attachment 94554 [details] Patch

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?

(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 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 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.

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

(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.

(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.

(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.

Created attachment 94634 [details] Patch

Perhaps “additionalOffset” would be a good name.

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

Created attachment 94663 [details] Patch

Created attachment 94719 [details] Patch Re-uploading without unintentional xcodeproj change.

Comment on attachment 94719 [details] Patch OK. Darin convinced me of the "offset" case. Thanks for all your work on this.

Comment on attachment 94719 [details] Patch Clearing flags on attachment: 94719 Committed r87302: <http://trac.webkit.org/changeset/87302>

All reviewed patches have been landed. Closing bug.