Bug 73503

Created attachment 117303 [details] Implementation + tests

Comment on attachment 117303 [details] Implementation + tests View in context: https://bugs.webkit.org/attachment.cgi?id=117303&action=review Just some high level cursory comments as I start to find my way in the code. > Source/JavaScriptCore/wtf/ArrayBuffer.h:-67 > - m_sizeInBytes = 0; In general resist the temptation to "clean up" trailing whitespace. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1149 > else { Don't have an else if the condition ends with a return. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1174 > + else if (V8ArrayBuffer::HasInstance(value)) { No { here due to single line statement. And don't have an else if the condition ends with a return. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2145 > + OwnPtr<ArrayBufferContentsArray> contents = adoptPtr(new ArrayBufferContentsArray(arrayBuffers.size())); We really should have a static create method on Vector. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2153 > + return PassOwnPtr<ArrayBufferContentsArray>(); Doesn't return 0 work? Or perhaps nullptr?

Comment on attachment 117303 [details] Implementation + tests View in context: https://bugs.webkit.org/attachment.cgi?id=117303&action=review Only one more comment. Maybe we should do one more round. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2154 > + } Weird issue here with half being neutered if an array buffer list more than once.

(In reply to comment #3) > (From update of attachment 117303 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117303&action=review > > Only one more comment. > > Maybe we should do one more round. > > > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2154 > > + } > > Weird issue here with half being neutered if an array buffer list more than once. I should have said this, but my review is basically complete. (The test is involved and I likely need to spend more time and brain power there but I'm done with the code.)

Comment on attachment 117303 [details] Implementation + tests View in context: https://bugs.webkit.org/attachment.cgi?id=117303&action=review >> Source/JavaScriptCore/wtf/ArrayBuffer.h:-67 >> - m_sizeInBytes = 0; > > In general resist the temptation to "clean up" trailing whitespace. Is it ok to keep this in the patch since I already did it? >> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1149 >> else { > > Don't have an else if the condition ends with a return. There are other conditions in these series of if .. else .. that do not end with a return (see e.g. 'if(value->IsString())' branch above). I bet this can be beautified, but not in this patch.. >> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1174 >> + else if (V8ArrayBuffer::HasInstance(value)) { > > No { here due to single line statement. > > And don't have an else if the condition ends with a return. First part - Done. Re return: see prev. comment. >> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2145 >> + OwnPtr<ArrayBufferContentsArray> contents = adoptPtr(new ArrayBufferContentsArray(arrayBuffers.size())); > > We really should have a static create method on Vector. Not in this patch - what do you think? Although with all the typedefs it will be really hard to find all occurrences of this... >> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2153 >> + return PassOwnPtr<ArrayBufferContentsArray>(); > > Doesn't return 0 work? Or perhaps nullptr? nullptr works - thanks! >>> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2154 >>> + } >> >> Weird issue here with half being neutered if an array buffer list more than once. > > I should have said this, but my review is basically complete. (The test is involved and I likely need to spend more time and brain power there but I'm done with the code.) Great catch - thank you very much!

Created attachment 117307 [details] CR feedback

(In reply to comment #5) > (From update of attachment 117303 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117303&action=review > >> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1149 > >> else { > > > > Don't have an else if the condition ends with a return. > > There are other conditions in these series of if .. else .. that do not end with a return (see e.g. 'if(value->IsString())' branch above). I bet this can be beautified, but not in this patch.. Ok, I get it. I was suggesting introducing a bug... whoops. > >> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2145 > >> + OwnPtr<ArrayBufferContentsArray> contents = adoptPtr(new ArrayBufferContentsArray(arrayBuffers.size())); > > > > We really should have a static create method on Vector. > > Not in this patch - what do you think? Agreed. I should have been more clear. Looking over the tests...

Created attachment 117312 [details] Rebased

Comment on attachment 117307 [details] CR feedback View in context: https://bugs.webkit.org/attachment.cgi?id=117307&action=review I think we are missing an update to "fast/dom/Window/window-postmessage-args-expected.txt" > LayoutTests/fast/dom/Window/window-postmessage-args.html:66 > +if (!(arrayBuffer.byteLength ===0)) { Consider adding a space after ===

Comment on attachment 117312 [details] Rebased Please consider the 2 comments from above (especially the updated of expected) and feel free to submit -- I thought I submit it a while ago but my change colilded with yours :).

Landed in http://trac.webkit.org/changeset/101682