Bug 71535 - Add the ability to transfer ArrayBuffer and "neuter" it
Summary: Add the ability to transfer ArrayBuffer and "neuter" it
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-03 20:21 PDT by Dmitry Lomov
Modified: 2011-11-04 14:44 PDT (History)
3 users (show)

See Also:


Attachments
Fix (13.07 KB, patch)
2011-11-03 20:23 PDT, Dmitry Lomov
no flags Details | Formatted Diff | Diff
Style (13.06 KB, patch)
2011-11-03 20:28 PDT, Dmitry Lomov
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Build fixed (13.11 KB, patch)
2011-11-03 20:53 PDT, Dmitry Lomov
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
CR feedback and tests fixed (12.73 KB, patch)
2011-11-04 11:41 PDT, Dmitry Lomov
levin: review-
Details | Formatted Diff | Diff
swap->transfer + assertion (12.78 KB, patch)
2011-11-04 12:02 PDT, Dmitry Lomov
no flags Details | Formatted Diff | Diff
Small style fix (12.79 KB, patch)
2011-11-04 12:04 PDT, Dmitry Lomov
no flags 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-03 20:21:03 PDT
Implement ArrayBuffer transfer and ArrayBuffer and related views neutering.
Comment 1 Dmitry Lomov 2011-11-03 20:23:31 PDT
Created attachment 113614 [details]
Fix
Comment 2 WebKit Review Bot 2011-11-03 20:25:41 PDT
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.
Comment 3 Dmitry Lomov 2011-11-03 20:28:49 PDT
Created attachment 113615 [details]
Style
Comment 4 Early Warning System Bot 2011-11-03 20:39:44 PDT
Comment on attachment 113615 [details]
Style

Attachment 113615 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10313040
Comment 5 Gyuyoung Kim 2011-11-03 20:46:51 PDT
Comment on attachment 113615 [details]
Style

Attachment 113615 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10319028
Comment 6 WebKit Review Bot 2011-11-03 20:50:44 PDT
Comment on attachment 113615 [details]
Style

Attachment 113615 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10314038
Comment 7 Dmitry Lomov 2011-11-03 20:53:31 PDT
Created attachment 113619 [details]
Build fixed
Comment 8 David Levin 2011-11-03 21:02:34 PDT
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.
Comment 9 WebKit Review Bot 2011-11-04 04:55:38 PDT
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
Comment 10 Dmitry Lomov 2011-11-04 10:00:24 PDT
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
Comment 11 Dmitry Lomov 2011-11-04 11:41:13 PDT
Created attachment 113689 [details]
CR feedback and tests fixed
Comment 12 WebKit Review Bot 2011-11-04 11:43:30 PDT
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.
Comment 13 David Levin 2011-11-04 11:51:22 PDT
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;
Comment 14 Darin Adler 2011-11-04 11:56:40 PDT
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.
Comment 15 Dmitry Lomov 2011-11-04 12:02:52 PDT
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.
Comment 16 Dmitry Lomov 2011-11-04 12:04:58 PDT
Created attachment 113695 [details]
Small style fix
Comment 17 WebKit Review Bot 2011-11-04 14:44:12 PDT
Comment on attachment 113695 [details]
Small style fix

Clearing flags on attachment: 113695

Committed r99324: <http://trac.webkit.org/changeset/99324>
Comment 18 WebKit Review Bot 2011-11-04 14:44:18 PDT
All reviewed patches have been landed.  Closing bug.