WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
71535
Add the ability to transfer ArrayBuffer and "neuter" it
https://bugs.webkit.org/show_bug.cgi?id=71535
Summary
Add the ability to transfer ArrayBuffer and "neuter" it
Dmitry Lomov
Reported
2011-11-03 20:21:03 PDT
Implement ArrayBuffer transfer and ArrayBuffer and related views neutering.
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Dmitry Lomov
Comment 1
2011-11-03 20:23:31 PDT
Created
attachment 113614
[details]
Fix
WebKit Review Bot
Comment 2
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.
Dmitry Lomov
Comment 3
2011-11-03 20:28:49 PDT
Created
attachment 113615
[details]
Style
Early Warning System Bot
Comment 4
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
Gyuyoung Kim
Comment 5
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
WebKit Review Bot
Comment 6
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
Dmitry Lomov
Comment 7
2011-11-03 20:53:31 PDT
Created
attachment 113619
[details]
Build fixed
David Levin
Comment 8
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.
WebKit Review Bot
Comment 9
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
Dmitry Lomov
Comment 10
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
Dmitry Lomov
Comment 11
2011-11-04 11:41:13 PDT
Created
attachment 113689
[details]
CR feedback and tests fixed
WebKit Review Bot
Comment 12
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.
David Levin
Comment 13
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;
Darin Adler
Comment 14
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.
Dmitry Lomov
Comment 15
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.
Dmitry Lomov
Comment 16
2011-11-04 12:04:58 PDT
Created
attachment 113695
[details]
Small style fix
WebKit Review Bot
Comment 17
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
>
WebKit Review Bot
Comment 18
2011-11-04 14:44:18 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug