Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10905259:
Refactor blocking network delegates (Closed)
|
Created:
8 years, 3 months ago by vabr (Chromium) Modified:
8 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThere are two blocking network delegates in url_request_unittest:
BlockingNetworkDelegate
and
BlockingNetworkDelegateWithManualCallback
They provide various functions:
* blocking in various stages of the request
* three ways of responding in blocked stages: synchronous, automatic callback, and user-triggered callback
* redirection
But not all combinations are available (e.g. synchronous blocking after headers are sent), because the user has to chose only one of the delegates. This CL aims at allowing to combine those functions without code duplication, by joining the two classes into one.
BUG=146816
TEST=N/A (this is only in unittests, also no change in behaviour of any unit-test, and no new added)
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158724
Patch Set 1 #
Total comments: 9
Patch Set 2 : #Patch Set 3 : Both original BlockingNetworkDelegates merged into one #
Total comments: 39
Patch Set 4 : Comments addressed #
Total comments: 1
Patch Set 5 : Rebased #
Total comments: 21
Patch Set 6 : Comments addressed, part 1 (without changing message loop usage yet) #
Total comments: 25
Patch Set 7 : Comments addressed, part 2 (changes to WaitForState) #Patch Set 8 : Conversion of tests using BlockingNetworkDelegate* corrected + getting rid of WaitUntilBlocked #
Total comments: 36
Patch Set 9 : Comments addressed #Patch Set 10 : Added requested unit-test + BlockOn rewritten to set_block_on #Patch Set 11 : Fixed clean-up in NetworkDelegateBlockAsynchronously #
Total comments: 4
Patch Set 12 : Moving NetworkDelegateBlockAsynchronously up #Patch Set 13 : Disabling NetworkDelegateBlockAsynchronously in CF #
Messages
Total messages: 40 (0 generated)
Hi Matt, To avoid code duplication when adding the synchronous-cancel unit-test in http://codereview.chromium.org/10911151/, I believe I need this refactoring of the blocking network delegates. Could you please take a look at it? Once this is landed, I will add the synchronous-cancel unit-test to http://codereview.chromium.org/10911151/ and also remove the TODO from this CL. Thanks! Vaclav
I'm a bit concerned about the extra complexity inheritance this introduces, and some of the code seems a little on the hackish side. While I'm not adamantly against this, I wonder if it would be simpler to just rename "BlockingNetworkDelegateBase" and use it directly, and revert the other changes. The downside is we'd have to duplicate a little code, including the state checks. Just FYI, I'm taking tomorrow off, but I'll be back next week. http://codereview.chromium.org/10905259/diff/1/net/url_request/url_request_un... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/1/net/url_request/url_request_un... net/url_request/url_request_unittest.cc:233: GURL* new_url) { Suggest you inline these, for consistency with the rest of this file. Also think the functions are short enough, that the choice increases readability in this case. http://codereview.chromium.org/10905259/diff/1/net/url_request/url_request_un... net/url_request/url_request_unittest.cc:235: if ((block_on_ & ON_BEFORE_URL_REQUEST) != 0) { nit: Suggest you put the "== 0" case first, for consistency. http://codereview.chromium.org/10905259/diff/1/net/url_request/url_request_un... net/url_request/url_request_unittest.cc:314: // TODO(vabr): remove when corresponding On* overrides are implemented. Aren't these already implemented by BlockingNetworkDelegateBase?
Hi Matt, Thanks for a quick response and the heads-up about your day off. In my opinion, avoiding the code duplication is worth introducing the inheritance. Having multiple classes with overlapping usage (synchronous cancelling on before URL request would be done by two classes) is also a bit confusing, at least for me. You mentioned extra complexity and hackish parts. I would gladly try to improve that rather than duplicate the code. Can you suggest in more detail, which parts should be paid attention to? I can see that, e.g., BlockingNetworkDelegateBase::OnAuthRequired looks hackish when returning *CANCEL_AUTH. If that part is confusing for its arbitraryness, I can try to find another way to signal (block_on_ & ON_AUTH_REQUIRED) to the descendants. What do you think? Thanks for your time, Vaclav http://codereview.chromium.org/10905259/diff/1/net/url_request/url_request_un... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/1/net/url_request/url_request_un... net/url_request/url_request_unittest.cc:233: GURL* new_url) { On 2012/09/13 15:28:02, Matt Menke wrote: > Suggest you inline these, for consistency with the rest of this file. Also > think the functions are short enough, that the choice increases readability in > this case. I see the lack of consistency, but these are the reasons I have against in-lining: * Cascading in-lining in a class hierarchy accumulates long chunks of code which are copied on each call place, leading to a code bloat. Scott Meyers makes this point especially about constructors in Item 30 of Effective C++ (3rd edition), but it applies to member functions as well. * The style guide says in particular that "virtual functions should never be declared this way" -- note that the On* methods are virtual. I'm actually tempted to change this in the existing two classes. In general, the style guide is against in-lining except for cases where it is proven as necessary. I guess such a proof should involve a profiler, not just readability or consistency. My last reason is subjective, but for readability, I actually think that moving function definition out of class definition is a plus. The interface of the class fits in a much less space, and is thus easier to read. People interested in the definition only have to seek at the end of the class. http://codereview.chromium.org/10905259/diff/1/net/url_request/url_request_un... net/url_request/url_request_unittest.cc:235: if ((block_on_ & ON_BEFORE_URL_REQUEST) != 0) { On 2012/09/13 15:28:02, Matt Menke wrote: > nit: Suggest you put the "== 0" case first, for consistency. Done, also on the places below. Thanks! http://codereview.chromium.org/10905259/diff/1/net/url_request/url_request_un... net/url_request/url_request_unittest.cc:314: // TODO(vabr): remove when corresponding On* overrides are implemented. On 2012/09/13 15:28:02, Matt Menke wrote: > Aren't these already implemented by BlockingNetworkDelegateBase? Yes, they are, but they will not post the callback task, so the behaviour would not be as expected.
On 2012/09/13 17:23:35, vabr (Chromium) wrote: > Hi Matt, > > Thanks for a quick response and the heads-up about your day off. > > In my opinion, avoiding the code duplication is worth introducing the > inheritance. Having multiple classes with overlapping usage (synchronous > cancelling on before URL request would be done by two classes) is also a bit > confusing, at least for me. You could use the new class for the one or two tests that use synchronous cancellation, and remove the capability of the old one doing so, if you so desired. > You mentioned extra complexity and hackish parts. I would gladly try to improve > that rather than duplicate the code. Can you suggest in more detail, which parts > should be paid attention to? The base class is just weird. I don't think the division of labor is natural - it inherits from an interface, and overrides functions for that interface, which actually don't do all the work they should - which is why it has a protected constructor, and not used individually. Given its description, I'd argue it would be more logical to make it a helper class, rather than a base class. Alternatively, if we're going to keep it as-is, it should be responsible for tracking callbacks, which would make a big chunk of BlockingNetworkDelegateWithManualCallback unnecessary. This would also help address your TODO. Also, it seems like auth_retval_ should be in the base class, not a child class. I think it should be set up more like the others. If the functions are not inlined, it should be documented when they do weird things in the class declaration. If you want to de-inline more stuff, I'm fine with that, on the condition that you provide detailed enough documentation so that the class declarations are usable without scrolling down to function bodies. Another oddity: The call to "network_delegate.set_retval(OK);" and calling "network_delegate.BlockOn(BlockingNetworkDelegateBase::ON_BEFORE_URL_REQUEST);" when retval_ is OK, since OK doesn't block. Think that covers the issues that bother me about this CL. My inline comments are basically redundant with the above text. > I can see that, e.g., BlockingNetworkDelegateBase::OnAuthRequired looks hackish > when returning *CANCEL_AUTH. If that part is confusing for its arbitraryness, I > can try to find another way to signal (block_on_ & ON_AUTH_REQUIRED) to the > descendants. > > What do you think? > > Thanks for your time, > > Vaclav http://codereview.chromium.org/10905259/diff/1/net/url_request/url_request_un... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/1/net/url_request/url_request_un... net/url_request/url_request_unittest.cc:233: GURL* new_url) { On 2012/09/13 17:23:35, vabr (Chromium) wrote: > On 2012/09/13 15:28:02, Matt Menke wrote: > > Suggest you inline these, for consistency with the rest of this file. Also > > think the functions are short enough, that the choice increases readability in > > this case. > > I see the lack of consistency, but these are the reasons I have against > in-lining: > > * Cascading in-lining in a class hierarchy accumulates long chunks of code which > are copied on each call place, leading to a code bloat. Scott Meyers makes this > point especially about constructors in Item 30 of Effective C++ (3rd edition), > but it applies to member functions as well. > > * The style guide says in particular that "virtual functions should never be > declared this way" -- note that the On* methods are virtual. I'm actually > tempted to change this in the existing two classes. In general, the style guide > is against in-lining except for cases where it is proven as necessary. I guess > such a proof should involve a profiler, not just readability or consistency. Rightly or wrongly, in practice this rule tends to be largely ignored in classes declared in C++ files. > My last reason is subjective, but for readability, I actually think that moving > function definition out of class definition is a plus. The interface of the > class fits in a much less space, and is thus easier to read. People interested > in the definition only have to seek at the end of the class. I don't agree with this. BlockingNetworkDelegateBase does not actually implement the TestNetworkDelegate interface, and in fact interacts with its child classes in a rather non-transparent manner. Without reading both the base class and the subclasses in their entirety, it's not clear what they actually do. As such, I found reading the BlockingNetworkDelegateBase declaration leaving me with more questions than answers. If you want to add documentation to make this no longer the case, I'm fine with that. http://codereview.chromium.org/10905259/diff/1/net/url_request/url_request_un... net/url_request/url_request_unittest.cc:314: // TODO(vabr): remove when corresponding On* overrides are implemented. On 2012/09/13 17:23:35, vabr (Chromium) wrote: > On 2012/09/13 15:28:02, Matt Menke wrote: > > Aren't these already implemented by BlockingNetworkDelegateBase? > > Yes, they are, but they will not post the callback task, so the behaviour would > not be as expected. This sort of gets at one of my main complaints - the functions of the base class implement the functionality needed by the interface functions of the same name. Callbacks are handled and recorded by the child classes based on the parent class's return value of the function with the same name.
http://codereview.chromium.org/10905259/diff/1/net/url_request/url_request_un... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/1/net/url_request/url_request_un... net/url_request/url_request_unittest.cc:314: // TODO(vabr): remove when corresponding On* overrides are implemented. On 2012/09/13 18:48:07, Matt Menke wrote: > On 2012/09/13 17:23:35, vabr (Chromium) wrote: > > On 2012/09/13 15:28:02, Matt Menke wrote: > > > Aren't these already implemented by BlockingNetworkDelegateBase? > > > > Yes, they are, but they will not post the callback task, so the behaviour > would > > not be as expected. > > This sort of gets at one of my main complaints - the functions of the base class > implement the functionality needed by the interface functions of the same name. > Callbacks are handled and recorded by the child classes based on the parent > class's return value of the function with the same name. Sorry, that was worded really poorly. And I ended up including the issue in my other comments, anyawys. If you need me to clarify anything, just ask.
Hi Matt, Thanks for being more specific with your objections! > (...) overrides functions for that interface, which actually don't do all the work they should That's a very good point, as well as the one about unnatural labor division. I tried to take a step back and see what kind of functions the two existing delegates provide: * blocking in various stages of the request * three ways of responding in blocked stages: synchronous, automatic callback, and user-triggered callback * redirection (Some of the combinations are currently missing, but completing that is the point of this CL.) It started to make much more sense to me to join them together in one class instead of adding a third one. It does not seem too big to me, what do you think? It now supports any combination of the above functions, and should make it easier to choose and configure a blocking network delegate. Following your suggestion, I added documentation for de-inlined methods. I would like to keep them not inline, because the methods grew a little bit after the join. Feel free to request more detailed documentation. What do you think about the code and comments now? I also had to add a line in TestNetworkDelegate::OnAuthRequired. I believe that the update of |next_states_| there was wrong -- if the request gets cancelled between OnAuthRequired and a callback, the next stage is kStageCompletedError. It did not show up in the original code, because BlockingNetworkDelegateWithManualCallback::OnAuthRequired did not call TestNetworkDelegate::OnAuthRequired. Once again, thanks for your time spent on this. You help me learn this part of code. Vaclav
I think merging the two is a reasonable idea - considered suggesting it myself. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:151: // * to synchronously return a pre-specified value, or nit: pre-specified error code? http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:160: enum Stage { // Stages in which the delegate can block. Please put the comment on the above line. For enums, classes, and functions, think it's best to put the API documentation on its own line. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:166: }; nit: Add blank line. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:170: USER_CALLBACK // User takes care of doing a callback. I suggest you rename this to "BlockMode" and rename the corresponding field as well. Open to other ideas - I just want the name to imply how it relates to other values. Also would like the comments to be clearer. Here's my suggestions, open to better ideas, or moving some of the comments elsewhere: // Behavior during BlockedStages. During other stages, just // returns net::OK or NetworkDelegate::AUTH_REQUIRED_RESPONSE_NO_ACTION enum BlockMode { SYNCHRONOUS, // No callback, returns specified return values. AUTO_CALLBACK, // |this| takes care of doing a callback using the specified return codes. USER_CALLBACK, // User takes care of doing a callback. |retval_| and |auth_retval_| are ignored. } http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:173: // Default constructor: creates a delegate which does not block at all. I think you can remove the "Default constructor:", since it's the only constructor. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:174: explicit BlockingNetworkDelegate(Processing p); Single letter variable names like this violate Google style. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:178: ASSERT_NE(retval, ERR_IO_PENDING); I suggest an ASSERT_NE(retval, OK) as well. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:203: void WaitForState(Stage stage) { Should we make sure the mode isn't USER_CALLBACK for this? http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:242: // Return values. Must not be *IO_PENDING. nit: "Values returned on blocking stages when mode is SYNCHRONOUS or AUTO_CALLBACK" http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:280: return OK; In all cases, I think it makes sense to clear blocked_stage_ when we don't block. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:282: blocked_stage_ = ON_BEFORE_URL_REQUEST; May want an EXPECT_NE(OK, retval_) around here. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:287: switch (processing_) { We have this nearly identical code in 3 locations. My suggestion: Move setting redirect_url_ up to just after the call to TestNetworkDelegate, then turn the shared code into a single shared function: return MaybeBlockOnStage(Stage); http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:289: return retval_; I don't think blocked_stage_ should be set on the synchronous case. May want to rename it stage_blocked_for_callback_ or something. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:304: return 0; optional nits: Suggest making this the default case, just so the switch statement returns on all paths. I also don't think the comment is needed, since the pattern is pretty common across the code base for the same reason, but if you disagree, that's fine. Same goes for the other cases as well. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:424: void BlockingNetworkDelegate::DoAutoCallback( I think having two completely unrelated pairs of callback functions is a little weird. Wonder if there's a way to make one call into the other. Unfortunately, I'm not sure there's a good way to do this, without either having to worry about destructor order, or changing behavior slightly. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:432: *credentials = auth_credentials_; Looks like we don't support AUTH_REQUIRED_RESPONSE_SET_AUTH in the USER_CALLBACK case. May want to add support, to keep the two paths consistent.
http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:178: ASSERT_NE(retval, ERR_IO_PENDING); On 2012/09/17 19:25:23, Matt Menke wrote: > I suggest an ASSERT_NE(retval, OK) as well. Err... ASSERT_NE(OK, retval), rather. You should reverse the order of your asserts - first param should be the expected / unexpected value, second should be the value you're checking. Rule only applies to _NE and _EQ, not to comparison functions.
Hi Matt, Thanks for your careful review. I like the improvements you suggested! PTAL, Vaclav http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:151: // * to synchronously return a pre-specified value, or On 2012/09/17 19:25:23, Matt Menke wrote: > nit: pre-specified error code? Done. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:160: enum Stage { // Stages in which the delegate can block. On 2012/09/17 19:25:23, Matt Menke wrote: > Please put the comment on the above line. For enums, classes, and functions, > think it's best to put the API documentation on its own line. Done. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:166: }; On 2012/09/17 19:25:23, Matt Menke wrote: > nit: Add blank line. Done. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:170: USER_CALLBACK // User takes care of doing a callback. On 2012/09/17 19:25:23, Matt Menke wrote: > I suggest you rename this to "BlockMode" and rename the corresponding field as > well. Open to other ideas - I just want the name to imply how it relates to > other values. Also would like the comments to be clearer. > > Here's my suggestions, open to better ideas, or moving some of the comments > elsewhere: > > // Behavior during BlockedStages. During other stages, just > // returns net::OK or NetworkDelegate::AUTH_REQUIRED_RESPONSE_NO_ACTION > enum BlockMode { > SYNCHRONOUS, // No callback, returns specified return values. > AUTO_CALLBACK, // |this| takes care of doing a callback using the specified > return codes. > USER_CALLBACK, // User takes care of doing a callback. |retval_| and > |auth_retval_| are ignored. > } I really like both the new enum name, and the extended comments. The only change I made was BlockedStages -> blocked stages, because the former is neither a variable or type, nor a common English word. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:173: // Default constructor: creates a delegate which does not block at all. On 2012/09/17 19:25:23, Matt Menke wrote: > I think you can remove the "Default constructor:", since it's the only > constructor. Done. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:174: explicit BlockingNetworkDelegate(Processing p); On 2012/09/17 19:25:23, Matt Menke wrote: > Single letter variable names like this violate Google style. Done. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:178: ASSERT_NE(retval, ERR_IO_PENDING); > You should reverse the order of your asserts - first param should be the > expected / unexpected value, second should be the value you're checking. Rule > only applies to _NE and _EQ, not to comparison functions. Thanks, that's good to know! Done. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:178: ASSERT_NE(retval, ERR_IO_PENDING); On 2012/09/17 19:25:23, Matt Menke wrote: > I suggest an ASSERT_NE(retval, OK) as well. Done. Do you think there might be a reason for the user to set AUTH_REQUIRED_RESPONSE_NO_ACTION (let's assume they ignore the fact that it's being set by the constructor) in set_auth_retval(), or should I check against that as well? If I should check, then I will also add an aut_retval_ != AUTH_REQUIRED_RESPONSE_NO_ACTION in OnAuthRequired(), and amend the comment at BlockOn() declaration. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:203: void WaitForState(Stage stage) { On 2012/09/17 19:25:23, Matt Menke wrote: > Should we make sure the mode isn't USER_CALLBACK for this? Actually, it only makes sense to call this if the mode _is_ USER_CALLBACK, no? At least this is how it's used now. I added the check against other modes, because I don't think there is any use for WaitForState with SYNCHRONOUS or AUTO_CALLBACK modes, and checking that might signal an incorrect use of the delegate. WDYT? http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:242: // Return values. Must not be *IO_PENDING. On 2012/09/17 19:25:23, Matt Menke wrote: > nit: "Values returned on blocking stages when mode is SYNCHRONOUS or > AUTO_CALLBACK" Done. I also removed the part about IO_PENDING, because it was confusing (the constructor is allowed to assign this, but not the user). http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:280: return OK; On 2012/09/17 19:25:23, Matt Menke wrote: > In all cases, I think it makes sense to clear blocked_stage_ when we don't > block. Done. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:282: blocked_stage_ = ON_BEFORE_URL_REQUEST; On 2012/09/17 19:25:23, Matt Menke wrote: > May want an EXPECT_NE(OK, retval_) around here. Done, with the exclusion of the case when we are just redirecting. Also amended the comment at BlockOn(). I would prefer ASSERT here rather than EXPECT, but that would not work in a function returning non-void. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:287: switch (processing_) { On 2012/09/17 19:25:23, Matt Menke wrote: > We have this nearly identical code in 3 locations. My suggestion: Move setting > redirect_url_ up to just after the call to TestNetworkDelegate, then turn the > shared code into a single shared function: > > return MaybeBlockOnStage(Stage); Done. I like this suggestion very much, thanks! I also changed the comments at the start of the class def., and at redirect_url_ declaration, to reflect that redirection is independent of blocking. To extend this also for use in OnAuthRequired a template function would probably needed, which does not look as worth the effort to me. WDYT? http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:289: return retval_; On 2012/09/17 19:25:23, Matt Menke wrote: > I don't think blocked_stage_ should be set on the synchronous case. May want to > rename it stage_blocked_for_callback_ or something. Done. Also in OnAuthRequired. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:304: return 0; On 2012/09/17 19:25:23, Matt Menke wrote: > optional nits: Suggest making this the default case, just so the switch > statement returns on all paths. I also don't think the comment is needed, since > the pattern is pretty common across the code base for the same reason, but if > you disagree, that's fine. > > Same goes for the other cases as well. I also don't have strong opinions. My argument against the "default:" is that compiler will check that all the cases are handled if I don't put "default:" there. I removed the comment, since I believe you with the common pattern, and also it takes probably less time to understand the code than to read the sentence. If you change your mind more strongly in favour of introducing "default:", just let me know and I'll do it. http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:424: void BlockingNetworkDelegate::DoAutoCallback( On 2012/09/17 19:25:23, Matt Menke wrote: > I think having two completely unrelated pairs of callback functions is a little > weird. Wonder if there's a way to make one call into the other. Unfortunately, > I'm not sure there's a good way to do this, without either having to worry about > destructor order, or changing behavior slightly. I tried to make DoCallback call DoAutoCallback (renamed to RunCallback) and analogously for auth. Which destructors are you worried about? http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:432: *credentials = auth_credentials_; On 2012/09/17 19:25:23, Matt Menke wrote: > Looks like we don't support AUTH_REQUIRED_RESPONSE_SET_AUTH in the USER_CALLBACK > case. May want to add support, to keep the two paths consistent. Done (see in OnAuthRequired, case SYNCHRONOUS). http://codereview.chromium.org/10905259/diff/16001/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/16001/net/url_request/url_reques... net/url_request/url_request_unittest.cc:400: ASSERT_TRUE(target_auth_credentials_ != NULL); For some reason ASSERT_NE(NULL, target_auth_credentials_) failed to compile, complaining about non-pointer types. Because failing the assert here implies that target_auth_credentials_ has NULL, there is no information lost against using ASSERT_NE.
Sorry I didn't get to this yesterday. And sorry to hold up your other CL for this - I didn't intend to get us so far off track. https://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reque... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reque... net/url_request/url_request_unittest.cc:203: void WaitForState(Stage stage) { On 2012/09/18 17:21:48, vabr (Chromium) wrote: > On 2012/09/17 19:25:23, Matt Menke wrote: > > Should we make sure the mode isn't USER_CALLBACK for this? > > Actually, it only makes sense to call this if the mode _is_ USER_CALLBACK, no? > At least this is how it's used now. > > I added the check against other modes, because I don't think there is any use > for WaitForState with SYNCHRONOUS or AUTO_CALLBACK modes, and checking that > might signal an incorrect use of the delegate. > > WDYT? Erm...You're right. I was thinking everything was done synchronously, but only the delegate is synchronous, its external callers may not be. I don't like this function - it's weird, and waiting for the wrong stage will result in it hanging. Seems like waiting until callback_ or auth_callback_ is non-NULL would make more sense, and renaming the function "WaitForNextBlockedState" or some such, and checking that the mode is USER_CALLBACK makes more sense. However, even simpler than that - we could just explicitly exit the message loop when we hit a blocked state in the USER_CALLBACK Mode, and get rid of this function entirely. https://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reque... net/url_request/url_request_unittest.cc:304: return 0; On 2012/09/18 17:21:48, vabr (Chromium) wrote: > On 2012/09/17 19:25:23, Matt Menke wrote: > > optional nits: Suggest making this the default case, just so the switch > > statement returns on all paths. I also don't think the comment is needed, > since > > the pattern is pretty common across the code base for the same reason, but if > > you disagree, that's fine. > > > > Same goes for the other cases as well. > > I also don't have strong opinions. > My argument against the "default:" is that compiler will check that all the > cases are handled if I don't put "default:" there. > I removed the comment, since I believe you with the common pattern, and also it > takes probably less time to understand the code than to read the sentence. > > If you change your mind more strongly in favour of introducing "default:", just > let me know and I'll do it. Your argument makes sense. I was not aware of that compiler warning. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: The new rule is to leave this alone, and last I checked, it's still 2012. Also, I may not be the fastest reviewer in the world, but I'm not *that* slow. :) https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:150: // to block in. On blocking, the delegate allows either: nit: For readability, suggest you replace the second sentence with "When blocking, the delegate can do one of the following:", and removing the "to" from the following lines. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:159: class BlockingNetworkDelegate : public TestNetworkDelegate { TestNetworkDelegate posts a task to quit the current message loop in OnResponseCompleted, by default. All the non-USER_CALLBACK tests expect this. The USER_CALLBACK tests do not - They use the WaitForWhatever function instead of running the message loop. We could change the behavior for the USER_CALLBACK tests to set TestDelegate::quit_on_complete_. Another option would be to just run the message loop at the end of the USER_CALLBACK tests, which would make them more consistent with the other tests. I believe cancelling should result in calling OnResponseCompleted. We could just replace USER_CALLBACK with ASYNC_CANCEL, since all the USER_CALLBACK tests do the same thing, but we'd lose flexibility that way. This behavior is sufficiently non-obvious that I suggest adding a comments to both BlockingNetworkDelegate and TestNetworkDelegate about it. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:175: // return codes. nit: "|this| posts a task to run the callback using..." https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:181: explicit BlockingNetworkDelegate(BlockMode mode); nit: Just for consistency with the class field name, suggest |block_mode|. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:342: stage_blocked_for_callback_ = NOT_BLOCKED; Don't think there's any way for this not to be true at this point, since it's reset in Do[Auth]Callback. May even want to EXPECT on it. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:377: ASSERT_NE(NOT_BLOCKED, stage_blocked_for_callback_); nit: ASSERT_NE(ON_AUTH_REQUIRED, stage_blocked_for_callback_); https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:409: stage_blocked_for_callback_ = NOT_BLOCKED; Don't think there's any way for this not to be true at this point, since it's reset in Do[Auth]Callback. May even want to EXPECT on it. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:415: if (redirect_url_.is_empty() || stage != ON_BEFORE_URL_REQUEST) This if statement isn't needed anymore, is it? If redirect_url_ is set and retval_ is OK, then we don't need to be blocking on ON_BEFORE_URL_REQUEST. If any tests currently do this, can just have them not block on that stage. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:418: stage_blocked_for_callback_ = stage; optional: Do we want to set this on AUTO_CALLBACK as well? Doesn't really matter, either way, but might be a little more consistent.
Hi Matt, Before I dive into the technical part: I'm really thankful to you for putting so much care and time into this review! And you are very fast, I did not expect you to answer this week at all, given it is the perf season right now. The "(c) 2013" mishap was by no means a comment on your pace :) -- I actually don't remember changing the copyright, and I suspect some of the pre-submit scripts or what went crazy and did that. Now, I had some troubles to understand the issues and solutions regarding automatic quitting the message loops. Please see my response to your comment: https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... Another point for discussion is the "WaitForWhatever" function -- again, please see the comment: https://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reque... Finally, I realised that after all those changes, the tests NetworkDelegateRedirectRequest and NetworkDelegateRedirectRequestSynchronously now do exactly the same. I'm not really sure what they were meant to do originally, and thinking of leaving only one of them. What do you think? https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... Again, thanks a lot for having put your effort into this, I really appreciate it! Vaclav https://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reque... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reque... net/url_request/url_request_unittest.cc:203: void WaitForState(Stage stage) { On 2012/09/19 19:39:47, Matt Menke wrote: > On 2012/09/18 17:21:48, vabr (Chromium) wrote: > > On 2012/09/17 19:25:23, Matt Menke wrote: > > > Should we make sure the mode isn't USER_CALLBACK for this? > > > > Actually, it only makes sense to call this if the mode _is_ USER_CALLBACK, no? > > At least this is how it's used now. > > > > I added the check against other modes, because I don't think there is any use > > for WaitForState with SYNCHRONOUS or AUTO_CALLBACK modes, and checking that > > might signal an incorrect use of the delegate. > > > > WDYT? > > Erm...You're right. I was thinking everything was done synchronously, but only > the delegate is synchronous, its external callers may not be. > > I don't like this function - it's weird, and waiting for the wrong stage will > result in it hanging. Seems like waiting until callback_ or auth_callback_ is > non-NULL would make more sense, and renaming the function > "WaitForNextBlockedState" or some such, and checking that the mode is > USER_CALLBACK makes more sense. You have a good point. I tried to modify it a bit differently, but with addressing the issues you raised: It still waits until the blocked stage is signalled, but also checks that we did not get into the final stage of the request. So it should not get stuck. I could remove the |blocked_stage_| and check that the callbacks have been set, but this seems a little bit clearer to me. > However, even simpler than that - we could just explicitly exit the message loop > when we hit a blocked state in the USER_CALLBACK Mode, and get rid of this > function entirely. Here I had a problem with blocking in the ON_BEFORE_REQUEST stage. The tests first start the request, and then the message loop, so attempting to exit the loop on before request fails for no loop being started. On the other hand, once the loop is started, we have no option to exit it, because OnBeforeURLRequest already ran. I feel that working this around would be more complex than the currently repaired WaitForWhatever function, but I'm open to suggestions. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2012/09/19 19:39:47, Matt Menke wrote: > nit: The new rule is to leave this alone, and last I checked, it's still 2012. > Also, I may not be the fastest reviewer in the world, but I'm not *that* slow. > :) :D I swear I did not touch that! Not sure how that number got there. I'll simply revert it manually to the current year. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:150: // to block in. On blocking, the delegate allows either: On 2012/09/19 19:39:47, Matt Menke wrote: > nit: For readability, suggest you replace the second sentence with "When > blocking, the delegate can do one of the following:", and removing the "to" from > the following lines. Done. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:159: class BlockingNetworkDelegate : public TestNetworkDelegate { I'm a bit confused: are we speaking about TestDelegate or TestNetworkDelegate? On 2012/09/19 19:39:47, Matt Menke wrote: > TestNetworkDelegate posts a task to quit the current message loop in > OnResponseCompleted, by default. All the non-USER_CALLBACK tests expect this. This happens on TestDelegate, TestNetworkDelegate does not have that method. > The USER_CALLBACK tests do not - They use the WaitForWhatever function instead > of running the message loop. We could change the behavior for the USER_CALLBACK > tests to set TestDelegate::quit_on_complete_. I'm not clear on how to do it. The URLrequest has a link to the TestDelegate, but does not expose it. Did you have in mind extending the BlockingNetworkDelegate to take a link to a TestDelegate on constriction, and set it through that? > Another option would be to just run the message loop at the end of the > USER_CALLBACK tests, which would make them more consistent with the other tests. > I believe cancelling should result in calling OnResponseCompleted. Here at least I have an idea how to proceed. Only I don't fully get the problem -- why is it so important that the message loop is run? I guess if quitting it in TestDelegate::OnResponseCompleted would be a problem, we would already have seen some asserts firing, no? > We could just replace USER_CALLBACK with ASYNC_CANCEL, since all the > USER_CALLBACK tests do the same thing, but we'd lose flexibility that way. I'm hesitant to lose the flexibility until I fully understand the problem above and check that the other solutions are worse. > This behavior is sufficiently non-obvious that I suggest adding a comments to > both BlockingNetworkDelegate and TestNetworkDelegate about it. I fully second that, as I'm confused already. :) I'll add comments once I understand the thing and implement the solution. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:175: // return codes. On 2012/09/19 19:39:47, Matt Menke wrote: > nit: "|this| posts a task to run the callback using..." Done. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:181: explicit BlockingNetworkDelegate(BlockMode mode); On 2012/09/19 19:39:47, Matt Menke wrote: > nit: Just for consistency with the class field name, suggest |block_mode|. Done. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:342: stage_blocked_for_callback_ = NOT_BLOCKED; On 2012/09/19 19:39:47, Matt Menke wrote: > Don't think there's any way for this not to be true at this point, since it's > reset in Do[Auth]Callback. May even want to EXPECT on it. Done. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:377: ASSERT_NE(NOT_BLOCKED, stage_blocked_for_callback_); On 2012/09/19 19:39:47, Matt Menke wrote: > nit: ASSERT_NE(ON_AUTH_REQUIRED, stage_blocked_for_callback_); Done. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:409: stage_blocked_for_callback_ = NOT_BLOCKED; On 2012/09/19 19:39:47, Matt Menke wrote: > Don't think there's any way for this not to be true at this point, since it's > reset in Do[Auth]Callback. May even want to EXPECT on it. Done. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:415: if (redirect_url_.is_empty() || stage != ON_BEFORE_URL_REQUEST) On 2012/09/19 19:39:47, Matt Menke wrote: > This if statement isn't needed anymore, is it? If redirect_url_ is set and > retval_ is OK, then we don't need to be blocking on ON_BEFORE_URL_REQUEST. If > any tests currently do this, can just have them not block on that stage. Done. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:418: stage_blocked_for_callback_ = stage; On 2012/09/19 19:39:47, Matt Menke wrote: > optional: Do we want to set this on AUTO_CALLBACK as well? Doesn't really > matter, either way, but might be a little more consistent. Done. https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:2051: TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequest) { Given that redirect now is done independently of blocking, I am not sure what should be the difference between NetworkDelegateRedirectRequest and NetworkDelegateRedirectRequestSynchronously (below). Matt, do you have any idea what the difference should be, or should I just delete one of those two tests?
https://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reque... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reque... net/url_request/url_request_unittest.cc:203: void WaitForState(Stage stage) { On 2012/09/20 11:45:44, vabr (Chromium) wrote: > > However, even simpler than that - we could just explicitly exit the message > loop > > when we hit a blocked state in the USER_CALLBACK Mode, and get rid of this > > function entirely. > > Here I had a problem with blocking in the ON_BEFORE_REQUEST stage. The tests > first start the request, and then the message loop, so attempting to exit the > loop on before request fails for no loop being started. On the other hand, once > the loop is started, we have no option to exit it, because OnBeforeURLRequest > already ran. > I feel that working this around would be more complex than the currently > repaired WaitForWhatever function, but I'm open to suggestions. Right. I had not realized starting the requests would call ON_BEFORE_REQUEST synchronously. The solution to this is to do what TestDelegate does: MessageLoop::current()->PostTask(FROM_HERE, MessageLoop::QuitClosure()); My issue with WaitForWhatever is that in one set of tests we run the message loop until quit, and in the other set, we run it bit by bit. I think using the different behaviors when not strictly needed is a bad design pattern, if we can easily avoid it. Also think that "while (!x) RunMessageLoop();" isn't a great pattern in general. https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/10905259/diff/20002/net/url_request/url_reque... net/url_request/url_request_unittest.cc:159: class BlockingNetworkDelegate : public TestNetworkDelegate { On 2012/09/20 11:45:44, vabr (Chromium) wrote: > I'm a bit confused: are we speaking about TestDelegate or TestNetworkDelegate? Sorry, I got them confused. > On 2012/09/19 19:39:47, Matt Menke wrote: > > TestNetworkDelegate posts a task to quit the current message loop in > > OnResponseCompleted, by default. All the non-USER_CALLBACK tests expect this. > > This happens on TestDelegate, TestNetworkDelegate does not have that method. > > > The USER_CALLBACK tests do not - They use the WaitForWhatever function instead > > of running the message loop. We could change the behavior for the > USER_CALLBACK > > tests to set TestDelegate::quit_on_complete_. > > I'm not clear on how to do it. The URLrequest has a link to the TestDelegate, > but does not expose it. Did you have in mind extending the > BlockingNetworkDelegate to take a link to a TestDelegate on constriction, and > set it through that? We actually create the TestDelegate in the individual tests, so can call it directly, rather than through the TestNetworkDelegate. Looks like the TestDelegate is used in a number of other cases without worrying about that quit message, so I'm probably just being overly paranoid here. Just forget about this comment. :) https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:186: ASSERT_NE(OK, retval); OK actually makes sense, if BlockMode is AUTO_CALLBACK. https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:376: ASSERT_EQ(block_mode_, USER_CALLBACK); nit: Reverse order https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:386: ASSERT_EQ(block_mode_, USER_CALLBACK); nit: Reverse order https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:416: EXPECT_NE(OK, retval_); OK is actually OK for the AUTO_CALLBACK case. Doesn't matter in the USER_CALLBACK case, so suggest you move this to the SYNCHRONOUS case. https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:2027: BlockingNetworkDelegate::AUTO_CALLBACK); This changes behavior. It should be SYNCHRONOUS. https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:2051: TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequest) { On 2012/09/20 11:45:44, vabr (Chromium) wrote: > Given that redirect now is done independently of blocking, I am not sure what > should be the difference between NetworkDelegateRedirectRequest and > NetworkDelegateRedirectRequestSynchronously (below). > > Matt, do you have any idea what the difference should be, or should I just > delete one of those two tests? Ahh...There's a bug here. This should use network_delegate.BlockOn(BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST); to have the same behavior as before. In fact, to exactly preserve behavior, all the AUTO_CALLBACK tests should do this, except those that used to call "network_delegate.set_retval(OK);", since the default behavior used to be async in ON_BEFORE_URL_REQUEST. All the AUTO_CALLBACK tests that used to use set_auth_retval should by SYNCHRONOUS and block on ON_AUTH_REQUIRED, and those that use set_auth_callback_retval should by AUTO_CALLBACK. I've noted a couple cases where the wrong mode is used, though haven't checked them all. https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:2123: BlockingNetworkDelegate::SYNCHRONOUS); This changes behavior. It should be AUTO_CALLBACK, with retval of OK (Which is default). https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:2123: BlockingNetworkDelegate::SYNCHRONOUS); If we want to preserve behavior here, we should make this async and block on headers, but don't think that's needed. https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:2165: BlockingNetworkDelegate::AUTO_CALLBACK); This should be SYNCHRONOUS. https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:2197: BlockingNetworkDelegate::AUTO_CALLBACK); SYNCHRONOUS https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:2231: BlockingNetworkDelegate::AUTO_CALLBACK); SYNCHRONOUS
https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:2051: TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequest) { On 2012/09/20 15:37:52, Matt Menke wrote: > On 2012/09/20 11:45:44, vabr (Chromium) wrote: > > Given that redirect now is done independently of blocking, I am not sure what > > should be the difference between NetworkDelegateRedirectRequest and > > NetworkDelegateRedirectRequestSynchronously (below). > > > > Matt, do you have any idea what the difference should be, or should I just > > delete one of those two tests? > > Ahh...There's a bug here. This should use > network_delegate.BlockOn(BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST); to > have the same behavior as before. > > In fact, to exactly preserve behavior, all the AUTO_CALLBACK tests should do > this, except those that used to call "network_delegate.set_retval(OK);", since > the default behavior used to be async in ON_BEFORE_URL_REQUEST. Sorry - comment below is more accurate than the one above, and also applies to set_retval / set_callback_retval / ON_BEFORE_URL_REQUEST. > All the AUTO_CALLBACK tests that used to use set_auth_retval should by > SYNCHRONOUS and block on ON_AUTH_REQUIRED, and those that use > set_auth_callback_retval should by AUTO_CALLBACK. > > I've noted a couple cases where the wrong mode is used, though haven't checked > them all.
Hi Matt, Thanks very much for spotting and checking the issue with migrating tests from old BlockingNetworkDelegate* to the new one. I double-checked the old implementation, and then each test, one after another. Summary is here: https://docs.google.com/a/google.com/document/d/1kUxp6kk8ilNgZzz2MyFfcRWpObRW... but in short: your comments caught pretty much everything, only at some places I added BlockOn calls. It was not always possible to replicate the old behaviour, e.g., in tests like NetworkDelegateOnAuthRequiredSyncNoAction the old BlockingNetworkDelegate responded asynchronously with an OK in onBeforeRequest (by default), and then synchronously in onAuthRequired. I cannot switch between synchronous and asynchronous independently for two stages. But in all such cases, the asynchronous block was a default behaviour, without significant side-effects and clearly not relevant to the test, so I ignored that. Sometimes it was possible to replicate the old behaviour of BlockingNetworkDelegate, which asynchronously returned *NO_ACTION in the ON_AUTH_REQUIRED stage by default. But it made not much sense, e.g., in NetworkDelegateCancelRequest, where setting up blockin in ON_AUTH_REQUIRED in the test would be really confusing. So in that cases I dropped the default behaviour as well (highlighted in the doc linked above). The "WaitForWhatever" function is now gone. Looking forward for your thoughts! Vaclav P.S. Today I spotted VIM inserting "(c) 2016" into the source file. I need to find out why does it do that. :) http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/13003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:203: void WaitForState(Stage stage) { On 2012/09/20 15:37:52, Matt Menke wrote: > On 2012/09/20 11:45:44, vabr (Chromium) wrote: > > > However, even simpler than that - we could just explicitly exit the message > > loop > > > when we hit a blocked state in the USER_CALLBACK Mode, and get rid of this > > > function entirely. > > > > Here I had a problem with blocking in the ON_BEFORE_REQUEST stage. The tests > > first start the request, and then the message loop, so attempting to exit the > > loop on before request fails for no loop being started. On the other hand, > once > > the loop is started, we have no option to exit it, because OnBeforeURLRequest > > already ran. > > I feel that working this around would be more complex than the currently > > repaired WaitForWhatever function, but I'm open to suggestions. > > Right. I had not realized starting the requests would call ON_BEFORE_REQUEST > synchronously. The solution to this is to do what TestDelegate does: > MessageLoop::current()->PostTask(FROM_HERE, MessageLoop::QuitClosure()); > > My issue with WaitForWhatever is that in one set of tests we run the message > loop until quit, and in the other set, we run it bit by bit. I think using the > different behaviors when not strictly needed is a bad design pattern, if we can > easily avoid it. Also think that "while (!x) RunMessageLoop();" isn't a great > pattern in general. Thanks for the idea with posting the QuitClosure task. That worked, WaitForWhatever is now removed. http://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reques... net/url_request/url_request_unittest.cc:186: ASSERT_NE(OK, retval); On 2012/09/20 15:37:52, Matt Menke wrote: > OK actually makes sense, if BlockMode is AUTO_CALLBACK. But in that case OK is the default value. Until there is a situation where |retval_| would be set to non-OK on an early stage, and then reset to OK, I would leave the ASSERT here. http://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reques... net/url_request/url_request_unittest.cc:376: ASSERT_EQ(block_mode_, USER_CALLBACK); On 2012/09/20 15:37:52, Matt Menke wrote: > nit: Reverse order Done. http://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reques... net/url_request/url_request_unittest.cc:386: ASSERT_EQ(block_mode_, USER_CALLBACK); On 2012/09/20 15:37:52, Matt Menke wrote: > nit: Reverse order Done. http://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reques... net/url_request/url_request_unittest.cc:416: EXPECT_NE(OK, retval_); On 2012/09/20 15:37:52, Matt Menke wrote: > OK is actually OK for the AUTO_CALLBACK case. Doesn't matter in the > USER_CALLBACK case, so suggest you move this to the SYNCHRONOUS case. Done. http://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2027: BlockingNetworkDelegate::AUTO_CALLBACK); On 2012/09/20 15:37:52, Matt Menke wrote: > This changes behavior. It should be SYNCHRONOUS. Done. http://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2051: TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequest) { On 2012/09/20 15:37:52, Matt Menke wrote: > On 2012/09/20 11:45:44, vabr (Chromium) wrote: > > Given that redirect now is done independently of blocking, I am not sure what > > should be the difference between NetworkDelegateRedirectRequest and > > NetworkDelegateRedirectRequestSynchronously (below). > > > > Matt, do you have any idea what the difference should be, or should I just > > delete one of those two tests? > > Ahh...There's a bug here. This should use > network_delegate.BlockOn(BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST); to > have the same behavior as before. > > In fact, to exactly preserve behavior, all the AUTO_CALLBACK tests should do > this, except those that used to call "network_delegate.set_retval(OK);", since > the default behavior used to be async in ON_BEFORE_URL_REQUEST. > > All the AUTO_CALLBACK tests that used to use set_auth_retval should by > SYNCHRONOUS and block on ON_AUTH_REQUIRED, and those that use > set_auth_callback_retval should by AUTO_CALLBACK. > > I've noted a couple cases where the wrong mode is used, though haven't checked > them all. Done. http://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2123: BlockingNetworkDelegate::SYNCHRONOUS); On 2012/09/20 15:37:52, Matt Menke wrote: > If we want to preserve behavior here, we should make this async and block on > headers, but don't think that's needed. Why headers? I don't think the original BlockingNetworkDelegate could block on sending headers. Did you mean onAuthRequired? http://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2123: BlockingNetworkDelegate::SYNCHRONOUS); On 2012/09/20 15:37:52, Matt Menke wrote: > This changes behavior. It should be AUTO_CALLBACK, with retval of OK (Which is > default). Done. http://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2165: BlockingNetworkDelegate::AUTO_CALLBACK); On 2012/09/20 15:37:52, Matt Menke wrote: > This should be SYNCHRONOUS. Done. http://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2197: BlockingNetworkDelegate::AUTO_CALLBACK); On 2012/09/20 15:37:52, Matt Menke wrote: > SYNCHRONOUS Done. http://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2231: BlockingNetworkDelegate::AUTO_CALLBACK); On 2012/09/20 15:37:52, Matt Menke wrote: > SYNCHRONOUS Done. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:185: ASSERT_NE(USER_CALLBACK, block_mode_); In USER_CALLBACK mode |retval_| is used at all. Similarly for |auth_retval_| below. (Maybe I should add it as a comment.)
Just nits, basically. It's looking pretty good to me now. Carefully went over the test changes, and they all look correct. Though your table incorrectly calls the async auth tests sync, the code looks correct. One other suggestion: We no longer have any test that returns OK asynchronously in OnBeforeURLRequest in the non-redirect case. Just for the sake of completeness, I suggest a very simple test that returns OK asynchronously in all three retval functions. http://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/28001/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2123: BlockingNetworkDelegate::SYNCHRONOUS); On 2012/09/21 11:11:36, vabr (Chromium) wrote: > On 2012/09/20 15:37:52, Matt Menke wrote: > > If we want to preserve behavior here, we should make this async and block on > > headers, but don't think that's needed. > > Why headers? I don't think the original BlockingNetworkDelegate could block on > sending headers. Did you mean onAuthRequired? Err...I meant on before send, actually (Hence the comment about it not mattering). http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:177: // |auth_retval_| are ignored. Should add something along the lines of "Also, The message loop is quit during blocked stages." http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:185: ASSERT_NE(USER_CALLBACK, block_mode_); On 2012/09/21 11:11:36, vabr (Chromium) wrote: > In USER_CALLBACK mode |retval_| is used at all. > Similarly for |auth_retval_| below. > (Maybe I should add it as a comment.) There's a comment about it in BlockMode, which I think should be enough. If you think it's worth another comment, though, feel free. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:208: void BlockOn(Stage stage) { nit: Since the |= behavior doesn't seem to be needed, or even used, suggest changing this to a simple setter: set_block_on(Stage block_on)... And updating the comment correspondingly. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:216: void DoAuthCallback(NetworkDelegate::AuthRequiredResponse response); optional nit: Suggest these two functions be put just below the constructor, just to keep all the simple getters and setters together. Or just below stage_blocked_for_callback. I have a slight preference for putting setters/getters last, but either is fine. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:218: Stage stage_blocked_for_callback() const { May want a comment on this, though it's not exactly a complicated function. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:220: return stage_blocked_for_callback_; Good idea. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:245: void Reset() { May want to add an: EXPECT_NE(NOT_BLOCKED, stage_blocked_for_callback_); http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:249: } Think this should be de-inlined, since everything else is. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:275: // Internal parameters: Think "internal parameters" is a little vague, and "parameters" doesn't quite seem accurate. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:294: AUTH_REQUIRED_RESPONSE_NO_ACTION), These USER_CALLBACK checks aren't needed, since they're never used never used in those cases. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:352: stage_blocked_for_callback_ = ON_AUTH_REQUIRED; Think that this line should just be move into the USER_CALLBACK case. Then we always set auth_callback_ and stage_blocked_for_callback_ together, also gets rid of the conditional. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:355: target_auth_credentials_ = credentials; This seems weird in the USER_CALLBACK case. My suggestion is to just always set this, even when it's not used. Alternatively, could add in a check for USER_CALLBACK, though I don't think the gain isn't worth the slightly added complexity. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:379: void BlockingNetworkDelegate::DoCallback(int response) { Per the style guide, The four callback functions should be in the same order they're declared in. Suggest moving the private callback functions up to the top of the private section, so they'll all still be together. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:403: const AuthCallback& callback) { nit: Fix indent. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:422: stage_blocked_for_callback_ = stage; As above, think this should be moved down right next to callback_ = callback. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2174: network_delegate.BlockOn(BlockingNetworkDelegate::ON_AUTH_REQUIRED); This is fine, though not actually needed.
http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:245: void Reset() { On 2012/09/21 15:15:59, Matt Menke wrote: > May want to add an: > > EXPECT_NE(NOT_BLOCKED, stage_blocked_for_callback_); Suggesting this more for documentation purposes than to catch bugs.
On 2012/09/21 15:15:59, Matt Menke wrote: > Just nits, basically. It's looking pretty good to me now. > > Carefully went over the test changes, and they all look correct. Though your > table incorrectly calls the async auth tests sync, the code looks correct. Sorry, just realized I misread your chart. It was the code I carefully went over, not the chart. :)
Hi Matt, Thanks for the quick turn-around! I addressed all your comments, except for the one about BlockOn (but I'm not opposing that strongly, see my response). Please let me know what you think, Vaclav P.S. The changing copyright year mystery was solved. I'm using VIM in one window and screen in another one. And I managed to type Ctrl+A, meant for screen, in the VIM window. The effect was to jump on the nearest number and increment it. :) I need to be more careful, though, other changed numbers may not be so easy to spot as the futuristic year. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:177: // |auth_retval_| are ignored. On 2012/09/21 15:15:59, Matt Menke wrote: > Should add something along the lines of "Also, The message loop is quit during > blocked stages." Done. Played with the wording, feel free to protest. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:185: ASSERT_NE(USER_CALLBACK, block_mode_); On 2012/09/21 15:15:59, Matt Menke wrote: > On 2012/09/21 11:11:36, vabr (Chromium) wrote: > > In USER_CALLBACK mode |retval_| is used at all. > > Similarly for |auth_retval_| below. > > (Maybe I should add it as a comment.) > > There's a comment about it in BlockMode, which I think should be enough. If you > think it's worth another comment, though, feel free. Good point with the BlockMode. I think it is OK to leave it just there. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:208: void BlockOn(Stage stage) { On 2012/09/21 15:15:59, Matt Menke wrote: > nit: Since the |= behavior doesn't seem to be needed, or even used, suggest > changing this to a simple setter: set_block_on(Stage block_on)... > > And updating the comment correspondingly. I don't think I agree that this is not needed -- what if, e.g., the user wants to do USER_CALLBACK both before request and in the authorisation stage. True, this does not happen right now, but removing it now and probably adding back later seems to me not worth the effort. If you still think it would be worth it, I'll do it, including changing the type of |block_on_| to Stage, removing the explicit values for Stage constants, and re-evaluating some of the related ASSERT/EXPECTs. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:216: void DoAuthCallback(NetworkDelegate::AuthRequiredResponse response); On 2012/09/21 15:15:59, Matt Menke wrote: > optional nit: Suggest these two functions be put just below the constructor, > just to keep all the simple getters and setters together. Or just below > stage_blocked_for_callback. I have a slight preference for putting > setters/getters last, but either is fine. Done. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:218: Stage stage_blocked_for_callback() const { On 2012/09/21 15:15:59, Matt Menke wrote: > May want a comment on this, though it's not exactly a complicated function. Done. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:245: void Reset() { On 2012/09/21 15:15:59, Matt Menke wrote: > May want to add an: > > EXPECT_NE(NOT_BLOCKED, stage_blocked_for_callback_); Done. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:249: } On 2012/09/21 15:15:59, Matt Menke wrote: > Think this should be de-inlined, since everything else is. Done. Plus added comment to the declaration. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:275: // Internal parameters: On 2012/09/21 15:15:59, Matt Menke wrote: > Think "internal parameters" is a little vague, and "parameters" doesn't quite > seem accurate. Attempted to be more specific, also with the "configuration parameters". http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:294: AUTH_REQUIRED_RESPONSE_NO_ACTION), On 2012/09/21 15:15:59, Matt Menke wrote: > These USER_CALLBACK checks aren't needed, since they're never used never used in > those cases. True, I forgot I hard-coded those IO_PENDING values. Thanks, for spotting this. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:352: stage_blocked_for_callback_ = ON_AUTH_REQUIRED; On 2012/09/21 15:15:59, Matt Menke wrote: > Think that this line should just be move into the USER_CALLBACK case. Then we > always set auth_callback_ and stage_blocked_for_callback_ together, also gets > rid of the conditional. Nice, thanks! Done. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:355: target_auth_credentials_ = credentials; On 2012/09/21 15:15:59, Matt Menke wrote: > This seems weird in the USER_CALLBACK case. My suggestion is to just always set > this, even when it's not used. Alternatively, could add in a check for > USER_CALLBACK, though I don't think the gain isn't worth the slightly added > complexity. I'm not sure I understood the alternative solution (adding a check for USER_CALLBACK), because this is used in both *_CALLBACK modes, only in SYNCHRONOUS it is not. Anyway, since it is only a pointer, I'm fine with removing the "if" and just copying it all the times. :) http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:379: void BlockingNetworkDelegate::DoCallback(int response) { On 2012/09/21 15:15:59, Matt Menke wrote: > Per the style guide, The four callback functions should be in the same order > they're declared in. Suggest moving the private callback functions up to the > top of the private section, so they'll all still be together. Done. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:403: const AuthCallback& callback) { On 2012/09/21 15:15:59, Matt Menke wrote: > nit: Fix indent. Done. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:422: stage_blocked_for_callback_ = stage; On 2012/09/21 15:15:59, Matt Menke wrote: > As above, think this should be moved down right next to callback_ = callback. Done. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2174: network_delegate.BlockOn(BlockingNetworkDelegate::ON_AUTH_REQUIRED); On 2012/09/21 15:15:59, Matt Menke wrote: > This is fine, though not actually needed. Removed to be consistent with NetworkDelegateRedirectRequestSynchronously (the same situation but before request and with OK).
LGTM, but you may want to add a unit test, as suggested below. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:208: void BlockOn(Stage stage) { On 2012/09/21 20:03:42, vabr (Chromium) wrote: > On 2012/09/21 15:15:59, Matt Menke wrote: > > nit: Since the |= behavior doesn't seem to be needed, or even used, suggest > > changing this to a simple setter: set_block_on(Stage block_on)... > > > > And updating the comment correspondingly. > > I don't think I agree that this is not needed -- what if, e.g., the user wants > to do USER_CALLBACK both before request and in the authorisation stage. > True, this does not happen right now, but removing it now and probably adding > back later seems to me not worth the effort. > > If you still think it would be worth it, I'll do it, including changing the type > of |block_on_| to Stage, removing the explicit values for Stage constants, and > re-evaluating some of the related ASSERT/EXPECTs. Both blocks could be set with a single call. I did forget the prototype would have to use "int stage" rather than "Stage stage", which is slightly less safe, but I think it's cleaner than the "|=". My main complaint is that aggregating flags like this is pretty uncommon across the portion of the Chrome code base I'm familiar with, and I completely missed the "|=" on my first read through the code. Much more common, when wanting to set a flag, seems to be blah->set_flags(blah->flags() | BLAH_SUPER_BLAH_FLAG);. Anyhow, I can live with it as-is, I just think the current behavior is completely unexpected, which makes it bug prone. You missed my (non-inline) suggestion of adding a test where all 3 return OK asynchronously, since we no longer have an async non-redirect test, and seems reasonable to test async behavior everywhere, which would actually require blocking on multiple stages. http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:355: target_auth_credentials_ = credentials; On 2012/09/21 20:03:42, vabr (Chromium) wrote: > On 2012/09/21 15:15:59, Matt Menke wrote: > > This seems weird in the USER_CALLBACK case. My suggestion is to just always > set > > this, even when it's not used. Alternatively, could add in a check for > > USER_CALLBACK, though I don't think the gain isn't worth the slightly added > > complexity. > > I'm not sure I understood the alternative solution (adding a check for > USER_CALLBACK), because this is used in both *_CALLBACK modes, only in > SYNCHRONOUS it is not. > > Anyway, since it is only a pointer, I'm fine with removing the "if" and just > copying it all the times. :) The alternative was "if (auth_retval_ == AUTH_REQUIRED_RESPONSE_SET_AUTH || block_mode_ == USER_CALLBACK)", but I prefer the approach you used.
On 2012/09/21 20:20:18, Matt Menke wrote: > LGTM, but you may want to add a unit test, as suggested below. And thanks for all your work on this - the new code's a lot more flexible than the old code, and fairly straightforward to use.
Hi Matt, Thanks for clarifying the comment about BlockOn -> set_block_on, it was now easy to address. Sorry about me forgetting the new unit-test. It is in now, please let me know if you want me to change it in any way. Once you are happy with the new test, I will send this to CQ. Thanks! Vaclav http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/34004/net/url_request/url_reques... net/url_request/url_request_unittest.cc:208: void BlockOn(Stage stage) { On 2012/09/21 20:20:18, Matt Menke wrote: > On 2012/09/21 20:03:42, vabr (Chromium) wrote: > > On 2012/09/21 15:15:59, Matt Menke wrote: > > > nit: Since the |= behavior doesn't seem to be needed, or even used, suggest > > > changing this to a simple setter: set_block_on(Stage block_on)... > > > > > > And updating the comment correspondingly. > > > > I don't think I agree that this is not needed -- what if, e.g., the user wants > > to do USER_CALLBACK both before request and in the authorisation stage. > > True, this does not happen right now, but removing it now and probably adding > > back later seems to me not worth the effort. > > > > If you still think it would be worth it, I'll do it, including changing the > type > > of |block_on_| to Stage, removing the explicit values for Stage constants, and > > re-evaluating some of the related ASSERT/EXPECTs. > > Both blocks could be set with a single call. I did forget the prototype would > have to use "int stage" rather than "Stage stage", which is slightly less safe, > but I think it's cleaner than the "|=". My main complaint is that aggregating > flags like this is pretty uncommon across the portion of the Chrome code base > I'm familiar with, and I completely missed the "|=" on my first read through the > code. Much more common, when wanting to set a flag, seems to be > blah->set_flags(blah->flags() | BLAH_SUPER_BLAH_FLAG);. Ah, I see. I misunderstood you previously. Implemented as requested. Also, I removed the comment at the no set_blocked_on, as the comment was out-dated, and now it is pretty obvious what the function does. > > Anyhow, I can live with it as-is, I just think the current behavior is > completely unexpected, which makes it bug prone. > > You missed my (non-inline) suggestion of adding a test where all 3 return OK > asynchronously, since we no longer have an async non-redirect test, and seems > reasonable to test async behavior everywhere, which would actually require > blocking on multiple stages. Sorry about that! The test is in now as NetworkDelegateBlockAsynchronously. Please let me know what you think.
LGTM. http://codereview.chromium.org/10905259/diff/41003/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/41003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2504: TEST_F(URLRequestTestHTTP, NetworkDelegateBlockAsynchronously) { optional nit: Suggest making this one of the first, if not the first, NetworkDelegate* tests, since it's one of the more basic tests. The idea is that more basic tests go earlier, so if multiple tests fail, it's easiest to debug the first one that failed. http://codereview.chromium.org/10905259/diff/41003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2536: MessageLoop::current()->Run(); Suggest also having: EXPECT_EQ(200, r.GetResponseCode()); EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status());
Thanks a lot, Matt! Addressed your last two comments, and sending to CQ. Vaclav http://codereview.chromium.org/10905259/diff/41003/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10905259/diff/41003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2504: TEST_F(URLRequestTestHTTP, NetworkDelegateBlockAsynchronously) { On 2012/09/24 14:49:44, Matt Menke wrote: > optional nit: Suggest making this one of the first, if not the first, > NetworkDelegate* tests, since it's one of the more basic tests. The idea is > that more basic tests go earlier, so if multiple tests fail, it's easiest to > debug the first one that failed. Done. http://codereview.chromium.org/10905259/diff/41003/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2536: MessageLoop::current()->Run(); On 2012/09/24 14:49:44, Matt Menke wrote: > Suggest also having: > > EXPECT_EQ(200, r.GetResponseCode()); > EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status()); Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/10905259/42005
Retried try job too often for step(s) chrome_frame_net_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/10905259/42005
Retried try job too often for step(s) chrome_frame_net_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/10905259/42005
On 2012/09/25 05:27:14, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/vabr%40chromium.org/10905259/42005 OK, it looks like the chrome_frame_net_tests are not flaky, because they fail consistently, and the source depends on url_request_unittest.cc (https://cs.corp.google.com/#chrome/src/chrome_frame/chrome_frame.gyp&sq=packa...). It looks like I introduced the time-out by adding the new unit-test. I will investigate ASAP (but not today, we have a team off-site).
On 2012/09/25 05:58:19, vabr (Chromium) wrote: > On 2012/09/25 05:27:14, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/vabr%2540chromium.org/10905259/42005 > > OK, it looks like the chrome_frame_net_tests are not flaky, because they fail > consistently, and the source depends on url_request_unittest.cc > (https://cs.corp.google.com/#chrome/src/chrome_frame/chrome_frame.gyp&sq=packa...). > It looks like I introduced the time-out by adding the new unit-test. I will > investigate ASAP (but not today, we have a team off-site). If push comes to shove, we can land it without the new unit test. I am a bit worried about why this is causing the chrome frame net tests to fail, though.
On 2012/09/25 06:03:11, Matt Menke wrote: > On 2012/09/25 05:58:19, vabr (Chromium) wrote: > > On 2012/09/25 05:27:14, I haz the power (commit-bot) wrote: > > > CQ is trying da patch. Follow status at > > > https://chromium-status.appspot.com/cq/vabr%252540chromium.org/10905259/42005 > > > > OK, it looks like the chrome_frame_net_tests are not flaky, because they fail > > consistently, and the source depends on url_request_unittest.cc > > > (https://cs.corp.google.com/#chrome/src/chrome_frame/chrome_frame.gyp&sq=packa...). > > It looks like I introduced the time-out by adding the new unit-test. I will > > investigate ASAP (but not today, we have a team off-site). > > If push comes to shove, we can land it without the new unit test. I am a bit > worried about why this is causing the chrome frame net tests to fail, though. I think the test needs only to be disabled for Chrome Frame, certainly not in net_unitests themselves: https://cs.corp.google.com/#chrome/src/chrome_frame/test/net/fake_external_ta... Lot of other URLRequestTestHTTP tests are disabled there because some of the stages do not seem to be supported by Chrome Frame. I'll update this with a patch disabling the new test very soon.
Erik, The net/url_request part of this CL has been reviewed by Matt, but one newly added test failed under Chrome Frame. I believe it should be disabled, see patchset 13. Can I get your owner's review for chrome_frame/test/net/fake_external_tab.cc? Matt, Please speak up if you disagree with the disabling (no other changes made). Thanks both, Vaclav
On 2012/09/25 17:38:50, vabr (Chromium) wrote: > Erik, > The net/url_request part of this CL has been reviewed by Matt, but one newly > added test failed under Chrome Frame. > I believe it should be disabled, see patchset 13. Can I get your owner's review > for chrome_frame/test/net/fake_external_tab.cc? > > Matt, > Please speak up if you disagree with the disabling (no other changes made). > > Thanks both, > Vaclav Sounds reasonable to disable it, assuming your explanation is correct. Seems weird that no test output is produced when the chrome frame tests run it, though perhaps that's expected from those tests.
On 2012/09/25 17:53:06, Matt Menke wrote: > On 2012/09/25 17:38:50, vabr (Chromium) wrote: > > Erik, > > The net/url_request part of this CL has been reviewed by Matt, but one newly > > added test failed under Chrome Frame. > > I believe it should be disabled, see patchset 13. Can I get your owner's > review > > for chrome_frame/test/net/fake_external_tab.cc? > > > > Matt, > > Please speak up if you disagree with the disabling (no other changes made). > > > > Thanks both, > > Vaclav > > Sounds reasonable to disable it, assuming your explanation is correct. Seems > weird that no test output is produced when the chrome frame tests run it, though > perhaps that's expected from those tests. Without looking at the details of the test, it probably hangs because it's waiting for a specific state transition that never happens (which would trigger a Quit() on the current message loop). LGTM.
On 2012/09/25 18:58:35, erikwright wrote: > On 2012/09/25 17:53:06, Matt Menke wrote: > > On 2012/09/25 17:38:50, vabr (Chromium) wrote: > > > Erik, > > > The net/url_request part of this CL has been reviewed by Matt, but one newly > > > added test failed under Chrome Frame. > > > I believe it should be disabled, see patchset 13. Can I get your owner's > > review > > > for chrome_frame/test/net/fake_external_tab.cc? > > > > > > Matt, > > > Please speak up if you disagree with the disabling (no other changes made). > > > > > > Thanks both, > > > Vaclav > > > > Sounds reasonable to disable it, assuming your explanation is correct. Seems > > weird that no test output is produced when the chrome frame tests run it, > though > > perhaps that's expected from those tests. > > Without looking at the details of the test, it probably hangs because it's > waiting for a specific state transition that never happens (which would trigger > a Quit() on the current message loop). > > LGTM. But should that result in the frame tests outputting nothing? Not even that they're running a single test?
On 2012/09/25 19:00:30, Matt Menke wrote: > On 2012/09/25 18:58:35, erikwright wrote: > > On 2012/09/25 17:53:06, Matt Menke wrote: > > > On 2012/09/25 17:38:50, vabr (Chromium) wrote: > > > > Erik, > > > > The net/url_request part of this CL has been reviewed by Matt, but one > newly > > > > added test failed under Chrome Frame. > > > > I believe it should be disabled, see patchset 13. Can I get your owner's > > > review > > > > for chrome_frame/test/net/fake_external_tab.cc? > > > > > > > > Matt, > > > > Please speak up if you disagree with the disabling (no other changes > made). > > > > > > > > Thanks both, > > > > Vaclav > > > > > > Sounds reasonable to disable it, assuming your explanation is correct. > Seems > > > weird that no test output is produced when the chrome frame tests run it, > > though > > > perhaps that's expected from those tests. > > > > Without looking at the details of the test, it probably hangs because it's > > waiting for a specific state transition that never happens (which would > trigger > > a Quit() on the current message loop). > > > > LGTM. > > But should that result in the frame tests outputting nothing? Not even that > they're running a single test? No - that is weird. Normally they output a goodly amount of prologue. Perhaps something has been changed WRT how logs from hung tests are collected? In any case, there were numerous failures with the same symptoms, so assuming that blacklisting the test works, I would assume that it is the right step.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/10905259/51001
Thanks both for your replies. The try job just passed the CF tests, so blacklisting worked and I'm re-checking the CQ checkbox. Once I get enough time to set-up my Windows machine, I'll try to check what would be the output of the CF tests locally. Thanks, Vaclav
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/10905259/51001
Change committed as 158724 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
