Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2436163002:
When InterceptingResourceHandler swaps in a handler, call OnWillStart. (Closed)
|
Created:
4 years, 2 months ago by mmenke Modified:
4 years, 1 month ago Reviewers:
Charlie Harrison CC:
chromium-reviews, loading-reviews_chromium.org, jam, darin-cc_chromium.org, Randy Smith (Not in Mondays) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWhen InterceptingResourceHandler swaps in a handler, call OnWillStart.
The new ResourceHandler's OnWillStart was never being called, which
violates the ResourceHandler's API contract, and has caused at least one
crash previously (Though that fixed by another CL).
BUG=650717
Committed: https://crrev.com/90cf7fc2ae2d522c9a0d5c9dee461f6b79277df9
Cr-Commit-Position: refs/heads/master@{#427183}
Patch Set 1 #Patch Set 2 : Remove unused value, reorder defer/results (They don't quite match...) #
Total comments: 1
Patch Set 3 : Update comments, remove prototype for method that never existed #
Dependent Patchsets: Messages
Total messages: 31 (19 generated)
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mmenke@chromium.org changed reviewers: + csharrison@chromium.org
[csharrison]: This was inspired by a downloads bug, but sending it to you since you're an owner, and I'd like to do some refactoring here (Which I'll probably never get around to, but you never know). https://codereview.chromium.org/2436163002/diff/20001/content/browser/loader/... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2436163002/diff/20001/content/browser/loader/... content/browser/loader/intercepting_resource_handler_unittest.cc:54: class TestResourceHandler : public ResourceHandler { I want to move this class to a separate file, and have the MimeSniffer tests use it as well (Right now they use their own TestResourceHandler, which does many of the same things)
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
looks good. Will go over the tests on Monday.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM! I think the modifications to the tests make things much clearer (i.e. what is default and what isn't).
On 2016/10/24 16:22:15, Charlie Harrison wrote: > LGTM! I think the modifications to the tests make things much clearer (i.e. what > is default and what isn't). Thanks! Have a followup I'll probably be sending out later today (Make the InteceptingRH and MimeSniffingRH tests use the same TestResourceHandler)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by mmenke@chromium.org
On 2016/10/24 22:01:32, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) Bots are slow, and none of these failures look remotely related to me CLs. :(
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== When InterceptingResourceHandler swaps in a handler, call OnWillStart. The new ResourceHandler's OnWillStart was never being called, which violates the ResourceHandler's API contract, and has caused at least one crash previously (Though that fixed by another CL). BUG=650717 ========== to ========== When InterceptingResourceHandler swaps in a handler, call OnWillStart. The new ResourceHandler's OnWillStart was never being called, which violates the ResourceHandler's API contract, and has caused at least one crash previously (Though that fixed by another CL). BUG=650717 Committed: https://crrev.com/90cf7fc2ae2d522c9a0d5c9dee461f6b79277df9 Cr-Commit-Position: refs/heads/master@{#427183} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/90cf7fc2ae2d522c9a0d5c9dee461f6b79277df9 Cr-Commit-Position: refs/heads/master@{#427183} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
