Bug 46336 - Make Document::nodesFromRect more flexible
Summary: Make Document::nodesFromRect more flexible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on: 46492 46580 47766
Blocks: 44089
  Show dependency treegraph
 
Reported: 2010-09-22 21:12 PDT by Antonio Gomes
Modified: 2010-11-12 12:27 PST (History)
11 users (show)

See Also:


Attachments
patch v1 (35.58 KB, patch)
2010-09-23 21:53 PDT, Antonio Gomes
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
patch v2 - addressed kenneth's comments. (34.93 KB, patch)
2010-09-24 08:23 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch v3 (37.21 KB, patch)
2010-09-26 18:14 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch v3.1 - same patch as v3, but builds on Mac (40.93 KB, patch)
2010-09-26 20:31 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
(committed r68475, r=kenneth) patch v3.2 - actual fixed patch. (43.47 KB, patch)
2010-09-26 20:45 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-09-22 21:12:04 PDT
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.
Comment 1 Kenneth Rohde Christiansen 2010-09-23 19:15:16 PDT
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.
Comment 2 Antonio Gomes 2010-09-23 19:19:14 PDT
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
Comment 3 Antonio Gomes 2010-09-23 21:53:22 PDT
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 4 Kenneth Rohde Christiansen 2010-09-24 06:44:07 PDT
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
Comment 5 Antonio Gomes 2010-09-24 08:23:09 PDT
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 6 Antonio Gomes 2010-09-24 16:40:43 PDT
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 7 Simon Fraser (smfr) 2010-09-24 16:42:42 PDT
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
Comment 8 Antonio Gomes 2010-09-24 16:48:34 PDT
(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.
Comment 9 Antonio Gomes 2010-09-26 12:17:24 PDT
(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.
Comment 10 Antonio Gomes 2010-09-26 18:14:54 PDT
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.
Comment 11 WebKit Review Bot 2010-09-26 18:17:24 PDT
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.
Comment 12 Eric Seidel (no email) 2010-09-26 19:16:50 PDT
Attachment 68867 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4002156
Comment 13 WebKit Review Bot 2010-09-26 20:30:16 PDT
Attachment 68867 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3997154
Comment 14 Antonio Gomes 2010-09-26 20:31:22 PDT
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.
Comment 15 Antonio Gomes 2010-09-26 20:45:02 PDT
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.
Comment 16 WebKit Review Bot 2010-09-26 20:50:13 PDT
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.
Comment 17 WebKit Review Bot 2010-09-26 21:01:26 PDT
Attachment 68867 [details] did not build on win:
Build output: http://queues.webkit.org/results/4060156
Comment 18 WebKit Review Bot 2010-09-26 23:19:25 PDT
Attachment 68872 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4055157
Comment 19 Antonio Gomes 2010-09-27 06:33:25 PDT
(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 20 Kenneth Rohde Christiansen 2010-09-27 17:13:06 PDT
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 21 Antonio Gomes 2010-09-27 19:31:51 PDT
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>
Comment 22 Antonio Gomes 2010-09-27 19:33:05 PDT
> > 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.
Comment 23 Antonio Gomes 2010-10-06 07:24:30 PDT
(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.
Comment 24 Antonio Gomes 2010-10-17 20:38:11 PDT
Better tests using the new API capabilities being worked on bug 47766.
Comment 25 Ademar Reis 2010-11-12 12:27:39 PST
Revision r68475 cherry-picked into qtwebkit-2.1 with commit 848c9ed <http://gitorious.org/webkit/qtwebkit/commit/848c9ed>