[webkit-reviews] review denied: [Bug 78459] [Chromium] [WebSocket] provide WebFrameClient with a chance to handle ExtraData of opening WebSocketStreamHandle : [Attachment 127955] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Takashi Toyoshima
<toyoshim at chromium.org>'s request for review:
Bug 78459: [Chromium] [WebSocket] provide WebFrameClient with a chance to
handle ExtraData of opening WebSocketStreamHandle
https://bugs.webkit.org/show_bug.cgi?id=78459

Attachment 127955: Patch
https://bugs.webkit.org/attachment.cgi?id=127955&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=127955&action=review


> Source/WebCore/loader/FrameLoaderClient.h:337
> +	   virtual void willOpenSocketStream(SocketStreamHandle*) { }

on this interface, methods like this are typically prefixed with "dispatch"
see for example dispatchWillSendRequest.

> Source/WebKit/chromium/public/platform/WebSocketStreamHandle.h:44
> +

nit: no new line here

> Source/WebKit/chromium/public/platform/WebSocketStreamHandle.h:62
> +    ExtraData* extraData() const { return 0; }

I just realized that the WebSocketStreamHandle is allocated by the embedder.
This implies that the embedder could also downcast WebSocketStreamHandle
implementations to the Impl class (or some other intermediate class), which
could provide methods for setting extra data fields.

WebURLRequest works differently.  There, WebKit implements WebURLRequest,
and so it is necessary for it to provide the ExtraData facility.

Maybe my advice to have you extend WebSocketStreamHandle like this was
actually not so good...  Maybe all you really need to do is plumb the
willOpenSocketStream call.