Bug 78459 – [Chromium] [WebSocket] provide WebFrameClient with a chance of accessing to opening WebSocketStreamHandle

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Takashi Toyoshima]

In Chromium port, we should bind a WebSocket object with its owner RenderView to share RenderView wide information between the objects.
RenderView implements WebFrameClient, so if a WebSocketStreamHandle object has a reference to the owner WebFrame object, it means that it also has a chance to call RenderView functions via WebFrameClient delegate.
Then, the RenderView could set owner RenderView information to WebSocketStreamHandle at the delegate function.

This is chromium side issue.
http://code.google.com/p/chromium/issues/detail?id=53836

[Takashi Toyoshima]

Created attachment 126731 [details]
to show overview to dependent change reviewers (has dependencies on them, so not ready for review)

[Takashi Toyoshima]

Created attachment 126939 [details]
patch candidate (depends on other dependent changes)

[Takashi Toyoshima]

The second patch will work with 78576 and 78581.

The whole change containing this CL and other dependent CLs could be shown in following CL.
https://codereview.appspot.com/5664047

[Takashi Toyoshima]

Created attachment 127126 [details]
Patch

[Kent Tamura]

Comment on attachment 127126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127126&action=review

> Source/WebCore/platform/network/chromium/SocketStreamHandle.h:69
> +        friend class FrameLoaderClientImpl;

Why is this needed?

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 127126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127126&action=review

> Source/WebKit/chromium/public/platform/WebSocketStreamHandle.h:55
> +    // Set requestor ID binded to RenderView.

nit: RenderView in the WebKit tree means something totally different.  See WebCore::RenderView.
It is really best to avoid mentioning Chrome-isms for this reason.

Can you please implement this using the ExtraData approach instead?  I know it is more work,
but we have a pending cleanup work item to remove WebURLRequest::requestorID.  The issue here
is that we don't want to pollute WebKit with things that really don't concern it.  requestorID
is a good example of that.

[Takashi Toyoshima]

Comment on attachment 127126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127126&action=review

>> Source/WebCore/platform/network/chromium/SocketStreamHandle.h:69
>> +        friend class FrameLoaderClientImpl;
> 
> Why is this needed?

Sorry, I mistakenly inherit this from an old experimental implementation.
I remove changes on this file.

>> Source/WebKit/chromium/public/platform/WebSocketStreamHandle.h:55
>> +    // Set requestor ID binded to RenderView.
> 
> nit: RenderView in the WebKit tree means something totally different.  See WebCore::RenderView.
> It is really best to avoid mentioning Chrome-isms for this reason.
> 
> Can you please implement this using the ExtraData approach instead?  I know it is more work,
> but we have a pending cleanup work item to remove WebURLRequest::requestorID.  The issue here
> is that we don't want to pollute WebKit with things that really don't concern it.  requestorID
> is a good example of that.

Thank you.
I define ExtraData internal class and change these functions to setExtraData()/extraData().

[Takashi Toyoshima]

Created attachment 127955 [details]
Patch

[Kent Tamura]

Comment on attachment 127955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127955&action=review

> Source/WebCore/websockets/WebSocketChannel.cpp:274
> +    m_document->frame()->loader()->willOpenSocketStream(handle);

m_document->frame() can be NULL.

[Darin Fisher (:fishd, Google)]

Comment on attachment 127955 [details]
Patch

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.

[Takashi Toyoshima]

Created attachment 128198 [details]
Patch

[Takashi Toyoshima]

Comment on attachment 127955 [details]
Patch

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.

I see.
Fixed.

>> Source/WebCore/websockets/WebSocketChannel.cpp:274
>> +    m_document->frame()->loader()->willOpenSocketStream(handle);
> 
> m_document->frame() can be NULL.

I added pointer check.

>> Source/WebKit/chromium/public/platform/WebSocketStreamHandle.h:44
>> +
> 
> nit: no new line here

Fixed.

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

I see.
OK, I remove ExtraData related changes and will implement similar functions in chromium side impl class.
Is this ExtraData approach overkill as a chromium side implementation approach?

I think removing pure virtual functions is still useful.
I'll remain them in the next CL.

[Takashi Toyoshima]

Hi Darin,
Could you review on my last patch?

[Adam Barth]

Comment on attachment 128198 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128198&action=review

> Source/WebCore/loader/FrameLoader.cpp:1422
> +void FrameLoader::willOpenSocketStream(SocketStreamHandle* handle)
> +{
> +    m_client->dispatchWillOpenSocketStream(handle);
> +}

We don't require these wrapper functions in FrameLoader anymore.  Folks can just call functions on loader()->client() directly.

[Takashi Toyoshima]

Comment on attachment 128198 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128198&action=review

>> Source/WebCore/loader/FrameLoader.cpp:1422
>> +}
> 
> We don't require these wrapper functions in FrameLoader anymore.  Folks can just call functions on loader()->client() directly.

Thanks.
I remove this wrapper and call client function from WebSocketChannel directly.

[Takashi Toyoshima]

Created attachment 129343 [details]
Patch

[Takashi Toyoshima]

Created attachment 131139 [details]
Patch

[Takashi Toyoshima]

Rebase to the latest code and fix a typo in ChangeLog.

Darin, could you review this change including API revision again?

[Takashi Toyoshima]

Created attachment 131382 [details]
just rebase

[Takashi Toyoshima]

Created attachment 131441 [details]
rename some methods to have dispatch prefix, too

[Takashi Toyoshima]

Hi, Darin.
Now, I notice that I must apply the same renaming to other methods.
Sorry for narrow understanding on your comments.

[Darin Fisher (:fishd, Google)]

Comment on attachment 131441 [details]
rename some methods to have dispatch prefix, too

View in context: https://bugs.webkit.org/attachment.cgi?id=131441&action=review

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:277
> +      frame->loader()->client()->dispatchWillOpenSocketStream(handle);

nit: indentation

nit: Document::frame() is a very cheap inline getter.  Better to just do:

   if (m_document->frame())
       m_document->frame()->loader()->client()->dispatch...

> Source/WebKit/chromium/public/WebFrameClient.h:392
> +    virtual void dispatchWillOpenSocketStream(WebSocketStreamHandle*) { }

on WebFrameClient, we drop the "dispatch" prefix.  that prefix is for
FrameLoaderClient methods.  it is telling the FrameLoaderClient to 
dispatch the "Foo" event, so if FrameLoaderClient has dispatchFoo,
then WebFrameClient should have a "foo" method.

[Takashi Toyoshima]

Created attachment 131747 [details]
Patch

[Takashi Toyoshima]

Comment on attachment 131441 [details]
rename some methods to have dispatch prefix, too

View in context: https://bugs.webkit.org/attachment.cgi?id=131441&action=review

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:277
>> +      frame->loader()->client()->dispatchWillOpenSocketStream(handle);
> 
> nit: indentation
> 
> nit: Document::frame() is a very cheap inline getter.  Better to just do:
> 
>    if (m_document->frame())
>        m_document->frame()->loader()->client()->dispatch...

Done.
Sorry for poor nits.

>> Source/WebKit/chromium/public/WebFrameClient.h:392
>> +    virtual void dispatchWillOpenSocketStream(WebSocketStreamHandle*) { }
> 
> on WebFrameClient, we drop the "dispatch" prefix.  that prefix is for
> FrameLoaderClient methods.  it is telling the FrameLoaderClient to 
> dispatch the "Foo" event, so if FrameLoaderClient has dispatchFoo,
> then WebFrameClient should have a "foo" method.

I see. Thank you for explaining.
I mistakenly followed upper dispatchIntent name.
I fixed this name again now.

[Takashi Toyoshima]

Created attachment 132638 [details]
Patch

[Takashi Toyoshima]

Upload a new CL.
Nothing is different from the previous one.
I just perform rebase.

[Darin Fisher (:fishd, Google)]

Comment on attachment 132638 [details]
Patch

R=me for WebKit API changes.

[Takashi Toyoshima]

Thank you Darin.

Kent-san, could you review other parts except WebKit API?

[Kent Tamura]

Comment on attachment 132638 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 132638 [details]
Patch

Rejecting attachment 132638 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 at 78.
Hunk #4 succeeded at 1623 (offset -26 lines).
2 out of 4 hunks FAILED -- saving rejects to file Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp.rej
patching file Source/WebKit/chromium/src/FrameLoaderClientImpl.h
Hunk #2 succeeded at 210 (offset -2 lines).
patching file Source/WebKit/chromium/src/SocketStreamHandle.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kent Tamura']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12035155

[Takashi Toyoshima]

Created attachment 133033 [details]
rebase

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 133033 [details]
rebase

Clearing flags on attachment: 133033

Committed r111537: <http://trac.webkit.org/changeset/111537>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.