Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
864923002:
In SdchDictionaryFetcher, don't set state when scheduling if loop is active. (Closed)
|
Created:
5 years, 11 months ago by Randy Smith (Not in Mondays) Modified:
5 years, 11 months ago Reviewers:
mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIn SdchDictionaryFetcher, don't set state when scheduling if loop is active.
BUG=450724
R=mmenke@chromium.org
Committed: https://crrev.com/81fe0e576f9cfe564fa85bfa545de68c26b8a02c
Cr-Commit-Position: refs/heads/master@{#313125}
Patch Set 1 #Patch Set 2 : Result of self review. #Patch Set 3 : Result of git cl format. #
Total comments: 29
Patch Set 4 : Incorporate Matt's comments. #Patch Set 5 : Incorporated comments. #Patch Set 6 : Sync'd to p313032. #
Total comments: 20
Patch Set 7 : Incorporated latest round of comments. #
Messages
Total messages: 13 (1 generated)
Matt: Sorry to add to your burdens, but I'd like someone to look at this that's more familiar with the DoLoop() idiom used through net/ code than I. If that's not you, let me know and I'll go looking for a different reviewer. Thanks!
https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:65: if (next_state_ == STATE_NONE) { I think an early return is cleaner here. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:66: next_state_ = STATE_IDLE; It's a bit tangential to this CL, but while we're here, can we rename IDLE, which sounds exactly like STATE_NONE? States are generally named after the method they run, or vice-versa, so it should be STATE_DISPATCH_REQUEST to match the method... Hrm...Actually, seems like there are a bunch of naming improvements that could be done here, and don't think it's reasonable to do all that in this CL. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:30: // addition callbacks. Class level are generally be put just above the class they describe - don't think there's anything here that warrants doing otherwise. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:39: class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob { This should be in an anonymous namespace, and the above statics can be moved in as well, remove the statics. I'd suggest sticking this entire file in, actually. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:105: static int jobs_outstanding_; I'd advise against using non-const static variables, it just seems ugly and bug prone. If you're running the tests multiple times without restarting the test fixture, if gets you into trouble. I notice that the old code sets jobs_requested_ to 0 to allow for that, but your code doesn't seem to. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:180: run_loop_ = &run_loop; Suggest just a scoped_ptr<RunLoop> in the class - seems cleaner to me. Can either reset it or not at the end of this method - I'm fine either way. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:190: if (waiting_for_no_jobs_) Any case where this is not the same as run_loop_ != NULL? Can certainly debate over which is cleaner, but I feel that having to both clear run_loop_ and set waiting_for_no_jobs_ to false outweighs the code clarity of having the book to check. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:271: } // net (And there should be a linebreak before the end of the namespace)
Oh, and about the reason you actually sent this to me: After switching to an early return, the use of DoLoop is very consistent with code elsewhere in net (Modulo state/method names, perhaps).
Responded to your comments; PTAL? https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:65: if (next_state_ == STATE_NONE) { On 2015/01/21 21:33:57, mmenke wrote: > I think an early return is cleaner here. Right you are. Done. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:66: next_state_ = STATE_IDLE; On 2015/01/21 21:33:57, mmenke wrote: > It's a bit tangential to this CL, but while we're here, can we rename IDLE, > which sounds exactly like STATE_NONE? > > States are generally named after the method they run, or vice-versa, so it > should be STATE_DISPATCH_REQUEST to match the method... > > Hrm...Actually, seems like there are a bunch of naming improvements that could > be done here, and don't think it's reasonable to do all that in this CL. Naming changes, while they touch a lot of lines, don't take a lot of time. Want to give me a nudge in the direction of what you'd like to see and I'll take a swing at it? State names should match the functions that they dispatch, check. What else? (Also let me know if you'd prefer them in a separate CL, else I'll just dump them in this CL for my own convenience.) https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:30: // addition callbacks. On 2015/01/21 21:33:57, mmenke wrote: > Class level are generally be put just above the class they describe - don't > think there's anything here that warrants doing otherwise. Hmmm. I can do that, and if you really want me to, I will. But I think it undercuts the reason I put the documentation in in the first place. When I go to modify a test file, the first thing I want to know is what the infrastructure & usual test pattern is. My goal with this comment was to provide that information so that the next time I add a test to this file I can quickly internalize what tools I have to work with and what the likely pattern of my test is. Tell me twice? (Or not.) https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:39: class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob { On 2015/01/21 21:33:57, mmenke wrote: > This should be in an anonymous namespace, and the above statics can be moved in > as well, remove the statics. > > I'd suggest sticking this entire file in, actually. Done, but I'd love to get a bit more guidance or pointers on idiom, if you have them. The reason I originally did it this was was the (lazy) urge not to have to put net:: everywhere. Switching over to using an anonymous namespace (without any nesting) results in having to put in namespace qualifiers. I could nest an anonymous namespace inside of a net:: namespace, but that seems a bit ugly, and "using net::" violates the style guide. Do you have a sense as to the best idiom? (I'm perfectly willing to believe it's "anonymous, use net:: everywhere, and suck it up", it's just a shame to have to do that in something that's conceptually a net/ file.) https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:105: static int jobs_outstanding_; On 2015/01/21 21:33:57, mmenke wrote: > I'd advise against using non-const static variables, it just seems ugly and bug > prone. If you're running the tests multiple times without restarting the test > fixture, if gets you into trouble. Good point, and maybe I'm just having a braino, but what's my alternative? I want to have a blocking point when the number of outstanding jobs goes to zero, which means I need to record creation and destruction of those jobs in some mutable variable that isn't part of a URLRequestSpecifiedResponseJob instance. If that's not a static of some sort (e.g. a member variable in the test fixture) then it needs to be registered with URLRequestSpecifiedResponseJob in some fashion, and that registration information will be in a static. What am I missing? (This is presuming I don't want to use RunUntilIdle(), which I don't but could be convinced to. But this doesn't seem like the right argument for that.) > I notice that the old code sets jobs_requested_ to 0 to allow for that, but your > code doesn't seem to. *confusion* I'm not seeing where my old code set jobs_requested_ to zero, unless you mean in AddUrlHandler(), in which case it still does. WRT jobs_outstanding_, I'm going to claim that if jobs_outstanding_ is non-zero between tests, resetting it won't help, because that indicates that there's other global state (the actual outstanding job) that could interfere with the new test. I can put in an assert if you'd like. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:180: run_loop_ = &run_loop; On 2015/01/21 21:33:57, mmenke wrote: > Suggest just a scoped_ptr<RunLoop> in the class - seems cleaner to me. Can > either reset it or not at the end of this method - I'm fine either way. Done. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:190: if (waiting_for_no_jobs_) On 2015/01/21 21:33:57, mmenke wrote: > Any case where this is not the same as run_loop_ != NULL? Can certainly debate > over which is cleaner, but I feel that having to both clear run_loop_ and set > waiting_for_no_jobs_ to false outweighs the code clarity of having the book to > check. CL history I should have cleaned up--I used to have two signals that this class could produce. Shifting over to just using run_loop_. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:271: } On 2015/01/21 21:33:57, mmenke wrote: > // net (And there should be a linebreak before the end of the namespace) Whoops. Done.
Responses. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:66: next_state_ = STATE_IDLE; On 2015/01/22 19:04:39, rdsmith wrote: > On 2015/01/21 21:33:57, mmenke wrote: > > It's a bit tangential to this CL, but while we're here, can we rename IDLE, > > which sounds exactly like STATE_NONE? > > > > States are generally named after the method they run, or vice-versa, so it > > should be STATE_DISPATCH_REQUEST to match the method... > > > > Hrm...Actually, seems like there are a bunch of naming improvements that could > > be done here, and don't think it's reasonable to do all that in this CL. > > Naming changes, while they touch a lot of lines, don't take a lot of time. Want > to give me a nudge in the direction of what you'd like to see and I'll take a > swing at it? State names should match the functions that they dispatch, check. > What else? > > (Also let me know if you'd prefer them in a separate CL, else I'll just dump > them in this CL for my own convenience.) I'd go with: STATE_IDLE => STATE_SEND_REQUEST / DoSendRequest() (Which returns ERR_IO_PENDING, sets next state to STATE_RESPONSE_STARTED) STATE_REQUEST_STARTED => STATE_SEND_REQUEST_COMPLETE / DoSendRequestComplete (Replaces DoRequestStarted, and is called *after* we get headers, instead of before, and just updates the state to the next one, which I'd no longer do in OnResponseStarted. OnResponseStarted also shouldn't be worrying about synchronous callbacks, as they can't happen) STATE_REQUEST_READING => STATE_READ_BODY *New state* => STATE_READ_BODY_COMPLETE (Move all the logic from OnReadCompleted into here, and again, remove the loop_ check) STATE_REQUEST_COMPLETE => STATE_REQUEST_COMPLETE That leaves OnResponseStarted and OnReadCompleted as functions that basically only call back into the DoLoop (Maybe after some DCHECKs). Anyhow, you can see why I was thinking all that might not belong in this CL. :) https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:30: // addition callbacks. On 2015/01/22 19:04:40, rdsmith wrote: > On 2015/01/21 21:33:57, mmenke wrote: > > Class level are generally be put just above the class they describe - don't > > think there's anything here that warrants doing otherwise. > > Hmmm. I can do that, and if you really want me to, I will. But I think it > undercuts the reason I put the documentation in in the first place. > > When I go to modify a test file, the first thing I want to know is what the > infrastructure & usual test pattern is. My goal with this comment was to > provide that information so that the next time I add a test to this file I can > quickly internalize what tools I have to work with and what the likely pattern > of my test is. > > Tell me twice? (Or not.) Rather than tell you twice, which is something I prefer not to do unless I feel it's really needed, I'll tell you my reasoning, and then let you decide. My feeling is that docs are more generally above classes than in one place like this, and that's where Chromites have learned to look first, particularly when navigating through files based on text search (As opposed to opening files and reading them top to bottom, which I, at least, generally don't do, except for headers). In header files, I know that I always miss docs just under the copyright header, and I've also been known to miss comments just above namespace declarations. I have less experience missing comments in those places in CC files, but I suspect that's because comments in those places are less common in CC files. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:37: static const char* kTestDomain = "top.domain.test"; nit: While you're here, "static const char kBlah[]" seems to be preferred. I assume because the "const" is better enforced that way. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:39: class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob { On 2015/01/22 19:04:40, rdsmith wrote: > On 2015/01/21 21:33:57, mmenke wrote: > > This should be in an anonymous namespace, and the above statics can be moved > in > > as well, remove the statics. > > > > I'd suggest sticking this entire file in, actually. > > Done, but I'd love to get a bit more guidance or pointers on idiom, if you have > them. The reason I originally did it this was was the (lazy) urge not to have > to put net:: everywhere. Switching over to using an anonymous namespace > (without any nesting) results in having to put in namespace qualifiers. I could > nest an anonymous namespace inside of a net:: namespace, but that seems a bit > ugly, and "using net::" violates the style guide. Do you have a sense as to the > best idiom? (I'm perfectly willing to believe it's "anonymous, use net:: > everywhere, and suck it up", it's just a shame to have to do that in something > that's conceptually a net/ file.) You don't need namespace qualifiers with an anonymous namespace nested in a net namespace. The best practice for net tests, in my opinion, is just to have a net namespace wrapping an anonymous namespace wrapping the entire file (Some people prefer to end the anonymous namespace just above the test fixture - test fixtures can't share names, anyways. I prefer not to do that, because people can declare classes below the test fixture, and because it discourages friending tests (If it's really needed, I'm fine with that, but if not, I prefer to avoid it). https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:105: static int jobs_outstanding_; On 2015/01/22 19:04:39, rdsmith wrote: > On 2015/01/21 21:33:57, mmenke wrote: > > I'd advise against using non-const static variables, it just seems ugly and > bug > > prone. If you're running the tests multiple times without restarting the test > > fixture, if gets you into trouble. > > Good point, and maybe I'm just having a braino, but what's my alternative? I > want to have a blocking point when the number of outstanding jobs goes to zero, > which means I need to record creation and destruction of those jobs in some > mutable variable that isn't part of a URLRequestSpecifiedResponseJob instance. > If that's not a static of some sort (e.g. a member variable in the test fixture) > then it needs to be registered with URLRequestSpecifiedResponseJob in some > fashion, and that registration information will be in a static. What am I > missing? > > (This is presuming I don't want to use RunUntilIdle(), which I don't but could > be convinced to. But this doesn't seem like the right argument for that.) I'm fine with avoiding RunUntilIdle, like this (I'm not sure the cure is better than the disease in this case, but I'm not sure it's worse, either, so I'm happy to defer to your preference). One option is to use a UrlRequestInterceptor class, which the URLRequestFilter can also take as an argument, and have it track the values instead. Then just keep a raw pointer to the interceptor, and get the values that way. > > I notice that the old code sets jobs_requested_ to 0 to allow for that, but > your > > code doesn't seem to. > > *confusion* I'm not seeing where my old code set jobs_requested_ to zero, > unless you mean in AddUrlHandler(), in which case it still does. WRT > jobs_outstanding_, I'm going to claim that if jobs_outstanding_ is non-zero > between tests, resetting it won't help, because that indicates that there's > other global state (the actual outstanding job) that could interfere with the > new test. I can put in an assert if you'd like. That's a valid point, and I suppose you're also setting signal_on_no_jobs_ on the test's constructor...But relying on all that just seems very icky, particularly since you have 3 class statics...And they're all initialized in completely different places. Does not strike me as a very safe pattern.
And some more elaboration, while I'm thinking about it. Clearly I need more excitement on my Saturday mornings. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:66: next_state_ = STATE_IDLE; On 2015/01/22 19:58:32, mmenke wrote: > On 2015/01/22 19:04:39, rdsmith wrote: > > On 2015/01/21 21:33:57, mmenke wrote: > > > It's a bit tangential to this CL, but while we're here, can we rename IDLE, > > > which sounds exactly like STATE_NONE? > > > > > > States are generally named after the method they run, or vice-versa, so it > > > should be STATE_DISPATCH_REQUEST to match the method... > > > > > > Hrm...Actually, seems like there are a bunch of naming improvements that > could > > > be done here, and don't think it's reasonable to do all that in this CL. > > > > Naming changes, while they touch a lot of lines, don't take a lot of time. > Want > > to give me a nudge in the direction of what you'd like to see and I'll take a > > swing at it? State names should match the functions that they dispatch, > check. > > What else? > > > > (Also let me know if you'd prefer them in a separate CL, else I'll just dump > > them in this CL for my own convenience.) > > I'd go with: > > STATE_IDLE => STATE_SEND_REQUEST / DoSendRequest() (Which returns > ERR_IO_PENDING, sets next state to STATE_RESPONSE_STARTED) > > STATE_REQUEST_STARTED => STATE_SEND_REQUEST_COMPLETE / DoSendRequestComplete > (Replaces DoRequestStarted, and is called *after* we get headers, instead of > before, and just updates the state to the next one, which I'd no longer do in > OnResponseStarted. OnResponseStarted also shouldn't be worrying about > synchronous callbacks, as they can't happen) > > STATE_REQUEST_READING => STATE_READ_BODY > > *New state* => STATE_READ_BODY_COMPLETE (Move all the logic from OnReadCompleted > into here, and again, remove the loop_ check) > > STATE_REQUEST_COMPLETE => STATE_REQUEST_COMPLETE > > > That leaves OnResponseStarted and OnReadCompleted as functions that basically > only call back into the DoLoop (Maybe after some DCHECKs). > > Anyhow, you can see why I was thinking all that might not belong in this CL. :) And a quick followup about my reasoning: I think keeping as much logic in the DoLoop as possible makes the code much easier to follow. You can just follow the state machine to figure out what the next state does, rather than follow the state machine + the callback methods (Which aren't terribly searchable). https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:105: static int jobs_outstanding_; On 2015/01/22 19:58:33, mmenke wrote: > On 2015/01/22 19:04:39, rdsmith wrote: > > On 2015/01/21 21:33:57, mmenke wrote: > > > I'd advise against using non-const static variables, it just seems ugly and > > bug > > > prone. If you're running the tests multiple times without restarting the > test > > > fixture, if gets you into trouble. > > > > Good point, and maybe I'm just having a braino, but what's my alternative? I > > want to have a blocking point when the number of outstanding jobs goes to > zero, > > which means I need to record creation and destruction of those jobs in some > > mutable variable that isn't part of a URLRequestSpecifiedResponseJob instance. > > > If that's not a static of some sort (e.g. a member variable in the test > fixture) > > then it needs to be registered with URLRequestSpecifiedResponseJob in some > > fashion, and that registration information will be in a static. What am I > > missing? > > > > (This is presuming I don't want to use RunUntilIdle(), which I don't but could > > be convinced to. But this doesn't seem like the right argument for that.) > > I'm fine with avoiding RunUntilIdle, like this (I'm not sure the cure is better > than the disease in this case, but I'm not sure it's worse, either, so I'm happy > to defer to your preference). One option is to use a UrlRequestInterceptor > class, which the URLRequestFilter can also take as an argument, and have it > track the values instead. Then just keep a raw pointer to the interceptor, and > get the values that way. And a quick followup... If keeping around an unowned pointer bothers you, could either use a weak pointer, or have a shared struct the intercept writes to, and the test fixture reads from.
Thanks for all the comments! Believed incorporated; PTAL. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:66: next_state_ = STATE_IDLE; On 2015/01/24 17:01:24, mmenke wrote: > On 2015/01/22 19:58:32, mmenke wrote: > > On 2015/01/22 19:04:39, rdsmith wrote: > > > On 2015/01/21 21:33:57, mmenke wrote: > > > > It's a bit tangential to this CL, but while we're here, can we rename > IDLE, > > > > which sounds exactly like STATE_NONE? > > > > > > > > States are generally named after the method they run, or vice-versa, so it > > > > should be STATE_DISPATCH_REQUEST to match the method... > > > > > > > > Hrm...Actually, seems like there are a bunch of naming improvements that > > could > > > > be done here, and don't think it's reasonable to do all that in this CL. > > > > > > Naming changes, while they touch a lot of lines, don't take a lot of time. > > Want > > > to give me a nudge in the direction of what you'd like to see and I'll take > a > > > swing at it? State names should match the functions that they dispatch, > > check. > > > What else? > > > > > > (Also let me know if you'd prefer them in a separate CL, else I'll just dump > > > them in this CL for my own convenience.) > > > > I'd go with: > > > > STATE_IDLE => STATE_SEND_REQUEST / DoSendRequest() (Which returns > > ERR_IO_PENDING, sets next state to STATE_RESPONSE_STARTED) > > > > STATE_REQUEST_STARTED => STATE_SEND_REQUEST_COMPLETE / DoSendRequestComplete > > (Replaces DoRequestStarted, and is called *after* we get headers, instead of > > before, and just updates the state to the next one, which I'd no longer do in > > OnResponseStarted. OnResponseStarted also shouldn't be worrying about > > synchronous callbacks, as they can't happen) > > > > STATE_REQUEST_READING => STATE_READ_BODY > > > > *New state* => STATE_READ_BODY_COMPLETE (Move all the logic from > OnReadCompleted > > into here, and again, remove the loop_ check) > > > > STATE_REQUEST_COMPLETE => STATE_REQUEST_COMPLETE > > > > > > That leaves OnResponseStarted and OnReadCompleted as functions that basically > > only call back into the DoLoop (Maybe after some DCHECKs). > > > > Anyhow, you can see why I was thinking all that might not belong in this CL. > :) > > And a quick followup about my reasoning: I think keeping as much logic in the > DoLoop as possible makes the code much easier to follow. You can just follow > the state machine to figure out what the next state does, rather than follow the > state machine + the callback methods (Which aren't terribly searchable). Yes, that makes sense to me (and was appealing to me as I was doing the rewrite). I'm doing this in a separate CL, coming soon to an inbox near you. Just to give you more exciting things for your weekend :-}. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:30: // addition callbacks. On 2015/01/22 19:58:33, mmenke wrote: > On 2015/01/22 19:04:40, rdsmith wrote: > > On 2015/01/21 21:33:57, mmenke wrote: > > > Class level are generally be put just above the class they describe - don't > > > think there's anything here that warrants doing otherwise. > > > > Hmmm. I can do that, and if you really want me to, I will. But I think it > > undercuts the reason I put the documentation in in the first place. > > > > When I go to modify a test file, the first thing I want to know is what the > > infrastructure & usual test pattern is. My goal with this comment was to > > provide that information so that the next time I add a test to this file I can > > quickly internalize what tools I have to work with and what the likely pattern > > of my test is. > > > > Tell me twice? (Or not.) > > Rather than tell you twice, which is something I prefer not to do unless I feel > it's really needed, I'll tell you my reasoning, and then let you decide. > > My feeling is that docs are more generally above classes than in one place like > this, and that's where Chromites have learned to look first, particularly when > navigating through files based on text search (As opposed to opening files and > reading them top to bottom, which I, at least, generally don't do, except for > headers). > > In header files, I know that I always miss docs just under the copyright header, > and I've also been known to miss comments just above namespace declarations. I > have less experience missing comments in those places in CC files, but I suspect > that's because comments in those places are less common in CC files. Thinking about it, the thing I care the most about is that this stays together as a comment, as opposed to being broken up into different places in the file. It sounds like your concerns are primarily about discoverability, so I'll put this entire comment just above the SdchDictionaryFetcherTest class. (The reason I didn't do that originally is that it's about the entire file, but I'm happy to have it there if it means people will find it.) Let me know if you have any objection to that compromise. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:37: static const char* kTestDomain = "top.domain.test"; On 2015/01/22 19:58:33, mmenke wrote: > nit: While you're here, "static const char kBlah[]" seems to be preferred. I > assume because the "const" is better enforced that way. Done. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:39: class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob { On 2015/01/22 19:58:32, mmenke wrote: > On 2015/01/22 19:04:40, rdsmith wrote: > > On 2015/01/21 21:33:57, mmenke wrote: > > > This should be in an anonymous namespace, and the above statics can be moved > > in > > > as well, remove the statics. > > > > > > I'd suggest sticking this entire file in, actually. > > > > Done, but I'd love to get a bit more guidance or pointers on idiom, if you > have > > them. The reason I originally did it this was was the (lazy) urge not to have > > to put net:: everywhere. Switching over to using an anonymous namespace > > (without any nesting) results in having to put in namespace qualifiers. I > could > > nest an anonymous namespace inside of a net:: namespace, but that seems a bit > > ugly, and "using net::" violates the style guide. Do you have a sense as to > the > > best idiom? (I'm perfectly willing to believe it's "anonymous, use net:: > > everywhere, and suck it up", it's just a shame to have to do that in something > > that's conceptually a net/ file.) > > You don't need namespace qualifiers with an anonymous namespace nested in a net > namespace. The best practice for net tests, in my opinion, is just to have a > net namespace wrapping an anonymous namespace wrapping the entire file (Some > people prefer to end the anonymous namespace just above the test fixture - test > fixtures can't share names, anyways. I prefer not to do that, because people > can declare classes below the test fixture, and because it discourages friending > tests (If it's really needed, I'm fine with that, but if not, I prefer to avoid > it). Sounds good. I'll adopt the whole file double nest idiom, with modifiers for when tests need to be friended. https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:105: static int jobs_outstanding_; On 2015/01/24 17:01:24, mmenke wrote: > On 2015/01/22 19:58:33, mmenke wrote: > > On 2015/01/22 19:04:39, rdsmith wrote: > > > On 2015/01/21 21:33:57, mmenke wrote: > > > > I'd advise against using non-const static variables, it just seems ugly > and > > > bug > > > > prone. If you're running the tests multiple times without restarting the > > test > > > > fixture, if gets you into trouble. > > > > > > Good point, and maybe I'm just having a braino, but what's my alternative? > I > > > want to have a blocking point when the number of outstanding jobs goes to > > zero, > > > which means I need to record creation and destruction of those jobs in some > > > mutable variable that isn't part of a URLRequestSpecifiedResponseJob > instance. > > > > > If that's not a static of some sort (e.g. a member variable in the test > > fixture) > > > then it needs to be registered with URLRequestSpecifiedResponseJob in some > > > fashion, and that registration information will be in a static. What am I > > > missing? > > > > > > (This is presuming I don't want to use RunUntilIdle(), which I don't but > could > > > be convinced to. But this doesn't seem like the right argument for that.) > > > > I'm fine with avoiding RunUntilIdle, like this (I'm not sure the cure is > better > > than the disease in this case, but I'm not sure it's worse, either, so I'm > happy > > to defer to your preference). One option is to use a UrlRequestInterceptor > > class, which the URLRequestFilter can also take as an argument, and have it > > track the values instead. Then just keep a raw pointer to the interceptor, > and > > get the values that way. > > And a quick followup... If keeping around an unowned pointer bothers you, could > either use a weak pointer, or have a shared struct the intercept writes to, and > the test fixture reads from. I'm good with your original suggestion, and it didn't require keeping around an unowned pointer (I just passed a callback with a weak pointer in at construction time). For some reason, I wasn't clueing into the possibilities inherent in intercepting construction from the instantiation side.
LGTM https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/864923002/diff/40001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:30: // addition callbacks. On 2015/01/24 23:21:01, rdsmith wrote: > On 2015/01/22 19:58:33, mmenke wrote: > > On 2015/01/22 19:04:40, rdsmith wrote: > > > On 2015/01/21 21:33:57, mmenke wrote: > > > > Class level are generally be put just above the class they describe - > don't > > > > think there's anything here that warrants doing otherwise. > > > > > > Hmmm. I can do that, and if you really want me to, I will. But I think it > > > undercuts the reason I put the documentation in in the first place. > > > > > > When I go to modify a test file, the first thing I want to know is what the > > > infrastructure & usual test pattern is. My goal with this comment was to > > > provide that information so that the next time I add a test to this file I > can > > > quickly internalize what tools I have to work with and what the likely > pattern > > > of my test is. > > > > > > Tell me twice? (Or not.) > > > > Rather than tell you twice, which is something I prefer not to do unless I > feel > > it's really needed, I'll tell you my reasoning, and then let you decide. > > > > My feeling is that docs are more generally above classes than in one place > like > > this, and that's where Chromites have learned to look first, particularly when > > navigating through files based on text search (As opposed to opening files and > > reading them top to bottom, which I, at least, generally don't do, except for > > headers). > > > > In header files, I know that I always miss docs just under the copyright > header, > > and I've also been known to miss comments just above namespace declarations. > I > > have less experience missing comments in those places in CC files, but I > suspect > > that's because comments in those places are less common in CC files. > > Thinking about it, the thing I care the most about is that this stays together > as a comment, as opposed to being broken up into different places in the file. > It sounds like your concerns are primarily about discoverability, so I'll put > this entire comment just above the SdchDictionaryFetcherTest class. (The reason > I didn't do that originally is that it's about the entire file, but I'm happy to > have it there if it means people will find it.) Let me know if you have any > objection to that compromise. Works for me. https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:46: if (!destruction_callback_.is_null()) Since you're never passing in a null callback, shouldn't bother with this (A DCHECK is fine, if you want. Note that even if the weak pointer is NULL, the callback is not - otherwise, couldn't null check on different threads). https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:60: base::Closure destruction_callback_; optional: Could make this const. https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:65: typedef base::Callback<void(int outstanding_job_delta)> LifecycleCallback; Think this could use a comment, what it doesn't what its argument does, etc. https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:67: SpecifiedResponseJobInterceptor(const LifecycleCallback& lifecycle_callback) nit: explicit https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:81: factory_.GetWeakPtr())); Don't need two weak pointers here (The test fixture has one, and this class has another)... In fact, since the callback is never null, can get rid of "OnSpecfiedResponseJobDestruction" and just do everything here: base::Bind(lifecycle_callback_, -1) https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:158: void GetDictionaryAdditions(std::vector<DictionaryAdditions>* out) { Suggest comments here. The clear behavior, in particular, is a bit confusing, given the name "Get". https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:166: int jobs_requested() { return jobs_requested_; } nit: const https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:191: if (jobs_outstanding_ == 0 && run_loop_.get()) nit: get() isn't necessary when coercing to bool (I figure since scoped_ptr has code specifically to support that, we should use it) https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:255: GURL dictionary_url(PathToGurl("dictionary")); Suggest replacing "dictionary" with "dictionary0". Having dictionary and dictionary1 is needlessly confusing. 0/1 instead of 1/2 results in matching the indices of the additions array (I assume that's why you used 1 instead of 2). https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:261: EXPECT_EQ(2, jobs_requested()); This should be an assert, I think.
Thanks! https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:46: if (!destruction_callback_.is_null()) On 2015/01/26 16:06:06, mmenke wrote: > Since you're never passing in a null callback, shouldn't bother with this (A > DCHECK is fine, if you want. Note that even if the weak pointer is NULL, the > callback is not - otherwise, couldn't null check on different threads). Done. https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:60: base::Closure destruction_callback_; On 2015/01/26 16:06:06, mmenke wrote: > optional: Could make this const. Done. https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:65: typedef base::Callback<void(int outstanding_job_delta)> LifecycleCallback; On 2015/01/26 16:06:06, mmenke wrote: > Think this could use a comment, what it doesn't what its argument does, etc. Done. https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:67: SpecifiedResponseJobInterceptor(const LifecycleCallback& lifecycle_callback) On 2015/01/26 16:06:06, mmenke wrote: > nit: explicit Done. https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:81: factory_.GetWeakPtr())); On 2015/01/26 16:06:06, mmenke wrote: > Don't need two weak pointers here (The test fixture has one, and this class has > another)... In fact, since the callback is never null, can get rid of > "OnSpecfiedResponseJobDestruction" and just do everything here: > > base::Bind(lifecycle_callback_, -1) Good point. I'm including the DCHECKs for interface specification, though. Done. https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:158: void GetDictionaryAdditions(std::vector<DictionaryAdditions>* out) { On 2015/01/26 16:06:06, mmenke wrote: > Suggest comments here. The clear behavior, in particular, is a bit confusing, > given the name "Get". Done. https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:166: int jobs_requested() { return jobs_requested_; } On 2015/01/26 16:06:06, mmenke wrote: > nit: const Done. https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:191: if (jobs_outstanding_ == 0 && run_loop_.get()) On 2015/01/26 16:06:06, mmenke wrote: > nit: get() isn't necessary when coercing to bool (I figure since scoped_ptr has > code specifically to support that, we should use it) Done. https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:255: GURL dictionary_url(PathToGurl("dictionary")); On 2015/01/26 16:06:06, mmenke wrote: > Suggest replacing "dictionary" with "dictionary0". Having dictionary and > dictionary1 is needlessly confusing. 0/1 instead of 1/2 results in matching the > indices of the additions array (I assume that's why you used 1 instead of 2). Probably--I don't remember :-}. Done. https://codereview.chromium.org/864923002/diff/100001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:261: EXPECT_EQ(2, jobs_requested()); On 2015/01/26 16:06:06, mmenke wrote: > This should be an assert, I think. Agreed; done.
The CQ bit was checked by rdsmith@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/864923002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/81fe0e576f9cfe564fa85bfa545de68c26b8a02c Cr-Commit-Position: refs/heads/master@{#313125} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
