Bug 66167 – [Chromium] Add WebFrameClient::didAdoptURLLoader() notification

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

[Dmitry Titov]

In order to update routing ids for loading requests when iframe is reparented between WebFrames, we need to add a notification to WebFrameClient to tell the WebFrame it now owns a specified resource in progress.

[Dmitry Titov]

Created attachment 103818 [details]
Patch

[Dmitry Titov]

This patch won't compile w/o previous one from bug 66165 since it uses a new RessourceLoader* parameter of FrameLoaderClient::transferLoadingResourceFromPage

[Darin Fisher (:fishd, Google)]

Comment on attachment 103818 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrameClient.h:291
> +    virtual void adoptLoadingResource(WebURLLoader*) { }

Perhaps this should be adoptURLLoader... or, maybe even better:  didAdoptURLLoader.

The second form suggests that this is informative.  The client does not necessarily need to do anything here unless it has special needs.  Chromium has special needs.  DRT does not.

> Source/WebKit/chromium/src/ResourceHandle.cpp:83
> +    static WebURLLoader* WebURLLoaderFromResourceHandle(ResourceHandle* handle) {

did you really mean for this to be a static method on ResourceHandleInternal?

[Dmitry Titov]

Created attachment 103821 [details]
Updated patch

Renamed method to didAdoptURLLoader.

As for static method - I hope there is a better way, I'm using the fact that ResourceHandleInternal is a 'friend' to ResourceHandle, to pull the 'd' member. We currently don't have access to it otherwise. Do I miss something?

[Dmitry Titov]

Created attachment 103822 [details]
Updated ChanegLog

[WebKit Review Bot]

Attachment 103821 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/src/ResourceHandle.cpp:83:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 103822 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/src/ResourceHandle.cpp:83:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dmitry Titov]

Created attachment 103825 [details]
Fixed style error.

[Darin Fisher (:fishd, Google)]

Comment on attachment 103825 [details]
Fixed style error.

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

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1463
> +    WebURLLoader* webURLLoader = WebURLLoaderFromResourceHandle(handle);

The thing that confuses me is that at this callsite, you are not doing ResourceHandleInternal::WebURLLoaderFromResourceHandle.  so, there must be a missing file that defines the global WebURLLoaderFromResourceHandle function?

[Dmitry Titov]

(In reply to comment #9)
> (From update of attachment 103825 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103825&action=review
> 
> > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1463
> > +    WebURLLoader* webURLLoader = WebURLLoaderFromResourceHandle(handle);
> 
> The thing that confuses me is that at this callsite, you are not doing ResourceHandleInternal::WebURLLoaderFromResourceHandle.  so, there must be a missing file that defines the global WebURLLoaderFromResourceHandle function?

Very true. It won't compile. My bad.

How do we crack open ResourceHandle?

[WebKit Review Bot]

Comment on attachment 103825 [details]
Fixed style error.

Attachment 103825 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9378018

[WebKit Review Bot]

Comment on attachment 103825 [details]
Fixed style error.

Attachment 103825 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9357844

[Dmitry Titov]

Created attachment 103850 [details]
Patch

As discussed offline, created ResourceHandleInternal.h in chromium/src.

The impl of the class still lives in ResourceHandle.cpp, I can pull it out into separate file if desired.
Data members of ResourceHandleInternal are made private, needed accessors are added.

The complete fix consists of WebKit patches for bug 66165 and this one, and also Chromium side here: http://codereview.chromium.org/7647003/

[WebKit Review Bot]

Attachment 103850 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/src/ResourceHandleInternal.h:83:  set_owner is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/src/ResourceHandleInternal.h:86:  set_client is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 103850 [details]
Patch

Attachment 103850 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9380061

[WebKit Review Bot]

Comment on attachment 103850 [details]
Patch

Attachment 103850 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9378131

[Darin Fisher (:fishd, Google)]

Comment on attachment 103850 [details]
Patch

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

> Source/WebKit/chromium/src/ResourceHandleInternal.h:99
> +    ConnectionState m_state;

is this still used for anything?  i don't see any way for this to change values.

[Dmitry Titov]

Comment on attachment 103850 [details]
Patch

Will have updated patch in a moment.

[Dmitry Titov]

Created attachment 103938 [details]
Fixed style issue

As for m_state member, it's used internally in implementation of Chromium's ResourceHandleInternal to track the states. I can see it changing values and then asserting on them in WebKit/chromium/src/ResourceHandle.cpp

[WebKit Review Bot]

Comment on attachment 103938 [details]
Fixed style issue

Attachment 103938 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9381846

[WebKit Review Bot]

Comment on attachment 103938 [details]
Fixed style issue

Attachment 103938 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9394617

[Darin Fisher (:fishd, Google)]

Comment on attachment 103938 [details]
Fixed style issue

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

> Source/WebKit/chromium/public/WebFrameClient.h:291
> +    // when iframe is transferred between windows while it is loading something.

"This happens when an iframe, that is loading a subresource, is transferred between windows."

[Dmitry Titov]

Updated and landed manually: http://trac.webkit.org/changeset/93076