Bug 105738

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 Flags
Key3 and Challenge Response fileds to be removed
none
Patch
none
Patch
none
Patch none

Description pdeng6 2012-12-24 23:55:32 PST
Since Key3 and Challenge response field are removed during IETF-websocket spec evolution, and corresponding interfaces are not invoked in current websocket implementation, I think remove them from web-inspector is reason-able.
@Toyoshima san, I guess they won't be back in future, am I right?

origin bug from https://code.google.com/p/chromium/issues/detail?id=142280
Comment 1 Takashi Toyoshima 2012-12-25 00:25:12 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.
Comment 2 pdeng6 2012-12-25 00:31:17 PST
(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.
Comment 3 pdeng6 2012-12-25 06:14:22 PST
Created attachment 180711 [details]
Key3 and Challenge Response fileds to be removed
Comment 4 pdeng6 2012-12-25 06:22:28 PST
Created attachment 180712 [details]
Patch
Comment 5 pdeng6 2012-12-28 04:43:52 PST
@Pavel, could you pelase help review this patch? 
thanks :)

Pan
Comment 6 Pavel Feldman 2012-12-28 04:53:46 PST
(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.
Comment 7 Li Yin 2013-01-14 21:21:05 PST
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?
Comment 8 Takashi Toyoshima 2013-01-14 22:13:52 PST
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,
Comment 9 pdeng6 2013-01-14 22:19:51 PST
Created attachment 182704 [details]
Patch
Comment 10 pdeng6 2013-01-14 23:31:59 PST
(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 11 Kent Tamura 2013-01-14 23:35:35 PST
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.
Comment 12 pdeng6 2013-01-15 00:32:34 PST
Created attachment 182712 [details]
Patch
Comment 13 pdeng6 2013-01-15 00:35:00 PST
(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 14 Kent Tamura 2013-01-15 01:19:15 PST
Comment on attachment 182712 [details]
Patch

Looks good.
Comment 15 WebKit Review Bot 2013-01-15 17:50:02 PST
Comment on attachment 182712 [details]
Patch

Clearing flags on attachment: 182712

Committed r139814: <http://trac.webkit.org/changeset/139814>
Comment 16 WebKit Review Bot 2013-01-15 17:50:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Takashi Toyoshima 2013-01-15 21:06:36 PST
Pan, thank you for fixing this issue.
Comment 18 pdeng6 2013-01-15 21:44:17 PST
(In reply to comment #17)
> Pan, thank you for fixing this issue.

you are welcome :)