Bug 46580 - [Mac][DRT] Implement LayoutTestController::nodesFromRect
Summary: [Mac][DRT] Implement LayoutTestController::nodesFromRect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on:
Blocks: 44089 46336
  Show dependency treegraph
 
Reported: 2010-09-26 12:16 PDT by Antonio Gomes
Modified: 2010-11-12 12:28 PST (History)
7 users (show)

See Also:


Attachments
patch v1 (20.59 KB, patch)
2010-09-26 15:04 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch v2 (committed r68356, r=kenneth) (21.14 KB, patch)
2010-09-26 16:23 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-26 12:16:47 PDT
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.
Comment 1 Antonio Gomes 2010-09-26 13:40:00 PDT
Patch coming ...
Comment 2 Antonio Gomes 2010-09-26 15:04:51 PDT
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.
Comment 3 WebKit Review Bot 2010-09-26 15:06:19 PDT
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.
Comment 4 Antonio Gomes 2010-09-26 15:08:49 PDT
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.
Comment 5 Eric Seidel 2010-09-26 15:14:40 PDT
Attachment 68861 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4130018
Comment 6 Antonio Gomes 2010-09-26 16:23:03 PDT
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.
Comment 7 WebKit Review Bot 2010-09-26 16:26:24 PDT
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.
Comment 8 Kenneth Rohde Christiansen 2010-09-26 16:32:32 PDT
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.
Comment 9 Antonio Gomes 2010-09-26 16:38:42 PDT
> > 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.
Comment 10 Kenneth Rohde Christiansen 2010-09-26 16:47:12 PDT
> 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!
Comment 11 Kenneth Rohde Christiansen 2010-09-26 16:50:23 PDT
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?
Comment 12 Antonio Gomes 2010-09-26 17:26:05 PDT
Comment on attachment 68865 [details]
patch v2 (committed r68356, r=kenneth)

Clearing flags on attachment: 68865.

Committed r68356: <http://trac.webkit.org/changeset/68356>
Comment 13 Antonio Gomes 2010-09-26 17:27:05 PDT
(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 :)
Comment 14 Ademar Reis 2010-11-12 12:28:15 PST
Revision r68356 cherry-picked into qtwebkit-2.1 with commit c49afb4 <http://gitorious.org/webkit/qtwebkit/commit/c49afb4>