Bug 71535 – Add the ability to transfer ArrayBuffer and "neuter" it

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dmitry Lomov]

Implement ArrayBuffer transfer and ArrayBuffer and related views neutering.

[Dmitry Lomov]

Created attachment 113614 [details]
Fix

[WebKit Review Bot]

Attachment 113614 [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/html/canvas/ArrayBuffer.h:85:  The parameter name "contents" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dmitry Lomov]

Created attachment 113615 [details]
Style

[Early Warning System Bot]

Comment on attachment 113615 [details]
Style

Attachment 113615 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10313040

[Gyuyoung Kim]

Comment on attachment 113615 [details]
Style

Attachment 113615 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10319028

[WebKit Review Bot]

Comment on attachment 113615 [details]
Style

Attachment 113615 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10314038

[Dmitry Lomov]

Created attachment 113619 [details]
Build fixed

[David Levin]

Comment on attachment 113614 [details]
Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=113614&action=review

> Source/WebCore/ChangeLog:27
> +        (WebCore::ArrayBufferView::neuter):

Why not clear or empty (instead of neuter)? (Of course disgorge would be a fun descriptive word but a more common term is probably better.)

> Source/WebCore/html/canvas/ArrayBuffer.cpp:139
> +    m_firstView = 0;

or

while (m_firstView) {
  ArrayBufferView* current = m_firstView;
  removeView(current);
  current->neuter(context);
}

> Source/WebCore/html/canvas/ArrayBuffer.cpp:173
> +      m_firstView->m_prevView = view;

4 space indent

> Source/WebCore/html/canvas/ArrayBuffer.cpp:182
> +    if (view->m_prevView)

There would be a minor a simplification in this area of the code if m_prevView was m_prevNextView (of type ArrayBufferView**).

Then 
if (view->m_prevView)
    view->m_prevView->m_nextView = view->m_nextView;
if (m_firstView == view)
     m_firstView = view->m_nextView;

becomes
    *(view->m_prevNextView) = view->m_nextView;

but perhaps that isn't worth the mental gymnastics to understand it.

> Source/WebCore/html/canvas/ArrayBuffer.h:40
> +  public:

no indent.

> Source/WebCore/html/canvas/ArrayBuffer.h:42
> +      : m_data(0), m_sizeInBytes(0)

4 space indent.

> Source/WebCore/html/canvas/ArrayBuffer.h:43
> +    { }

Either line this all up on one line (41-43) or put these (the two initializer and the braces) all on separate lines.

> Source/WebCore/html/canvas/ArrayBuffer.h:53
> +    void release() { m_data = 0; m_sizeInBytes = 0; }

I don't like the name release here. I think leak is more typical.

Perhaps replace this with transfer(ArrayBufferContents&) or swap(ArrayBufferContents&) and use that in the two instances where you are using this. 
In fact I rather like "swap" because that will make it obvious that no leaks are happening in transfer.

> Source/WebCore/html/canvas/ArrayBuffer.h:57
> +    { }

Same as above.

> Source/WebCore/html/canvas/ArrayBufferView.cpp:78
> +

extra blank lines.

> Source/WebCore/html/canvas/TypedArrayBase.h:120
> +    }

Add blank line.

[WebKit Review Bot]

Comment on attachment 113619 [details]
Build fixed

Attachment 113619 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10314130

New failing tests:
http/tests/canvas/webgl/origin-clean-conformance.html
fast/canvas/webgl/array-get-and-set-method-removal.html
fast/canvas/webgl/gl-enum-tests.html
fast/canvas/webgl/draw-elements-out-of-bounds.html
fast/canvas/webgl/copy-tex-image-and-sub-image-2d.html
fast/canvas/webgl/array-get-out-of-bounds.html
fast/canvas/webgl/drawingbuffer-test.html
fast/canvas/webgl/context-lost-restored.html
fast/canvas/webgl/array-set-out-of-bounds.html
fast/canvas/webgl/framebuffer-object-attachment.html
fast/canvas/webgl/gl-bind-attrib-location-test.html
fast/canvas/webgl/array-setters.html
fast/canvas/webgl/gl-teximage.html
fast/canvas/webgl/array-message-passing.html
fast/canvas/webgl/data-view-test.html
fast/canvas/webgl/context-lost.html
fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html
fast/canvas/webgl/gl-vertex-attrib-zero-issues.html
fast/canvas/webgl/array-unit-tests.html
fast/canvas/webgl/gl-get-calls.html

[Dmitry Lomov]

Comment on attachment 113614 [details]
Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=113614&action=review

>> Source/WebCore/ChangeLog:27
>> +        (WebCore::ArrayBufferView::neuter):
> 
> Why not clear or empty (instead of neuter)? (Of course disgorge would be a fun descriptive word but a more common term is probably better.)

'neuter' is a technical term: http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#transferable-objects

>> Source/WebCore/html/canvas/ArrayBuffer.cpp:139
>> +    m_firstView = 0;
> 
> or
> 
> while (m_firstView) {
>   ArrayBufferView* current = m_firstView;
>   removeView(current);
>   current->neuter(context);
> }

Thanks - it is better! I'll do that.

>> Source/WebCore/html/canvas/ArrayBuffer.cpp:182
>> +    if (view->m_prevView)
> 
> There would be a minor a simplification in this area of the code if m_prevView was m_prevNextView (of type ArrayBufferView**).
> 
> Then 
> if (view->m_prevView)
>     view->m_prevView->m_nextView = view->m_nextView;
> if (m_firstView == view)
>      m_firstView = view->m_nextView;
> 
> becomes
>     *(view->m_prevNextView) = view->m_nextView;
> 
> but perhaps that isn't worth the mental gymnastics to understand it.

Yikes. No way. :)

>> Source/WebCore/html/canvas/ArrayBuffer.h:40
>> +  public:
> 
> no indent.

Done

>> Source/WebCore/html/canvas/ArrayBuffer.h:42
>> +      : m_data(0), m_sizeInBytes(0)
> 
> 4 space indent.

Done

>> Source/WebCore/html/canvas/ArrayBuffer.h:43
>> +    { }
> 
> Either line this all up on one line (41-43) or put these (the two initializer and the braces) all on separate lines.

Done

>> Source/WebCore/html/canvas/ArrayBuffer.h:53
>> +    void release() { m_data = 0; m_sizeInBytes = 0; }
> 
> I don't like the name release here. I think leak is more typical.
> 
> Perhaps replace this with transfer(ArrayBufferContents&) or swap(ArrayBufferContents&) and use that in the two instances where you are using this. 
> In fact I rather like "swap" because that will make it obvious that no leaks are happening in transfer.

swap is a great idea - will do!

>> Source/WebCore/html/canvas/ArrayBuffer.h:57
>> +    { }
> 
> Same as above.

Done

>> Source/WebCore/html/canvas/ArrayBufferView.cpp:78
>> +
> 
> extra blank lines.

Done

[Dmitry Lomov]

Created attachment 113689 [details]
CR feedback and tests fixed

[WebKit Review Bot]

Attachment 113689 [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/html/canvas/ArrayBuffer.h:61:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

Comment on attachment 113689 [details]
CR feedback and tests fixed

View in context: https://bugs.webkit.org/attachment.cgi?id=113689&action=review

> Source/WebCore/html/canvas/ArrayBuffer.cpp:132
> +      ArrayBufferView* current = m_firstView;

4 spaces

> Source/WebCore/html/canvas/ArrayBuffer.h:65
> +        m_sizeInBytes = 0; 

This isn't really a swap.

       void* otherData = other.m_data;
       unsigned otherSizeInBytes = other.m_sizeInBytes;
       other.m_data = m_data;
       other.m_sizeInBytes = m_sizeInBytes;
       m_data = otherData; 
       m_sizeInBytes = otherSizeInBytes;

[Darin Adler]

Comment on attachment 113689 [details]
CR feedback and tests fixed

View in context: https://bugs.webkit.org/attachment.cgi?id=113689&action=review

>> Source/WebCore/html/canvas/ArrayBuffer.h:65
>> +        m_sizeInBytes = 0; 
> 
> This isn't really a swap.
> 
>        void* otherData = other.m_data;
>        unsigned otherSizeInBytes = other.m_sizeInBytes;
>        other.m_data = m_data;
>        other.m_sizeInBytes = m_sizeInBytes;
>        m_data = otherData; 
>        m_sizeInBytes = otherSizeInBytes;

The best way to write a swap is to just swap the data members with the swap function:

    std::swap(m_data, other.m_data);
    std::swap(m_sizeInBytes, other.m_sizeInBytes);

You almost never need to write it any other way.

[Dmitry Lomov]

Created attachment 113693 [details]
swap->transfer + assertion

I don't need a swapping at all, and it is distracting from what the code tries to do.

[Dmitry Lomov]

Created attachment 113695 [details]
Small style fix

[WebKit Review Bot]

Comment on attachment 113695 [details]
Small style fix

Clearing flags on attachment: 113695

Committed r99324: <http://trac.webkit.org/changeset/99324>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.