Bug 46336

If you do this Antonio, I'm OK with you changing the API you introduced in QWebSettings, but due to our QtWebKit 2.1 release, I would prefer you to make that change in the Qt API asap.

I got a patch running on Qt. Building on Mac to check the layout tests. Going to upload soon. (In reply to comment #1) > If you do this Antonio, I'm OK with you changing the API you introduced in QWebSettings, but due to our QtWebKit 2.1 release, I would prefer you to make that change in the Qt API asap. OK

Created attachment 68644 [details] patch v1 Summary 1) Patch changes Document::nodesFromRect API from - RefPtf<NodeList> nodesFromRect(int x, int y, unsigned horizontalPadding, unsigned verticalPadding, bool ignoreClipping) to - RefPtf<NodeList> nodesFromRect(int x, int y, unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding, bool ignoreClipping) 2) Patch adjust WebCore related classes accordingly to support this, incluing HitTestResult, RenderLayer, etc. 3) Fixes the layout test accordingly to the proposed API change.

Comment on attachment 68644 [details] patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=68644&action=review > WebCore/ChangeLog:9 > + Make Document::nodesFromRect more flexible > + https://bugs.webkit.org/show_bug.cgi?id=46336 > + > + Make Document::nodesFromRect more flexible > + https://bugs.webkit.org/show_bug.cgi?id=46336 Why twice? :-) > WebCore/ChangeLog:26 > + In practive, the horizontal padding was being used to expand a given center point in both practice* > WebCore/ChangeLog:54 > + (WebCore::EventHandler::hitTestResultAtPoint): For simplicity, I did not change the signature > + but the way the padding value passed in as parameter > + is being used to construct a HitTestResult. > + * rendering/HitTestResult.cpp: Changed the rect-based bits from using IntSize (padding) to separated paddings > + for each direction. Why not write the comment on the line below... this looks really strange! > WebCore/dom/Document.cpp:1072 > + else if (!frameView->visibleContentRect().intersects(HitTestResult::rectFromPoint(IntPoint(centerX, centerY), topPadding, rightPadding, bottomPadding, leftPadding))) Is this the order we normally use? top, right, bottom, left? Just checking > WebCore/rendering/HitTestResult.cpp:555 > +IntRect HitTestResult::rectFromPoint(const IntPoint& point, unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding) ForPoint? > WebCore/rendering/HitTestResult.cpp:560 > + IntSize realPadding(leftPadding + rightPadding, topPadding + bottomPadding); Is there a fake padding :) ? Maybe actualPadding? or something more descriptive. > WebCore/rendering/HitTestResult.cpp:561 > + realPadding += IntSize(1, 1); Maybe add a comment why you are doing this > WebCore/rendering/HitTestResult.h:107 > + int topPadding() const { return m_topPadding; } > + int rightPadding() const { return m_rightPadding; } > + int bottomPadding() const { return m_bottomPadding; } > + int leftPadding() const { return m_leftPadding; } Is it useful to know the padding in the result? This is the result of a hit test

Created attachment 68683 [details] patch v2 - addressed kenneth's comments. > > WebCore/ChangeLog:26 > > + In practive, the horizontal padding was being used to expand a given center point in both > > practice* Fixed. > > WebCore/ChangeLog:54 > > + (WebCore::EventHandler::hitTestResultAtPoint): For simplicity, I did not change the signature > > + but the way the padding value passed in as parameter > > + is being used to construct a HitTestResult. > > + * rendering/HitTestResult.cpp: Changed the rect-based bits from using IntSize (padding) to separated paddings > > + for each direction. > > Why not write the comment on the line below... this looks really strange! Fixed. > > WebCore/dom/Document.cpp:1072 > > + else if (!frameView->visibleContentRect().intersects(HitTestResult::rectFromPoint(IntPoint(centerX, centerY), topPadding, rightPadding, bottomPadding, leftPadding))) > > Is this the order we normally use? top, right, bottom, left? Just checking To match mozilla's signature for the their similar method. > > WebCore/rendering/HitTestResult.cpp:555 > > +IntRect HitTestResult::rectFromPoint(const IntPoint& point, unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding) > > ForPoint? ForPoint sounds good. Can I keep this as is for now, and fix in a follow up? Reason: there are other two method overloaded in this class that would have to change, and many call sites of these methods that would need change. It would make the patch too bloat. > > > WebCore/rendering/HitTestResult.cpp:560 > > + IntSize realPadding(leftPadding + rightPadding, topPadding + bottomPadding); > > Is there a fake padding :) ? Maybe actualPadding? or something more descriptive. 'Actual' sounds good. Changed. > > WebCore/rendering/HitTestResult.cpp:561 > > + realPadding += IntSize(1, 1); > > Maybe add a comment why you are doing this Comment added. > > WebCore/rendering/HitTestResult.h:107 > > + int topPadding() const { return m_topPadding; } > > + int rightPadding() const { return m_rightPadding; } > > + int bottomPadding() const { return m_bottomPadding; } > > + int leftPadding() const { return m_leftPadding; } > > Is it useful to know the padding in the result? This is the result of a hit test See we using them in RenderLineBoxList.cpp

Comment on attachment 68683 [details] patch v2 - addressed kenneth's comments. Clearing review flag. Let me fix bug 46492 and a few others first.

Comment on attachment 68683 [details] patch v2 - addressed kenneth's comments. View in context: https://bugs.webkit.org/attachment.cgi?id=68683&action=review > WebCore/dom/Document.idl:190 > NodeList nodesFromRect(in long x, in long y, This should really have the webkit prefix, since it is not standardized

(In reply to comment #7) > (From update of attachment 68683 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68683&action=review > > > WebCore/dom/Document.idl:190 > > NodeList nodesFromRect(in long x, in long y, > > This should really have the webkit prefix, since it is not standardized Simon, we are going to remove it from the Document.idl instead, and not expose to web content, at least for now. This will also require changes on how it is being tested by DRT. Probably I am going to add a new method to LayoutTestController.

(In reply to comment #6) > (From update of attachment 68683 [details]) > Clearing review flag. > > Let me fix bug 46492 and a few others first. ... and also bug 46580.

Created attachment 68867 [details] patch v3 Basically same patch as v2 (attachment 68683 [details]), but rebased against ToT and including the adjustment needed by the new LayoutTestController::nodesFromRect (added by bug 46580) to test this feature.

Attachment 68867 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKitTools/DumpRenderTree/LayoutTestController.cpp:523: Use 0 instead of NULL. [readability/null] [5] WebKitTools/DumpRenderTree/LayoutTestController.cpp:524: Use 0 instead of NULL. [readability/null] [5] WebKitTools/DumpRenderTree/LayoutTestController.cpp:525: Use 0 instead of NULL. [readability/null] [5] WebKitTools/DumpRenderTree/LayoutTestController.cpp:526: Use 0 instead of NULL. [readability/null] [5] Total errors found: 4 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 68867 [details] did not build on mac: Build output: http://queues.webkit.org/results/4002156

Attachment 68867 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3997154

Created attachment 68872 [details] patch v3.1 - same patch as v3, but builds on Mac Fixed Mac build: updated WebCore.exp.in symbols. Nothing changed from v2.

Created attachment 68873 [details] (committed r68475, r=kenneth) patch v3.2 - actual fixed patch. Build Mac, Gtk and others. Uploaded the real patch, this time. The style error is FALSE ALARM.

Attachment 68873 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKitTools/DumpRenderTree/LayoutTestController.cpp:523: Use 0 instead of NULL. [readability/null] [5] WebKitTools/DumpRenderTree/LayoutTestController.cpp:524: Use 0 instead of NULL. [readability/null] [5] WebKitTools/DumpRenderTree/LayoutTestController.cpp:525: Use 0 instead of NULL. [readability/null] [5] WebKitTools/DumpRenderTree/LayoutTestController.cpp:526: Use 0 instead of NULL. [readability/null] [5] Total errors found: 4 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 68867 [details] did not build on win: Build output: http://queues.webkit.org/results/4060156

Attachment 68872 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4055157

(In reply to comment #17) > Attachment 68867 [details] did not build on win: > Build output: http://queues.webkit.org/results/4060156 (In reply to comment #18) > Attachment 68872 [details] did not build on gtk: > Build output: http://queues.webkit.org/results/4055157 Warnings about obsolete versions. Latest patch (attachment 68873 [details]) built on all platforms.

Comment on attachment 68873 [details] (committed r68475, r=kenneth) patch v3.2 - actual fixed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=68873&action=review LGTM > WebCore/dom/Document.cpp:1071 > + else if (!frameView->visibleContentRect().intersects(HitTestResult::rectFromPoint(point, topPadding, rightPadding, bottomPadding, leftPadding))) rectForPoint? > WebCore/dom/Document.cpp:1078 > + if (!topPadding && !rightPadding && !bottomPadding && !leftPadding) { are all these unsigned? > WebCore/dom/Document.h:326 > + PassRefPtr<NodeList> nodesFromRect(int centerX, > + int centerY, > + unsigned topPadding, > + unsigned rightPadding, > + unsigned bottomPadding, > + unsigned leftPadding, > + bool ignoreClipping) const; we usually put all these on one line, I even think the style guide says somethiong about that

Comment on attachment 68873 [details] (committed r68475, r=kenneth) patch v3.2 - actual fixed patch. Clearing review flags on attachment: 68873. Committed r68475: <http://trac.webkit.org/changeset/68475>

> > WebCore/dom/Document.cpp:1071 > > + else if (!frameView->visibleContentRect().intersects(HitTestResult::rectFromPoint(point, topPadding, rightPadding, bottomPadding, leftPadding))) > > rectForPoint? Sure. I can fix in a follow up, just to avoid inflating this patch with renaming changes. > > > WebCore/dom/Document.h:326 > > + PassRefPtr<NodeList> nodesFromRect(int centerX, > > + int centerY, > > + unsigned topPadding, > > + unsigned rightPadding, > > + unsigned bottomPadding, > > + unsigned leftPadding, > > + bool ignoreClipping) const; > > we usually put all these on one line, I even think the style guide says somethiong about that Fixed before landing.

(In reply to comment #22) > > > WebCore/dom/Document.cpp:1071 > > > + else if (!frameView->visibleContentRect().intersects(HitTestResult::rectFromPoint(point, topPadding, rightPadding, bottomPadding, leftPadding))) > > > > rectForPoint? Filed bug 47261 about this renaming.

Better tests using the new API capabilities being worked on bug 47766.

Revision r68475 cherry-picked into qtwebkit-2.1 with commit 848c9ed <http://gitorious.org/webkit/qtwebkit/commit/848c9ed>