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
Nate Chapin
2011-10-28 15:24:51 PDT
Created attachment 112923 [details]
Attempt #1
Comment on attachment 112923 [details] Attempt #1 Attachment 112923 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10244058 Created attachment 113070 [details]
Address compile failure
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 Created attachment 114360 [details]
Fix chromium test failure + merge to trunk
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.
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 Created attachment 115194 [details]
Patch for landing
Comment on attachment 115194 [details] Patch for landing Clearing flags on attachment: 115194 Committed r100311: <http://trac.webkit.org/changeset/100311> All reviewed patches have been landed. Closing bug. This patch seems to have caused http://code.google.com/p/chromium/issues/detail?id=104582 downstream. 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? Another regression from this change: bug 83918. |