Bug 46580 – [Mac][DRT] Implement LayoutTestController::nodesFromRect

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Antonio Gomes]

document.nodesFromRect is not standard and should not be exposed to JS/Web content. Bug 46492 removed it from Document.idl and had to skip fast/dom/nodesFromRect-basic.html.

Each DRT has to implement LayoutTestController::nodesFromRect in order to test this.

[Antonio Gomes]

Patch coming ...

[Antonio Gomes]

Created attachment 68861 [details]
patch v1

Patch implements LayoutTestController::nodesFromRect for Mac, and add stubs for Gtk, Win and Wx DRTs.

A few symbols had to be added to WebCore.exp.in, and JSNodeList.h and JSDocument had to be set as Private headers.

Patch also changes fast/dom/nodesFromRect-basic.html accordingly, and unskips it.

[WebKit Review Bot]

Attachment 68861 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/mac/WebView/WebViewPrivate.h:633:  Extra space before ( in function call  [whitespace/parens] [4]
WebKitTools/DumpRenderTree/LayoutTestController.h:63:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebKitTools/DumpRenderTree/LayoutTestController.cpp:521:  Use 0 instead of NULL.  [readability/null] [5]
WebKitTools/DumpRenderTree/LayoutTestController.cpp:522:  Use 0 instead of NULL.  [readability/null] [5]
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]
Total errors found: 6 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antonio Gomes]

Fixed locally.

> WebKit/mac/WebView/WebViewPrivate.h:633:  Extra space before ( in function call  [whitespace/parens] [4]
> WebKitTools/DumpRenderTree/LayoutTestController.h:63:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
> WebKitTools/DumpRenderTree/LayoutTestController.cpp:521:  Use 0 instead of NULL.  [readability/null] [5]
> WebKitTools/DumpRenderTree/LayoutTestController.cpp:522:  Use 0 instead of NULL.  [readability/null] [5]
> 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]
> Total errors found: 6 in 15 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

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

[Antonio Gomes]

Created attachment 68865 [details]
patch v2 (committed r68356, r=kenneth)

Same as patch v1 (attachment 68861 [details]), but not it builds and works.

See comment #2 for summary.

[WebKit Review Bot]

Attachment 68865 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/mac/WebView/WebViewPrivate.h:633:  Extra space before ( in function call  [whitespace/parens] [4]
WebKitTools/DumpRenderTree/LayoutTestController.cpp:521:  Use 0 instead of NULL.  [readability/null] [5]
WebKitTools/DumpRenderTree/LayoutTestController.cpp:522:  Use 0 instead of NULL.  [readability/null] [5]
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]
Total errors found: 5 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 68865 [details]
patch v2 (committed r68356, r=kenneth)

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

> WebKitTools/ChangeLog:11
> +        adding stubs for Gtk and Windows.

I believe it is called GTK+

> WebKitTools/DumpRenderTree/LayoutTestController.cpp:521
> +    int x = JSValueToNumber(context, arguments[1], NULL);

Thought this is a C API call, we are calling it from C++, so I think we should use 0 instead of NULL.

> WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:142
> +    // FIXME: Implement this.

Better to add a notImplemented() call, I think.

[Antonio Gomes]

> > WebKitTools/ChangeLog:11
> > +        adding stubs for Gtk and Windows.
> 
> I believe it is called GTK+

Ok, can fix before landing.
 
> > WebKitTools/DumpRenderTree/LayoutTestController.cpp:521
> > +    int x = JSValueToNumber(context, arguments[1], NULL);
> Thought this is a C API call, we are calling it from C++, so I think we should use 0 instead of NULL.

I saw that other callers of JSValueToXXX are using NULL, so I kept that for consistency, but I might change w/out a problem.
 
> > WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:142
> > +    // FIXME: Implement this.
> Better to add a notImplemented() call, I think.

Again, I could had added a call for it, but no other methods are doing so, so I did not add for consistency.

All can be fixed before landed.

I want to land this before getting back to bug 46336, so we do not keep a feature/method untested.

[Kenneth Rohde Christiansen]

> I saw that other callers of JSValueToXXX are using NULL, so I kept that for consistency, but I might change w/out a problem.

Ok, then NULL (for C functions) it is for consistency.
> 
> > > WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:142
> > > +    // FIXME: Implement this.
> > Better to add a notImplemented() call, I think.
> 
> Again, I could had added a call for it, but no other methods are doing so, so I did not add for consistency.

OK then!

[Kenneth Rohde Christiansen]

Comment on attachment 68865 [details]
patch v2 (committed r68356, r=kenneth)

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

I'm not a master on the mac javascript stuff, but it seems quite sensible.

> WebKitTools/DumpRenderTree/LayoutTestController.cpp:527
> +    // Has mac implementation

Misses dot at the end. But what do you mean with this?

[Antonio Gomes]

Comment on attachment 68865 [details]
patch v2 (committed r68356, r=kenneth)

Clearing flags on attachment: 68865.

Committed r68356: <http://trac.webkit.org/changeset/68356>

[Antonio Gomes]

(In reply to comment #11)
> (From update of attachment 68865 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68865&action=review
> 
> I'm not a master on the mac javascript stuff, but it seems quite sensible.
> 
> > WebKitTools/DumpRenderTree/LayoutTestController.cpp:527
> > +    // Has mac implementation
> 
> Misses dot at the end. But what do you mean with this?

Again consistency. This file it full of it. 

But "." added :)

[Ademar Reis]

Revision r68356 cherry-picked into qtwebkit-2.1 with commit c49afb4 <http://gitorious.org/webkit/qtwebkit/commit/c49afb4>