Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1926193002:
Clean up ResourceLoader finish more, remove ConnectionState enum (Closed)
|
Created:
4 years, 7 months ago by Nate Chapin Modified:
4 years, 7 months ago CC:
chromium-reviews, blink-reviews, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@finish_smaller Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean up ResourceLoader finish more, remove ConnectionState enum
* Move multipart first-part completion to ResourceFetcher's
didFinishLoading.
* Cancel and clear ResourceLoader::m_loader at the start
of finish/failure, guaranteeing we won't get any unexpected
messages from the WebURLLoader.
* Move didLoadResource() call to ResourceFetcher, which means
releaseResources() just clears pointers that don't strictly
need to be cleared.
* Null-check m_loader to determine if ResourceLoader is
finishing. The ResourceLoader::ConnectionState is now
assert-only, so drop it.
BUG=
Committed: https://crrev.com/7e772321d9a1f0f71c093747a3138762d9e6f67d
Cr-Commit-Position: refs/heads/master@{#393958}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 10
Patch Set 5 : Document call order, enum for didFinishLoading #Patch Set 6 : rebase #
Messages
Total messages: 40 (19 generated)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1926193002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1926193002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1926193002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1926193002/40001
Description was changed from ========== Drop releaseResources(), cancel() helper, ConnectionStateReleased BUG= ========== to ========== Clean up ResourceLoader finish more, remove ConnectionState enum * Move multipart first-part completion to ResourceFetcher's didFinishLoading. * Cancel and clear ResourceLoader::m_loader at the start of finish/failure, guaranteeing we won't get any unexpected messages from the WebURLLoader. * Move didLoadResource() call to ResourceFetcher, which means releaseResources() just clears pointers that don't strictly need to be cleared. * Null-check m_loader to determine if ResourceLoader is finishing. The ResourceLoader::ConnectionState is now assert-only, so drop it. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1926193002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1926193002/60001
japhet@chromium.org changed reviewers: + hiroshige@chromium.org, yhirano@chromium.org
More ResourceLoader cleanup :D https://codereview.chromium.org/1926193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (left): https://codereview.chromium.org/1926193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:57: , m_notifiedLoadComplete(false) Like didFailLoading(), didFinishLoading() should now be resilient to being called after the first part of a multipart response completes, so this bit isn't needed. https://codereview.chromium.org/1926193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:76: void ResourceLoader::releaseResources() didLoadResource() moved to ResourceFetcher's didFinishLoading/didFailLoading. m_loader is now cleared at the start of finish/fail. Everything else isn't strictly necessary. https://codereview.chromium.org/1926193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:118: RELEASE_ASSERT(m_state == ConnectionStateReceivedResponse); If you're not ok with dropping all these ASSERTs, i'd suggest that we replace them with m_loader asserts. AFAIK, the only way the state machine has ever gotten corrupted is when a load is cancelled but the WebURLLoader isn't notified promptly. Becasue WebURLLoader is now immediately cleared when we don't want any more messages from it, this should suffice. https://codereview.chromium.org/1926193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:128: if (m_state == ConnectionStateFinishedLoading) { This block is in ResourceFetcher::didFinishLoading now, allowing removeResourceLoader() and moveResourceLoaderToNonBlocking() private. https://codereview.chromium.org/1926193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:170: if (m_loader) { Because didFail() now starts by clearing m_loader, this isn't necessary. https://codereview.chromium.org/1926193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:275: if (m_resource->response().httpStatusCode() >= 400 && !m_resource->shouldIgnoreHTTPStatusCodeErrors()) This exists to defend against nested loops causing data to be received when we call didFail() in didReceiveResponse() and a nested message loop starts while handling the error. Because the loader gets cancelled at the start of didFail(), this reeentrancy should no longer be possible. https://codereview.chromium.org/1926193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:294: m_resource->finish(finishTime); Call finish/error from ResourceFetcher, so that it still happens between calls to FetchContext's didFinishLoading/didFailLoading and didLoadResource. https://codereview.chromium.org/1926193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/testing/weburl_loader_mock.cc (right): https://codereview.chromium.org/1926193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/weburl_loader_mock.cc:24: cancel(); This matches WebURLLoaderImpl, which cancel()s in the destructor, letting ResourceLoader just clear its OwnPtr<ResourceLoader> instead of needing to call cancel() first.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Great refactoring! Regarding assertions, it is useful to detect caller-side (i.e., content) bugs and it helped me understand ResourceLoader assumptions, so it would be great if you could keep them or write some documentation. https://codereview.chromium.org/1926193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/1926193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.h:107: void didFinishLoading(Resource*, double finishTime, int64_t encodedDataLength, bool isFirstPartInMultipart); It would be good to replace this boolean with enum.
Added a comment in ResourceLoader.h that documents the order, and reordered the WebURLLoaderClient overrides to match the expected call order. I'm inclined to say this is sufficient, but I'm willing to be overruled :) https://codereview.chromium.org/1926193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/1926193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.h:107: void didFinishLoading(Resource*, double finishTime, int64_t encodedDataLength, bool isFirstPartInMultipart); On 2016/05/13 10:33:03, yhirano wrote: > It would be good to replace this boolean with enum. Done.
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1926193002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1926193002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1926193002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1926193002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1926193002/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1926193002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1926193002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
japhet@chromium.org changed reviewers: + kinuko@chromium.org
kinuko, would you mind reviewing the 1-line change to platform/testing/weburl_loader_mock.cc ?
lgtm!
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1926193002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1926193002/100001
Message was sent while issue was closed.
Description was changed from ========== Clean up ResourceLoader finish more, remove ConnectionState enum * Move multipart first-part completion to ResourceFetcher's didFinishLoading. * Cancel and clear ResourceLoader::m_loader at the start of finish/failure, guaranteeing we won't get any unexpected messages from the WebURLLoader. * Move didLoadResource() call to ResourceFetcher, which means releaseResources() just clears pointers that don't strictly need to be cleared. * Null-check m_loader to determine if ResourceLoader is finishing. The ResourceLoader::ConnectionState is now assert-only, so drop it. BUG= ========== to ========== Clean up ResourceLoader finish more, remove ConnectionState enum * Move multipart first-part completion to ResourceFetcher's didFinishLoading. * Cancel and clear ResourceLoader::m_loader at the start of finish/failure, guaranteeing we won't get any unexpected messages from the WebURLLoader. * Move didLoadResource() call to ResourceFetcher, which means releaseResources() just clears pointers that don't strictly need to be cleared. * Null-check m_loader to determine if ResourceLoader is finishing. The ResourceLoader::ConnectionState is now assert-only, so drop it. BUG= ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Clean up ResourceLoader finish more, remove ConnectionState enum * Move multipart first-part completion to ResourceFetcher's didFinishLoading. * Cancel and clear ResourceLoader::m_loader at the start of finish/failure, guaranteeing we won't get any unexpected messages from the WebURLLoader. * Move didLoadResource() call to ResourceFetcher, which means releaseResources() just clears pointers that don't strictly need to be cleared. * Null-check m_loader to determine if ResourceLoader is finishing. The ResourceLoader::ConnectionState is now assert-only, so drop it. BUG= ========== to ========== Clean up ResourceLoader finish more, remove ConnectionState enum * Move multipart first-part completion to ResourceFetcher's didFinishLoading. * Cancel and clear ResourceLoader::m_loader at the start of finish/failure, guaranteeing we won't get any unexpected messages from the WebURLLoader. * Move didLoadResource() call to ResourceFetcher, which means releaseResources() just clears pointers that don't strictly need to be cleared. * Null-check m_loader to determine if ResourceLoader is finishing. The ResourceLoader::ConnectionState is now assert-only, so drop it. BUG= Committed: https://crrev.com/7e772321d9a1f0f71c093747a3138762d9e6f67d Cr-Commit-Position: refs/heads/master@{#393958} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7e772321d9a1f0f71c093747a3138762d9e6f67d Cr-Commit-Position: refs/heads/master@{#393958} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
