Use WebFrame::createAssociatedURLLoader....

Use WebFrame::createAssociatedURLLoader.

See https://bugs.webkit.org/show_bug.cgi?id=49764 for the
WebKit side of this change.

R=michaeln
BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69146

[michaeln, 2010-12-07 00:27:31]

http://codereview.chromium.org/5139005/diff/1/webkit/glue/resource_fetcher.cc
File webkit/glue/resource_fetcher.cc (right):

http://codereview.chromium.org/5139005/diff/1/webkit/glue/resource_fetcher.cc...
webkit/glue/resource_fetcher.cc:50:
loader_->loadAsynchronously(WebURLRequest(url_), this);
I wonder about ResourceFetcher resource loads now being subject to the appcache
loading logic where they aren't currently (appcache logic intentionally fails
requests for resources that are not in the cache and are not in a network
namespace). The change in behavior may trip some things up.

For example, the RenderView uses this class to load a chromeapp's manifest file.

  app_definition_fetcher_.reset(new ResourceFetcher(
      pending_app_info_->manifest_url, webview()->mainFrame(),
      NewCallback(this, &RenderView::DidDownloadApplicationDefinition)));

I'd definitely be more comfortable if we had a mode where if the resource is
avail in the appcache its utilized, but a missing resource wouldn't result in an
error... and that mode was used at this callsite.

Also, the 'mainFrame' is used in all cases when RenderView::DownloadImage is
called, do you know if DownloadImage is ever called on behalf of any other
frames?

[darin (slow to review), 2010-12-07 00:29:31]

How about I add a parameter to createAssociatedURLLoader that disables
appcache use?

-Darin

On Mon, Dec 6, 2010 at 4:27 PM, <michaeln@chromium.org> wrote:

>
>
> http://codereview.chromium.org/5139005/diff/1/webkit/glue/resource_fetcher.cc
> File webkit/glue/resource_fetcher.cc (right):
>
>
>
http://codereview.chromium.org/5139005/diff/1/webkit/glue/resource_fetcher.cc...
> webkit/glue/resource_fetcher.cc:50:
> loader_->loadAsynchronously(WebURLRequest(url_), this);
> I wonder about ResourceFetcher resource loads now being subject to the
> appcache loading logic where they aren't currently (appcache logic
> intentionally fails requests for resources that are not in the cache and
> are not in a network namespace). The change in behavior may trip some
> things up.
>
> For example, the RenderView uses this class to load a chromeapp's
> manifest file.
>
>  app_definition_fetcher_.reset(new ResourceFetcher(
>      pending_app_info_->manifest_url, webview()->mainFrame(),
>      NewCallback(this,
> &RenderView::DidDownloadApplicationDefinition)));
>
> I'd definitely be more comfortable if we had a mode where if the
> resource is avail in the appcache its utilized, but a missing resource
> wouldn't result in an error... and that mode was used at this callsite.
>
> Also, the 'mainFrame' is used in all cases when
> RenderView::DownloadImage is called, do you know if DownloadImage is
> ever called on behalf of any other frames?
>
> http://codereview.chromium.org/5139005/
>

[michaeln, 2010-12-07 02:11:46]

On 2010/12/07 00:29:31, darin wrote:
> How about I add a parameter to createAssociatedURLLoader that disables
> appcache use?

So no change in behavior at all for now... that sounds like a good idea.

[darin (slow to review), 2010-12-14 00:14:52]

I just went ahead and removed the resource_fetcher.cc changes entirely.  Now
that I think about it, I suspect that most consumers of that API also wouldn't
want window.stop() to interrupt their network requests.  We may want a different
solution for ResourceFetcher.

[michaeln, 2010-12-14 00:18:56]

LGTM, I buy that!

On 2010/12/14 00:14:52, darin wrote:
> I just went ahead and removed the resource_fetcher.cc changes entirely.  Now
> that I think about it, I suspect that most consumers of that API also wouldn't
> want window.stop() to interrupt their network requests.  We may want a
different
> solution for ResourceFetcher.

[darin (slow to review), 2010-12-14 17:23:07]

oops, this caused some layout tests to fail:

Regressions: Unexpected tests timed out : (6)
  http/tests/plugins/get-url.html = TIMEOUT
  http/tests/plugins/npapi-response-headers.html = TIMEOUT
  http/tests/plugins/third-party-cookie-accept-policy.html = TIMEOUT
  plugins/destroy-stream-twice.html = TIMEOUT
  plugins/document-open.html = TIMEOUT
  plugins/return-error-from-new-stream-doesnt-invoke-destroy-stream.html =
TIMEOUT

So, I had to revert the change.

[michaeln, 2010-12-14 20:24:57]

Doh!

Do you know why... is this because the change to the relative ordering of...
  devtools_agent->willSendRequest(resource_id, info.request);
vs
  webframe_->dispatchWillSendRequest(info.request);

i see you pinged pavel on the revert

[darin (slow to review), 2010-12-14 22:57:39]

It is because AssociatedURLLoader allows m_realLoader to be passed to the
WebURLLoaderClient.  That confuses the plugin code, which tracks
WebURLLoader instances by address.

-Darin



On Tue, Dec 14, 2010 at 12:24 PM, <michaeln@chromium.org> wrote:

> Doh!
>
> Do you know why... is this because the change to the relative ordering
> of...
>  devtools_agent->willSendRequest(resource_id, info.request);
> vs
>  webframe_->dispatchWillSendRequest(info.request);
>
> i see you pinged pavel on the revert
>
>
> http://codereview.chromium.org/5139005/
>

[darin (slow to review), 2010-12-14 23:02:56]

See:  https://bugs.webkit.org/show_bug.cgi?id=51062

On Tue, Dec 14, 2010 at 2:57 PM, Darin Fisher <darin@chromium.org> wrote:

> It is because AssociatedURLLoader allows m_realLoader to be passed to the
> WebURLLoaderClient.  That confuses the plugin code, which tracks
> WebURLLoader instances by address.
>
> -Darin
>
>
>
> On Tue, Dec 14, 2010 at 12:24 PM, <michaeln@chromium.org> wrote:
>
>> Doh!
>>
>> Do you know why... is this because the change to the relative ordering
>> of...
>>  devtools_agent->willSendRequest(resource_id, info.request);
>> vs
>>  webframe_->dispatchWillSendRequest(info.request);
>>
>> i see you pinged pavel on the revert
>>
>>
>> http://codereview.chromium.org/5139005/
>>
>
>