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.
Created attachment 112606 [details] patch No cq? because I want to ensure this passes the chromium trybots before landing.
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.
@jochen: Any thoughts on this patch?
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.
(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.
(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?
The goal is to have almost all requests go through the cached resource pipeline. Some (e.g., NetscapePluginStream) won't though.
Created attachment 112914 [details] Add TargetIsUnspecified
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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"
> 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?
(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"?
Created attachment 113238 [details] Patch for landing
Comment on attachment 113238 [details] Patch for landing Clearing flags on attachment: 113238 Committed r99015: <http://trac.webkit.org/changeset/99015>
All reviewed patches have been landed. Closing bug.