Bug 62145

Created attachment 96153 [details] Patch

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

Created attachment 96163 [details] Patch

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.

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?

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.

Please let the EWSes have their say.

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

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

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

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

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

Created attachment 96364 [details] Patch for landing

Comment on attachment 96364 [details] Patch for landing Clearing flags on attachment: 96364 Committed r88319: <http://trac.webkit.org/changeset/88319>

All reviewed patches have been landed. Closing bug.