Bug 71535

Created attachment 113614 [details] Fix

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.

Created attachment 113615 [details] Style

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

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

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

Created attachment 113619 [details] Build fixed

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 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 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

Created attachment 113689 [details] CR feedback and tests fixed

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 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 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.

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.

Created attachment 113695 [details] Small style fix

Comment on attachment 113695 [details] Small style fix Clearing flags on attachment: 113695 Committed r99324: <http://trac.webkit.org/changeset/99324>

All reviewed patches have been landed. Closing bug.