Bug 60348 - WebSocket: Add run-time flag for new HyBi protocol
: WebSocket: Add run-time flag for new HyBi protocol
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 50099
  Show dependency treegraph
 
Reported: 2011-05-06 00:23 PST by
Modified: 2011-06-24 05:18 PST (History)


Attachments
Patch (14.69 KB, patch)
2011-05-06 00:49 PST, Yuta Kitamura
no flags Review Patch | Details | Formatted Diff | Diff
Patch v2 (15.17 KB, patch)
2011-05-09 00:04 PST, Yuta Kitamura
no flags Review Patch | Details | Formatted Diff | Diff
Patch v3 (12.67 KB, patch)
2011-06-23 05:52 PST, Yuta Kitamura
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-05-06 00:23:51 PST
Add a run-time flag in Settings that will be used for switching WebSocket protocols.
------- Comment #1 From 2011-05-06 00:49:08 PST -------
Created an attachment (id=92555) [details]
Patch
------- Comment #2 From 2011-05-06 01:12:24 PST -------
(From update of attachment 92555 [details])
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 From 2011-05-06 01:16:26 PST -------
(From update of attachment 92555 [details])
Can we access (per page) Settings if WebSocket is used in worker context?
------- Comment #4 From 2011-05-06 01:20:29 PST -------
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 From 2011-05-06 07:22:08 PST -------
(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 From 2011-05-08 19:49:56 PST -------
(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 From 2011-05-08 19:57:59 PST -------
(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 From 2011-05-08 20:04:39 PST -------
In WebKit, we always special case "old", not "new".
------- Comment #9 From 2011-05-08 23:10:54 PST -------
(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 From 2011-05-09 00:04:09 PST -------
Created an attachment (id=92762) [details]
Patch v2
------- Comment #11 From 2011-05-09 01:06:47 PST -------
> 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 From 2011-05-09 07:19:40 PST -------
(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 From 2011-05-09 15:03:53 PST -------
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 From 2011-05-09 15:08:09 PST -------
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 From 2011-05-09 15:29:25 PST -------
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 From 2011-05-09 16:51:19 PST -------
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 From 2011-05-09 16:55:09 PST -------
Yes, that sounds good to me.
------- Comment #18 From 2011-05-09 22:56:40 PST -------
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 From 2011-05-10 01:05:42 PST -------
You don't want to make a branch.  Approximately zero branches of WebKit have ever merged with trunk.
------- Comment #20 From 2011-05-10 10:43:00 PST -------
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 From 2011-05-11 00:15:47 PST -------
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 From 2011-06-09 00:28:52 PST -------
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 From 2011-06-23 05:52:20 PST -------
Created an attachment (id=98339) [details]
Patch v3
------- Comment #24 From 2011-06-23 09:55:43 PST -------
(From update of attachment 98339 [details])
Ok.  I probably would have called this "Draft76" instead of "Hixie76", but I'm not sure it matters that much.
------- Comment #25 From 2011-06-23 09:57:18 PST -------
(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.
------- Comment #26 From 2011-06-24 04:37:12 PST -------
(In reply to comment #25)
> (From update of attachment 98339 [details] [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 From 2011-06-24 05:18:32 PST -------
(From update of attachment 98339 [details])
Clearing flags on attachment: 98339

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