Summary: | Remove Key3 & Challenge Response from WebInspector-NetworkPanel-HeaderTab for websocket | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | pdeng6 <pan.deng> | ||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | li.yin, pfeldman, tkent, toyoshim, webkit.review.bot, yutak | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
pdeng6
2012-12-24 23:55:32 PST
Yes. It's time to remove these obsolete fields. Now that all WebKit ports migrate to the latest WebSocket protocol spec, RFC 6455, these fields are not used forever. (In reply to comment #1) > Yes. It's time to remove these obsolete fields. > Now that all WebKit ports migrate to the latest WebSocket protocol spec, RFC 6455, these fields are not used forever. thanks :), I will prepare a patch. Created attachment 180711 [details]
Key3 and Challenge Response fileds to be removed
Created attachment 180712 [details]
Patch
@Pavel, could you pelase help review this patch? thanks :) Pan (In reply to comment #1) > Yes. It's time to remove these obsolete fields. > Now that all WebKit ports migrate to the latest WebSocket protocol spec, RFC 6455, these fields are not used forever. Could you clarify whether ports have migrated to the latest protocol? Any new fields need to be inspected? Why are these still in the WebCore APIs? I would imagine that when they are removed, inspector's code will get fixed automatically. See https://bugs.webkit.org/show_bug.cgi?id=88620 hixie76 protocol related implementation should be deleted. Hi pan, Could you please delete Key3 related codes in WebCore also? Sorry, I missed the comment #6. Yes, as Li said, old protocol doesn't work now, then all ports have no choice to use old protocol in WebKit. Issue 88620 removed major parts of old protocol implementation. But it wasn't perfect. Removing other hixie76 implementation looks fine. Please git grep with 'useHixie76Protocol' in WebCore. Also layout tests in http/tests/websocket/tests/hixie76 can be removed. Deng: If you don't have a time to clean them up, please let me know. I'll do. Thanks, Created attachment 182704 [details]
Patch
(In reply to comment #8) > Sorry, I missed the comment #6. > > Yes, as Li said, old protocol doesn't work now, then all ports have no choice to use old protocol in WebKit. > Issue 88620 removed major parts of old protocol implementation. But it wasn't perfect. > Removing other hixie76 implementation looks fine. > > Please git grep with 'useHixie76Protocol' in WebCore. > Also layout tests in http/tests/websocket/tests/hixie76 can be removed. > > Deng: > If you don't have a time to clean them up, please let me know. I'll do. > > Thanks, thanks, obsoleted fields in WebCore are removed in this patch, I think it is clean as my grep. :) @tkent, @Pavel, could you please review this change? thanks Pan Comment on attachment 182704 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182704&action=review > Source/WebCore/ChangeLog:5 > + Need the bug URL (OOPS!). OOPS! Remove this line. > Source/WebCore/ChangeLog:12 > + No new tests (OOPS!). OOPS!. Please replace this OOPS! with a reason why the patch has no tests. > Source/WebCore/ChangeLog:31 > + * Modules/websockets/WebSocketHandshakeRequest.cpp: > + * Modules/websockets/WebSocketHandshakeRequest.h: > + (WebSocketHandshakeRequest): > + * Modules/websockets/WebSocketHandshakeResponse.cpp: > + * Modules/websockets/WebSocketHandshakeResponse.h: > + (WebSocketHandshakeResponse): > + * inspector/Inspector.json: > + * inspector/InspectorResourceAgent.cpp: > + (WebCore): > + (WebCore::InspectorResourceAgent::willSendWebSocketHandshakeRequest): > + (WebCore::InspectorResourceAgent::didReceiveWebSocketHandshakeResponse): > + * inspector/front-end/NetworkManager.js: > + (WebInspector.NetworkDispatcher.prototype.webSocketWillSendHandshakeRequest): > + (WebInspector.NetworkDispatcher.prototype.webSocketHandshakeResponseReceived): > + * inspector/front-end/RequestHeadersView.js: > + (WebInspector.RequestHeadersView.prototype._refreshRequestHeaders): > + (WebInspector.RequestHeadersView.prototype._refreshResponseHeaders): > + (WebInspector.RequestHeadersView.prototype._refreshHeaders): Please write per-file / per-function comments as possible. Created attachment 182712 [details]
Patch
(In reply to comment #11) > (From update of attachment 182704 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=182704&action=review > > > Source/WebCore/ChangeLog:5 > > + Need the bug URL (OOPS!). > > OOPS! Remove this line. > Done, thanks. > > Source/WebCore/ChangeLog:12 > > + No new tests (OOPS!). > > OOPS!. Please replace this OOPS! with a reason why the patch has no tests. > thanks, done. > > Source/WebCore/ChangeLog:31 > > + * Modules/websockets/WebSocketHandshakeRequest.cpp: > > + * Modules/websockets/WebSocketHandshakeRequest.h: > > + (WebSocketHandshakeRequest): > > + * Modules/websockets/WebSocketHandshakeResponse.cpp: > > + * Modules/websockets/WebSocketHandshakeResponse.h: > > + (WebSocketHandshakeResponse): > > + * inspector/Inspector.json: > > + * inspector/InspectorResourceAgent.cpp: > > + (WebCore): > > + (WebCore::InspectorResourceAgent::willSendWebSocketHandshakeRequest): > > + (WebCore::InspectorResourceAgent::didReceiveWebSocketHandshakeResponse): > > + * inspector/front-end/NetworkManager.js: > > + (WebInspector.NetworkDispatcher.prototype.webSocketWillSendHandshakeRequest): > > + (WebInspector.NetworkDispatcher.prototype.webSocketHandshakeResponseReceived): > > + * inspector/front-end/RequestHeadersView.js: > > + (WebInspector.RequestHeadersView.prototype._refreshRequestHeaders): > > + (WebInspector.RequestHeadersView.prototype._refreshResponseHeaders): > > + (WebInspector.RequestHeadersView.prototype._refreshHeaders): > > Please write per-file / per-function comments as possible. major comments added, thanks :) Comment on attachment 182712 [details]
Patch
Looks good.
Comment on attachment 182712 [details] Patch Clearing flags on attachment: 182712 Committed r139814: <http://trac.webkit.org/changeset/139814> All reviewed patches have been landed. Closing bug. Pan, thank you for fixing this issue. (In reply to comment #17) > Pan, thank you for fixing this issue. you are welcome :) |