Bug 98139 - Disambiguate innerFramePoint and mainFramePoint
Summary: Disambiguate innerFramePoint and mainFramePoint
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on: 101915
Blocks: 95204 101590 102907 102909
  Show dependency treegraph
 
Reported: 2012-10-02 03:43 PDT by Allan Sandfeld Jensen
Modified: 2012-11-21 07:47 PST (History)
14 users (show)

See Also:


Attachments
Patch (19.52 KB, patch)
2012-10-02 04:18 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (23.11 KB, patch)
2012-10-02 05:10 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (24.04 KB, patch)
2012-10-02 06:05 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (22.68 KB, patch)
2012-10-04 04:40 PDT, Allan Sandfeld Jensen
gtk-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (22.68 KB, patch)
2012-10-05 02:40 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (29.93 KB, patch)
2012-10-19 07:30 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (30.62 KB, patch)
2012-11-07 08:18 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (32.05 KB, patch)
2012-11-08 05:13 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (29.39 KB, patch)
2012-11-12 03:44 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (29.26 KB, patch)
2012-11-19 07:15 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (29.25 KB, patch)
2012-11-21 01:39 PST, Allan Sandfeld Jensen
jchaffraix: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2012-10-02 03:43:23 PDT
HitTestResult derives from HitTestLocation and thereby doubles as a point. In the current algorithm on render-tree level, this point is always both the original hit point, and the point in the result frame, because it stays within the same frame. The HitTestResult returned by EventHandler::hitTestResultAtPoint however descends into sub-frames, and currently returns a HitTestResult which contains a point transformed into result-frame coordinates.

The problem with this semantics is that descending into sub-frames is also necessary for area-based hit-testing, and the concept of the frame of the result become meaningless when the results can be from multiple frames.

The issue was seen with patch landed and reverted in bug #95204. As a stepping stone before relanding a solution there, we need to make the users of HitTestResult explicitly use either the result-frame point or original hit-test point. This is likely to solve a some corner-case bugs as well.
Comment 1 Allan Sandfeld Jensen 2012-10-02 04:18:19 PDT
Created attachment 166658 [details]
Patch

This patch is designed to cause compile errors everywhere the disambiguation still needs to be resolved.
Comment 2 Allan Sandfeld Jensen 2012-10-02 04:19:59 PDT
Note that replacing HitTestResult::point or HitTestResult::roundedPoint with resultFramePoint is the safe choice that does not change existing behaviour. I feel however the opportunity should be taken to examine if using the resultFramePoint is really the right choice.
Comment 3 Build Bot 2012-10-02 04:24:29 PDT
Comment on attachment 166658 [details]
Patch

Attachment 166658 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14123317
Comment 4 WebKit Review Bot 2012-10-02 04:25:20 PDT
Comment on attachment 166658 [details]
Patch

Attachment 166658 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14129192
Comment 5 Gyuyoung Kim 2012-10-02 04:28:22 PDT
Comment on attachment 166658 [details]
Patch

Attachment 166658 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14135169
Comment 6 Peter Beverloo (cr-android ews) 2012-10-02 04:48:48 PDT
Comment on attachment 166658 [details]
Patch

Attachment 166658 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14134165
Comment 7 Allan Sandfeld Jensen 2012-10-02 05:10:23 PDT
Created attachment 166674 [details]
Patch

Fix remaining calls in EFL, Mac and Chromium.
Comment 8 Build Bot 2012-10-02 05:18:44 PDT
Comment on attachment 166674 [details]
Patch

Attachment 166674 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14132205
Comment 9 Allan Sandfeld Jensen 2012-10-02 06:05:16 PDT
Created attachment 166681 [details]
Patch

Fix remaining call in Mac WK2.
Comment 10 Darin Adler 2012-10-02 10:00:37 PDT
Comment on attachment 166681 [details]
Patch

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

> Source/WebCore/rendering/HitTestResult.h:124
> +    LayoutPoint resultFramePoint() const { return HitTestLocation::point(); }

What is a “frame point”? This is a new term you are introducing here, and I don’t understand what it means. Is it a reference to some particular coordinate system? The coordinate system of some document? Some viewport?

> Source/WebCore/rendering/HitTestResult.h:128
> +    void setOriginalFramePoint(const LayoutPoint& p) { m_originalFramePoint = p; }

Please use a word instead of a single letter.
Comment 11 Darin Adler 2012-10-02 10:02:13 PDT
Comment on attachment 166681 [details]
Patch

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

>> Source/WebCore/rendering/HitTestResult.h:124
>> +    LayoutPoint resultFramePoint() const { return HitTestLocation::point(); }
> 
> What is a “frame point”? This is a new term you are introducing here, and I don’t understand what it means. Is it a reference to some particular coordinate system? The coordinate system of some document? Some viewport?

Further, what is a “result frame point”? Is it a point in something called a “result frame”? Or is it a “result point” in some particular “frame” coordinate system?

> Source/WebCore/rendering/HitTestResult.h:127
> +    LayoutPoint originalFramePoint() const { return m_originalFramePoint; }

Same question here. Is this the original “frame point” or is it a point in the “original frame”?
Comment 12 Allan Sandfeld Jensen 2012-10-02 10:22:07 PDT
(In reply to comment #11)
> (From update of attachment 166681 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166681&action=review
> 
> >> Source/WebCore/rendering/HitTestResult.h:124
> >> +    LayoutPoint resultFramePoint() const { return HitTestLocation::point(); }
> > 
> > What is a “frame point”? This is a new term you are introducing here, and I don’t understand what it means. Is it a reference to some particular coordinate system? The coordinate system of some document? Some viewport?
> 
> Further, what is a “result frame point”? Is it a point in something called a “result frame”? Or is it a “result point” in some particular “frame” coordinate system?
> 
It is the point in coordinates of frame of result (the inner node).

> > Source/WebCore/rendering/HitTestResult.h:127
> > +    LayoutPoint originalFramePoint() const { return m_originalFramePoint; }
> 
> Same question here. Is this the original “frame point” or is it a point in the “original frame”?

It is the point in coordinates of original frame. It is also the original point given to the hit test.
Comment 13 Allan Sandfeld Jensen 2012-10-04 03:47:25 PDT
(In reply to comment #11)
> (From update of attachment 166681 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166681&action=review
> 
> >> Source/WebCore/rendering/HitTestResult.h:124
> >> +    LayoutPoint resultFramePoint() const { return HitTestLocation::point(); }
> > 
> > What is a “frame point”? This is a new term you are introducing here, and I don’t understand what it means. Is it a reference to some particular coordinate system? The coordinate system of some document? Some viewport?
> 
> Further, what is a “result frame point”? Is it a point in something called a “result frame”? Or is it a “result point” in some particular “frame” coordinate system?
> 
> > Source/WebCore/rendering/HitTestResult.h:127
> > +    LayoutPoint originalFramePoint() const { return m_originalFramePoint; }
> 
> Same question here. Is this the original “frame point” or is it a point in the “original frame”?

It is not a frame point, it is point in the original frame or in the result frame. If these names are confusing, perhaps better names can be chosen. I just don't know what that should be.
Comment 14 Allan Sandfeld Jensen 2012-10-04 04:40:30 PDT
Created attachment 167075 [details]
Patch

Clarify method names, now using pointInResultFrame and pointInOriginalFrame.
Comment 15 kov's GTK+ EWS bot 2012-10-04 04:50:04 PDT
Comment on attachment 167075 [details]
Patch

Attachment 167075 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14138928
Comment 16 Allan Sandfeld Jensen 2012-10-05 02:40:25 PDT
Created attachment 167291 [details]
Patch

Fix one forgotten rename in GTK.
Comment 17 Julien Chaffraix 2012-10-16 10:27:24 PDT
Comment on attachment 167291 [details]
Patch

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

I second Darin's comments about the naming. The current naming doesn't really convey the idea that the results are in a certain frame's coordinate.

> Source/WebCore/rendering/HitTestResult.h:103
> -class HitTestResult : public HitTestLocation {
> +class HitTestResult : protected HitTestLocation {

This line took me a while to understand. The fact that you are using 'protected' inheritance - along with the forwarding functions below - makes me wonder if the relationship between the 2 classes shouldn't be composition (has-a) instead of inheritance (is-a).

> Source/WebCore/rendering/HitTestResult.h:127
> +    // The point in the coordinates of the result frame, that is the frame containing innerNode.
> +    LayoutPoint pointInResultFrame() const { return HitTestLocation::point(); }

Some suggestions:
* pointInMostInnerFrameCoordinate()
* pointInInnerFrameCoordinate()

> Source/WebCore/rendering/HitTestResult.h:130
> +    // The point in the coordinates of the original frame hit-tested usually the main frame.
> +    LayoutPoint pointInOriginalFrame() const { return m_originalFramePoint; }

Is it really possible for the original frame not to be the main frame? One of the changes in WebKit/chromium/ContextMenuClientImpl.cpp (getCustomMenuFromDefaultItems) assumes that this will always return the point in the main frame's coordinates.

> Source/WebKit/win/WebView.cpp:1364
> -    POINT point(view->contentsToWindow(contextMenuController->hitTestResult().roundedPoint()));
> +    POINT point(view->contentsToWindow(roundedIntPoint(contextMenuController->hitTestResult().pointInResultFrame())));

All platforms round the return point so it would be better to move the rounding in HitTestResult.
Comment 18 Allan Sandfeld Jensen 2012-10-18 06:03:20 PDT
(In reply to comment #17)
> (From update of attachment 167291 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167291&action=review
> 
> I second Darin's comments about the naming. The current naming doesn't really convey the idea that the results are in a certain frame's coordinate.
> 
> > Source/WebCore/rendering/HitTestResult.h:103
> > -class HitTestResult : public HitTestLocation {
> > +class HitTestResult : protected HitTestLocation {
> 
> This line took me a while to understand. The fact that you are using 'protected' inheritance - along with the forwarding functions below - makes me wonder if the relationship between the 2 classes shouldn't be composition (has-a) instead of inheritance (is-a).
> 
That has been my secret plan all along (shh!), but it is not the focus of this patch.

> 
> Is it really possible for the original frame not to be the main frame? One of the changes in WebKit/chromium/ContextMenuClientImpl.cpp (getCustomMenuFromDefaultItems) assumes that this will always return the point in the main frame's coordinates.
> 
API wise yes, in practice not really. EventHandler::hitTestResultAtPoint has a weird trick where it runs the hit-test again from the main-frame, but only if it found something.


> > Source/WebKit/win/WebView.cpp:1364
> > -    POINT point(view->contentsToWindow(contextMenuController->hitTestResult().roundedPoint()));
> > +    POINT point(view->contentsToWindow(roundedIntPoint(contextMenuController->hitTestResult().pointInResultFrame())));
> 
> All platforms round the return point so it would be better to move the rounding in HitTestResult.

Though a patch a year ago did try to convert all the platforms to only using the rounded point, there are a number of (new?) places where the point is used without rounding.
Comment 19 Allan Sandfeld Jensen 2012-10-19 07:30:29 PDT
Created attachment 169617 [details]
Patch

Ensure outer frame is main frame, and do appropiate renaming and clean-up
Comment 20 Build Bot 2012-10-19 08:03:22 PDT
Comment on attachment 169617 [details]
Patch

Attachment 169617 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14473001
Comment 21 Allan Sandfeld Jensen 2012-11-07 08:18:57 PST
Created attachment 172809 [details]
Patch

Rebased and updated exported symbols for Mac build system
Comment 22 Julien Chaffraix 2012-11-07 14:34:28 PST
Comment on attachment 172809 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Split the point() method in HitTestResult into two, resultFramePoint and originalFramePoint().
> +        To do this HitTestResult no longer inherits publically from HitTestLocation, which should also
> +        help to later separate the two classes more completely.

I think this is stale: resultFramePoint doesn't exist in this iteration. The bug title also refers to this method.

As a whole, your ChangeLogs should really say MORE rather than less (as it is the case now). Here there are questions I can't answer by looking at it so it's difficult to know if you just overlooked them.

> Source/WebCore/page/EventHandler.cpp:1120
> +

The previous code would fallback to doing this extra hit-testing at the main frame level *after* the original one: couldn't this change the result of EventHandler::hitTestResultAtPoint on inner frames as we could hit a parent frame first?

(that's exactly the kind of change that must be explained and argued in the ChangeLog)

> Source/WebCore/rendering/HitTestResult.h:103
> -class HitTestResult : public HitTestLocation {
> +class HitTestResult : protected HitTestLocation {

I would like a FIXME added to track changing the relationship between HitTestLocation and HitTestResult. Extra points if you have a bug!

> Source/WebCore/rendering/HitTestResult.h:126
> +    LayoutPoint pointInMainFrame() const { return m_pointInMainFrame; }

This should return a const LayoutPoint&, this can avoid a copy if the caller is careful. It also shouldn't break existing code.

> Source/WebCore/rendering/HitTestResult.h:131
> +    LayoutPoint pointInInnerFrame() const { return HitTestLocation::point(); }

Ditto (but would need to modify HitTestLocation::point() so it's a nit)

> Source/WebCore/rendering/HitTestResult.h:136
> +    LayoutPoint localPoint() const { return m_localPoint; }

Ditto.

> Source/WebCore/rendering/HitTestResult.h:203
> +    LayoutPoint m_pointInMainFrame;

pointInMainFrame is not super clear (what should this be if you have a point in an inner frame?). It should be pointInMainFrameCoordinates.

> Source/WebKit/gtk/webkit/webkithittestresult.cpp:323
> +    innerFrame = result.innerFrame();
> +    if (innerFrame && innerFrame->view()) {
>          // Convert document coords to widget coords.
> -        point = targetFrame->view()->contentsToWindow(result.roundedPoint());
> +        point = innerFrame->view()->contentsToWindow(result.roundedPointInInnerFrame());
>      } else
> -        point = result.roundedPoint();
> +        point = result.roundedPointInMainFrame();

This really looks like

point = result.roundedPointInMainFrame();

You wouldn't even need |innerFrame|.
Comment 23 Allan Sandfeld Jensen 2012-11-08 04:39:35 PST
(In reply to comment #22)
> (From update of attachment 172809 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172809&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Split the point() method in HitTestResult into two, resultFramePoint and originalFramePoint().
> > +        To do this HitTestResult no longer inherits publically from HitTestLocation, which should also
> > +        help to later separate the two classes more completely.
> 
> I think this is stale: resultFramePoint doesn't exist in this iteration. The bug title also refers to this method.
> 
> As a whole, your ChangeLogs should really say MORE rather than less (as it is the case now). Here there are questions I can't answer by looking at it so it's difficult to know if you just overlooked them.
> 
You are right. I will update the ChangeLogs to be more helpful.

> > Source/WebCore/page/EventHandler.cpp:1120
> > +
> 
> The previous code would fallback to doing this extra hit-testing at the main frame level *after* the original one: couldn't this change the result of EventHandler::hitTestResultAtPoint on inner frames as we could hit a parent frame first?
> 
It is a tricky case, but it shouldn't really be called on non-main frames in the first place, and there is the quirk in RenderLayer which ensure all hit always pretends to hit something, so it always ends up as two hit-tests if one is started in a non main-frame.

> (that's exactly the kind of change that must be explained and argued in the ChangeLog)
> 
Right.

> > Source/WebKit/gtk/webkit/webkithittestresult.cpp:323
> > +    innerFrame = result.innerFrame();
> > +    if (innerFrame && innerFrame->view()) {
> >          // Convert document coords to widget coords.
> > -        point = targetFrame->view()->contentsToWindow(result.roundedPoint());
> > +        point = innerFrame->view()->contentsToWindow(result.roundedPointInInnerFrame());
> >      } else
> > -        point = result.roundedPoint();
> > +        point = result.roundedPointInMainFrame();
> 
> This really looks like
> 
> point = result.roundedPointInMainFrame();
> 
> You wouldn't even need |innerFrame|.

I am trying to fix as little code as possible in all the ports. More cleanups will be possible in most of them after this patch lands, but I am not in the best position to test all those changes. This case is one of the only ones I changed because it also managed to confuse targetFrame with innerFrame.
Comment 24 Allan Sandfeld Jensen 2012-11-08 05:10:56 PST
(In reply to comment #22)
> > Source/WebKit/gtk/webkit/webkithittestresult.cpp:323
> > +    innerFrame = result.innerFrame();
> > +    if (innerFrame && innerFrame->view()) {
> >          // Convert document coords to widget coords.
> > -        point = targetFrame->view()->contentsToWindow(result.roundedPoint());
> > +        point = innerFrame->view()->contentsToWindow(result.roundedPointInInnerFrame());
> >      } else
> > -        point = result.roundedPoint();
> > +        point = result.roundedPointInMainFrame();
> 
> This really looks like
> 
> point = result.roundedPointInMainFrame();
> 
Actually no. This confused me a few times as well, but window coordinates are not the same a main frame coordinates.
Comment 25 Allan Sandfeld Jensen 2012-11-08 05:13:46 PST
Created attachment 173020 [details]
Patch

Updated changelogs and API, but will probably need an update on exported symbols as well
Comment 26 Antonio Gomes 2012-11-08 10:24:34 PST
Comment on attachment 173020 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        Many of the call-sites of hitTestResultAtPoint tries to convert back from innerFramePoint to
> +        mainFramePoint. These conversions have not been all cleaned up in this patch, and are left for
> +        the ports to handle, since they are in a better position to test it. This is additionally 

when it land, please file port-specific follow-up bugs for port maintainers to flip it.

> Source/WebCore/ChangeLog:25
> +            Used to do a second hit-test from the main-frame if started on a non main-frame. Now it
> +            does only one hit-test and redirects the test to the main-frame immidiately. Improving
> +            both performance, and guaranteeing its tests always start in the main-frame.

can this be a fix by itself?

> Source/WebCore/page/EventHandler.cpp:1119
> +    // We always send hitTestResultAtPoint to the main frame if we have one,
> +    // otherwise we might hit areas that are obscured by higher frames.
> +    if (Page* page = m_frame->page()) {
> +        Frame* mainFrame = page->mainFrame();
> +        if (m_frame != mainFrame) {
> +            FrameView* frameView = m_frame->view();
> +            FrameView* mainView = mainFrame->view();
> +            if (frameView && mainView) {
> +                IntPoint mainFramePoint = mainView->rootViewToContents(frameView->contentsToRootView(roundedIntPoint(point)));
> +                return mainFrame->eventHandler()->hitTestResultAtPoint(mainFramePoint, allowShadowContent, ignoreClipping, testScrollbars, hitType, padding);
> +            }
> +        }
> +    }

That is the same about the changelog question above: should not this part by itself be a fix and even be testable? Or this whole refactor depends on this premise?

> Source/WebCore/page/EventHandler.cpp:1148
> -        frame->contentRenderer()->hitTest(HitTestRequest(hitType), widgetHitTestResult);
> +        widgetHitTestResult.setPointInMainFrame(result.pointInMainFrame());
> +        frame->contentRenderer()->hitTest(request, widgetHitTestResult);

I have to admit it gets a bit confusing here.

> Source/WebCore/rendering/HitTestResult.cpp:286
> +Frame* HitTestResult::innerFrame() const

actualHitFrame?

::innerFrame() returning a main frame might sound confusing as well.

> Source/WebCore/rendering/HitTestResult.h:126
> +    // The hit-tested point in the coordinates of the main frame.

contents coordinates or viewport coordinates?

> Source/WebCore/rendering/HitTestResult.h:131
> +    // The hit-tested point in the coordinates of the inner frame, the frame containing innerNode.

same question here.

> Source/WebCore/rendering/HitTestResult.h:134
> +    Frame* innerFrame() const;

const Frame*?

> Source/WebCore/rendering/HitTestResult.h:138
> +    void setLocalPoint(const LayoutPoint& p) { m_localPoint = p; }

better (but waaaaay too long) name would be: setPointInActualHitFrameContentsCoordinates
Comment 27 Allan Sandfeld Jensen 2012-11-09 06:50:52 PST
(In reply to comment #26)
> That is the same about the changelog question above: should not this part by itself be a fix and even be testable? Or this whole refactor depends on this premise?
> 
It could be a fix in itself, which was my original plan, but it was foced in here to make the naming simpler. If this isn't done first then we can not guarantee that mainFramePoint is infact the point in main-frame coordinates, and it will instead just be the point in the original frame the hit-test was started in. So essentially scope-creep due to bikesheding.

> > Source/WebCore/page/EventHandler.cpp:1148
> > -        frame->contentRenderer()->hitTest(HitTestRequest(hitType), widgetHitTestResult);
> > +        widgetHitTestResult.setPointInMainFrame(result.pointInMainFrame());
> > +        frame->contentRenderer()->hitTest(request, widgetHitTestResult);
> 
> I have to admit it gets a bit confusing here.
> 
> > Source/WebCore/rendering/HitTestResult.cpp:286
> > +Frame* HitTestResult::innerFrame() const
> 
> actualHitFrame?
> 
> ::innerFrame() returning a main frame might sound confusing as well.
> 
It is meant to match innerNode terminology. If the innernode is a node in the main frame the inner frame is the main frame. 

> > Source/WebCore/rendering/HitTestResult.h:126
> > +    // The hit-tested point in the coordinates of the main frame.
> 
> contents coordinates or viewport coordinates?
> 
Content, the viewport of the main frame is the window I think.

> > Source/WebCore/rendering/HitTestResult.h:138
> > +    void setLocalPoint(const LayoutPoint& p) { m_localPoint = p; }
> 
> better (but waaaaay too long) name would be: setPointInActualHitFrameContentsCoordinates

The localPoint is actually the point in the coordinates of the innerNode, it was only moved to make the header cleaner and it is not a functional part of the patch. 

Anyway, we can not rename everything in WebCore or simplify inheritently complex technical terms, but can we agree that the new names  are clearer and less ambigous than those used before and move on?
Comment 28 Antonio Gomes 2012-11-09 10:17:35 PST
> 
> > > Source/WebCore/page/EventHandler.cpp:1148
> > > -        frame->contentRenderer()->hitTest(HitTestRequest(hitType), widgetHitTestResult);
> > > +        widgetHitTestResult.setPointInMainFrame(result.pointInMainFrame());
> > > +        frame->contentRenderer()->hitTest(request, widgetHitTestResult);
> > 
> > I have to admit it gets a bit confusing here.
> > 
> > > Source/WebCore/rendering/HitTestResult.cpp:286
> > > +Frame* HitTestResult::innerFrame() const
> > 
> > actualHitFrame?
> > 
> > ::innerFrame() returning a main frame might sound confusing as well.
> > 
> It is meant to match innerNode terminology. If the innernode is a node in the main frame the inner frame is the main frame. 

However, "iframe" is an abbreviation  "inner frame" on the Web, which could confuse others (it did with me). grep for  'innerframe' in webcore, for example. 

> Anyway, we can not rename everything in WebCore or simplify inheritently complex technical terms, but can we agree that the new names  are clearer and less ambigous than those used before and move on?

I agree that we have to agree on the names. I am not so sure I've agreed with the ones proposed here though.
Comment 29 Allan Sandfeld Jensen 2012-11-09 10:56:00 PST
(In reply to comment #28)
> > It is meant to match innerNode terminology. If the innernode is a node in the main frame the inner frame is the main frame. 
> 
> However, "iframe" is an abbreviation  "inner frame" on the Web, which could confuse others (it did with me). grep for  'innerframe' in webcore, for example. 
> 
That makes sense. ActualHitFrame to me is confusing because all the frames are hit, this is just supposed to be the inner most of the frames hit. How about resultFrame or innerNodeFrame?

> > Anyway, we can not rename everything in WebCore or simplify inheritently complex technical terms, but can we agree that the new names  are clearer and less ambigous than those used before and move on?
> 
> I agree that we have to agree on the names. I am not so sure I've agreed with the ones proposed here though.

That is fair. I was just feeling we were getting into a situation where I was changing the names to suggestions from one reviewer, only to have another reviewer then come with new suggestions (or old ones), and none of you commiting to an actual r+. If none of the names are perfect, it just seems like we would be better off just going with one and move on.
Comment 30 Antonio Gomes 2012-11-09 11:01:17 PST
(In reply to comment #29)
> (In reply to comment #28)
> > > It is meant to match innerNode terminology. If the innernode is a node in the main frame the inner frame is the main frame. 
> > 
> > However, "iframe" is an abbreviation  "inner frame" on the Web, which could confuse others (it did with me). grep for  'innerframe' in webcore, for example. 
> > 
> That makes sense. ActualHitFrame to me is confusing because all the frames are hit, this is just supposed to be the inner most of the frames hit. How about resultFrame or innerNodeFrame?

I like the later.

> That is fair. I was just feeling we were getting into a situation where I was changing the names to suggestions from one reviewer, only to have another reviewer then come with new suggestions (or old ones), and none of you commiting to an actual r+. If none of the names are perfect, it just seems like we would be better off just going with one and move on.

I see how it can be frustrating. The above was my biggest concern and you came with a good enough name, imo.
Comment 31 Allan Sandfeld Jensen 2012-11-12 03:44:40 PST
Created attachment 173611 [details]
Patch

Renamed innerFrame to innerNodeFrame, and separated the change in hitTestResultAtPoint to a separate bug
Comment 32 Early Warning System Bot 2012-11-12 03:53:07 PST
Comment on attachment 173611 [details]
Patch

Attachment 173611 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14794950
Comment 33 Early Warning System Bot 2012-11-12 03:54:35 PST
Comment on attachment 173611 [details]
Patch

Attachment 173611 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14809358
Comment 34 Build Bot 2012-11-12 04:07:42 PST
Comment on attachment 173611 [details]
Patch

Attachment 173611 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14818106
Comment 35 EFL EWS Bot 2012-11-12 04:26:19 PST
Comment on attachment 173611 [details]
Patch

Attachment 173611 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14806516
Comment 36 Allan Sandfeld Jensen 2012-11-12 04:36:06 PST
Comment on attachment 173611 [details]
Patch

The patch depends on the patch for bug #101915.
Comment 37 Build Bot 2012-11-12 05:01:38 PST
Comment on attachment 173611 [details]
Patch

Attachment 173611 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14811397
Comment 38 WebKit Review Bot 2012-11-12 05:43:05 PST
Comment on attachment 173611 [details]
Patch

Attachment 173611 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14820102
Comment 39 Peter Beverloo (cr-android ews) 2012-11-12 06:45:18 PST
Comment on attachment 173611 [details]
Patch

Attachment 173611 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14820117
Comment 40 Julien Chaffraix 2012-11-13 11:12:12 PST
Comment on attachment 173611 [details]
Patch

I agree with Antonio, the naming is good enough now. I cannot really comment on the inner details of the change (like is the extra fallback in innerNodeFrame() fine for all platforms?) but it shouldn't be blocked on naming now.
Comment 41 Allan Sandfeld Jensen 2012-11-19 07:15:57 PST
Created attachment 174976 [details]
Patch

Reupload after blocking patch landed.
Comment 42 Julien Chaffraix 2012-11-20 16:28:19 PST
Comment on attachment 174976 [details]
Patch

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

Your change is subtle and could really cause badness on other ports (as we have already witnessed). I don't know the testing you went through on the different ports whose API you have changed (Qt, Gtk and EFL) so it makes me itchy to accept the patch without some maintainers acknowledging that it won't break their clients.

> Source/WebCore/ChangeLog:9
> +        To do this HitTestResult no longer inherits publically from HitTestLocation, which should also

Nice use of 'publically' :)

> Source/WebCore/ChangeLog:39
> +        (WebCore::RenderLayer::hitTest):

Please file those entries. They help see what's going on.

> Source/WebCore/page/EventHandler.cpp:790
> -    return result.innerNode() && page->dragController()->draggableNode(m_frame, result.innerNode(), roundedIntPoint(result.point()), state);
> +    return result.innerNode() && page->dragController()->draggableNode(m_frame, result.innerNode(), result.roundedPointInMainFrame(), state);

Shouldn't this be result.roundedPointInInnerFrame()?

> Source/WebCore/rendering/HitTestResult.cpp:204
>  

You have forgotten a constructor above for m_pointInMainFrame initialization (HitTestResult(const LayoutPoint& point)).

> Source/WebCore/rendering/HitTestResult.cpp:207
> +    , m_pointInMainFrame(point())

This construction made me wonder if it was right (and it is), not sure if explicitly initializing with centerPoint or whatever m_point gets would be more readable.

> Source/WebKit/gtk/webkit/webkithittestresult.cpp:318
> -    targetFrame = result.targetFrame();
> -    if (targetFrame && targetFrame->view()) {
> +    innerNodeFrame = result.innerNodeFrame();

Anything unrelated with the core renaming / disambiguation should be pushed to a follow-bug (my preference as it limits the potential for badness) or explained. In this case, I don't know why it's fine to switch from targetFrame to innerNodeFrame and this being exposed, you are changing a (probably broken) behavior.

> Source/WebKit/gtk/webkit/webkithittestresult.cpp:323
> -        point = result.roundedPoint();
> +        point = result.roundedPointInMainFrame();

This seems like a change in behavior as roundedPoint() was in the innerNodeFrame's coordinates.
Comment 43 Allan Sandfeld Jensen 2012-11-21 01:11:24 PST
(In reply to comment #42)
> (From update of attachment 174976 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174976&action=review
> 
> Your change is subtle and could really cause badness on other ports (as we have already witnessed). I don't know the testing you went through on the different ports whose API you have changed (Qt, Gtk and EFL) so it makes me itchy to accept the patch without some maintainers acknowledging that it won't break their clients.
> 
We could stick to the direct translation in this patch and just translate all the old point calls to innerNodePoint. I only changed a few places were I could tell from the context or documentation that the wrong point was used. Maybe it would be better to land those changes in separate patches though.

> > Source/WebKit/gtk/webkit/webkithittestresult.cpp:318
> > -    targetFrame = result.targetFrame();
> > -    if (targetFrame && targetFrame->view()) {
> > +    innerNodeFrame = result.innerNodeFrame();
> 
> Anything unrelated with the core renaming / disambiguation should be pushed to a follow-bug (my preference as it limits the potential for badness) or explained. In this case, I don't know why it's fine to switch from targetFrame to innerNodeFrame and this being exposed, you are changing a (probably broken) behavior.
> 
Yeah, this is the big one. It was a complete mess. TargetFrame is the same as innerNodeFrame except when the mouse is over a link that would open in a new window, but they are using it as it was always innerNodeFrame.
Comment 44 Allan Sandfeld Jensen 2012-11-21 01:39:28 PST
Created attachment 175377 [details]
Patch

Remove all functional changes in the ports, and make this patch do purely renames. Updated one missed constructor in HitTestResult.cpp
Comment 45 Julien Chaffraix 2012-11-21 07:20:52 PST
Comment on attachment 175377 [details]
Patch

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

r=me.

> Source/WebCore/ChangeLog:14
> +        Some, but not all call-sites have been all cleaned up in this patch, and they should be
> +        audited by the individual ports.

You don't do any clean-up now.

> Source/WebCore/ChangeLog:39
> +        (WebCore::RenderLayer::hitTest):

Please don't forget to fill those before landing.

> Source/WebCore/rendering/HitTestResult.cpp:200
>  HitTestResult::HitTestResult(const LayoutPoint& point) : HitTestLocation(point)

Let's fix the style of this line while we touch it (the parent class constructor call should be on its own line).

> Source/WebKit/qt/Api/qwebframe.cpp:1601
> +    // FIXME: This should probably use roundedPointInMainFrame if it is to match the documentation of QWebHitTestResult.

Yeah, FIXMEs and follow-up bugs are the way to go. This way you get more focused reviews and don't get stuck on different port's policy or reviewers.
Comment 46 Allan Sandfeld Jensen 2012-11-21 07:47:30 PST
Committed r135405: <http://trac.webkit.org/changeset/135405>