Bug 60348

Created attachment 92555 [details] Patch

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 on attachment 92555 [details] Patch Can we access (per page) Settings if WebSocket is used in worker context?

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.

(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.

(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.

(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.

In WebKit, we always special case "old", not "new".

(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.

Created attachment 92762 [details] Patch v2

> 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.

(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.

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.

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.

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.

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.)

Yes, that sounds good to me.

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...)

You don't want to make a branch. Approximately zero branches of WebKit have ever merged with trunk.

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

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...

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.

Created attachment 98339 [details] Patch v3

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 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.

(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 on attachment 98339 [details] Patch v3 Clearing flags on attachment: 98339 Committed r89669: <http://trac.webkit.org/changeset/89669>

All reviewed patches have been landed. Closing bug.