Bug 62145 – Convert nodeAtPoint to IntPoint

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

[Emil A Eklund]

Ongoing tx, ty removal.

[Emil A Eklund]

Created attachment 96153 [details]
Patch

[WebKit Review Bot]

Comment on attachment 96153 [details]
Patch

Attachment 96153 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8744012

[Emil A Eklund]

Created attachment 96163 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 96153 [details]
Patch

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

The more I see of these, the less I like "accumulatedOffset".  I conveys about as much meaning as "addedInt".  I think we need a better name.  I am entertained that this patch had some (in my view) at least slightly better names for these values prior to this change.  r-, but only because of the red bots.

> Source/WebCore/rendering/svg/RenderSVGModelObject.h:70
> -    bool nodeAtPoint(const HitTestRequest&, HitTestResult&, const IntPoint& pointInContainer, int dxParentToContainer, int dyParentToContainer, HitTestAction);
> +    bool nodeAtPoint(const HitTestRequest&, HitTestResult&, const IntPoint& pointInContainer, const IntPoint& accumulatedOffset, HitTestAction);

Again, more attempts by me to describe what I thought was going on as I wrote RenderSVGModelObject. :)

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:-442
> -    IntPoint pointInParent = pointInContainer - containerToParentOffset;

This was my understanding at the time, was that this offset was from the container (of whatever form that is) to our parent.  I actually kinda think containerToParentOffset is more descriptive than accumulatedOffset. :)  I wish we had dug up this code earlier.

We can always of course rename accumulatedOffset everywhere now that we're slowly understanding what this is all doing.

[Emil A Eklund]

The name containerToParentOffset is much more descriptive than accumulatedOffset. How about I change it to containerToParentOffset in this patch and then go back and replace the existing uses of accumulatedOffset?

[Eric Seidel (no email)]

I think you should run any naming by Simon since he's had opinions in the past.

But I'm OK with "containerToParentOffset" used in this or other places.

[Eric Seidel (no email)]

Please let the EWSes have their say.

[Emil A Eklund]

Simon, what's your take on this? Do you prefer accumulatedOffset, containerToParentOffset or something else entirely?

[Simon Fraser (smfr)]

(In reply to comment #8)
> Simon, what's your take on this? Do you prefer accumulatedOffset, containerToParentOffset or something else entirely?

accumulatedOffset is more accurate than containerToParentOffset, but I'm not sure that 'accumulated' adds much.

[Emil A Eklund]

> accumulatedOffset is more accurate than containerToParentOffset, but I'm not sure that 'accumulated' adds much.

True, naming is hard. Just calling it offset seems confusing though.

[Eric Seidel (no email)]

(In reply to comment #9)
> (In reply to comment #8)
> > Simon, what's your take on this? Do you prefer accumulatedOffset, containerToParentOffset or something else entirely?
> 
> accumulatedOffset is more accurate than containerToParentOffset, but I'm not sure that 'accumulated' adds much.

containerToParentOffset is just describing what I thought the offset meant, when I wrote RenderSVGModelObject 2 years ago.

If we actually know what these offsets do... then we can name them better. :)  I still don't understand what they mean.

[Emil A Eklund]

I plan to land this with accumulatedOffset. I'll change it here and elsewhere if and when we come up with a better name.

[Emil A Eklund]

Created attachment 96364 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 96364 [details]
Patch for landing

Clearing flags on attachment: 96364

Committed r88319: <http://trac.webkit.org/changeset/88319>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.