Bug 78449 - [WebSocket] Add deflater/inflater classes
Summary: [WebSocket] Add deflater/inflater classes
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: Kenichi Ishibashi
URL:
Keywords:
Depends on: 78665
Blocks: 77522
  Show dependency treegraph
 
Reported: 2012-02-12 18:24 PST by Kenichi Ishibashi
Modified: 2012-02-20 01:00 PST (History)
6 users (show)

See Also:


Attachments
Patch (29.62 KB, patch)
2012-02-12 18:40 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (29.59 KB, patch)
2012-02-13 02:46 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch(rebased) (29.59 KB, patch)
2012-02-13 03:07 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (29.89 KB, patch)
2012-02-14 01:22 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (28.18 KB, patch)
2012-02-14 03:51 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (28.27 KB, patch)
2012-02-14 16:33 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (28.14 KB, patch)
2012-02-14 16:41 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (29.97 KB, patch)
2012-02-15 02:05 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (29.73 KB, patch)
2012-02-19 23:46 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2012-02-12 18:24:01 PST
Adding wrapper classes of zlib functions to prepare WebSocket deflate-frame extension.
Comment 1 Kenichi Ishibashi 2012-02-12 18:40:40 PST
Created attachment 126702 [details]
Patch
Comment 2 Yuta Kitamura 2012-02-12 22:14:45 PST
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.
Comment 3 Kenichi Ishibashi 2012-02-13 02:46:41 PST
Created attachment 126742 [details]
Patch
Comment 4 Kenichi Ishibashi 2012-02-13 02:59:05 PST
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.
Comment 5 Kenichi Ishibashi 2012-02-13 03:07:44 PST
Created attachment 126744 [details]
Patch(rebased)
Comment 6 Yuta Kitamura 2012-02-13 23:39:50 PST
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?
Comment 7 Kenichi Ishibashi 2012-02-14 01:22:44 PST
Created attachment 126931 [details]
Patch
Comment 8 Kenichi Ishibashi 2012-02-14 01:24:56 PST
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 9 Yuta Kitamura 2012-02-14 01:52:03 PST
Comment on attachment 126931 [details]
Patch

Looks OK. [Note: I don't have r+ privilege.]
Comment 10 Kenichi Ishibashi 2012-02-14 01:53:44 PST
Kent-san, Alexey,

Could you review the patch?
Comment 11 Kent Tamura 2012-02-14 02:26:29 PST
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.
Comment 12 Kenichi Ishibashi 2012-02-14 03:51:24 PST
Created attachment 126952 [details]
Patch
Comment 13 WebKit Review Bot 2012-02-14 03:54:33 PST
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 14 Kenichi Ishibashi 2012-02-14 03:55:35 PST
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.
Comment 15 Kent Tamura 2012-02-14 15:02:34 PST
(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?
Comment 16 Kenichi Ishibashi 2012-02-14 15:52:26 PST
(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 17 Kent Tamura 2012-02-14 16:00:04 PST
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
Comment 18 Kenichi Ishibashi 2012-02-14 16:33:08 PST
Created attachment 127075 [details]
Patch
Comment 19 Kent Tamura 2012-02-14 16:35:12 PST
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?
Comment 20 Kenichi Ishibashi 2012-02-14 16:41:04 PST
Created attachment 127078 [details]
Patch
Comment 21 Kenichi Ishibashi 2012-02-14 16:42:17 PST
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 22 Kent Tamura 2012-02-14 17:25:43 PST
Comment on attachment 127078 [details]
Patch

ok
Comment 23 Kenichi Ishibashi 2012-02-14 17:33:47 PST
Comment on attachment 127078 [details]
Patch

Thanks!
Comment 24 WebKit Review Bot 2012-02-14 18:03:10 PST
Comment on attachment 127078 [details]
Patch

Clearing flags on attachment: 127078

Committed r107766: <http://trac.webkit.org/changeset/107766>
Comment 25 WebKit Review Bot 2012-02-14 18:03:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Kenichi Ishibashi 2012-02-15 01:49:52 PST
This patch was reverted:
http://trac.webkit.org/changeset/107778

I'll upload revised patch to fix the build failure of Chromium Win.
Comment 27 Kenichi Ishibashi 2012-02-15 02:05:37 PST
Created attachment 127139 [details]
Patch
Comment 28 Kenichi Ishibashi 2012-02-15 02:06:45 PST
(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
Comment 29 Kent Tamura 2012-02-15 02:59:58 PST
(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?
Comment 30 WebKit Review Bot 2012-02-15 04:08:30 PST
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.
Comment 31 Kenichi Ishibashi 2012-02-15 05:18:10 PST
(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.
Comment 32 Kenichi Ishibashi 2012-02-19 23:26:46 PST
(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 33 Kent Tamura 2012-02-19 23:28:29 PST
Comment on attachment 127139 [details]
Patch

ok
Comment 34 WebKit Review Bot 2012-02-19 23:34:39 PST
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
Comment 35 Kenichi Ishibashi 2012-02-19 23:46:24 PST
Created attachment 127770 [details]
Patch for landing
Comment 36 WebKit Review Bot 2012-02-20 00:59:59 PST
Comment on attachment 127770 [details]
Patch for landing

Clearing flags on attachment: 127770

Committed r108221: <http://trac.webkit.org/changeset/108221>
Comment 37 WebKit Review Bot 2012-02-20 01:00:06 PST
All reviewed patches have been landed.  Closing bug.