Bug 73054

Created attachment 116445 [details] The fix

Attachment 116445 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/v8/V8Utilities.cpp:36: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/bindings/v8/custom/V8WorkerCustom.cpp:38: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 116445 [details] The fix View in context: https://bugs.webkit.org/attachment.cgi?id=116445&action=review Just a few comments. I think this will likely be ok with these changes, but I need to take more time to look at it to be sure. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1992 > + MessagePortArray* messagePorts, ArrayBufferArray* arrayBuffers, arrayBuffers is unused. I guess that is next? Anyway omit the var name for now to avoid build errors on some platforms. > Source/WebCore/bindings/v8/SerializedScriptValue.h:39 > +class ArrayBuffer; I think in this one case the contents of the namespace is indented -- double check the style guide pls. (Yes stylebot will complain.) > Source/WebCore/bindings/v8/V8Utilities.cpp:84 > +bool extractTransferables(v8::Local<v8::Value> value, MessagePortArray& ports, ArrayBufferArray& arrayBuffers) make it static since it is local to a file. >> Source/WebCore/bindings/v8/custom/V8WorkerCustom.cpp:38 >> +#include "ArrayBuffer.h" > > Alphabetical sorting problem. [build/include_order] [4] This seems like an ok error to have.

(In reply to comment #3) > (From update of attachment 116445 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116445&action=review > > Just a few comments. I think this will likely be ok with these changes, but I need to take more time to look at it to be sure. > > > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1992 > > + MessagePortArray* messagePorts, ArrayBufferArray* arrayBuffers, > > arrayBuffers is unused. I guess that is next? Yes it is next > > Anyway omit the var name for now to avoid build errors on some platforms. Done > > > Source/WebCore/bindings/v8/SerializedScriptValue.h:39 > > +class ArrayBuffer; > > I think in this one case the contents of the namespace is indented -- double check the style guide pls. (Yes stylebot will complain.) Checked the style guide - no exceptions. > > > Source/WebCore/bindings/v8/V8Utilities.cpp:84 > > +bool extractTransferables(v8::Local<v8::Value> value, MessagePortArray& ports, ArrayBufferArray& arrayBuffers) > > make it static since it is local to a file. It is not local. > > >> Source/WebCore/bindings/v8/custom/V8WorkerCustom.cpp:38 > >> +#include "ArrayBuffer.h" > > > > Alphabetical sorting problem. [build/include_order] [4] > > This seems like an ok error to have.

Created attachment 116446 [details] CR feedback

Comment on attachment 116446 [details] CR feedback View in context: https://bugs.webkit.org/attachment.cgi?id=116446&action=review Last round. > Source/WebCore/bindings/v8/V8Utilities.cpp:41 > +#include <v8.h> < after " (sorry I wasn't clear about this) > Source/WebCore/bindings/v8/V8Utilities.cpp:121 > + ports.append(V8MessagePort::toNative(v8::Handle<v8::Object>::Cast(transferrable))); Would be nice to move "v8::Handle<v8::Object>::Cast(transferrable)" out of the clauses. > Source/WebCore/bindings/v8/V8Utilities.h:90 > + bool getMessagePortArray(v8::Local<v8::Value>, MessagePortArray&); This function is only used for the current postMessage support, right? (Might be nice to have a small comment to that affect.) Who calls it anymore in fact? > Source/WebCore/bindings/v8/custom/V8MessagePortCustom.cpp:56 > + SerializedScriptValue::create(args[0], I wish that we didn't have to do this same thing at every callsite. Should doTransfer be a parameter? > Source/WebCore/bindings/v8/custom/V8WorkerCustom.cpp:63 > + doTransfer ? &ports : 0, doTransfer ? &arrayBuffers : 0, Might be nice to put the &arrayBuffer arg on a separate line as the current state makes it much harder to read quickly.

Comment on attachment 116446 [details] CR feedback Only nits/preferences are left, so r+. Please consider addressing some of them.

(In reply to comment #6) > (From update of attachment 116446 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116446&action=review > > Last round. > > > Source/WebCore/bindings/v8/V8Utilities.cpp:41 > > +#include <v8.h> > > < after " (sorry I wasn't clear about this) Done. > > > Source/WebCore/bindings/v8/V8Utilities.cpp:121 > > + ports.append(V8MessagePort::toNative(v8::Handle<v8::Object>::Cast(transferrable))); > > Would be nice to move "v8::Handle<v8::Object>::Cast(transferrable)" out of the clauses. Can't. The conditions in ifs determine validity of the cast. > > > Source/WebCore/bindings/v8/V8Utilities.h:90 > > + bool getMessagePortArray(v8::Local<v8::Value>, MessagePortArray&); > > This function is only used for the current postMessage support, right? (Might be nice to have a small comment to that affect.) > > Who calls it anymore in fact? It is used elsewhere (OptionsObject and some other places) > > > Source/WebCore/bindings/v8/custom/V8MessagePortCustom.cpp:56 > > + SerializedScriptValue::create(args[0], > > I wish that we didn't have to do this same thing at every callsite. Should doTransfer be a parameter? Well, I want to keep it flexible for now (given the way specs are atm - message port transfer is in "ready for implementation" state, while array buffer transfer is a "strawman" - I think we will be in the state where postMessage allow transferring the former but not the latter) > > > Source/WebCore/bindings/v8/custom/V8WorkerCustom.cpp:63 > > + doTransfer ? &ports : 0, doTransfer ? &arrayBuffers : 0, > > Might be nice to put the &arrayBuffer arg on a separate line as the current state makes it much harder to read quickly. Done

Created attachment 116464 [details] Final

Attachment 116464 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/v8/custom/V8WorkerCustom.cpp:38: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 116464 [details] Final Attachment 116464 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10642173 New failing tests: fast/events/constructors/message-event-constructor.html fast/dom/Window/window-postmessage-args.html

Created attachment 116476 [details] Rebaselined tests David: I took the liberty to cq+ rebaseline unreviewed, please yell if that is not ok with you.

Attachment 116476 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebCore/bindings/v8/custom/V8WorkerCustom.cpp:38: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 116476 [details] Rebaselined tests Clearing flags on attachment: 116476 Committed r101118: <http://trac.webkit.org/changeset/101118>

All reviewed patches have been landed. Closing bug.