Bug 60348 - WebSocket: Add run-time flag for new HyBi protocol
Summary: WebSocket: Add run-time flag for new HyBi protocol
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-05-06 00:23 PDT by Yuta Kitamura
Modified: 2011-06-24 05:18 PDT (History)
9 users (show)

See Also:


Attachments
Patch (14.69 KB, patch)
2011-05-06 00:49 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v2 (15.17 KB, patch)
2011-05-09 00:04 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v3 (12.67 KB, patch)
2011-06-23 05:52 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-05-06 00:23:51 PDT
Add a run-time flag in Settings that will be used for switching WebSocket protocols.
Comment 1 Yuta Kitamura 2011-05-06 00:49:08 PDT
Created attachment 92555 [details]
Patch
Comment 2 Kent Tamura 2011-05-06 01:12:24 PDT
Comment on attachment 92555 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        (WebCore::Settings::setUseNewHyBiWebSocketProtocol):
> +        (WebCore::Settings::useNewHyBiWebSocketProtocol):

A drive-by comment.

Can you make the name more specific, instead of 'NewHyBi'?
'NewHyBi' can be ambiguous.  If we introduced another incompatible protocol, 'new' would not be identical.
Comment 3 Fumitoshi Ukai 2011-05-06 01:16:26 PDT
Comment on attachment 92555 [details]
Patch

Can we access (per page) Settings if WebSocket is used in worker context?
Comment 4 Adam Barth 2011-05-06 01:20:29 PDT
Why are we adding a setting for enabling the new version?  It seems like this should either be an ENABLE macro or we should just replace the old version.
Comment 5 Joe Mason 2011-05-06 07:22:08 PDT
(In reply to comment #4)
> Why are we adding a setting for enabling the new version?  It seems like this should either be an ENABLE macro or we should just replace the old version.

I agree.  It should be a compile-time switch for now, and eventually we should just remove the old version.
Comment 6 Yuta Kitamura 2011-05-08 19:49:56 PDT
(In reply to comment #4)
> Why are we adding a setting for enabling the new version?  It seems like this should either be an ENABLE macro or we should just replace the old version.

It was my intention to just replace the old version, but Alexey (ap) was not happy about it because of legacy servers.

ENABLE macro is fine with me, but I'm a bit worried about testing. The new protocol requires a different set of tests by its nature, and I don't know how to test both versions when we create a new ENABLE macro. If we use a Setting flag, we can switch protocol versions via LayoutTestController.
Comment 7 Yuta Kitamura 2011-05-08 19:57:59 PDT
(In reply to comment #2)
> Can you make the name more specific, instead of 'NewHyBi'?
> 'NewHyBi' can be ambiguous.  If we introduced another incompatible protocol, 'new' would not be identical.

It's hard to name the new protocol; We may call it "HyBi07", but the name is not very stable as we will have hybi-08 specification in the near future. Or we could just call it "HyBi", but it is also ambiguous.
Comment 8 Alexey Proskuryakov 2011-05-08 20:04:39 PDT
In WebKit, we always special case "old", not "new".
Comment 9 Yuta Kitamura 2011-05-08 23:10:54 PDT
(In reply to comment #8)
> In WebKit, we always special case "old", not "new".

Sure, I will update the patch to change the name of the flag to "useHixie76WebSocketProtocol", which should be less ambiguous.
Comment 10 Yuta Kitamura 2011-05-09 00:04:09 PDT
Created attachment 92762 [details]
Patch v2
Comment 11 Adam Barth 2011-05-09 01:06:47 PDT
> It was my intention to just replace the old version, but Alexey (ap) was not happy about it because of legacy servers.

I don't think we want to commit to supporting legacy servers.  My understanding is that none of the other browsers are going to either.
Comment 12 Joe Mason 2011-05-09 07:19:40 PDT
(In reply to comment #11)
> > It was my intention to just replace the old version, but Alexey (ap) was not happy about it because of legacy servers.
> 
> I don't think we want to commit to supporting legacy servers.  My understanding is that none of the other browsers are going to either.

As soon as supporting both versions at once causes complications or slows down development, we should drop the old version.  But if it turns out to be simple to do, we might as well continue to support both until the new version is completely implemented and stable, and then make a political decision whether to drop support for the old version in core or leave it up to the browsers.
Comment 13 Alexey Proskuryakov 2011-05-09 15:03:53 PDT
Nobody says that we should commit to eternally supporting the version that's used by 100% of sites utilizing WebSockets on the Web today, and implemented by all browsers that support WebSockets.

Dropping that support for a version that's not in use by anyone - and may never be in its current state - is a different thing.
Comment 14 Adam Barth 2011-05-09 15:08:09 PDT
Isn't this a case of "pay me now or pay me later" ?  It's unclear what we gain by delaying paying off this compat monster.
Comment 15 Alexey Proskuryakov 2011-05-09 15:29:25 PDT
Delaying switching at least until IE and/or Firefox ship the new version would mean that we won't have to switch again, to hybi-08 or whatever. Tracking the most recent specification only caused us and our users trouble - a lot of servers already have to support -75 and -76, and they will have to support the version that IE and Firefox decide to implement. Adding -07 to the mix would be extremely unhelpful.
Comment 16 Joe Mason 2011-05-09 16:51:19 PDT
The spec churn is slow enough now that most of the work done to support HyBi-07 will still apply to HyBi-08 when it's out.  We shouldn't throw the switch to make the new version the default / shut off the old version until there's a consensus between several browser vendors on a stable supported version, but we can get started on implementing the new version now.

If it turns out to be too messy to support both in parallel in svn head, we could develop this as a clean replacement in a separate branch and not pull it into head until it's time to switch.  (Don't know how hard that would be using svn - I've been using git so long that I've forgotten svn's capabilities.)
Comment 17 Alexey Proskuryakov 2011-05-09 16:55:09 PDT
Yes, that sounds good to me.
Comment 18 Yuta Kitamura 2011-05-09 22:56:40 PDT
Hmm, to make a branch, do I need just to copy the trunk into "branch/" directory? Do I need any review to do that?

(I'm going to have to fight against svn because I usually use git, but that's another story...)
Comment 19 Adam Barth 2011-05-10 01:05:42 PDT
You don't want to make a branch.  Approximately zero branches of WebKit have ever merged with trunk.
Comment 20 Eric Seidel (no email) 2011-05-10 10:43:00 PDT
Depends on how long you plan to keep the branch around and how much you plan to change in your branch.  We've had 3 instances of non-trunk changes merging into WebKit successfully.  SVG (which took me months), WebKit Windows (which took Apple months), and Chrome (which took me and others months again!).

If you need to make invasive changes to webkit, it's better for your sanity to do them behind an ENABLE_ flag (which, per our recent feature guildelines, should be announced on webkit-dev).
http://www.webkit.org/coding/adding-features.html
Comment 21 Yuta Kitamura 2011-05-11 00:15:47 PDT
I think I prefer adding a run-time flag.

ENABLE macro could be fine, but we already have ENABLE(WEB_SOCKETS) macro and I don't know how to add exclusive options (new protocol vs old protocol) to WebKit.

Further, I don't know how to add tests for new protocol cleanly (without breaking existing ones).

Adding a branch may be fine too but working with a branch is very much like a mystery (to me).

Either way, I really want to move forward and implement new protocol...
Comment 22 Yuta Kitamura 2011-06-09 00:28:52 PDT
I think we need to make a consensus on this.

Here is my summary of strategies:

A. Just replacing old protocol with new one
  - Able to resolve a security concern raised for the old protocol.
  - Breaks existing sites immediately.
  - Affects all ports immediately.

B. Runtime switch
  - Able to test both protocols at the same time (by adding a method to LayoutTestController).
  - Code may look tedious.
  - Each port will be able to switch protocol versions easily.
  - Dead code will be included in executables.

C. Compile-time switch
  - It's probably hard to test both protocol implementations at the same time.
  - Each port will be able to switch protocol versions by changing compiler options.
  - We already have a flag ENABLE(WEB_SOCKETS). Adding another flag will make the code dirtier.

D. Using branch
  - Will not be ran by buildbots.
  - Each port can't get the new protocol before the code is merged to the mainline.

--

My preference is A, but ap has expressed his concern about breaking existing sites. My next preference is B, because I'd like to keep test coverage for both protocol implementations.
Comment 23 Yuta Kitamura 2011-06-23 05:52:20 PDT
Created attachment 98339 [details]
Patch v3
Comment 24 Adam Barth 2011-06-23 09:55:43 PDT
Comment on attachment 98339 [details]
Patch v3

Ok.  I probably would have called this "Draft76" instead of "Hixie76", but I'm not sure it matters that much.
Comment 25 Alexey Proskuryakov 2011-06-23 09:57:18 PDT
Comment on attachment 98339 [details]
Patch v3

This patch looks good to me. In addition, to meaningfully test new protocol implementation, we'll need WebKit2 support, too.
Comment 26 Yuta Kitamura 2011-06-24 04:37:12 PDT
(In reply to comment #25)
> (From update of attachment 98339 [details])
> This patch looks good to me. In addition, to meaningfully test new protocol implementation, we'll need WebKit2 support, too.

I will add WebKit2 support in a later patch. Thanks!
Comment 27 WebKit Review Bot 2011-06-24 05:18:32 PDT
Comment on attachment 98339 [details]
Patch v3

Clearing flags on attachment: 98339

Committed r89669: <http://trac.webkit.org/changeset/89669>
Comment 28 WebKit Review Bot 2011-06-24 05:18:38 PDT
All reviewed patches have been landed.  Closing bug.