Bug 73503 - [Chromium][V8] Implement ArrayBuffer transfer in chromium
Summary: [Chromium][V8] Implement ArrayBuffer transfer in chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dmitry Lomov
URL:
Keywords:
Depends on:
Blocks: 66578
  Show dependency treegraph
 
Reported: 2011-11-30 17:06 PST by Dmitry Lomov
Modified: 2011-12-01 09:58 PST (History)
6 users (show)

See Also:


Attachments
Implementation + tests (41.50 KB, patch)
2011-11-30 17:15 PST, Dmitry Lomov
levin: review-
levin: commit-queue-
Details | Formatted Diff | Diff
CR feedback (43.59 KB, patch)
2011-11-30 18:45 PST, Dmitry Lomov
no flags Details | Formatted Diff | Diff
Rebased (43.61 KB, patch)
2011-11-30 19:19 PST, Dmitry Lomov
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Lomov 2011-11-30 17:06:10 PST
Implement ArrayBuffer transfer for SerializedScriptValue in chromium port.
Comment 1 Dmitry Lomov 2011-11-30 17:15:51 PST
Created attachment 117303 [details]
Implementation + tests
Comment 2 David Levin 2011-11-30 17:27:23 PST
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 3 David Levin 2011-11-30 18:01:19 PST
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.
Comment 4 David Levin 2011-11-30 18:02:43 PST
(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 5 Dmitry Lomov 2011-11-30 18:43:29 PST
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!
Comment 6 Dmitry Lomov 2011-11-30 18:45:50 PST
Created attachment 117307 [details]
CR feedback
Comment 7 David Levin 2011-11-30 19:15:48 PST
(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...
Comment 8 Dmitry Lomov 2011-11-30 19:19:41 PST
Created attachment 117312 [details]
Rebased
Comment 9 David Levin 2011-11-30 22:19:50 PST
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 10 David Levin 2011-11-30 22:21:20 PST
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 :).
Comment 11 Dmitry Lomov 2011-12-01 09:58:16 PST
Landed in http://trac.webkit.org/changeset/101682