Bug 64852 - [Chromium] Turn on new WebSocket protocol in worker shadow page
Summary: [Chromium] Turn on new WebSocket protocol in worker shadow page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yuta Kitamura
URL:
Keywords:
Depends on:
Blocks: 50099
  Show dependency treegraph
 
Reported: 2011-07-20 01:12 PDT by Yuta Kitamura
Modified: 2011-07-23 07:55 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.57 KB, patch)
2011-07-20 05:48 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v2 (add FIXME comment) (1.84 KB, patch)
2011-07-22 06:06 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch for landing (1.96 KB, patch)
2011-07-23 06:40 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuta Kitamura 2011-07-20 01:12:10 PDT
I'm trying to turn on the new WebSocket protocol by default.

This should be done in the following steps:

(1) Change the default preference value in webkit/glue/webpreferences.{h,cc} (in Chromium tree).
(2) Change the default preference value in worker shadow page initialized in WebWorkerBase::initializeLoader() (in WebKit tree).

This bug takes care of step (2). Step (2) is required to enable the new protocol in WebSocket objects initialized in worker context, because they live in worker loader context and look at the value of a Settings flag of the shadow page.
Comment 1 Yuta Kitamura 2011-07-20 05:45:33 PDT
A patch for step (1) is under review at <http://codereview.chromium.org/7462009/>.
Comment 2 Yuta Kitamura 2011-07-20 05:48:09 PDT
Created attachment 101454 [details]
Patch
Comment 3 Yuta Kitamura 2011-07-20 05:48:54 PDT
Comment on attachment 101454 [details]
Patch

Setting CQ- since this patch should be committed after <http://codereview.chromium.org/7462009/>.
Comment 4 Yuta Kitamura 2011-07-20 05:54:07 PDT
Dmitry, could you take a look at this patch?

I think it's better to copy the value of Settings::useHixie76WebSocketProtocol() of the main thread (rather than setting a constant "false"), but I could not figure out a good way to do this.
Comment 5 Kent Tamura 2011-07-21 23:00:46 PDT
(In reply to comment #4)
> Dmitry, could you take a look at this patch?
> 
> I think it's better to copy the value of Settings::useHixie76WebSocketProtocol() of the main thread (rather than setting a constant "false"), but I could not figure out a good way to do this.

Though I don't know how to retrieve the value from the main thread, I think we need a FIXME comment.
Comment 6 Yuta Kitamura 2011-07-22 06:06:59 PDT
Created attachment 101723 [details]
Patch v2 (add FIXME comment)
Comment 7 Dmitry Titov 2011-07-22 14:36:03 PDT
Comment on attachment 101723 [details]
Patch v2 (add FIXME comment)

View in context: https://bugs.webkit.org/attachment.cgi?id=101723&action=review

r=me with a note.

> Source/WebKit/chromium/src/WebWorkerBase.cpp:242
> +    // FIXME: Should copy the value of Settings::hixie76ProtocolEnabled() of a view in the main thread.

This code is running in the main WebKit thread, but it runs in a sandboxed Worker process. Settings information should be passed to the Worker process from Browser process when the worker is created (similar to RenderThread::OnCreateNewView).  Could you please change the comment to reflect this?
Comment 8 Yuta Kitamura 2011-07-23 06:40:53 PDT
Created attachment 101809 [details]
Patch for landing
Comment 9 Yuta Kitamura 2011-07-23 06:42:56 PDT
Comment on attachment 101723 [details]
Patch v2 (add FIXME comment)

View in context: https://bugs.webkit.org/attachment.cgi?id=101723&action=review

Thank you for your review!

>> Source/WebKit/chromium/src/WebWorkerBase.cpp:242
>> +    // FIXME: Should copy the value of Settings::hixie76ProtocolEnabled() of a view in the main thread.
> 
> This code is running in the main WebKit thread, but it runs in a sandboxed Worker process. Settings information should be passed to the Worker process from Browser process when the worker is created (similar to RenderThread::OnCreateNewView).  Could you please change the comment to reflect this?

Updated the comment as per your suggestion.
Comment 10 WebKit Review Bot 2011-07-23 07:55:07 PDT
Comment on attachment 101809 [details]
Patch for landing

Clearing flags on attachment: 101809

Committed r91634: <http://trac.webkit.org/changeset/91634>
Comment 11 WebKit Review Bot 2011-07-23 07:55:12 PDT
All reviewed patches have been landed.  Closing bug.