Bug 78449

Created attachment 126702 [details] Patch

Comment on attachment 126702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126702&action=review > Source/WebCore/websockets/WebSocketDeflater.cpp:108 > + deflateEnd(&m_stream); Could you check the return value? > Source/WebCore/websockets/WebSocketDeflater.cpp:125 > + int result = deflate(&m_stream, Z_SYNC_FLUSH); > + if (result != Z_OK || m_stream.avail_in > 0) > + return false; You shouldn't call deflate() with Z_SYNC_FLUSH every time you add something to the deflater, because it puts extra data to the output buffer. You probably need to call defalte() with Z_NO_FLUSH here, and call it again with Z_FINISH in finish(). > Source/WebCore/websockets/WebSocketDeflater.cpp:135 > + // Remove 4 octets from the tail as the spec reqested. typo: reqested spec -> specification (we don't usually use such abbreviations) Do you mean: "as the specification requires" > Source/WebCore/websockets/WebSocketDeflater.h:43 > +class DeflateBuffer { Is this class necessary? Why don't you use Vector<char>? > Source/WebCore/websockets/WebSocketDeflater.h:52 > + ~DeflateBuffer(); Bad indent. > Source/WebCore/websockets/WebSocketDeflater.h:69 > + static PassOwnPtr<WebSocketDeflater> create(int windowBits, bool noContextTakeOver); We prefer enums over booleans in cases where bare literals ("true" or "false") would appear without context. > Source/WebKit/chromium/tests/WebSocketDeflaterTest.cpp:41 > + const char* compressData = "Hello"; nit: Maybe "uncompressedData", or "inputData"? > Source/WebKit/chromium/tests/WebSocketDeflaterTest.cpp:97 > + compressData = reinterpret_cast<char*>(fastMalloc(compressLength)); Vector<char> should be better than manual memory management with fastMalloc/fastFree. > Source/WebKit/chromium/tests/WebSocketDeflaterTest.cpp:118 > + compressData = reinterpret_cast<char*>(fastMalloc(compressLength)); Ditto.

Created attachment 126742 [details] Patch

Comment on attachment 126702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126702&action=review Hi Yuta-san, Thank you for review! >> Source/WebCore/websockets/WebSocketDeflater.cpp:108 >> + deflateEnd(&m_stream); > > Could you check the return value? Done. >> Source/WebCore/websockets/WebSocketDeflater.cpp:125 >> + return false; > > You shouldn't call deflate() with Z_SYNC_FLUSH every time you add something to the deflater, because it puts extra data to the output buffer. > > You probably need to call defalte() with Z_NO_FLUSH here, and call it again with Z_FINISH in finish(). Done. (Used Z_SYNC_FLUSH instead Z_FINISH to align on a byte boundary) >> Source/WebCore/websockets/WebSocketDeflater.cpp:135 >> + // Remove 4 octets from the tail as the spec reqested. > > typo: reqested > spec -> specification (we don't usually use such abbreviations) > > Do you mean: "as the specification requires" Done. >> Source/WebCore/websockets/WebSocketDeflater.h:43 >> +class DeflateBuffer { > > Is this class necessary? Why don't you use Vector<char>? Removed this class and use Vector<char>. >> Source/WebCore/websockets/WebSocketDeflater.h:69 >> + static PassOwnPtr<WebSocketDeflater> create(int windowBits, bool noContextTakeOver); > > We prefer enums over booleans in cases where bare literals ("true" or "false") would appear without context. Done. >> Source/WebKit/chromium/tests/WebSocketDeflaterTest.cpp:41 >> + const char* compressData = "Hello"; > > nit: Maybe "uncompressedData", or "inputData"? Done. >> Source/WebKit/chromium/tests/WebSocketDeflaterTest.cpp:97 >> + compressData = reinterpret_cast<char*>(fastMalloc(compressLength)); > > Vector<char> should be better than manual memory management with fastMalloc/fastFree. Done. Actually, I rewrote the test completely. >> Source/WebKit/chromium/tests/WebSocketDeflaterTest.cpp:118 >> + compressData = reinterpret_cast<char*>(fastMalloc(compressLength)); > > Ditto. Done.

Created attachment 126744 [details] Patch(rebased)

Comment on attachment 126744 [details] Patch(rebased) View in context: https://bugs.webkit.org/attachment.cgi?id=126744&action=review > Source/WebCore/websockets/WebSocketDeflater.cpp:53 > + if (deflateInit2(&deflater->m_stream, Z_DEFAULT_COMPRESSION, Z_DEFLATED, -windowBits, defaultMemLevel, Z_DEFAULT_STRATEGY) == Z_OK) To me, it seems strange to call defalateInit2() outside member functions (deflateEnd() is called in the destructor, so Init and End look unpaired). How about adding initialize() member function which calls deflateInit2(), or doing this in the constructor? > Source/WebCore/websockets/WebSocketDeflater.cpp:79 > + if (!m_buffer.tryReserveCapacity(m_buffer.size() + maxLength)) reserveCapacity() increases the size of the backing buffer, but we should not touch the area beyond data() + size(). For details, see std::vector<>'s documentation (which is almost identical to Vector<>). http://www.cplusplus.com/reference/stl/vector/capacity/ Here you should do: size_t oldSize = m_buffer.size(); // Optional: call tryReserveCapacity to make sure we have enough memory. m_buffer.grow(oldSize + maxLength); And instead of line 90: m_buffer.shrink(oldSize + compressedSize); > Source/WebCore/websockets/WebSocketDeflater.cpp:108 > + if (!m_buffer.tryReserveCapacity(m_buffer.capacity() * 2)) This usage of tryReserveCapacity() is also wrong. See above. And I'm unsure about the reason of doubling the capacity here. > Source/WebCore/websockets/WebSocketDeflater.cpp:130 > + if (inflateInit2(&inflater->m_stream, -windowBits) == Z_OK) Same as line 53. > Source/WebCore/websockets/WebSocketDeflater.cpp:153 > + if (!m_buffer.tryReserveCapacity(m_buffer.size() + initialBufferSize)) Same as line 79. > Source/WebCore/websockets/WebSocketDeflater.cpp:197 > + if (!m_buffer.tryReserveCapacity(m_buffer.capacity() * 2)) Here as well. > Source/WebCore/websockets/WebSocketDeflater.h:46 > + enum ShouldTakeOverContext { For names of enum type, FooBarMode or FooBarType seem conventional. Maybe "ContextTakeOverMode"? > Source/WebCore/websockets/WebSocketDeflater.h:48 > + DoNotTakeOverContext = 0, > + TakeOverContext = 1 "= 0" and "= 1" aren't necessary in this case. > Source/WebCore/websockets/WebSocketDeflater.h:61 > + WebSocketDeflater(int, ShouldTakeOverContext); Could you add a parameter name for the first argument? "int" looks ambiguous. > Source/WebCore/websockets/WebSocketDeflater.h:82 > + WebSocketInflater(int); Could you add "explicit" and parameter name?

Created attachment 126931 [details] Patch

Comment on attachment 126744 [details] Patch(rebased) View in context: https://bugs.webkit.org/attachment.cgi?id=126744&action=review >> Source/WebCore/websockets/WebSocketDeflater.cpp:53 >> + if (deflateInit2(&deflater->m_stream, Z_DEFAULT_COMPRESSION, Z_DEFLATED, -windowBits, defaultMemLevel, Z_DEFAULT_STRATEGY) == Z_OK) > > To me, it seems strange to call defalateInit2() outside member functions (deflateEnd() is called in the destructor, so Init and End look unpaired). > > How about adding initialize() member function which calls deflateInit2(), or doing this in the constructor? Added initialize() member function. >> Source/WebCore/websockets/WebSocketDeflater.cpp:79 >> + if (!m_buffer.tryReserveCapacity(m_buffer.size() + maxLength)) > > reserveCapacity() increases the size of the backing buffer, but we should not touch the area beyond data() + size(). > > For details, see std::vector<>'s documentation (which is almost identical to Vector<>). > http://www.cplusplus.com/reference/stl/vector/capacity/ > > Here you should do: > size_t oldSize = m_buffer.size(); > // Optional: call tryReserveCapacity to make sure we have enough memory. > m_buffer.grow(oldSize + maxLength); > > And instead of line 90: > m_buffer.shrink(oldSize + compressedSize); Done. >> Source/WebCore/websockets/WebSocketDeflater.cpp:108 >> + if (!m_buffer.tryReserveCapacity(m_buffer.capacity() * 2)) > > This usage of tryReserveCapacity() is also wrong. See above. > > And I'm unsure about the reason of doubling the capacity here. The reason I doubled the capacity was performance issue, but using grow() and shrink() is fast as the previous one so I changed increasing size linearly. >> Source/WebCore/websockets/WebSocketDeflater.cpp:130 >> + if (inflateInit2(&inflater->m_stream, -windowBits) == Z_OK) > > Same as line 53. Done. >> Source/WebCore/websockets/WebSocketDeflater.cpp:153 >> + if (!m_buffer.tryReserveCapacity(m_buffer.size() + initialBufferSize)) > > Same as line 79. Done. >> Source/WebCore/websockets/WebSocketDeflater.cpp:197 >> + if (!m_buffer.tryReserveCapacity(m_buffer.capacity() * 2)) > > Here as well. Done >> Source/WebCore/websockets/WebSocketDeflater.h:46 >> + enum ShouldTakeOverContext { > > For names of enum type, FooBarMode or FooBarType seem conventional. Maybe "ContextTakeOverMode"? Done. >> Source/WebCore/websockets/WebSocketDeflater.h:48 >> + TakeOverContext = 1 > > "= 0" and "= 1" aren't necessary in this case. Done. >> Source/WebCore/websockets/WebSocketDeflater.h:61 >> + WebSocketDeflater(int, ShouldTakeOverContext); > > Could you add a parameter name for the first argument? "int" looks ambiguous. Done >> Source/WebCore/websockets/WebSocketDeflater.h:82 >> + WebSocketInflater(int); > > Could you add "explicit" and parameter name? Done

Comment on attachment 126931 [details] Patch Looks OK. [Note: I don't have r+ privilege.]

Kent-san, Alexey, Could you review the patch?

Comment on attachment 126931 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126931&action=review > Source/WebCore/websockets/WebSocketDeflater.cpp:59 > + ASSERT(m_windowBits >= 8 && m_windowBits <= 15); Please split this into two ASSERTs. ASSERT(m_windowBits >= 8); ASSERT(m_windowBits <= 15); > Source/WebCore/websockets/WebSocketDeflater.cpp:86 > + m_stream.next_in = reinterpret_cast<Bytef*>(const_cast<char*>(data)); > + m_stream.avail_in = length; > + m_stream.next_out = reinterpret_cast<Bytef*>(m_buffer.data() + writePosition); > + m_stream.avail_out = maxLength; Repeating reinterpret_cast<> and const_cast<> is not good. Please add a helper function like setStreamParameters(data, length, m_buffer.data() + writePosition, maxLength); > Source/WebCore/websockets/WebSocketDeflater.cpp:104 > + m_stream.next_in = reinterpret_cast<Bytef*>(0); > + m_stream.avail_in = 0; > + m_stream.next_out = reinterpret_cast<Bytef*>(m_buffer.data() + writePosition); > + m_stream.avail_out = availableCapacity; ditto. > Source/WebCore/websockets/WebSocketDeflater.cpp:163 > + m_stream.next_in = reinterpret_cast<Bytef*>(const_cast<char*>(data) + consumedSoFar); > + m_stream.avail_in = remainingLength; > + m_stream.next_out = reinterpret_cast<Bytef*>(m_buffer.data() + writePosition); > + m_stream.avail_out = availableCapacity; ditto. > Source/WebCore/websockets/WebSocketDeflater.cpp:191 > + m_stream.next_in = strippedFields + consumedSoFar; > + m_stream.avail_in = remainingLength; > + m_stream.next_out = reinterpret_cast<Bytef*>(m_buffer.data() + writePosition); > + m_stream.avail_out = availableCapacity; ditto.

Created attachment 126952 [details] Patch

Attachment 126952 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource From git://git.webkit.org/WebKit 512e439..3821275 master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 107699 = 512e43965f3c85a210634c39adbabda26e7d72c0 r107700 = 14993e0b953560e2aabeb23e631330550529886e r107701 = 934bc755aada5227698952084e81a09172d65db2 r107702 = 3821275ca36d73082abe77a514141ea2d24b147a Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets Using index info to reconstruct a base tree... <stdin>:1578: trailing whitespace. <stdin>:1647: trailing whitespace. <stdin>:1657: trailing whitespace. <stdin>:1672: trailing whitespace. return 0; <stdin>:1674: trailing whitespace. warning: squelched 7 whitespace errors warning: 12 lines add whitespace errors. Falling back to patching base and 3-way merge... warning: too many files (created: 168768 deleted: 3), skipping inexact rename detection Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Auto-merging Tools/ChangeLog CONFLICT (content): Merge conflict in Tools/ChangeLog Failed to merge in the changes. Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 126931 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126931&action=review Thank you for review. Revised the patch. Looks like qt and efl doesn't depend on zlib. I removed WebSocketDeflater.(cpp|h) from CMakefileList.txt and Target.pri for now. >> Source/WebCore/websockets/WebSocketDeflater.cpp:59 >> + ASSERT(m_windowBits >= 8 && m_windowBits <= 15); > > Please split this into two ASSERTs. > > ASSERT(m_windowBits >= 8); > ASSERT(m_windowBits <= 15); Done. >> Source/WebCore/websockets/WebSocketDeflater.cpp:86 >> + m_stream.avail_out = maxLength; > > Repeating reinterpret_cast<> and const_cast<> is not good. > Please add a helper function like setStreamParameters(data, length, m_buffer.data() + writePosition, maxLength); Done. >> Source/WebCore/websockets/WebSocketDeflater.cpp:104 >> + m_stream.avail_out = availableCapacity; > > ditto. Done. >> Source/WebCore/websockets/WebSocketDeflater.cpp:163 >> + m_stream.avail_out = availableCapacity; > > ditto. Done.

(In reply to comment #14) > Looks like qt and efl doesn't depend on zlib. I removed WebSocketDeflater.(cpp|h) from CMakefileList.txt and Target.pri for now. How will you resolve this issue?

(In reply to comment #15) > (In reply to comment #14) > > Looks like qt and efl doesn't depend on zlib. I removed WebSocketDeflater.(cpp|h) from CMakefileList.txt and Target.pri for now. > > How will you resolve this issue? I'll add a compile flag like USE(ZLIB) and wrap zlib dependent code in following patch.

Comment on attachment 126952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126952&action=review > Source/WebCore/websockets/WebSocketDeflater.cpp:178 > + static const char strippedFields[] = {0x00, 0x00, 0xff, 0xff}; Windows EWS is red. 3>..\websockets\WebSocketDeflater.cpp(178) : error C2220: warning treated as error - no 'object' file generated 3>..\websockets\WebSocketDeflater.cpp(178) : warning C4309: 'initializing' : truncation of constant value 3>..\websockets\WebSocketDeflater.cpp(178) : warning C4309: 'initializing' : truncation of constant value

Created attachment 127075 [details] Patch

Comment on attachment 127075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127075&action=review > Source/WebCore/websockets/WebSocketDeflater.cpp:179 > + // Use Bytef[] instead of char[] to avoid "truncation of constant value" warning of MSVC. > + static const Bytef strippedFields[] = {0x00, 0x00, 0xff, 0xff}; static const char strippedFields[4] = "\0\0\xff\xff"; is enough?

Created attachment 127078 [details] Patch

Comment on attachment 127075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127075&action=review >> Source/WebCore/websockets/WebSocketDeflater.cpp:179 >> + static const Bytef strippedFields[] = {0x00, 0x00, 0xff, 0xff}; > > static const char strippedFields[4] = "\0\0\xff\xff"; is enough? It will be the best. Thanks!

Comment on attachment 127078 [details] Patch ok

Comment on attachment 127078 [details] Patch Thanks!

Comment on attachment 127078 [details] Patch Clearing flags on attachment: 127078 Committed r107766: <http://trac.webkit.org/changeset/107766>

All reviewed patches have been landed. Closing bug.

This patch was reverted: http://trac.webkit.org/changeset/107778 I'll upload revised patch to fix the build failure of Chromium Win.

Created attachment 127139 [details] Patch

(In reply to comment #27) > Created an attachment (id=127139) [details] > Patch I confirmed this patch built successfully on win_rel trybot. http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/1809

(In reply to comment #28) > (In reply to comment #27) > > Created an attachment (id=127139) [details] [details] > > Patch > > I confirmed this patch built successfully on win_rel trybot. > http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/1809 Did you confirm WebKit without Chromium build for Chromium/Windows?

Attachment 127139 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource Index mismatch: 0dfd183742a71cb5de5dadc3ae177fc72b63a194 != 9cdcda984def14b8bf8a32b6da6784c8a6ef7b3a rereading 8567f8d3c2539a28a496edaf1048483e973975c2 M LayoutTests/fast/forms/radio-nested-labels.html M LayoutTests/ChangeLog 107798 = 3671b2d23de7ade4cb1d1e78a3f6f7673db6a6c9 already exists! Why are we refetching it? at /usr/lib/git-core/git-svn line 5210 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #29) > (In reply to comment #28) > > (In reply to comment #27) > > > Created an attachment (id=127139) [details] [details] [details] > > > Patch > > > > I confirmed this patch built successfully on win_rel trybot. > > http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/1809 > > Did you confirm WebKit without Chromium build for Chromium/Windows? No. I'll check it next week.

(In reply to comment #31) > > Did you confirm WebKit without Chromium build for Chromium/Windows? > > No. I'll check it next week. I confirmed WebKit Chromium/Win port build (DumpRenderTree/webkit_unit_tests) on my Win 7 machine (I use MSVS2010 because I couldn't install older version of MSVS correctly..).

Comment on attachment 127139 [details] Patch ok

Comment on attachment 127139 [details] Patch Rejecting attachment 127139 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: atching file Source/WebCore/websockets/WebSocketDeflater.cpp patching file Source/WebCore/websockets/WebSocketDeflater.h patching file Source/WebKit/chromium/WebKit.gypi Hunk #1 succeeded at 118 with fuzz 1. patching file Source/WebKit/chromium/WebKitUnitTests.gyp patching file Source/WebKit/chromium/tests/WebSocketDeflaterTest.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kent Tamura']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/11546406

Created attachment 127770 [details] Patch for landing

Comment on attachment 127770 [details] Patch for landing Clearing flags on attachment: 127770 Committed r108221: <http://trac.webkit.org/changeset/108221>

All reviewed patches have been landed. Closing bug.