Bug 46336 – Make Document::nodesFromRect more flexible

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Antonio Gomes]

I propose we follow the flexibility of Mozilla's nodesFromRect as in http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#416


It would require also a feel changes in HitTestResult and in the existing LayoutTests for this API.

[Kenneth Rohde Christiansen]

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.

[Antonio Gomes]

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

[Antonio Gomes]

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.

[Kenneth Rohde Christiansen]

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

[Antonio Gomes]

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

[Antonio Gomes]

Comment on attachment 68683 [details]
patch v2 - addressed kenneth's comments.

Clearing review flag.

Let me fix bug 46492 and a few others first.

[Simon Fraser (smfr)]

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

[Antonio Gomes]

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

[Antonio Gomes]

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

[Antonio Gomes]

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.

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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

[WebKit Review Bot]

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

[Antonio Gomes]

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.

[Antonio Gomes]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Antonio Gomes]

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

[Kenneth Rohde Christiansen]

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

[Antonio Gomes]

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>

[Antonio Gomes]

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

[Antonio Gomes]

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

[Antonio Gomes]

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

[Ademar Reis]

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