Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
807503002:
Implement throttling logic for fetching affiliation information. (Closed)
|
Created:
6 years ago by engedy Modified:
5 years, 10 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@aff_database Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement throttling logic for fetching affiliation information.
BUG=437865
Committed: https://crrev.com/89726c2516dfc9ca5dc4cc23acff4841608ad97c
Cr-Commit-Position: refs/heads/master@{#314127}
Patch Set 1 #Patch Set 2 : Mass rename, extended tests, no longer spamming TaskRunner. #Patch Set 3 : Fix header guards. #
Total comments: 40
Patch Set 4 : Addressed first round of comments. #
Total comments: 24
Patch Set 5 : Addressed moar comments. #Patch Set 6 : Nit #
Total comments: 38
Patch Set 7 : Addressed most unittest comments. #
Total comments: 7
Patch Set 8 : Addressed more comments. #
Total comments: 10
Patch Set 9 : Addressed final comments from mmenke@, format nits. #Patch Set 10 : Rebase. #Patch Set 11 : Missing override. #Patch Set 12 : Fix ambiguity with pow() on Android. #Messages
Total messages: 33 (3 generated)
engedy@chromium.org changed reviewers: + mmenke@chromium.org
Hi Matt, as discussed, could you please take a preliminary look at this? This would control how often the AffiliationFetcher (introduced in https://codereview.chromium.org/767163005/) will be allowed to fetch in case of network errors and/or during offline periods. Unit tests are very rudimentary, I wanted to make sure that it makes at least a tiny bit of sense before I refine the CL any further. Please see the bug for a link to the design doc.
Matt, I have implemented the changes we have discussed yesterday. Could you please take a look? (Please ignore any style nits for now.) Also, you mentioned that there are some error codes that warrant special treatment. Could you please provide more details regarding that?
On 2014/12/16 14:34:06, engedy wrote: > Matt, I have implemented the changes we have discussed yesterday. Could you > please take a look? (Please ignore any style nits for now.) > > Also, you mentioned that there are some error codes that warrant special > treatment. Could you please provide more details regarding that? Matt, as discussed, now that the new year is here, let us look into this.
On 2015/01/05 10:06:12, engedy wrote: > On 2014/12/16 14:34:06, engedy wrote: > > Matt, I have implemented the changes we have discussed yesterday. Could you > > please take a look? (Please ignore any style nits for now.) > > > > Also, you mentioned that there are some error codes that warrant special > > treatment. Could you please provide more details regarding that? > > Matt, as discussed, now that the new year is here, let us look into this. So sorry for slowness. I've done at least 20 reviews this week (Prioritizing bug fixes, and small ones that require relatively little thought). I'll try to get to this today, but if not, Monday. Feel free to ping if I don't.
On 2015/01/09 17:17:39, mmenke wrote: > On 2015/01/05 10:06:12, engedy wrote: > > On 2014/12/16 14:34:06, engedy wrote: > > > Matt, I have implemented the changes we have discussed yesterday. Could you > > > please take a look? (Please ignore any style nits for now.) > > > > > > Also, you mentioned that there are some error codes that warrant special > > > treatment. Could you please provide more details regarding that? > > > > Matt, as discussed, now that the new year is here, let us look into this. > > So sorry for slowness. I've done at least 20 reviews this week (Prioritizing > bug fixes, and small ones that require relatively little thought). I'll try to > get to this today, but if not, Monday. Feel free to ping if I don't. No worries, Monday is perfectly fine!
First pass comments - stronly recommend focusing on the higher level questions rather worrying about the nits. Now that I've drilled down on my queue, and taken a first look at this, I should be *much* more responsive. Thanks for your patience. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... File components/password_manager/core/browser/affiliation_fetch_throttler.cc (right): https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:17: net::BackoffEntry::Policy kAffiliationBackoffParameters = { const https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:47: const int64 kGracePeriodAfterNetworkRestoredInMs = 10 * 1000; // 10 seconds Should include basictypes.h for int64 https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:52: BackoffEntryImpl(base::TickClock* tick_clock) explicit https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:54: ~BackoffEntryImpl() override{}; nit: space after "override" https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:58: base::TimeTicks ImplGetTimeNow() const override { optional: This can be private (parent classes can call their own virtual methods, even if private in subclasses). Generally feel if something doesn't need to be protected, may as well just make it private, but if you prefer to keep protected, I can certainly live with that. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:84: net::NetworkChangeNotifier::AddConnectionTypeObserver(this); Putting this in a constructor seems like a bad idea - could end up dereferencing a pointer this |this|'s vtable on another thread before construction completes. Since we don't subclass this, that works, but, if we did subclass this, that would be a problem. Think it's best not to depend on such things, so I'd suggest only start watching when a network request is needed, and stop when we return to idle state (Or add some other method to start watching the NCN). https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:141: if (has_network_connectivity_ == !net::NetworkChangeNotifier::IsOffline()) Shouldn't call NetworkChangeNotifier::IsOffline twice - it can be expensive on windows. Actually, we're already given a ConnectionType, let's just use "type == net::NetworkChangeNotifier::CONNECTION_NONE" instead. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:141: if (has_network_connectivity_ == !net::NetworkChangeNotifier::IsOffline()) What if we're switching from WIFI to cellular? https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:146: return; No provisions to retry here? https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:146: return; While being offline is one of the most common causes of network errors, it only accounts for about 15% of total network errors for main frames. If you're concerned about being offline, you should be concerned about the other errors, too, and do some sort of retry with backoff for them, too, right? I'd suggest just retrying on errors, with exponential backoff, gathering metrics, and then figuring out if adding in NCN support would be helpful in reducing the number of retries / getting the actual thing we want sooner. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... File components/password_manager/core/browser/affiliation_fetch_throttler.h (right): https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:11: #include "net/base/backoff_entry.h" Think we can forward declare this? https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:21: // A delegate to receive a callback once it is okay to issue a network request. This needs clearer documentation, since it's the first thing one sees when looking at this file. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:22: class AffiliationFetchThrottlerDelegate { option: Believe it's currently recommended these go in their own file https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:26: // since the last request. The implementor should return whether a request was nit: implementor -> implemention Should avoid ambiguity, and implementor can also be mean the person who writes the implementation. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:27: // actually initiated, and if so, should call InformOfNetworkRequestComplete() nit: should -> must (Maybe I've been reading too many RFCs) https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:42: : public net::NetworkChangeNotifier::ConnectionTypeObserver { I wonder about whether we really should be using NCN - it's not the most reliable signal in the world, so this code should also work if it's incorrectly claiming we're online. Using it can be an optimization (If we're offline, no need to do exponential backoff, and could save some effort as well), but should work without it. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:76: virtual void OnConnectionTypeChanged( nit: --virtual https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:87: scoped_ptr<net::BackoffEntry> exponential_backoff_; Need to include scoped_ptr.h
https://codereview.chromium.org/807503002/diff/40001/components/password_mana... File components/password_manager/core/browser/affiliation_fetch_throttler.cc (right): https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:104: exponential_backoff_->InformOfRequest(success); Oops...Got so sidetracked thinking about the NCN stuff, that I somehow missed this. I'd still suggest we start without the NCN stuff for now, because smaller CLs make life easier, and reviewing 600 line CLs is a pain, but I'm less concerned that we're missing stuff.
https://codereview.chromium.org/807503002/diff/40001/components/password_mana... File components/password_manager/core/browser/affiliation_fetch_throttler.cc (right): https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:17: net::BackoffEntry::Policy kAffiliationBackoffParameters = { On 2015/01/14 19:25:07, mmenke wrote: > const Done. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:47: const int64 kGracePeriodAfterNetworkRestoredInMs = 10 * 1000; // 10 seconds On 2015/01/14 19:25:07, mmenke wrote: > Should include basictypes.h for int64 Ahh, it looks like that is already deprecated. I have included <stdint.h> and changed this to int64_t instead. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:52: BackoffEntryImpl(base::TickClock* tick_clock) On 2015/01/14 19:25:07, mmenke wrote: > explicit Done. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:54: ~BackoffEntryImpl() override{}; On 2015/01/14 19:25:07, mmenke wrote: > nit: space after "override" Ahh, actually, there is also a superfluous semi-colon in here. The lack of space was, in fact, a clang-format bug. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:58: base::TimeTicks ImplGetTimeNow() const override { On 2015/01/14 19:25:06, mmenke wrote: > optional: This can be private (parent classes can call their own virtual > methods, even if private in subclasses). Generally feel if something doesn't > need to be protected, may as well just make it private, but if you prefer to > keep protected, I can certainly live with that. Done. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:84: net::NetworkChangeNotifier::AddConnectionTypeObserver(this); On 2015/01/14 19:25:06, mmenke wrote: > Putting this in a constructor seems like a bad idea - could end up dereferencing > a pointer this |this|'s vtable on another thread before construction completes. > Since we don't subclass this, that works, but, if we did subclass this, that > would be a problem. > > Think it's best not to depend on such things, so I'd suggest only start watching > when a network request is needed, and stop when we return to idle state (Or add > some other method to start watching the NCN). I think that is a bit over-cautious. The contract of ObserverList explicitly states that notifications will be delivered to |this| on the thread where it has registered from. It also states that it will always PostTask(), so we cannot get a synchronous callback either. In fact, 10 out of 12 users of NetworkChangeNotifier register themselves in their constructors, so if this ever changes without proper refactorings, it will break lots of users. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:104: exponential_backoff_->InformOfRequest(success); On 2015/01/14 20:43:43, mmenke wrote: > Oops...Got so sidetracked thinking about the NCN stuff, that I somehow missed > this. I'd still suggest we start without the NCN stuff for now, because smaller > CLs make life easier, and reviewing 600 line CLs is a pain, but I'm less > concerned that we're missing stuff. Please see my other comment. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:141: if (has_network_connectivity_ == !net::NetworkChangeNotifier::IsOffline()) On 2015/01/14 19:25:07, mmenke wrote: > Shouldn't call NetworkChangeNotifier::IsOffline twice - it can be expensive on > windows. Actually, we're already given a ConnectionType, let's just use "type > == net::NetworkChangeNotifier::CONNECTION_NONE" instead. Done. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:141: if (has_network_connectivity_ == !net::NetworkChangeNotifier::IsOffline()) On 2015/01/14 19:25:06, mmenke wrote: > What if we're switching from WIFI to cellular? Hmm. So far I did not consider the kind of the connection to be relevant: during normal operation, the requests should be few and far between (1-2 requests per day, <10kB in size). Do you think we should make a distinction? https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:146: return; On 2015/01/14 19:25:06, mmenke wrote: > While being offline is one of the most common causes of network errors, it only > accounts for about 15% of total network errors for main frames. If you're > concerned about being offline, you should be concerned about the other errors, > too, and do some sort of retry with backoff for them, too, right? > > I'd suggest just retrying on errors, with exponential backoff, gathering > metrics, and then figuring out if adding in NCN support would be helpful in > reducing the number of retries / getting the actual thing we want sooner. Hmm. I believe this is somewhat different than network errors during mainframe navigations. For one, I would suspect that those statistics apply here only partially, as users do not try to browse for much longer after they have realized there is no network connection, but the requests controlled by this class would be initiated in the background, periodically, and there would be no way for the user to forcibly trigger them. For other kinds of network errors, yes, I agree that there should be exponential back-off and retry. But in the case of the pseudo-error that there is no connectivity, it would strike me as bad UX if password filling didn't work for some hours after you returned from a hike in the mountains. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:146: return; On 2015/01/14 19:25:07, mmenke wrote: > No provisions to retry here? I am not sure I understand. Are you implying that NetworkChangeNotifier can have false-negatives to the extent that we should retry after some time even while NetworkChangeNotifier reports no connectivity? https://codereview.chromium.org/807503002/diff/40001/components/password_mana... File components/password_manager/core/browser/affiliation_fetch_throttler.h (right): https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:11: #include "net/base/backoff_entry.h" On 2015/01/14 19:25:07, mmenke wrote: > Think we can forward declare this? Done. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:21: // A delegate to receive a callback once it is okay to issue a network request. On 2015/01/14 19:25:07, mmenke wrote: > This needs clearer documentation, since it's the first thing one sees when > looking at this file. I have moved the delegate to its own file. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:22: class AffiliationFetchThrottlerDelegate { On 2015/01/14 19:25:07, mmenke wrote: > option: Believe it's currently recommended these go in their own file Done. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:26: // since the last request. The implementor should return whether a request was On 2015/01/14 19:25:07, mmenke wrote: > nit: implementor -> implemention > > Should avoid ambiguity, and implementor can also be mean the person who writes > the implementation. Done. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:27: // actually initiated, and if so, should call InformOfNetworkRequestComplete() On 2015/01/14 19:25:07, mmenke wrote: > nit: should -> must (Maybe I've been reading too many RFCs) Done. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:42: : public net::NetworkChangeNotifier::ConnectionTypeObserver { On 2015/01/14 19:25:07, mmenke wrote: > I wonder about whether we really should be using NCN - it's not the most > reliable signal in the world, so this code should also work if it's incorrectly > claiming we're online. Using it can be an optimization (If we're offline, no > need to do exponential backoff, and could save some effort as well), but should > work without it. Yes, even as a signal that is known to produce false-positives, it would be more than merely an optimization. My biggest worry: if the user goes offline for some hours, then, by the time connectivity is reestablished, the backoff delay will be so large that the first network request will not be issued until hours after the offline-online transition. Cached data will likely expire, so dependent functionality will not work in this time window. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:76: virtual void OnConnectionTypeChanged( On 2015/01/14 19:25:07, mmenke wrote: > nit: --virtual Done. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:87: scoped_ptr<net::BackoffEntry> exponential_backoff_; On 2015/01/14 19:25:07, mmenke wrote: > Need to include scoped_ptr.h Done.
Plan to go through tests on Tuesday. https://codereview.chromium.org/807503002/diff/40001/components/password_mana... File components/password_manager/core/browser/affiliation_fetch_throttler.cc (right): https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:141: if (has_network_connectivity_ == !net::NetworkChangeNotifier::IsOffline()) On 2015/01/15 19:16:42, engedy wrote: > On 2015/01/14 19:25:06, mmenke wrote: > > What if we're switching from WIFI to cellular? > > Hmm. So far I did not consider the kind of the connection to be relevant: during > normal operation, the requests should be few and far between (1-2 requests per > day, <10kB in size). Do you think we should make a distinction? Now that I realize how this class actually works, I think we're fine. (Also, looking at NCN, it gives us a bogus transaction to being disconnected in that case, anyways) https://codereview.chromium.org/807503002/diff/40001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:146: return; On 2015/01/15 19:16:42, engedy wrote: > On 2015/01/14 19:25:07, mmenke wrote: > > No provisions to retry here? > > I am not sure I understand. Are you implying that NetworkChangeNotifier can have > false-negatives to the extent that we should retry after some time even while > NetworkChangeNotifier reports no connectivity? Sorry, got confused about how this would plug in (This works like BackoffEntry, which works a bit weirdly - instead of being responsible for dealing with the request itself, the consumer is responsible for telling it the status of the request, so some of my earlier comments were confused. Been a long week). https://codereview.chromium.org/807503002/diff/60001/components/password_mana... File components/password_manager/core/browser/affiliation_fetch_throttler.cc (right): https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:88: has_network_connectivity_ = !net::NetworkChangeNotifier::IsOffline(); Think it's worth a comment why this shouldn't be in the initializer list (Has to be done after being added as an observer...) https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:96: if (state_ != IDLE) Should we DCHECK on this? I assume one class will own |this| and be solely responsible for talking to it..Seems kinda weird to call this multiple times, though I have no idea about what we're actually getting. Either way, if it's allowed or not, should have some docs about it. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:99: state_ = FETCH_NEEDED; Does the delegate have to be able to handle a call to OnCanSendNetworkRequest while a fetch is in flight? If not, you're going to need a new state where a fetch is needed *and* a fetch is in flight - two bools would make more sense, in that case. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:105: DCHECK_EQ(state_, FETCH_IN_FLIGHT); Suggest making expected value the first argument (It's ASSERT_EQ/EXPECT_EQ where it's required, but I think it's best to be consistent). Goes for the others in this file as well. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:115: is_fetch_scheduled_ = true; Suggest using a OneShotTimer instead, it keeps track of this, and makes life simpler (And less regression prone, in my opinion). Can also easily stop it or restart it when connectivity changes, which I think makes life simpler. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... File components/password_manager/core/browser/affiliation_fetch_throttler.h (right): https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:40: scoped_refptr<base::SingleThreadTaskRunner> task_runner, const scoped_reftr<...>&, to avoid an extra ref count increment/decrement? https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:53: protected: I think these can all be private.
https://codereview.chromium.org/807503002/diff/60001/components/password_mana... File components/password_manager/core/browser/affiliation_fetch_throttler.cc (right): https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:107: exponential_backoff_->InformOfRequest(success); It's up to the consumer to tell us if/when they want to try again, as opposed to automatically triggering retries, with backoff? Think that's also worth mentioning in the header.
https://codereview.chromium.org/807503002/diff/60001/components/password_mana... File components/password_manager/core/browser/affiliation_fetch_throttler.cc (right): https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:99: state_ = FETCH_NEEDED; On 2015/01/16 21:28:35, mmenke wrote: > Does the delegate have to be able to handle a call to OnCanSendNetworkRequest > while a fetch is in flight? If not, you're going to need a new state where a > fetch is needed *and* a fetch is in flight - two bools would make more sense, in > that case. Hrm...Was thinking there was a case where that could happen, but looks like I was mistaken. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:111: DCHECK_EQ(state_, FETCH_NEEDED); DCHECK(has_network_connectivity_)? https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:120: exponential_backoff_->GetTimeUntilRelease()); Also, do we need async behavior here? Seems like we have a lot more checks than needed here. If we could just have: if (!has_network_connectivity_ || is_fetch_scheduled_) return; if (exponential_backoff_->ShouldRejectRequest()) // Post task else // run task, update state Could get rid of OnBackoffDelayExpiredCallback, remove some logic from AffiliationFetchThrottler::OnConnectionTypeChanged, and remove an if from SignalNetworkRequestNeeded.
Oh, and you forgot to upload the new delegate file. :)
https://codereview.chromium.org/807503002/diff/60001/components/password_mana... File components/password_manager/core/browser/affiliation_fetch_throttler.cc (right): https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:88: has_network_connectivity_ = !net::NetworkChangeNotifier::IsOffline(); On 2015/01/16 21:28:34, mmenke wrote: > Think it's worth a comment why this shouldn't be in the initializer list (Has to > be done after being added as an observer...) Done. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:96: if (state_ != IDLE) On 2015/01/16 21:28:35, mmenke wrote: > Should we DCHECK on this? I assume one class will own |this| and be solely > responsible for talking to it..Seems kinda weird to call this multiple times, > though I have no idea about what we're actually getting. > > Either way, if it's allowed or not, should have some docs about it. Being able to call this multiple times allows the caller to not keep track of whether or not it has already signaled that a request is needed. Added comments to clarify this. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:99: state_ = FETCH_NEEDED; On 2015/01/16 21:49:54, mmenke wrote: > On 2015/01/16 21:28:35, mmenke wrote: > > Does the delegate have to be able to handle a call to OnCanSendNetworkRequest > > while a fetch is in flight? If not, you're going to need a new state where a > > fetch is needed *and* a fetch is in flight - two bools would make more sense, > in > > that case. > > Hrm...Was thinking there was a case where that could happen, but looks like I > was mistaken. Yes, I believe this can never happen. I have added a class comment to clarify that the class supports one request at a time. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:105: DCHECK_EQ(state_, FETCH_IN_FLIGHT); On 2015/01/16 21:28:34, mmenke wrote: > Suggest making expected value the first argument (It's ASSERT_EQ/EXPECT_EQ where > it's required, but I think it's best to be consistent). Goes for the others in > this file as well. I'd prefer to keep it this way. As opposed to EXPECT_EQ(), DCHECK_EQ() makes no distinction between its arguments, and having them listed in this order will result in the more readable output: Check failed: state_ == FETCH_NEEDED (1 vs. 2) https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:107: exponential_backoff_->InformOfRequest(success); On 2015/01/16 21:31:28, mmenke wrote: > It's up to the consumer to tell us if/when they want to try again, as opposed to > automatically triggering retries, with backoff? Think that's also worth > mentioning in the header. Done. PTAL at the class comment. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:111: DCHECK_EQ(state_, FETCH_NEEDED); On 2015/01/16 21:49:54, mmenke wrote: > DCHECK(has_network_connectivity_)? Done. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:115: is_fetch_scheduled_ = true; On 2015/01/16 21:28:34, mmenke wrote: > Suggest using a OneShotTimer instead, it keeps track of this, and makes life > simpler (And less regression prone, in my opinion). Can also easily stop it or > restart it when connectivity changes, which I think makes life simpler. I wanted to use that, but it does not allow time to be mocked out, which is needed for the unit tests. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:120: exponential_backoff_->GetTimeUntilRelease()); On 2015/01/16 21:49:54, mmenke wrote: > Also, do we need async behavior here? Seems like we have a lot more checks than > needed here. If we could just have: > > if (!has_network_connectivity_ || is_fetch_scheduled_) > return; > > if (exponential_backoff_->ShouldRejectRequest()) > // Post task > else > // run task, update state > > Could get rid of OnBackoffDelayExpiredCallback, remove some logic from > AffiliationFetchThrottler::OnConnectionTypeChanged, and remove an if from > SignalNetworkRequestNeeded. While we could certainly do that, do you think putting all that logic into a single method would be more readable? I usually get suspicious when I cannot come up with a name for a method, and I cannot seem to. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... File components/password_manager/core/browser/affiliation_fetch_throttler.h (right): https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:40: scoped_refptr<base::SingleThreadTaskRunner> task_runner, On 2015/01/16 21:28:35, mmenke wrote: > const scoped_reftr<...>&, to avoid an extra ref count increment/decrement? Done. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.h:53: protected: On 2015/01/16 21:28:35, mmenke wrote: > I think these can all be private. Done.
Sorry I didn't get to this today - first thing on my slate tomorrow.
https://codereview.chromium.org/807503002/diff/60001/components/password_mana... File components/password_manager/core/browser/affiliation_fetch_throttler.cc (right): https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:115: is_fetch_scheduled_ = true; On 2015/01/19 18:13:11, engedy wrote: > On 2015/01/16 21:28:34, mmenke wrote: > > Suggest using a OneShotTimer instead, it keeps track of this, and makes life > > simpler (And less regression prone, in my opinion). Can also easily stop it > or > > restart it when connectivity changes, which I think makes life simpler. > > I wanted to use that, but it does not allow time to be mocked out, which is > needed for the unit tests. Ah, I wasn't aware we had a task runner class with mockable time. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:120: exponential_backoff_->GetTimeUntilRelease()); On 2015/01/19 18:13:11, engedy wrote: > On 2015/01/16 21:49:54, mmenke wrote: > > Also, do we need async behavior here? Seems like we have a lot more checks > than > > needed here. If we could just have: > > > > if (!has_network_connectivity_ || is_fetch_scheduled_) > > return; > > > > if (exponential_backoff_->ShouldRejectRequest()) > > // Post task > > else > > // run task, update state > > > > Could get rid of OnBackoffDelayExpiredCallback, remove some logic from > > AffiliationFetchThrottler::OnConnectionTypeChanged, and remove an if from > > SignalNetworkRequestNeeded. > > While we could certainly do that, do you think putting all that logic into a > single method would be more readable? > > I usually get suspicious when I cannot come up with a name for a method, and I > cannot seem to. ScheduleCallbackIfNeeded? (Could even have the state_ == FETCH_NEEDED in there... Or could move some of the checks, and keep the current always async behavior). Anyhow, up to you. I grow suspicious if I see very similar logic repeated in a file a couple times, though, admittedly, it isn't too bad. https://codereview.chromium.org/807503002/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler.cc (right): https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler.cc:123: weak_ptr_factory_.GetWeakPtr()), Don't think we have any test that fails if this used base::Unretained(this). Should we? I'm paranoid. https://codereview.chromium.org/807503002/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler_delegate.h (right): https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_delegate.h:20: // called once request is complete. And if not? https://codereview.chromium.org/807503002/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc (right): https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:30: MOCK_METHOD0(OnCanSendNetworkRequest, bool()); It's generally recommended you don't use GMOCK, to limit the amount of code people need to be familiar with to start hacking on Chrome. Doesn't seem to save us all that much here. Suggest just keeping a count of times called, and asserting on that. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:30: MOCK_METHOD0(OnCanSendNetworkRequest, bool()); In no tests does OnCanSendNetworkRequest return false. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:58: : network_change_notifier_(new MockNetworkChangeNotifier), Any reason this needs to be a scoped_ptr? https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:64: &mock_delegate_, task_runner_, task_runner_->GetMockTickClock())); Can this be done in the constructor, preferable in the initializer list? https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:85: size_t GetPendingTaskCount() { return task_runner_->GetPendingTaskCount(); } const https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:106: AffiliationFetchThrottler& policy() { return *policy_.get(); } Seems like this should be returning an AffiliationFetchThrottler* - generally doesn't seem a great idea to use non-const references. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:116: scoped_ptr<AffiliationFetchThrottler> policy_; Calling an "AffiliationFetchThrottler" policy_ is very confusing. Should call it throttler_, at least. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:127: // Signal while request is in flight should be ignored. We should make sure that it's ignored here. Is the mock class doing so? I guess the "StrictMock" makes sure of that. Regardless, see earlier comment about avoiding gmock. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:150: ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(5, 10)); Relying on exact values for doubles just seems like a really bad idea. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:154: // fuzzing factor, applied twice. This isn't true. Jitter factor is applied independently for each retry. Jitty for a request is: initial_backoff * multiply_factor^(effective_failure_count - 1) * Uniform(1 - jitter_factor, 1] https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:165: policy().SignalNetworkRequestNeeded(); FastForwardUntilNoTasksRemain();? (And same goes for right after all sets of events that should not trigger a request, just before even event that actually will) https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:167: ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(5, 10)); These numbers should not be hardcoded. Should use constants from the AffiliationFetchThrottler (Both for the max time, and for the jitter). Could even just declare the backoff params as private a member of the class, and make the test fixture a friend. (Or could make the params a public member). https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:182: ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(5, 10)); You may want to add a way to EXPECT on the actual delay used in addition or instead of these checks. Could either create a subclass of the TaskRunner that records the times, or a subclass of AffiliationFetchThrottlerTest that has access to them (And have the subclass call into the TaskRunner or not). Anyhow, up to you, I just think if you output the actual delays on failure, it makes life much easier in the case of regression (Or, say, flaky tests...). https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:220: // of tasks. I don't think this is needed... The NCN itself already has protection against this - delaying and de-duping network change messages, with platform-dependent delays varying from 500 ms to 4 seconds. Delaying when a connection is re-established may be worth doing anyways, to prevent a flurry of activity when online, but I'm not sure the 5 second delay gets us anything the ~1 second delay elsewhere doesn't get us. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:229: } Do we have a test where connectivity is lost and restored during a request (Particularly one that ends up failing)?
I have addressed most comments, there are 2 open questions in the tests. PTAL. https://codereview.chromium.org/807503002/diff/60001/components/password_mana... File components/password_manager/core/browser/affiliation_fetch_throttler.cc (right): https://codereview.chromium.org/807503002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_fetch_throttler.cc:120: exponential_backoff_->GetTimeUntilRelease()); On 2015/01/21 17:18:52, mmenke wrote: > On 2015/01/19 18:13:11, engedy wrote: > > On 2015/01/16 21:49:54, mmenke wrote: > > > Also, do we need async behavior here? Seems like we have a lot more checks > > than > > > needed here. If we could just have: > > > > > > if (!has_network_connectivity_ || is_fetch_scheduled_) > > > return; > > > > > > if (exponential_backoff_->ShouldRejectRequest()) > > > // Post task > > > else > > > // run task, update state > > > > > > Could get rid of OnBackoffDelayExpiredCallback, remove some logic from > > > AffiliationFetchThrottler::OnConnectionTypeChanged, and remove an if from > > > SignalNetworkRequestNeeded. > > > > While we could certainly do that, do you think putting all that logic into a > > single method would be more readable? > > > > I usually get suspicious when I cannot come up with a name for a method, and I > > cannot seem to. > > ScheduleCallbackIfNeeded? (Could even have the state_ == FETCH_NEEDED in > there... Or could move some of the checks, and keep the current always async > behavior). Anyhow, up to you. > > I grow suspicious if I see very similar logic repeated in a file a couple times, > though, admittedly, it isn't too bad. I would like to keep always async behavior. On that precondition I do not see any substantial simplification opportunities (the ones I see would hinder readability). https://codereview.chromium.org/807503002/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler.cc (right): https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler.cc:123: weak_ptr_factory_.GetWeakPtr()), On 2015/01/21 17:18:52, mmenke wrote: > Don't think we have any test that fails if this used base::Unretained(this). > Should we? I'm paranoid. Done. https://codereview.chromium.org/807503002/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler_delegate.h (right): https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_delegate.h:20: // called once request is complete. On 2015/01/21 17:18:52, mmenke wrote: > And if not? Done. https://codereview.chromium.org/807503002/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc (right): https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:30: MOCK_METHOD0(OnCanSendNetworkRequest, bool()); On 2015/01/21 17:18:53, mmenke wrote: > In no tests does OnCanSendNetworkRequest return false. Done. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:30: MOCK_METHOD0(OnCanSendNetworkRequest, bool()); On 2015/01/21 17:18:52, mmenke wrote: > It's generally recommended you don't use GMOCK, to limit the amount of code > people need to be familiar with to start hacking on Chrome. Doesn't seem to > save us all that much here. Suggest just keeping a count of times called, and > asserting on that. While the password_manager component uses GMock extensively, you are correct that there is no need for it in this case, so I have stopped using it. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:58: : network_change_notifier_(new MockNetworkChangeNotifier), On 2015/01/21 17:18:53, mmenke wrote: > Any reason this needs to be a scoped_ptr? Nope. Done. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:64: &mock_delegate_, task_runner_, task_runner_->GetMockTickClock())); On 2015/01/21 17:18:52, mmenke wrote: > Can this be done in the constructor, preferable in the initializer list? N/A. The instance under test is created in the test methods now. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:85: size_t GetPendingTaskCount() { return task_runner_->GetPendingTaskCount(); } On 2015/01/21 17:18:52, mmenke wrote: > const Done. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:106: AffiliationFetchThrottler& policy() { return *policy_.get(); } On 2015/01/21 17:18:53, mmenke wrote: > Seems like this should be returning an AffiliationFetchThrottler* - generally > doesn't seem a great idea to use non-const references. Done. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:116: scoped_ptr<AffiliationFetchThrottler> policy_; On 2015/01/21 17:18:53, mmenke wrote: > Calling an "AffiliationFetchThrottler" policy_ is very confusing. Should call > it throttler_, at least. Done. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:127: // Signal while request is in flight should be ignored. On 2015/01/21 17:18:53, mmenke wrote: > We should make sure that it's ignored here. Is the mock class doing so? I > guess the "StrictMock" makes sure of that. Regardless, see earlier comment > about avoiding gmock. Yes, the strict mock ensured this, but I have made this more explicit. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:150: ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(5, 10)); On 2015/01/21 17:18:53, mmenke wrote: > Relying on exact values for doubles just seems like a really bad idea. Not that we are not relying on *exact* value: this gets converted to a base::TimeDelta, which is integer-based with a microsecond granularity. Now that I have added rounding, as long as the floating point error is less than 0.0005 milliseconds, we are fine. In most cases, this is fulfilled, by a large margin. The only exception is the AffiliationFetchThrottlerTest.FailedRequests test where the |maximum_delay_ms| is so large that its precision is pretty close to this margin. I will investigate that. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:154: // fuzzing factor, applied twice. On 2015/01/21 17:18:52, mmenke wrote: > This isn't true. Jitter factor is applied independently for each retry. Jitty > for a request is: > > initial_backoff * multiply_factor^(effective_failure_count - 1) * Uniform(1 - > jitter_factor, 1] Right. Done. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:165: policy().SignalNetworkRequestNeeded(); On 2015/01/21 17:18:52, mmenke wrote: > FastForwardUntilNoTasksRemain();? (And same goes for right after all sets of > events that should not trigger a request, just before even event that actually > will) I have added it here, and verified elsewhere, please let me know if I have missed a place. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:167: ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(5, 10)); On 2015/01/21 17:18:53, mmenke wrote: > These numbers should not be hardcoded. Should use constants from the > AffiliationFetchThrottler (Both for the max time, and for the jitter). Could > even just declare the backoff params as private a member of the class, and make > the test fixture a friend. (Or could make the params a public member). Done. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:182: ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(5, 10)); On 2015/01/21 17:18:53, mmenke wrote: > You may want to add a way to EXPECT on the actual delay used in addition or > instead of these checks. Could either create a subclass of the TaskRunner that > records the times, or a subclass of AffiliationFetchThrottlerTest that has > access to them (And have the subclass call into the TaskRunner or not). Anyhow, > up to you, I just think if you output the actual delays on failure, it makes > life much easier in the case of regression (Or, say, flaky tests...). Done. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:220: // of tasks. On 2015/01/21 17:18:52, mmenke wrote: > I don't think this is needed... The NCN itself already has protection against > this - delaying and de-duping network change messages, with platform-dependent > delays varying from 500 ms to 4 seconds. Delaying when a connection is > re-established may be worth doing anyways, to prevent a flurry of activity when > online, but I'm not sure the 5 second delay gets us anything the ~1 second delay > elsewhere doesn't get us. This mostly test what you have pointed out earlier: that we do not flood the task runner with lots of future tasks even if the connection state changes often. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:229: } On 2015/01/21 17:18:53, mmenke wrote: > Do we have a test where connectivity is lost and restored during a request > (Particularly one that ends up failing)? I have added two, though I wasn't sure if the URLFetcher would fail immediately after connection is deemed to have been lost, or later, or only when connection is restored, or at an unrelated time? I guess the tests should not rely on this anyway, so we might want to add a 3rd test here.
Quick responses. I'll do a full pass (hopefully the last, modulo agreement on the AssertReleaseBetweenSecs stuff) https://codereview.chromium.org/807503002/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc (right): https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:150: ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(5, 10)); On 2015/01/23 22:09:44, engedy wrote: > On 2015/01/21 17:18:53, mmenke wrote: > > Relying on exact values for doubles just seems like a really bad idea. > > Not that we are not relying on *exact* value: this gets converted to a > base::TimeDelta, which is integer-based with a microsecond granularity. Now that > I have added rounding, as long as the floating point error is less than 0.0005 > milliseconds, we are fine. In most cases, this is fulfilled, by a large margin. > > The only exception is the AffiliationFetchThrottlerTest.FailedRequests test > where the |maximum_delay_ms| is so large that its precision is pretty close to > this margin. I will investigate that. If we get a 5 or a 10 in the random calculation (Assuming we can get a 5 or 10), you're relying on a range including an exact match [5, 10] vs (5, 10). If we can't get a 5 or a 10 in the random calculation, you're depending on that, which also doesn't seem like a good idea. That makes me very nervous. https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:229: } On 2015/01/23 22:09:44, engedy wrote: > On 2015/01/21 17:18:53, mmenke wrote: > > Do we have a test where connectivity is lost and restored during a request > > (Particularly one that ends up failing)? > > I have added two, though I wasn't sure if the URLFetcher would fail immediately > after connection is deemed to have been lost, or later, or only when connection > is restored, or at an unrelated time? I guess the tests should not rely on this > anyway, so we might want to add a 3rd test here. You shouldn't depend on what it does - it depends on what it's doing, whether we're using SPDY or HTTP, whether we're in proxy resolution phase or DNS resolution, reading from the cache (Guess we're probably not doing that here), etc.
https://codereview.chromium.org/807503002/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc (right): https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:150: ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(5, 10)); > you're relying on a range including an exact match [5, 10] vs (5, 10). Not sure I understand what you mean by this. Except for that one test I mentioned, the current solution is correct in that it accepts the boundary values. The production code being non-deterministic, and so having to resort to checking ranges in the tests makes me a whole lot more nervous.
On 2015/01/24 00:35:39, engedy wrote: > https://codereview.chromium.org/807503002/diff/100001/components/password_man... > File > components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc > (right): > > https://codereview.chromium.org/807503002/diff/100001/components/password_man... > components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:150: > ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(5, 10)); > > you're relying on a range including an exact match [5, 10] vs (5, 10). > > Not sure I understand what you mean by this. Except for that one test I > mentioned, the current solution is correct in that it accepts the boundary > values. > > The production code being non-deterministic, and so having to resort to checking > ranges in the tests makes me a whole lot more nervous. You're assuming that when BackoffEntry calculates 5 seconds as a result of a mathematical operation, it doesn't result in 4.999999999999999.
https://codereview.chromium.org/807503002/diff/120001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc (right): https://codereview.chromium.org/807503002/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:57: class MockNetworkChangeNotifier : public net::NetworkChangeNotifier { This class isn't needed. See NetworkChangeNotifier::CreateMock. If you need it to track connection types, which the current mock does not do, I think it's better to have the mock class in net/ track the changes, rather than having everyone who wants to do this kind of test creating their own solution. https://codereview.chromium.org/807503002/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:118: EXPECT_GE(RoundToNearestMicrosecond(max_delay_ms), delay); Per comment, it may theoretically be possible for delay to be less than min_delay_ms, due to floating point error. While if you go through all the math, it *may* be possible that the behavior of the pow method, and how floating point numbers are represented, and your choice of values, this never occurs, the test should not depend on all that. https://codereview.chromium.org/807503002/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:181: const auto& kP = AffiliationFetchThrottler::kBackoffPolicy; Per Google style guide, uncommon abbreviations should not be used in variable names. I suggest just kPolicy.
Please take another look. https://codereview.chromium.org/807503002/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc (right): https://codereview.chromium.org/807503002/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:229: } On 2015/01/23 22:39:53, mmenke wrote: > On 2015/01/23 22:09:44, engedy wrote: > > On 2015/01/21 17:18:53, mmenke wrote: > > > Do we have a test where connectivity is lost and restored during a request > > > (Particularly one that ends up failing)? > > > > I have added two, though I wasn't sure if the URLFetcher would fail > immediately > > after connection is deemed to have been lost, or later, or only when > connection > > is restored, or at an unrelated time? I guess the tests should not rely on > this > > anyway, so we might want to add a 3rd test here. > > You shouldn't depend on what it does - it depends on what it's doing, whether > we're using SPDY or HTTP, whether we're in proxy resolution phase or DNS > resolution, reading from the cache (Guess we're probably not doing that here), > etc. I have added an additional test. https://codereview.chromium.org/807503002/diff/120001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc (right): https://codereview.chromium.org/807503002/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:57: class MockNetworkChangeNotifier : public net::NetworkChangeNotifier { On 2015/01/26 18:49:13, mmenke wrote: > This class isn't needed. See NetworkChangeNotifier::CreateMock. If you need it > to track connection types, which the current mock does not do, I think it's > better to have the mock class in net/ track the changes, rather than having > everyone who wants to do this kind of test creating their own solution. Done. https://codereview.chromium.org/807503002/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:118: EXPECT_GE(RoundToNearestMicrosecond(max_delay_ms), delay); On 2015/01/26 18:49:13, mmenke wrote: > Per comment, it may theoretically be possible for delay to be less than > min_delay_ms, due to floating point error. While if you go through all the > math, it *may* be possible that the behavior of the pow method, and how floating > point numbers are represented, and your choice of values, this never occurs, the > test should not depend on all that. The reasons that it should never occur is because the result of the floating point calculation is rounder (not truncated) to the nearest integer microsecond. Precision of this calculations should always be sufficient to make sure this is the correct microsecond. But you are right that the test should not expect this. I have changed to tests so that they allow +-1 millisecond slack. https://codereview.chromium.org/807503002/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:181: const auto& kP = AffiliationFetchThrottler::kBackoffPolicy; On 2015/01/26 18:49:13, mmenke wrote: > Per Google style guide, uncommon abbreviations should not be used in variable > names. I suggest just kPolicy. Done.
Friendly ping.
On 2015/01/28 23:40:33, engedy wrote: > Friendly ping. Sorry, sick Monday, took Tuesday off because of the blizzard, and ended up taking most of the week off. I'll get back to you tomorrow.
LGTM (Sorry again about slowness - really took an unreasonable amount of time on this one) https://codereview.chromium.org/807503002/diff/120001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc (right): https://codereview.chromium.org/807503002/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:118: EXPECT_GE(RoundToNearestMicrosecond(max_delay_ms), delay); On 2015/01/26 19:39:34, engedy wrote: > On 2015/01/26 18:49:13, mmenke wrote: > > Per comment, it may theoretically be possible for delay to be less than > > min_delay_ms, due to floating point error. While if you go through all the > > math, it *may* be possible that the behavior of the pow method, and how > floating > > point numbers are represented, and your choice of values, this never occurs, > the > > test should not depend on all that. > > The reasons that it should never occur is because the result of the floating > point calculation is rounder (not truncated) to the nearest integer microsecond. > Precision of this calculations should always be sufficient to make sure this is > the correct microsecond. > > But you are right that the test should not expect this. I have changed to tests > so that they allow +-1 millisecond slack. Hrm...I thought I checked and it was truncated...May have missed something, though. I'm happy with the +/- 1's, though. https://codereview.chromium.org/807503002/diff/140001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler.h (right): https://codereview.chromium.org/807503002/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler.h:53: // post to |task_runner| to call the |delegate|'s OnSendNetworkRequest(). Should mention the delegate must outlive the throttler. https://codereview.chromium.org/807503002/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler.h:62: // sent. Think it's worth noting that OnCanSendNetworkRequest will always be called asynchronously. https://codereview.chromium.org/807503002/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler.h:121: base::WeakPtrFactory<AffiliationFetchThrottler> weak_ptr_factory_; Suggest a blank line before the factory, as it should always go last. https://codereview.chromium.org/807503002/diff/140001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc (right): https://codereview.chromium.org/807503002/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:30: MockAffiliationFetchThrottlerDelegate(scoped_ptr<base::TickClock> tick_clock) nit: explicit https://codereview.chromium.org/807503002/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:37: void reset_release_count() { release_count_ = 0u; } optional: Think this variable name is a little unclear. Suggest either a comment or replacing release with can_send or just send in the two variable names that use it.
engedy@chromium.org changed reviewers: + mkwst@chromium.org, phajdan.jr@chromium.org
@Matt, many thanks for the very thorough review! @Mike, could you please review c/pm/* for OWNER's approval? @Paweł, could you please review the tiny change to base/test? If you prefer, I am happy to separate it into another CL. https://codereview.chromium.org/807503002/diff/140001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler.h (right): https://codereview.chromium.org/807503002/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler.h:53: // post to |task_runner| to call the |delegate|'s OnSendNetworkRequest(). On 2015/01/30 19:50:41, mmenke wrote: > Should mention the delegate must outlive the throttler. Done. https://codereview.chromium.org/807503002/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler.h:62: // sent. On 2015/01/30 19:50:41, mmenke wrote: > Think it's worth noting that OnCanSendNetworkRequest will always be called > asynchronously. Done. https://codereview.chromium.org/807503002/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler.h:121: base::WeakPtrFactory<AffiliationFetchThrottler> weak_ptr_factory_; On 2015/01/30 19:50:41, mmenke wrote: > Suggest a blank line before the factory, as it should always go last. Done. https://codereview.chromium.org/807503002/diff/140001/components/password_man... File components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc (right): https://codereview.chromium.org/807503002/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:30: MockAffiliationFetchThrottlerDelegate(scoped_ptr<base::TickClock> tick_clock) On 2015/01/30 19:50:41, mmenke wrote: > nit: explicit Done. https://codereview.chromium.org/807503002/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc:37: void reset_release_count() { release_count_ = 0u; } On 2015/01/30 19:50:41, mmenke wrote: > optional: Think this variable name is a little unclear. Suggest either a > comment or replacing release with can_send or just send in the two variable > names that use it. Done.
base/test LGTM
components/password_manager LGTM, thanks!
The CQ bit was checked by engedy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/807503002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/89726c2516dfc9ca5dc4cc23acff4841608ad97c Cr-Commit-Position: refs/heads/master@{#314127} |
