Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2327463002:
Relax ad-hoc assumptions on InterceptingResourceHandler (Closed)
DescriptionRelax ad-hoc assumptions on InterceptingResourceHandler
InterceptingResourceHandler holds two ResourceHandlers but it requires
those ResourceHandlers to meet certain conditions in addition to the
usual ResourceHandler contract.
This CL removes some (not all) requirements so that it can work with
MojoAsyncResourceHandler.
With this CL,
- OnResponseStarted on the old handler can return false.
- OnResponseStarted on the new handler can return false.
- OnResponseStarted on the new handler can defer processing.
- OnWillStart on the old handler can return false.
- OnReadCompleted on the old handler can return false.
- OnReadCompleted on the old handler can defer processing.
- the payload given to |UseHandler| can be longer than the size of the
buffer exposed via OnWillRead.
- InterceptingResourceHandler reads only "committed" data.
Previously, it read data on the buffer exposed by OnWillRead
before OnReadCompleted was called.
BUG=603396
Committed: https://crrev.com/22b69a9b20e099fbe14c70b621bc630043222146
Cr-Commit-Position: refs/heads/master@{#426134}
Patch Set 1 : fix #
Total comments: 16
Patch Set 2 : fix #
Total comments: 9
Patch Set 3 : fix #Patch Set 4 : fix #Patch Set 5 : fix #Patch Set 6 : fix #Patch Set 7 : fix #Patch Set 8 : fix #
Total comments: 36
Patch Set 9 : fix #Patch Set 10 : fix #
Total comments: 24
Patch Set 11 : fix #Patch Set 12 : fix #Patch Set 13 : fix #Patch Set 14 : fix #
Total comments: 2
Patch Set 15 : fix #
Total comments: 2
Patch Set 16 : fix #Patch Set 17 : fix #Patch Set 18 : fix #Patch Set 19 : fix #
Messages
Total messages: 139 (107 generated)
The CQ bit was checked by yhirano@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yhirano@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: Exceeded global retry quota
The CQ bit was checked by yhirano@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by yhirano@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by yhirano@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 checked by yhirano@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by yhirano@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...
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== [WIP] Relax some ad-hoc assumptions expected by InterceptingResourceHandler. BUG=603396 ========== to ========== Relax ad-hoc assumptions on InterceptingResourceHandler InterceptingResourceHandler holds two ResourceHandlers but it requires those ResourceHandlers to meet certain conditions in addition to the usual ResourceHandler contracts. This CL removes some (not all) requirements so that it can work with MojoAsyncResourceHandler. BUG=603396 ==========
Description was changed from ========== Relax ad-hoc assumptions on InterceptingResourceHandler InterceptingResourceHandler holds two ResourceHandlers but it requires those ResourceHandlers to meet certain conditions in addition to the usual ResourceHandler contracts. This CL removes some (not all) requirements so that it can work with MojoAsyncResourceHandler. BUG=603396 ========== to ========== Relax ad-hoc assumptions on InterceptingResourceHandler InterceptingResourceHandler holds two ResourceHandlers but it requires those ResourceHandlers to meet certain conditions in addition to the usual ResourceHandler contracts. This CL removes some (not all) requirements so that it can work with MojoAsyncResourceHandler. - OnResponseStarted on the old handler can return false. - OnResponseStarted on the new handler can return false. - OnResponseStarted on the new handler can defer processing. - OnWillStart on the old handler can return false. - OnWillStart on the new handler can return false. - OnReadCompleted on the old handler can return false. - OnReadCompleted on the new handler can return false. - OnReadCompleted on the old handler can defer processing. - OnReadCompleted on the new handler can defer processing. - InterceptingResourceHandler now reads only "committed" data. Previously, it read data on the buffer exposed by OnWillRead before OnReadCompleted was called. BUG=603396 ==========
Description was changed from ========== Relax ad-hoc assumptions on InterceptingResourceHandler InterceptingResourceHandler holds two ResourceHandlers but it requires those ResourceHandlers to meet certain conditions in addition to the usual ResourceHandler contracts. This CL removes some (not all) requirements so that it can work with MojoAsyncResourceHandler. - OnResponseStarted on the old handler can return false. - OnResponseStarted on the new handler can return false. - OnResponseStarted on the new handler can defer processing. - OnWillStart on the old handler can return false. - OnWillStart on the new handler can return false. - OnReadCompleted on the old handler can return false. - OnReadCompleted on the new handler can return false. - OnReadCompleted on the old handler can defer processing. - OnReadCompleted on the new handler can defer processing. - InterceptingResourceHandler now reads only "committed" data. Previously, it read data on the buffer exposed by OnWillRead before OnReadCompleted was called. BUG=603396 ========== to ========== Relax ad-hoc assumptions on InterceptingResourceHandler InterceptingResourceHandler holds two ResourceHandlers but it requires those ResourceHandlers to meet certain conditions in addition to the usual ResourceHandler contracts. This CL removes some (not all) requirements so that it can work with MojoAsyncResourceHandler. - OnResponseStarted on the old handler can return false. - OnResponseStarted on the new handler can return false. - OnResponseStarted on the new handler can defer processing. - OnWillStart on the old handler can return false. - OnReadCompleted on the old handler can return false. - OnReadCompleted on the old handler can defer processing. - The payload given to |UseHandler| can be longer than the size of the buffer exposed via OnWillRead. - InterceptingResourceHandler now reads only "committed" data. Previously, it read data on the buffer exposed by OnWillRead before OnReadCompleted was called. BUG=603396 ==========
Description was changed from ========== Relax ad-hoc assumptions on InterceptingResourceHandler InterceptingResourceHandler holds two ResourceHandlers but it requires those ResourceHandlers to meet certain conditions in addition to the usual ResourceHandler contracts. This CL removes some (not all) requirements so that it can work with MojoAsyncResourceHandler. - OnResponseStarted on the old handler can return false. - OnResponseStarted on the new handler can return false. - OnResponseStarted on the new handler can defer processing. - OnWillStart on the old handler can return false. - OnReadCompleted on the old handler can return false. - OnReadCompleted on the old handler can defer processing. - The payload given to |UseHandler| can be longer than the size of the buffer exposed via OnWillRead. - InterceptingResourceHandler now reads only "committed" data. Previously, it read data on the buffer exposed by OnWillRead before OnReadCompleted was called. BUG=603396 ========== to ========== Relax ad-hoc assumptions on InterceptingResourceHandler InterceptingResourceHandler holds two ResourceHandlers but it requires those ResourceHandlers to meet certain conditions in addition to the usual ResourceHandler contract. This CL removes some (not all) requirements so that it can work with MojoAsyncResourceHandler. - OnResponseStarted on the old handler can return false. - OnResponseStarted on the new handler can return false. - OnResponseStarted on the new handler can defer processing. - OnWillStart on the old handler can return false. - OnReadCompleted on the old handler can return false. - OnReadCompleted on the old handler can defer processing. - The payload given to |UseHandler| can be longer than the size of the buffer exposed via OnWillRead. - InterceptingResourceHandler now reads only "committed" data. Previously, it read data on the buffer exposed by OnWillRead before OnReadCompleted was called. BUG=603396 ==========
yhirano@chromium.org changed reviewers: + mmenke@chromium.org
Matt, can you take a look? This CL addresses [1]. 1: https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/29 09:15:41, yhirano wrote: > Matt, can you take a look? This CL addresses [1]. > > 1: > https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade... Thanks for doing this! May not have time today, but will be able to get to it tomorrow (Also no review backlog this time, so hopefully can be more responsive on this one than the other).
mmenke@chromium.org changed reviewers: + csharrison@chromium.org
[+csharrison]: You said you wanted to shadow me on the Mojo reviews. While this doesn't directly add any Mojo code, this is one of the CLs adding stuff needed for Mojo.
This looks good. I think having the logic in InterceptingResourceHandler is the right move conceptually. Haven't yet looked at the tests, will try to do that tomorrow. One nit: in your CL description can you make it a bit clearer that the bullets are the things you are *doing* and not *removing* in the CL? https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:83: DCHECK_EQ(State::STARTING, state_); It seems strange that we are STARTING at this point. Does it make sense to have another book-keeping state after OnResponseStarted finishes? https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:135: DCHECK_EQ(first_read_buffer_bytes_written_, 0)? https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:155: bool InterceptingResourceHandler::SendPayloadToOldHandler(bool* defer) { Optional: Should this be a MaybeSendPayloadToOldHandler, because payload_for_old_handler_ could be empty? https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:161: if (first_read_buffer_) { Can this first condition be pulled out of the loop? It seems like it will always be true on first iteration? If that ends up being too ugly a DCHECK(first_read_buffer_) outside of the loop would be good. https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:31: // A ResourceHandler holds two handlers (next_handler and new_handler). It A InterceptingResourceHandler? https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:73: enum class State { Can you expand the comments here a bit? In particular, it would be helpful to know the difference between "buffer" and "payload", and what payload is needed to be sent to the old handler. https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:79: // In this state, the InterceptingResourcHandler is notifying nit: remove "In this state" from these description as the phrase is just noise. https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:36: void Cancel() override { is_cancel_called_ = true; } nit: mixing present tense "is" and past tense "called" is awkward. Maybe just "called_cancel_"? Same for this whole class.
Description was changed from ========== Relax ad-hoc assumptions on InterceptingResourceHandler InterceptingResourceHandler holds two ResourceHandlers but it requires those ResourceHandlers to meet certain conditions in addition to the usual ResourceHandler contract. This CL removes some (not all) requirements so that it can work with MojoAsyncResourceHandler. - OnResponseStarted on the old handler can return false. - OnResponseStarted on the new handler can return false. - OnResponseStarted on the new handler can defer processing. - OnWillStart on the old handler can return false. - OnReadCompleted on the old handler can return false. - OnReadCompleted on the old handler can defer processing. - The payload given to |UseHandler| can be longer than the size of the buffer exposed via OnWillRead. - InterceptingResourceHandler now reads only "committed" data. Previously, it read data on the buffer exposed by OnWillRead before OnReadCompleted was called. BUG=603396 ========== to ========== Relax ad-hoc assumptions on InterceptingResourceHandler InterceptingResourceHandler holds two ResourceHandlers but it requires those ResourceHandlers to meet certain conditions in addition to the usual ResourceHandler contract. This CL removes some (not all) requirements so that it can work with MojoAsyncResourceHandler. With this CL, - OnResponseStarted on the old handler can return false. - OnResponseStarted on the new handler can return false. - OnResponseStarted on the new handler can defer processing. - OnWillStart on the old handler can return false. - OnReadCompleted on the old handler can return false. - OnReadCompleted on the old handler can defer processing. - the payload given to |UseHandler| can be longer than the size of the buffer exposed via OnWillRead. - InterceptingResourceHandler reads only "committed" data. Previously, it read data on the buffer exposed by OnWillRead before OnReadCompleted was called. BUG=603396 ==========
The CQ bit was checked by yhirano@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...
Thank you for your comments. https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:83: DCHECK_EQ(State::STARTING, state_); On 2016/09/29 22:32:58, Charlie Harrison wrote: > It seems strange that we are STARTING at this point. Does it make sense to have > another book-keeping state after OnResponseStarted finishes? Makes sense, but I couldn't come up with a good name. What do you think about WAITING_FOR_ON_READ_COMPLETED? Do you have a good idea? https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:135: On 2016/09/29 22:32:58, Charlie Harrison wrote: > DCHECK_EQ(first_read_buffer_bytes_written_, 0)? No, it can be greater than zero when called from Resume. https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:155: bool InterceptingResourceHandler::SendPayloadToOldHandler(bool* defer) { On 2016/09/29 22:32:58, Charlie Harrison wrote: > Optional: Should this be a MaybeSendPayloadToOldHandler, because > payload_for_old_handler_ could be empty? Hmm, I don't think we should append Maybe (weak opinion), but if we do so, we should do the same thing for SendFirstReadBufferToNewHandler (strong opinion). https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:161: if (first_read_buffer_) { On 2016/09/29 22:32:58, Charlie Harrison wrote: > Can this first condition be pulled out of the loop? It seems like it will always > be true on first iteration? If that ends up being too ugly a > DCHECK(first_read_buffer_) outside of the loop would be good. |first_read_buffer_| is null at the function top when called from Resume. https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:31: // A ResourceHandler holds two handlers (next_handler and new_handler). It On 2016/09/29 22:32:58, Charlie Harrison wrote: > A InterceptingResourceHandler? Thanks, done. https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:73: enum class State { On 2016/09/29 22:32:58, Charlie Harrison wrote: > Can you expand the comments here a bit? In particular, it would be helpful to > know the difference between "buffer" and "payload", and what payload is needed > to be sent to the old handler. Added some explanation about "payload". https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:79: // In this state, the InterceptingResourcHandler is notifying On 2016/09/29 22:32:58, Charlie Harrison wrote: > nit: remove "In this state" from these description as the phrase is just noise. Done. https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2327463002/diff/100001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:36: void Cancel() override { is_cancel_called_ = true; } On 2016/09/29 22:32:58, Charlie Harrison wrote: > nit: mixing present tense "is" and past tense "called" is awkward. Maybe just > "called_cancel_"? Same for this whole class. I originally intended to use them but currently my code doesn't. So I removed them.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Sorry for the delay - ended up taking Friday off, low on sleep today (Insomnia, not out late partying). I'll try to do a pass this evening, but no promises.
https://codereview.chromium.org/2327463002/diff/120001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/120001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:133: bool InterceptingResourceHandler::SendFirstReadBufferToNewHandler(bool* defer) { Why are you reversing the order of this and sending the payload to the new handler? To save the memcpy? Per https://crbug.com/650717, we'll run into issues a little down the line (When adding a call to OnWillStartRequest on the new handler) if we do this before we pass the payload to the old handler, so I think we're stuck with the copy. https://codereview.chromium.org/2327463002/diff/120001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:155: bool InterceptingResourceHandler::SendPayloadToOldHandler(bool* defer) { Would more of a DoLoop approach be simpler? i.e., make a method: bool DoLoop(bool* defer) { bool result = true; while (result && !defer && state_ != State::WHATEVER) { // Switch statement much like Resume() has. } return result; } Then call SendFirstReadBufferToNewHandler (Which no longer has a while loop, or a call to SendPayloadToOldHandler - it just sets the state_ to handle whether it should be called again, or the next method should be called). Resume then calls that method as well... Anyhow, just a suggestion. https://codereview.chromium.org/2327463002/diff/120001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:225: case State::NOTIFYING_ON_RESPONSE_COMPLETED_TO_NEW_HANDLER: I don't think we ever set this state in the first place?
Other than those comments, code looks reasonable to me (Still need to look at tests).
The CQ bit was checked by yhirano@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yhirano@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...
https://codereview.chromium.org/2327463002/diff/120001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/120001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:133: bool InterceptingResourceHandler::SendFirstReadBufferToNewHandler(bool* defer) { On 2016/10/04 15:08:01, mmenke wrote: > Why are you reversing the order of this and sending the payload to the new > handler? To save the memcpy? > > Per https://crbug.com/650717, we'll run into issues a little down the line (When > adding a call to OnWillStartRequest on the new handler) if we do this before we > pass the payload to the old handler, so I think we're stuck with the copy. Yes, I reversed order in order to remove a copy. Sorry I still don't understand why this is problematic. Can you explain again? https://codereview.chromium.org/2327463002/diff/120001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:155: bool InterceptingResourceHandler::SendPayloadToOldHandler(bool* defer) { On 2016/10/04 15:08:01, mmenke wrote: > Would more of a DoLoop approach be simpler? > > i.e., make a method: > > bool DoLoop(bool* defer) { > bool result = true; > while (result && !defer && state_ != State::WHATEVER) { > // Switch statement much like Resume() has. > } > return result; > } > > Then call SendFirstReadBufferToNewHandler (Which no longer has a while loop, or > a call to SendPayloadToOldHandler - it just sets the state_ to handle whether it > should be called again, or the next method should be called). > > Resume then calls that method as well... Anyhow, just a suggestion. Done. https://codereview.chromium.org/2327463002/diff/120001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:225: case State::NOTIFYING_ON_RESPONSE_COMPLETED_TO_NEW_HANDLER: On 2016/10/04 15:08:01, mmenke wrote: > I don't think we ever set this state in the first place? I set |state_| to the state in InterceptingResourceHandler::OnResponseCompleted.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2327463002/diff/120001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/120001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:133: bool InterceptingResourceHandler::SendFirstReadBufferToNewHandler(bool* defer) { On 2016/10/06 11:41:44, yhirano wrote: > On 2016/10/04 15:08:01, mmenke wrote: > > Why are you reversing the order of this and sending the payload to the new > > handler? To save the memcpy? > > > > Per https://crbug.com/650717, we'll run into issues a little down the line > (When > > adding a call to OnWillStartRequest on the new handler) if we do this before > we > > pass the payload to the old handler, so I think we're stuck with the copy. > > Yes, I reversed order in order to remove a copy. > > Sorry I still don't understand why this is problematic. Can you explain again? OnWillStart isn't being called on NewHandler, but should be. I added it before the OnReadCompleted call to both handlers, using the old code (Which, as far as DownloadResourceHandler, is much the same as reversing the order of the read calls). DownloadTest.DownloadResourceThrottleCancels expects the opposite order, so started failing, not sure why it's passing on some platforms when you ran the tests. I think the change would also mean we spin the spinner while showing the loading dialog, unsure about other fallout here - could the old request get a cancellation message while we're calling into the new handler? I was thinking I ran into some other fallout as well, but can't remember what it was.
The CQ bit was checked by yhirano@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_rel_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 yhirano@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 checked by yhirano@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 checked by yhirano@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 checked by yhirano@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.
https://codereview.chromium.org/2327463002/diff/120001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/120001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:133: bool InterceptingResourceHandler::SendFirstReadBufferToNewHandler(bool* defer) { On 2016/10/06 16:10:18, mmenke wrote: > On 2016/10/06 11:41:44, yhirano wrote: > > On 2016/10/04 15:08:01, mmenke wrote: > > > Why are you reversing the order of this and sending the payload to the new > > > handler? To save the memcpy? > > > > > > Per https://crbug.com/650717, we'll run into issues a little down the line > > (When > > > adding a call to OnWillStartRequest on the new handler) if we do this before > > we > > > pass the payload to the old handler, so I think we're stuck with the copy. > > > > Yes, I reversed order in order to remove a copy. > > > > Sorry I still don't understand why this is problematic. Can you explain again? > > OnWillStart isn't being called on NewHandler, but should be. I added it before > the OnReadCompleted call to both handlers, using the old code (Which, as far as > DownloadResourceHandler, is much the same as reversing the order of the read > calls). > > DownloadTest.DownloadResourceThrottleCancels expects the opposite order, so > started failing, not sure why it's passing on some platforms when you ran the > tests. I think the change would also mean we spin the spinner while showing the > loading dialog, unsure about other fallout here - could the old request get a > cancellation message while we're calling into the new handler? I was thinking I > ran into some other fallout as well, but can't remember what it was. Thank you. I changed the state transition to keep the existing behavior as much as possible.
I'll try to do a pass later today, have a couple other good size reviews, but think I'll get to them all. https://codereview.chromium.org/2327463002/diff/120001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/120001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:133: bool InterceptingResourceHandler::SendFirstReadBufferToNewHandler(bool* defer) { On 2016/10/07 15:03:53, yhirano wrote: > On 2016/10/06 16:10:18, mmenke wrote: > > On 2016/10/06 11:41:44, yhirano wrote: > > > On 2016/10/04 15:08:01, mmenke wrote: > > > > Why are you reversing the order of this and sending the payload to the new > > > > handler? To save the memcpy? > > > > > > > > Per https://crbug.com/650717, we'll run into issues a little down the line > > > (When > > > > adding a call to OnWillStartRequest on the new handler) if we do this > before > > > we > > > > pass the payload to the old handler, so I think we're stuck with the copy. > > > > > > Yes, I reversed order in order to remove a copy. > > > > > > Sorry I still don't understand why this is problematic. Can you explain > again? > > > > OnWillStart isn't being called on NewHandler, but should be. I added it > before > > the OnReadCompleted call to both handlers, using the old code (Which, as far > as > > DownloadResourceHandler, is much the same as reversing the order of the read > > calls). > > > > DownloadTest.DownloadResourceThrottleCancels expects the opposite order, so > > started failing, not sure why it's passing on some platforms when you ran the > > tests. I think the change would also mean we spin the spinner while showing > the > > loading dialog, unsure about other fallout here - could the old request get a > > cancellation message while we're calling into the new handler? I was thinking > I > > ran into some other fallout as well, but can't remember what it was. > > Thank you. I changed the state transition to keep the existing behavior as much > as possible. Sorry, "loading dialog" == "save as dialog".
Sorry for the delay. Think there may be some further cleanup of the DoLoop - want to take another look at it later today or tomorrow. (Feel free to wait on those second set of comments before re-uploading, just wanted to get you some comments now) https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:73: *buf = first_read_buffer_double_; This code is optimized for the case where we switch to a new handler, at the cost of doing an extra copy in the case when we don't switch to a new handler. I think that 90+% of the time, we don't switch handlers? Would it be better to do two extra copies in the case we switch handlers, and 0 when we don't? https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:185: } nit: Add blank line here? https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:187: net::URLRequestStatus status = {net::URLRequestStatus::SUCCESS, 0}; Just URLRequestStatus? Or use the constructor explicitly? net::URLRequestStatus status(net::URLRequestStatus::SUCCESS, 0); https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:191: net::ERR_ABORTED); net::URLREQuestStatus::FromError(ERR_ABORTED). We want to get rid of URLRequestStatus::CANCELED, some day. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:193: next_handler_->OnResponseCompleted(status, &defer_ignored); Could trivially add support for this deferring the operation - don't think it's worth a new test. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:202: return DoLoop(defer); This method is only called from DoLoop, so the above 5 lines should just be: return next_handler_->OnResponseStarted(response_.get(), defer); https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:34: // - OnResponseCompleted on next_handler never sets |*defer|. nit: |next_handler| (x3), |new_handler|. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:80: // |UseNewHandler| to the old handler and waiting for its completion via nit: |UseNewHandler| -> UseNewHandler. I don't think || are generally used around method names. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:116: bool DoLoop(bool* defer); nit: It's the general ResourceHandler/ResourceThrottle pattern, but seems like the return value / defer should be documented, with maybe a little extra documentation for DoLoop.
https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:73: *buf = first_read_buffer_double_; On 2016/10/10 19:40:15, mmenke wrote: > This code is optimized for the case where we switch to a new handler, at the > cost of doing an extra copy in the case when we don't switch to a new handler. > I think that 90+% of the time, we don't switch handlers? Would it be better to > do two extra copies in the case we switch handlers, and 0 when we don't? My understanding is that we need this copy for correctness. When the following functions are called in order, 1. OnWillRead 2. UseNewHandler 3. OnResponseStarted we need to call old handler's OnReadCompleted, but the buffer we exposed to the client at OnWillRead should not be used because the client hasn't finished writing to the buffer yet. I wanted to remove [1], but you think the performance loss is unacceptable, right? 1: https://cs.chromium.org/chromium/src/content/browser/loader/intercepting_reso...
https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:73: *buf = first_read_buffer_double_; On 2016/10/11 11:06:40, yhirano wrote: > On 2016/10/10 19:40:15, mmenke wrote: > > This code is optimized for the case where we switch to a new handler, at the > > cost of doing an extra copy in the case when we don't switch to a new handler. > > > I think that 90+% of the time, we don't switch handlers? Would it be better > to > > do two extra copies in the case we switch handlers, and 0 when we don't? > > My understanding is that we need this copy for correctness. When the following > functions are called in order, > > 1. OnWillRead > 2. UseNewHandler > 3. OnResponseStarted > > we need to call old handler's OnReadCompleted, but the buffer we exposed to the > client at OnWillRead should not be used because the client hasn't finished > writing to the buffer yet. I wanted to remove [1], but you think the performance > loss is unacceptable, right? > > 1: > https://cs.chromium.org/chromium/src/content/browser/loader/intercepting_reso... You're absolutely correct - I mean, the buffer has actually been written to, by that point, but we haven't been informed about it. And even if we had, we really should not modify it before OnReadCompleted is called on the intermediate ResourceHandler, between the sniffer and the interceptor. It's not the performance cost of removing [1], it's that it messes up ordering in which we do other things. I can't think of a better way to make everything sane than this current code, so let's keep this as-is.
https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:117: new_handler_->OnResponseCompleted(status, defer); This seems not quite right. If we're in State::NOTIFYING_ON_RESPONSE_STARTED_TO_NEW_HANDLER or State::WAITING_FOR_ON_READ_COMPLETED, we'll never tell any handler about the completed response. Seems like the safest thing to do here is: void InterceptingResourceHandler::OnResponseCompleted(...) { if (state_ != State::PASS_THROUGH) { // |this| either is in the middle of transferring control to a handler, // or has not yet been notified of the response starting. // In the first case, just swap in the new handler and tell it of the // failure. In the second case, just switch to pass through mode. if (new_handler_) next_handler_ = std::move(new_handler_); state_ = State::PASS_THROUGH; first_read_buffer_double_ = nullptr; next_handler_->SetController(controller()); } DCHECK(!new_handler_); return LayeredController::OnResponseCompleted(...); } This would also let us get rid of two states. Also, add tests for this? https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:123: void InterceptingResourceHandler::Cancel() { In all of these 4, DCHECK_NE(state_, State::PASS_THROUGH);? Could also DCHECK it's not STARTING. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:265: } while (result && !*defer && state_ != State::STARTING && I don't think State::STARTING is allowed here? We never enter the DoLoop with that state, and we never return to that state, right? https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:41: int num_resume_called() const { return num_resume_called_; } times_resume_called? num_times_resumed_called? resume_calls? https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:44: int num_resume_called_ = 0; DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:206: std::string old_handler_body; include <string> https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler_unittest.cc (right): https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:608: new InterceptingResourceHandler(base::MakeUnique<TestResourceHandler>( Hrm...Do we have any mime sniffer integration tests, that test both the mime sniffer and interceptor together? A bit concerned that we may be lacking coverage, given how closely the two classes' APIs are welded together.
The CQ bit was checked by yhirano@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.
The CQ bit was checked by yhirano@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...
https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:117: new_handler_->OnResponseCompleted(status, defer); On 2016/10/11 21:34:35, mmenke wrote: > This seems not quite right. If we're in > State::NOTIFYING_ON_RESPONSE_STARTED_TO_NEW_HANDLER or > State::WAITING_FOR_ON_READ_COMPLETED, we'll never tell any handler about the > completed response. > > Seems like the safest thing to do here is: > > void InterceptingResourceHandler::OnResponseCompleted(...) { > if (state_ != State::PASS_THROUGH) { > // |this| either is in the middle of transferring control to a handler, > // or has not yet been notified of the response starting. > // In the first case, just swap in the new handler and tell it of the > // failure. In the second case, just switch to pass through mode. > > if (new_handler_) > next_handler_ = std::move(new_handler_); > > state_ = State::PASS_THROUGH; > first_read_buffer_double_ = nullptr; > next_handler_->SetController(controller()); > } > > DCHECK(!new_handler_); > > return LayeredController::OnResponseCompleted(...); > } > > This would also let us get rid of two states. > > Also, add tests for this? When |state_| is STARTING and UseNewHandler has been called, there are two handlers but the suggested code calls only one OnResponseCompleted. Is it the expected behavior? When |state_| is one of NOTIFYING_ON_RESPONSE_STARTED_TO_NEW_HANDLER, WAITING_FOR_ON_READ_COMPLETED and SENDING_BUFFER_TO_NEW_HANDLER, |next_handler_| here is actually the new handler and |new_handler_| is null. In that case the suggested code works as same as PS8 (except for |defer| handling, which I fixed at PS10), I guess? Switching the controller is a great idea, Thank you! https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:123: void InterceptingResourceHandler::Cancel() { On 2016/10/11 21:34:35, mmenke wrote: > In all of these 4, DCHECK_NE(state_, State::PASS_THROUGH);? Could also DCHECK > it's not STARTING. Done. The state can be STARTING because uninteresting method calls can trigger the operations. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:185: } On 2016/10/10 19:40:15, mmenke wrote: > nit: Add blank line here? Done. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:187: net::URLRequestStatus status = {net::URLRequestStatus::SUCCESS, 0}; On 2016/10/10 19:40:16, mmenke wrote: > Just URLRequestStatus? Or use the constructor explicitly? > net::URLRequestStatus status(net::URLRequestStatus::SUCCESS, 0); Done. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:191: net::ERR_ABORTED); On 2016/10/10 19:40:16, mmenke wrote: > net::URLREQuestStatus::FromError(ERR_ABORTED). We want to get rid of > URLRequestStatus::CANCELED, some day. Done. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:193: next_handler_->OnResponseCompleted(status, &defer_ignored); On 2016/10/10 19:40:16, mmenke wrote: > Could trivially add support for this deferring the operation - don't think it's > worth a new test. Let me confirm what you mean by "trivially" - we need to add a new state to stop the controller from disposing the request. We may need another state for OnresponseCompleted. Do you agree with that? https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:202: return DoLoop(defer); On 2016/10/10 19:40:16, mmenke wrote: > This method is only called from DoLoop, so the above 5 lines should just be: > > return next_handler_->OnResponseStarted(response_.get(), defer); Done. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:265: } while (result && !*defer && state_ != State::STARTING && On 2016/10/11 21:34:35, mmenke wrote: > I don't think State::STARTING is allowed here? We never enter the DoLoop with > that state, and we never return to that state, right? You're right, thanks. Done. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:34: // - OnResponseCompleted on next_handler never sets |*defer|. On 2016/10/10 19:40:16, mmenke wrote: > nit: |next_handler| (x3), |new_handler|. Done. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:80: // |UseNewHandler| to the old handler and waiting for its completion via On 2016/10/10 19:40:16, mmenke wrote: > nit: |UseNewHandler| -> UseNewHandler. I don't think || are generally used > around method names. Done. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:116: bool DoLoop(bool* defer); On 2016/10/10 19:40:16, mmenke wrote: > nit: It's the general ResourceHandler/ResourceThrottle pattern, but seems like > the return value / defer should be documented, with maybe a little extra > documentation for DoLoop. Done. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:41: int num_resume_called() const { return num_resume_called_; } On 2016/10/11 21:34:35, mmenke wrote: > times_resume_called? num_times_resumed_called? resume_calls? Done. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:44: int num_resume_called_ = 0; On 2016/10/11 21:34:35, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:206: std::string old_handler_body; On 2016/10/11 21:34:35, mmenke wrote: > include <string> Done. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler_unittest.cc (right): https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:608: new InterceptingResourceHandler(base::MakeUnique<TestResourceHandler>( On 2016/10/11 21:34:35, mmenke wrote: > Hrm...Do we have any mime sniffer integration tests, that test both the mime > sniffer and interceptor together? A bit concerned that we may be lacking > coverage, given how closely the two classes' APIs are welded together. Actually I'm not sure how well MimeSniffingResourceHandler is tested. I agree that it would be great if MimeSniffingResourceHandler is tested with InterceptingResourceHandler, but isn't it a separate issue?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Want to do a pass over the tests today, and hopefully be ready to sign off on this tomorrow. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:193: next_handler_->OnResponseCompleted(status, &defer_ignored); On 2016/10/12 10:36:21, yhirano wrote: > On 2016/10/10 19:40:16, mmenke wrote: > > Could trivially add support for this deferring the operation - don't think > it's > > worth a new test. > > Let me confirm what you mean by "trivially" - we need to add a new state to stop > the controller from disposing the request. We may need another state for > OnresponseCompleted. Do you agree with that? Exactly that. We'd need one more state, but don't think it's a big deal. https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler_unittest.cc (right): https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:608: new InterceptingResourceHandler(base::MakeUnique<TestResourceHandler>( On 2016/10/12 10:36:21, yhirano wrote: > On 2016/10/11 21:34:35, mmenke wrote: > > Hrm...Do we have any mime sniffer integration tests, that test both the mime > > sniffer and interceptor together? A bit concerned that we may be lacking > > coverage, given how closely the two classes' APIs are welded together. > > Actually I'm not sure how well MimeSniffingResourceHandler is tested. I agree > that it would be great if MimeSniffingResourceHandler is tested with > InterceptingResourceHandler, but isn't it a separate issue? Oops...Misread your changes, thought you were getting rid of the user of InterceptingResourceHandler in these tests, rather than adding a lower level ResourceHandler (Erm...Yea, that wouldn't have compiled). Sorry about that. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:114: } You're right that I wasn't quite right about what happened here before. I had thought we started talking to new_handler before we swapped it in, but we don't, may have been thinking of an earlier patch set...or just confused. This code still seems weird, though...We only send the message to the new handler if the old handler didn't defer the response? I think we should either DCHECK(!*defer); after the first call, or just unconditionally destroy new_handler_. WDYT? I'm not set on that path, I just want to be careful here - cancellation is fraught with peril, and I've run into a bunch of bugs with how it works (Mostly in URLRequestJobs, which are below the URLRequest layer, admittedly)
This code was hard for me to follow :/ What follows are sort of suggestions but I'm not entirely sure how feasible they are. ---------------------- first_read_buffer_ and first_read_buffer_double_ are confusingly named. For the non pass-through case, first_read_buffer_double_ is initially written into (which in some sense makes me think it is the primary, not secondary buffer). first_read_buffer_ is used for all data sent to the old handler. Ideally it would be nice to be able to clearly demarcate which buffer is used for which handler (old_handler_buf, new_handler_buf). I have similar qualms about the handler names. I suspect it would complicate the code a lot but I'd love to be able to tell which handler is old and which is new when I see "next_handler_". https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:205: if (first_read_buffer_) { Add a comment that the old handler already received the first read? https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:74: // The InterceptingResourceHandler is waiting for the mime nit: reflow comment.
https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:66: false /* defer_on_response_completed */) {} Should this just call the next constructor down? https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:66: false /* defer_on_response_completed */) {} mime sniffer tests do: on_blah_result defer_on_blah on_blump_result defer_on_blump etc. Given how closely related the tests are, I don't think we should be using a different pattern between the two... On the other hand, this pattern does make sense, if we want the 2 argument consumer. This does seem like a painful refactor, but as-is, working on both sets of tests seems really confusing. Maybe fix it in a followup, and make the two test fixtures share a TestResourceHandler? https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:400: ASSERT_EQ("", old_handler_body); std::string()? I'm fine wither way, as long as we're consistent, but you're using std::string() in a bunch of other tests in this file. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:401: EXPECT_NE(kPayload, std::string(old_buffer->data())); Do we still need these old_buffer? They seem redundant. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:665: // The old handler and the new handler set |defer| to true. I'd mention where they set it to true (old handler in OnReadCompleted, new one in OnResponseStarted and OnReadCompleted) https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:738: EXPECT_EQ("The long l", old_handler_body); Think this needs a comment - think that goes for most code blocks below this point. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:763: EXPECT_EQ(old_handler_status.status(), net::URLRequestStatus::SUCCESS); Should we check this two blocks up, when it's actually set? https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:772: } Per comment, I'd like to see two cancellation (UnexpectedOnResponseCompleted called) tests (I don't think we have them yet) One while the old handler is deferring reading the response, one while the new handler is deferring OnResponseStarted. Main thing to check is the lower layer resource handlers gets informed of the call. Full correct behavior depends on the next ResourceHandler implementation, which we can't check.
The CQ bit was checked by yhirano@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 checked by yhirano@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 checked by yhirano@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 checked by yhirano@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...
https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:193: next_handler_->OnResponseCompleted(status, &defer_ignored); On 2016/10/12 15:04:01, mmenke wrote: > On 2016/10/12 10:36:21, yhirano wrote: > > On 2016/10/10 19:40:16, mmenke wrote: > > > Could trivially add support for this deferring the operation - don't think > > it's > > > worth a new test. > > > > Let me confirm what you mean by "trivially" - we need to add a new state to > stop > > the controller from disposing the request. We may need another state for > > OnresponseCompleted. Do you agree with that? > > Exactly that. We'd need one more state, but don't think it's a big deal. Done. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:114: } On 2016/10/12 15:04:01, mmenke wrote: > You're right that I wasn't quite right about what happened here before. I had > thought we started talking to new_handler before we swapped it in, but we don't, > may have been thinking of an earlier patch set...or just confused. > > This code still seems weird, though...We only send the message to the new > handler if the old handler didn't defer the response? I think we should either > DCHECK(!*defer); after the first call, or just unconditionally destroy > new_handler_. WDYT? I'm not set on that path, I just want to be careful here - > cancellation is fraught with peril, and I've run into a bunch of bugs with how > it works (Mostly in URLRequestJobs, which are below the URLRequest layer, > admittedly) The confusion may come from the fact that |next_handler_| is sometimes the new handler (as written in the comment). In PS10, we assume that the old handler's OnResponseCompleted never sets |defer|. That means |next_handler_| is the new handler at L109. It also means |new_handler_| is null, and these are what L108-L113 say. In PS12 that assumption is gone and I modified the code again to handle the new situation (but as written below I'm suspecting we need the assumption). https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:205: if (first_read_buffer_) { On 2016/10/12 19:38:21, Charlie Harrison wrote: > Add a comment that the old handler already received the first read? Done. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:74: // The InterceptingResourceHandler is waiting for the mime On 2016/10/12 19:38:21, Charlie Harrison wrote: > nit: reflow comment. Done. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:66: false /* defer_on_response_completed */) {} On 2016/10/12 20:10:47, mmenke wrote: > Should this just call the next constructor down? Done. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:66: false /* defer_on_response_completed */) {} On 2016/10/12 20:10:46, mmenke wrote: > mime sniffer tests do: > > on_blah_result > defer_on_blah > on_blump_result > defer_on_blump > > etc. Given how closely related the tests are, I don't think we should be using > a different pattern between the two... On the other hand, this pattern does > make sense, if we want the 2 argument consumer. This does seem like a painful > refactor, but as-is, working on both sets of tests seems really confusing. > Maybe fix it in a followup, and make the two test fixtures share a > TestResourceHandler? Added a TODO comment. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:400: ASSERT_EQ("", old_handler_body); On 2016/10/12 20:10:46, mmenke wrote: > std::string()? I'm fine wither way, as long as we're consistent, but you're > using std::string() in a bunch of other tests in this file. Done. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:401: EXPECT_NE(kPayload, std::string(old_buffer->data())); On 2016/10/12 20:10:46, mmenke wrote: > Do we still need these old_buffer? They seem redundant. Done. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:665: // The old handler and the new handler set |defer| to true. On 2016/10/12 20:10:46, mmenke wrote: > I'd mention where they set it to true (old handler in OnReadCompleted, new one > in OnResponseStarted and OnReadCompleted) Done. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:738: EXPECT_EQ("The long l", old_handler_body); On 2016/10/12 20:10:47, mmenke wrote: > Think this needs a comment - think that goes for most code blocks below this > point. Done. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:763: EXPECT_EQ(old_handler_status.status(), net::URLRequestStatus::SUCCESS); On 2016/10/12 20:10:46, mmenke wrote: > Should we check this two blocks up, when it's actually set? Done. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:772: } On 2016/10/12 20:10:47, mmenke wrote: > Per comment, I'd like to see two cancellation (UnexpectedOnResponseCompleted > called) tests (I don't think we have them yet) > > One while the old handler is deferring reading the response, one while the new > handler is deferring OnResponseStarted. Main thing to check is the lower layer > resource handlers gets informed of the call. Full correct behavior depends on > the next ResourceHandler implementation, which we can't check. I don't fully understand when we need to expect OnResponseCompleted. Let me check. 1. I'm sure once a ResourceHandler receives OnResponseCompleted it will never receive any message including OnResponseCompleted. That means deferred OnResponseCompleted cannot be canceled. 2. I'm not sure if it's ok to call OnResponseCompleted on a deferred ResourceHandler. If 2 is true (i.e., we need to expect OnResponseCompleted when deferred), I think it's difficult to throw away the assumption that the old handler's OnResponseComplete never sets |defer| which I removed between PS10 - PS11 based on [A]. The old handler's OnResonseCompleted cannot be canceled, so we need to take care of it in InterceptingResourceHandler, which needs more states. I'm also worried about the cancellation timing: Currently we fix the order of OnResponseCompleted (PS10 called the old handler's OnResponseCompleted first, PS12 calls the new handler's OnResponseCompleted first). But we need to call deferred ResourceHandler's OnResponseCompleted immediately - otherwise it will be very hard to control the deferred state. In short, If 2 is true, we need that assumption, I believe. WDYT? A: https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Cancellation is dark and full of horrors. https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:772: } On 2016/10/13 11:36:21, yhirano wrote: > On 2016/10/12 20:10:47, mmenke wrote: > > Per comment, I'd like to see two cancellation (UnexpectedOnResponseCompleted > > called) tests (I don't think we have them yet) > > > > One while the old handler is deferring reading the response, one while the new > > handler is deferring OnResponseStarted. Main thing to check is the lower > layer > > resource handlers gets informed of the call. Full correct behavior depends on > > the next ResourceHandler implementation, which we can't check. > > I don't fully understand when we need to expect OnResponseCompleted. Let me > check. > > 1. I'm sure once a ResourceHandler receives OnResponseCompleted it will never > receive any message including OnResponseCompleted. That means deferred > OnResponseCompleted cannot be canceled. This is true. I meant two tests with cancellation, not a test with two cancellations. > 2. I'm not sure if it's ok to call OnResponseCompleted on a deferred > ResourceHandler. This does happen. There's nothing stopping a request from being cancelled during the OnResponseCompleted call. It could be another ResourceHandler that does it (Even if it's not the ResourceHandler's turn, we have nothing to prevent it), or there could be something else that happens. If we enter suspend mode, for instance, we'll get a notification of cancellation, or if the renderer suddenly cancels the request (Maybe due to a new navigation?), we'll see a cancel in the middle of a deferred OnResponseCompleted call. It's precisely this case (And the same for OnReadCompleted) that I want to test, because it's a weird case. > If 2 is true (i.e., we need to expect OnResponseCompleted when deferred), I > think it's difficult to throw away the assumption that the old handler's > OnResponseComplete never sets |defer| which I removed between PS10 - PS11 based > on [A]. The old handler's OnResonseCompleted cannot be canceled, so we need to > take care of it in InterceptingResourceHandler, which needs more states. I'm not following this. > I'm also worried about the cancellation timing: Currently we fix the order of > OnResponseCompleted (PS10 called the old handler's OnResponseCompleted first, > PS12 calls the new handler's OnResponseCompleted first). But we need to call > deferred ResourceHandler's OnResponseCompleted immediately - otherwise it will > be very hard to control the deferred state. I don't understand what you mean by "calling deferred ResourceHandler's OnResponseCompleted". > In short, If 2 is true, we need that assumption, I believe. > > WDYT? > > A: > https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... https://codereview.chromium.org/2327463002/diff/360001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/360001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:104: DCHECK_NE(State::NOTIFYING_ON_RESPONSE_COMPLETED_TO_NEW_HANDLER, state_); What logic prevents this from happening?
https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2327463002/diff/280001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:772: } On 2016/10/13 17:35:47, mmenke wrote: > On 2016/10/13 11:36:21, yhirano wrote: > > On 2016/10/12 20:10:47, mmenke wrote: > > > Per comment, I'd like to see two cancellation (UnexpectedOnResponseCompleted > > > called) tests (I don't think we have them yet) > > > > > > One while the old handler is deferring reading the response, one while the > new > > > handler is deferring OnResponseStarted. Main thing to check is the lower > > layer > > > resource handlers gets informed of the call. Full correct behavior depends > on > > > the next ResourceHandler implementation, which we can't check. > > > > I don't fully understand when we need to expect OnResponseCompleted. Let me > > check. > > > > 1. I'm sure once a ResourceHandler receives OnResponseCompleted it will never > > receive any message including OnResponseCompleted. That means deferred > > OnResponseCompleted cannot be canceled. > > This is true. I meant two tests with cancellation, not a test with two > cancellations. > > > 2. I'm not sure if it's ok to call OnResponseCompleted on a deferred > > ResourceHandler. > > This does happen. There's nothing stopping a request from being cancelled > during the OnResponseCompleted call. It could be another ResourceHandler that > does it (Even if it's not the ResourceHandler's turn, we have nothing to prevent > it), or there could be something else that happens. If we enter suspend mode, > for instance, we'll get a notification of cancellation, or if the renderer > suddenly cancels the request (Maybe due to a new navigation?), we'll see a > cancel in the middle of a deferred OnResponseCompleted call. > > It's precisely this case (And the same for OnReadCompleted) that I want to test, > because it's a weird case. > > > If 2 is true (i.e., we need to expect OnResponseCompleted when deferred), I > > think it's difficult to throw away the assumption that the old handler's > > OnResponseComplete never sets |defer| which I removed between PS10 - PS11 > based > > on [A]. The old handler's OnResonseCompleted cannot be canceled, so we need to > > take care of it in InterceptingResourceHandler, which needs more states. > > I'm not following this. > > > I'm also worried about the cancellation timing: Currently we fix the order of > > OnResponseCompleted (PS10 called the old handler's OnResponseCompleted first, > > PS12 calls the new handler's OnResponseCompleted first). But we need to call > > deferred ResourceHandler's OnResponseCompleted immediately - otherwise it will > > be very hard to control the deferred state. > > I don't understand what you mean by "calling deferred ResourceHandler's > OnResponseCompleted". > > > In short, If 2 is true, we need that assumption, I believe. > > > > WDYT? > > > > A: > > > https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader... > I wrote slides for explanation: https://docs.google.com/a/chromium.org/presentation/d/1eZa6oOaU-BoRAqLB3JN3jU... https://codereview.chromium.org/2327463002/diff/360001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/360001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:104: DCHECK_NE(State::NOTIFYING_ON_RESPONSE_COMPLETED_TO_NEW_HANDLER, state_); On 2016/10/13 17:35:47, mmenke wrote: > What logic prevents this from happening? This comes from the fact that OnResponseCompleted will never be called again.
The CQ bit was checked by yhirano@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.
PS15 brings back the assumption that the old handler's OnResponseCompleted finished synchronously, because I now believe removing it costs a lot of complexity.
https://codereview.chromium.org/2327463002/diff/380001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/380001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:104: if (!new_handler_) { Let's add tests that go through both these paths, and I'm happy with this.
The CQ bit was checked by yhirano@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 checked by yhirano@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 checked by yhirano@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 checked by yhirano@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...
Patchset #19 (id:460001) has been deleted
Dry run: 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...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2327463002/diff/380001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2327463002/diff/380001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:104: if (!new_handler_) { On 2016/10/14 14:52:15, mmenke wrote: > Let's add tests that go through both these paths, and I'm happy with this. Done.
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 yhirano@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.
LGTM!
The CQ bit was checked by yhirano@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
Exceeded global retry quota
The CQ bit was checked by yhirano@chromium.org
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.
Description was changed from ========== Relax ad-hoc assumptions on InterceptingResourceHandler InterceptingResourceHandler holds two ResourceHandlers but it requires those ResourceHandlers to meet certain conditions in addition to the usual ResourceHandler contract. This CL removes some (not all) requirements so that it can work with MojoAsyncResourceHandler. With this CL, - OnResponseStarted on the old handler can return false. - OnResponseStarted on the new handler can return false. - OnResponseStarted on the new handler can defer processing. - OnWillStart on the old handler can return false. - OnReadCompleted on the old handler can return false. - OnReadCompleted on the old handler can defer processing. - the payload given to |UseHandler| can be longer than the size of the buffer exposed via OnWillRead. - InterceptingResourceHandler reads only "committed" data. Previously, it read data on the buffer exposed by OnWillRead before OnReadCompleted was called. BUG=603396 ========== to ========== Relax ad-hoc assumptions on InterceptingResourceHandler InterceptingResourceHandler holds two ResourceHandlers but it requires those ResourceHandlers to meet certain conditions in addition to the usual ResourceHandler contract. This CL removes some (not all) requirements so that it can work with MojoAsyncResourceHandler. With this CL, - OnResponseStarted on the old handler can return false. - OnResponseStarted on the new handler can return false. - OnResponseStarted on the new handler can defer processing. - OnWillStart on the old handler can return false. - OnReadCompleted on the old handler can return false. - OnReadCompleted on the old handler can defer processing. - the payload given to |UseHandler| can be longer than the size of the buffer exposed via OnWillRead. - InterceptingResourceHandler reads only "committed" data. Previously, it read data on the buffer exposed by OnWillRead before OnReadCompleted was called. BUG=603396 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== Relax ad-hoc assumptions on InterceptingResourceHandler InterceptingResourceHandler holds two ResourceHandlers but it requires those ResourceHandlers to meet certain conditions in addition to the usual ResourceHandler contract. This CL removes some (not all) requirements so that it can work with MojoAsyncResourceHandler. With this CL, - OnResponseStarted on the old handler can return false. - OnResponseStarted on the new handler can return false. - OnResponseStarted on the new handler can defer processing. - OnWillStart on the old handler can return false. - OnReadCompleted on the old handler can return false. - OnReadCompleted on the old handler can defer processing. - the payload given to |UseHandler| can be longer than the size of the buffer exposed via OnWillRead. - InterceptingResourceHandler reads only "committed" data. Previously, it read data on the buffer exposed by OnWillRead before OnReadCompleted was called. BUG=603396 ========== to ========== Relax ad-hoc assumptions on InterceptingResourceHandler InterceptingResourceHandler holds two ResourceHandlers but it requires those ResourceHandlers to meet certain conditions in addition to the usual ResourceHandler contract. This CL removes some (not all) requirements so that it can work with MojoAsyncResourceHandler. With this CL, - OnResponseStarted on the old handler can return false. - OnResponseStarted on the new handler can return false. - OnResponseStarted on the new handler can defer processing. - OnWillStart on the old handler can return false. - OnReadCompleted on the old handler can return false. - OnReadCompleted on the old handler can defer processing. - the payload given to |UseHandler| can be longer than the size of the buffer exposed via OnWillRead. - InterceptingResourceHandler reads only "committed" data. Previously, it read data on the buffer exposed by OnWillRead before OnReadCompleted was called. BUG=603396 Committed: https://crrev.com/22b69a9b20e099fbe14c70b621bc630043222146 Cr-Commit-Position: refs/heads/master@{#426134} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/22b69a9b20e099fbe14c70b621bc630043222146 Cr-Commit-Position: refs/heads/master@{#426134} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
