Bug 78459 - [Chromium] [WebSocket] provide WebFrameClient with a chance of accessing to opening WebSocketStreamHandle
Summary: [Chromium] [WebSocket] provide WebFrameClient with a chance of accessing to o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Takashi Toyoshima
URL:
Keywords:
Depends on: 78460 78576 78581
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-12 22:26 PST by Takashi Toyoshima
Modified: 2012-03-21 07:01 PDT (History)
9 users (show)

See Also:


Attachments
to show overview to dependent change reviewers (has dependencies on them, so not ready for review) (7.20 KB, patch)
2012-02-13 00:42 PST, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
patch candidate (depends on other dependent changes) (15.46 KB, patch)
2012-02-14 02:12 PST, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (15.58 KB, patch)
2012-02-15 00:40 PST, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (16.23 KB, patch)
2012-02-21 05:03 PST, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (15.94 KB, patch)
2012-02-22 06:16 PST, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (13.27 KB, patch)
2012-02-28 15:24 PST, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (13.26 KB, patch)
2012-03-09 17:19 PST, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
just rebase (13.24 KB, patch)
2012-03-12 12:37 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
rename some methods to have dispatch prefix, too (13.31 KB, patch)
2012-03-12 16:18 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (13.28 KB, patch)
2012-03-13 16:33 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (13.27 KB, patch)
2012-03-19 13:11 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
rebase (13.33 KB, patch)
2012-03-21 06:27 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Toyoshima 2012-02-12 22:26:53 PST
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
Comment 1 Takashi Toyoshima 2012-02-13 00:42:57 PST
Created attachment 126731 [details]
to show overview to dependent change reviewers (has dependencies on them, so not ready for review)
Comment 2 Takashi Toyoshima 2012-02-14 02:12:11 PST
Created attachment 126939 [details]
patch candidate (depends on other dependent changes)
Comment 3 Takashi Toyoshima 2012-02-14 02:16:01 PST
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
Comment 4 Takashi Toyoshima 2012-02-15 00:40:17 PST
Created attachment 127126 [details]
Patch
Comment 5 Kent Tamura 2012-02-15 01:26:01 PST
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?
Comment 6 WebKit Review Bot 2012-02-15 03:54:34 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 7 Darin Fisher (:fishd, Google) 2012-02-16 11:34:21 PST
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.
Comment 8 Takashi Toyoshima 2012-02-21 04:44:21 PST
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().
Comment 9 Takashi Toyoshima 2012-02-21 05:03:18 PST
Created attachment 127955 [details]
Patch
Comment 10 Kent Tamura 2012-02-21 16:06:48 PST
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.
Comment 11 Darin Fisher (:fishd, Google) 2012-02-21 16:17:19 PST
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.
Comment 12 Takashi Toyoshima 2012-02-22 06:16:03 PST
Created attachment 128198 [details]
Patch
Comment 13 Takashi Toyoshima 2012-02-22 06:16:40 PST
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.
Comment 14 Takashi Toyoshima 2012-02-27 14:13:19 PST
Hi Darin,
Could you review on my last patch?
Comment 15 Adam Barth 2012-02-28 14:19:31 PST
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.
Comment 16 Takashi Toyoshima 2012-02-28 14:44:06 PST
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.
Comment 17 Takashi Toyoshima 2012-02-28 15:24:09 PST
Created attachment 129343 [details]
Patch
Comment 18 Takashi Toyoshima 2012-03-09 17:19:15 PST
Created attachment 131139 [details]
Patch
Comment 19 Takashi Toyoshima 2012-03-09 17:21:30 PST
Rebase to the latest code and fix a typo in ChangeLog.

Darin, could you review this change including API revision again?
Comment 20 Takashi Toyoshima 2012-03-12 12:37:47 PDT
Created attachment 131382 [details]
just rebase
Comment 21 Takashi Toyoshima 2012-03-12 16:18:42 PDT
Created attachment 131441 [details]
rename some methods to have dispatch prefix, too
Comment 22 Takashi Toyoshima 2012-03-12 16:22:58 PDT
Hi, Darin.
Now, I notice that I must apply the same renaming to other methods.
Sorry for narrow understanding on your comments.
Comment 23 Darin Fisher (:fishd, Google) 2012-03-13 16:11:12 PDT
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.
Comment 24 Takashi Toyoshima 2012-03-13 16:33:40 PDT
Created attachment 131747 [details]
Patch
Comment 25 Takashi Toyoshima 2012-03-13 16:34:44 PDT
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.
Comment 26 Takashi Toyoshima 2012-03-19 13:11:12 PDT
Created attachment 132638 [details]
Patch
Comment 27 Takashi Toyoshima 2012-03-19 13:14:05 PDT
Upload a new CL.
Nothing is different from the previous one.
I just perform rebase.
Comment 28 Darin Fisher (:fishd, Google) 2012-03-19 13:43:22 PDT
Comment on attachment 132638 [details]
Patch

R=me for WebKit API changes.
Comment 29 Takashi Toyoshima 2012-03-19 14:00:41 PDT
Thank you Darin.

Kent-san, could you review other parts except WebKit API?
Comment 30 Kent Tamura 2012-03-20 18:33:20 PDT
Comment on attachment 132638 [details]
Patch

ok
Comment 31 WebKit Review Bot 2012-03-20 18:36:27 PDT
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
Comment 32 Takashi Toyoshima 2012-03-21 06:27:57 PDT
Created attachment 133033 [details]
rebase
Comment 33 WebKit Review Bot 2012-03-21 06:29:25 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 34 WebKit Review Bot 2012-03-21 07:00:53 PDT
Comment on attachment 133033 [details]
rebase

Clearing flags on attachment: 133033

Committed r111537: <http://trac.webkit.org/changeset/111537>
Comment 35 WebKit Review Bot 2012-03-21 07:01:01 PDT
All reviewed patches have been landed.  Closing bug.