Bug 116178 - Remove WebSocketHandshakeRequest class
Summary: Remove WebSocketHandshakeRequest class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anders Carlsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-15 12:44 PDT by Anders Carlsson
Modified: 2013-05-17 12:06 PDT (History)
6 users (show)

See Also:


Attachments
Patch (30.25 KB, patch)
2013-05-15 12:45 PDT, Anders Carlsson
no flags Details | Formatted Diff | Diff
Patch (29.47 KB, patch)
2013-05-15 13:06 PDT, Anders Carlsson
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Carlsson 2013-05-15 12:44:17 PDT
Remove WebSocketHandshakeRequest class
Comment 1 Anders Carlsson 2013-05-15 12:45:50 PDT
Created attachment 201867 [details]
Patch
Comment 2 Early Warning System Bot 2013-05-15 12:58:26 PDT
Comment on attachment 201867 [details]
Patch

Attachment 201867 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/476631
Comment 3 EFL EWS Bot 2013-05-15 13:02:42 PDT
Comment on attachment 201867 [details]
Patch

Attachment 201867 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/473621
Comment 4 Anders Carlsson 2013-05-15 13:06:38 PDT
Created attachment 201871 [details]
Patch
Comment 5 Andreas Kling 2013-05-15 13:20:24 PDT
Comment on attachment 201871 [details]
Patch

I'ma go ahead and trust you on this one, Carlsson.
Comment 6 Anders Carlsson 2013-05-15 13:21:53 PDT
Committed r150142: <http://trac.webkit.org/changeset/150142>
Comment 7 Alexey Proskuryakov 2013-05-15 19:59:55 PDT
> Turns out WebSocketHandshakeRequest is just used by the web inspector, and there's no reason
why we can't just use a ResourceRequest instead.

This is not accurate. WebSocket has different semantics than HTTP, in particular, order of header fields matters.

I'm puzzled that you are making such changes without any attempt of talking to me. Andreas, I suggest waiting at least 24 hours before reviewing patches in areas you are not sure that you are an expert in.
Comment 8 Andreas Kling 2013-05-16 09:58:30 PDT
(In reply to comment #7)
> > Turns out WebSocketHandshakeRequest is just used by the web inspector, and there's no reason
> why we can't just use a ResourceRequest instead.
> 
> This is not accurate. WebSocket has different semantics than HTTP, in particular, order of header fields matters.
> 
> I'm puzzled that you are making such changes without any attempt of talking to me. Andreas, I suggest waiting at least 24 hours before reviewing patches in areas you are not sure that you are an expert in.

Alexey, I trusted Anders's judgment on this change and it looked like a straightforward removal of an unnecessary abstraction. I set my r+ without fully understanding the implications, and for that I apologize.
Comment 9 Alexey Proskuryakov 2013-05-17 12:06:08 PDT
> order of header fields matters

This is no longer the case, the spec no longer requires that. I could not quickly find any other differences that would require using a distinct class.