Bug 98139

Created attachment 166658 [details] Patch This patch is designed to cause compile errors everywhere the disambiguation still needs to be resolved.

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 on attachment 166658 [details] Patch Attachment 166658 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14123317

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

Comment on attachment 166658 [details] Patch Attachment 166658 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14135169

Comment on attachment 166658 [details] Patch Attachment 166658 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14134165

Created attachment 166674 [details] Patch Fix remaining calls in EFL, Mac and Chromium.

Comment on attachment 166674 [details] Patch Attachment 166674 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14132205

Created attachment 166681 [details] Patch Fix remaining call in Mac WK2.

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

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

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

Created attachment 167075 [details] Patch Clarify method names, now using pointInResultFrame and pointInOriginalFrame.

Comment on attachment 167075 [details] Patch Attachment 167075 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14138928

Created attachment 167291 [details] Patch Fix one forgotten rename in GTK.

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.

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

Created attachment 169617 [details] Patch Ensure outer frame is main frame, and do appropiate renaming and clean-up

Comment on attachment 169617 [details] Patch Attachment 169617 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14473001

Created attachment 172809 [details] Patch Rebased and updated exported symbols for Mac build system

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

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

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

Created attachment 173020 [details] Patch Updated changelogs and API, but will probably need an update on exported symbols as well

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

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

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

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

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

Created attachment 173611 [details] Patch Renamed innerFrame to innerNodeFrame, and separated the change in hitTestResultAtPoint to a separate bug

Comment on attachment 173611 [details] Patch Attachment 173611 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14794950

Comment on attachment 173611 [details] Patch Attachment 173611 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14809358

Comment on attachment 173611 [details] Patch Attachment 173611 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14818106

Comment on attachment 173611 [details] Patch Attachment 173611 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14806516

Comment on attachment 173611 [details] Patch The patch depends on the patch for bug #101915.

Comment on attachment 173611 [details] Patch Attachment 173611 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14811397

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

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

Created attachment 174976 [details] Patch Reupload after blocking patch landed.

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.

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

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

Committed r135405: <http://trac.webkit.org/changeset/135405>