Bug 70972 - [chromium] CachedResourceRequest clobbers the TargetType of ThreadableLoaderClients
Summary: [chromium] CachedResourceRequest clobbers the TargetType of ThreadableLoaderC...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-26 15:31 PDT by Nate Chapin
Modified: 2011-11-01 17:09 PDT (History)
5 users (show)

See Also:


Attachments
patch (1.74 KB, patch)
2011-10-26 15:36 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Add TargetIsUnspecified (5.29 KB, patch)
2011-10-28 14:52 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch for landing (5.52 KB, patch)
2011-11-01 15:31 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2011-10-26 15:31:32 PDT
Originally reported at: http://code.google.com/p/chromium/issues/detail?id=101651

In http://trac.webkit.org/changeset/98380, ThreadableLoaderClients started being loaded through the CachedResource codepath. ThreadableLoaderClients have traditionally had to set their own ResourceRequest::TargetType, but CachedResourceRequest sets it for CachedResource loads. CachedResourceRequest currently clobbers TargetType without checking whether it had been previously set.  That has interesting (and bad) side effects in chromium-specific code.
Comment 1 Nate Chapin 2011-10-26 15:36:28 PDT
Created attachment 112606 [details]
patch

No cq? because I want to ensure this passes the chromium trybots before landing.
Comment 2 Adam Barth 2011-10-26 16:36:31 PDT
Comment on attachment 112606 [details]
patch

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

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:104
> -    resourceRequest.setTargetType(cachedResourceTypeToTargetType(resource->type()));
> +    if (resourceRequest.targetType() == ResourceRequest::TargetIsSubresource)
> +        resourceRequest.setTargetType(cachedResourceTypeToTargetType(resource->type()));

It's too bad there isn't an uninitialized value to compare against.  It's strange that TargetIsSubresource will magically behave differently.
Comment 3 Adam Barth 2011-10-26 16:36:49 PDT
@jochen: Any thoughts on this patch?
Comment 4 Darin Fisher (:fishd, Google) 2011-10-26 22:02:40 PDT
Comment on attachment 112606 [details]
patch

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

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:104
>> +        resourceRequest.setTargetType(cachedResourceTypeToTargetType(resource->type()));
> 
> It's too bad there isn't an uninitialized value to compare against.  It's strange that TargetIsSubresource will magically behave differently.

Agreed.  It seems like TargetIsUnspecified would be a reasonable value to define.
Comment 5 Nate Chapin 2011-10-27 11:20:51 PDT
(In reply to comment #4)
> (From update of attachment 112606 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112606&action=review
> 
> >> Source/WebCore/loader/cache/CachedResourceRequest.cpp:104
> >> +        resourceRequest.setTargetType(cachedResourceTypeToTargetType(resource->type()));
> > 
> > It's too bad there isn't an uninitialized value to compare against.  It's strange that TargetIsSubresource will magically behave differently.
> 
> Agreed.  It seems like TargetIsUnspecified would be a reasonable value to define.

My main objection is that this proposal will turn this patch into a two-sided one.  TargetIsSubresource is the default, and changing the default to a not-yet-defined causes a bunch of assertion failures in browser_tests and interactive_ui_tests.
Comment 6 jochen 2011-10-28 01:00:59 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 112606 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=112606&action=review
> > 
> > >> Source/WebCore/loader/cache/CachedResourceRequest.cpp:104
> > >> +        resourceRequest.setTargetType(cachedResourceTypeToTargetType(resource->type()));
> > > 
> > > It's too bad there isn't an uninitialized value to compare against.  It's strange that TargetIsSubresource will magically behave differently.
> > 
> > Agreed.  It seems like TargetIsUnspecified would be a reasonable value to define.
> 
> My main objection is that this proposal will turn this patch into a two-sided one.  TargetIsSubresource is the default, and changing the default to a not-yet-defined causes a bunch of assertion failures in browser_tests and interactive_ui_tests.

I don't think this needs to be two-sided. We can map TargetIsUnspecified to TargetIsSubresource when converting the ResourceRequest::TargetType to a WebURLRequest::TargetType

I think overall, the patch looks reasonable.

It's a pity that we have TargetType and CachedResourceType with slightly different definitions. I wonder whether it was possible to have all resource requests go through the cache, and replace TargetType by CachedResourceType entirely?
Comment 7 Adam Barth 2011-10-28 01:05:40 PDT
The goal is to have almost all requests go through the cached resource pipeline.  Some (e.g., NetscapePluginStream) won't though.
Comment 8 Nate Chapin 2011-10-28 14:52:13 PDT
Created attachment 112914 [details]
Add TargetIsUnspecified
Comment 9 WebKit Review Bot 2011-10-28 14:54:13 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 10 Darin Fisher (:fishd, Google) 2011-10-30 23:01:19 PDT
Comment on attachment 112914 [details]
Add TargetIsUnspecified

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

OK

> Source/WebKit/chromium/public/WebURLRequest.h:75
> +        TargetIsCue = 15,

It took me a while to figure out what "Cue" refers too.  It seems like it might be
a name that makes sense in some other world?  Video something or other?

> Source/WebCore/ChangeLog:4
> +        ReosurceRequest::TargetType clobbered. They set their own

nit: "ReosurceRequest"
Comment 11 Nate Chapin 2011-10-31 09:44:39 PDT
> It took me a while to figure out what "Cue" refers too.  It seems like it might be
> a name that makes sense in some other world?  Video something or other?

I believe it's related to http://trac.webkit.org/browser/trunk/Source/WebCore/loader/cache/CachedTextTrack.h.  I thought these names had been standardized, but I think the ones in CachedResourceRequest and ResourceRequest may have been missed?
Comment 12 Darin Fisher (:fishd, Google) 2011-10-31 11:11:09 PDT
(In reply to comment #11)
> > It took me a while to figure out what "Cue" refers too.  It seems like it might be
> > a name that makes sense in some other world?  Video something or other?
> 
> I believe it's related to http://trac.webkit.org/browser/trunk/Source/WebCore/loader/cache/CachedTextTrack.h.  I thought these names had been standardized, but I think the ones in CachedResourceRequest and ResourceRequest may have been missed?

So, perhaps "Cue" should be renamed "TextTrack"?
Comment 13 Nate Chapin 2011-11-01 15:31:31 PDT
Created attachment 113238 [details]
Patch for landing
Comment 14 WebKit Review Bot 2011-11-01 17:09:34 PDT
Comment on attachment 113238 [details]
Patch for landing

Clearing flags on attachment: 113238

Committed r99015: <http://trac.webkit.org/changeset/99015>
Comment 15 WebKit Review Bot 2011-11-01 17:09:39 PDT
All reviewed patches have been landed.  Closing bug.