Bug 71149 – Merge CachedResourceRequest and SubresourceLoader, delete SubresourceClient

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

[Nate Chapin]

As of http://trac.webkit.org/changeset/98380, CachedResourceRequest is the only SubresourceLoaderClient.  SubresourceLoader does little more than call its client.  Ergo, merge them.

Call the merged class SubresourceLoader, because CachedResourceRequest is the worst naming decision I've made on this project.

[Nate Chapin]

Created attachment 112923 [details]
Attempt #1

[WebKit Review Bot]

Comment on attachment 112923 [details]
Attempt #1

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

[Nate Chapin]

Created attachment 113070 [details]
Address compile failure

[WebKit Review Bot]

Comment on attachment 113070 [details]
Address compile failure

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

New failing tests:
http/tests/inspector/resource-tree/resource-tree-reload.html

[Nate Chapin]

Created attachment 114360 [details]
Fix chromium test failure + merge to trunk

[Adam Barth]

Comment on attachment 114360 [details]
Fix chromium test failure + merge to trunk

This looks great.  A couple ideas for a follow-up patch:

1) Add some ASSERTS to the destructor that show we properly advance the state machine to the end (and that documenting becomes null).
2) Use a member variable to track whether we've incremented/decremented the request count to ensure we balance them properly (e.g., by ASSERTing in its destructor that we've balanced).  That will also remove a bunch of multipart related banches because we can tell this object to decrement the request count if it hasn't already done so.

[WebKit Review Bot]

Comment on attachment 114360 [details]
Fix chromium test failure + merge to trunk

Rejecting attachment 114360 [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:
ubresourceLoaderClient.h
rm 'Source/WebCore/loader/SubresourceLoaderClient.h'
patching file Source/WebCore/loader/cf/SubresourceLoaderCF.cpp
patching file Source/WebCore/loader/ResourceLoader.cpp
patching file Source/WebCore/loader/ResourceLoadScheduler.h
patching file Source/WebCore/loader/SubresourceLoader.cpp
patching file Source/WebCore/loader/SubresourceLoader.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10372598

[Nate Chapin]

Created attachment 115194 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 115194 [details]
Patch for landing

Clearing flags on attachment: 115194

Committed r100311: <http://trac.webkit.org/changeset/100311>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Peter Kasting]

This patch seems to have caused http://code.google.com/p/chromium/issues/detail?id=104582 downstream.

[Vsevolod Vlasov]

This patch caused http://code.google.com/p/chromium/issues/detail?id=104920 upstream. 
didFailLoading is now called on InspectorInstrumentation when XHR is loaded correctly, because resource loading is cancelled after successful load.

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

> Source/WebCore/loader/cache/CachedRawResource.cpp:82
> +        m_loader->cancel();

This triggers cancellation.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:-159
> -    if (m_finishing)

This check is missing now. We can override ResourceLoader::cancel() in SubresourceLoader::cancel() and add this check there, does that seem correct to you?

[Alexey Proskuryakov]

Another regression from this change: bug 83918.