Bug 71149

Summary: Merge CachedResourceRequest and SubresourceLoader, delete SubresourceClient
Product: WebKit Reporter: Nate Chapin <japhet>
Component: WebCore Misc.Assignee: Nate Chapin <japhet>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, dglazkov, koivisto, pkasting, vsevik, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 72491    
Bug Blocks:    
Attachments:
Description Flags
Attempt #1
webkit.review.bot: commit-queue-
Address compile failure
webkit.review.bot: commit-queue-
Fix chromium test failure + merge to trunk
none
Patch for landing none

Nate Chapin
Reported 2011-10-28 15:24:51 PDT
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.
Attachments
Attempt #1 (75.61 KB, patch)
2011-10-28 15:34 PDT, Nate Chapin 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Address compile failure (75.70 KB, patch)
2011-10-31 11:40 PDT, Nate Chapin 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Fix chromium test failure + merge to trunk (78.00 KB, patch)
2011-11-09 13:58 PST, Nate Chapin 		no flags
DetailsFormatted DiffDiff
Patch for landing (78.29 KB, patch)
2011-11-15 10:35 PST, Nate Chapin 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Nate Chapin
Comment 1 2011-10-28 15:34:06 PDT
Created attachment 112923 [details] Attempt #1
WebKit Review Bot
Comment 2 2011-10-28 16:10:31 PDT
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
Comment 3 2011-10-31 11:40:39 PDT
Created attachment 113070 [details] Address compile failure
WebKit Review Bot
Comment 4 2011-10-31 14:32:34 PDT
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
Comment 5 2011-11-09 13:58:23 PST
Created attachment 114360 [details] Fix chromium test failure + merge to trunk
Adam Barth
Comment 6 2011-11-15 10:10:26 PST
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 7 2011-11-15 10:23:01 PST
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
Comment 8 2011-11-15 10:35:46 PST
Created attachment 115194 [details] Patch for landing
WebKit Review Bot
Comment 9 2011-11-15 12:40:31 PST
Comment on attachment 115194 [details] Patch for landing Clearing flags on attachment: 115194 Committed r100311: <http://trac.webkit.org/changeset/100311>
WebKit Review Bot
Comment 10 2011-11-15 12:40:37 PST
All reviewed patches have been landed. Closing bug.
Peter Kasting
Comment 11 2011-11-17 10:38:57 PST
This patch seems to have caused http://code.google.com/p/chromium/issues/detail?id=104582 downstream.
Vsevolod Vlasov
Comment 12 2011-11-21 04:46:42 PST
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
Comment 13 2012-04-13 11:11:30 PDT
Another regression from this change: bug 83918.
Note You need to log in before you can comment on or make changes to this bug.