Bug 46580

Patch coming ...

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.

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.

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.

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

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.

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

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

> 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 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 on attachment 68865 [details] patch v2 (committed r68356, r=kenneth) Clearing flags on attachment: 68865. Committed r68356: <http://trac.webkit.org/changeset/68356>

(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 :)

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