Bug 105738 – Remove Key3 & Challenge Response from WebInspector-NetworkPanel-HeaderTab for websocket

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[pdeng6]

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

[Takashi Toyoshima]

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.

[pdeng6]

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

[pdeng6]

Created attachment 180711 [details]
Key3 and Challenge Response fileds to be removed

[pdeng6]

Created attachment 180712 [details]
Patch

[pdeng6]

@Pavel, could you pelase help review this patch? 
thanks :)

Pan

[Pavel Feldman]

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

[Li Yin]

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?

[Takashi Toyoshima]

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,

[pdeng6]

Created attachment 182704 [details]
Patch

[pdeng6]

(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

[Kent Tamura]

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.

[pdeng6]

Created attachment 182712 [details]
Patch

[pdeng6]

(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 :)

[Kent Tamura]

Comment on attachment 182712 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 182712 [details]
Patch

Clearing flags on attachment: 182712

Committed r139814: <http://trac.webkit.org/changeset/139814>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Takashi Toyoshima]

Pan, thank you for fixing this issue.

[pdeng6]

(In reply to comment #17)
> Pan, thank you for fixing this issue.

you are welcome :)