Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2526973002:
Added retry policy (Closed)
|
Created:
4 years ago by kumarniranjan Modified:
3 years, 10 months ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded retry policy
When hands-off zero-touch enrollment is invoked,
various problems may be encountered.
Examples include the TPM chip not yet being ready for enrollment,
and network outages occurring before enrollment is finished.
BUG=667613
TEST=none
Review-Url: https://codereview.chromium.org/2526973002
Cr-Commit-Position: refs/heads/master@{#448860}
Committed: https://chromium.googlesource.com/chromium/src/+/6349e12992c74e7cb830b76d9357716eba808796
Patch Set 1 #
Total comments: 18
Patch Set 2 : Added retry policy #
Total comments: 23
Patch Set 3 : Wrote Unit Test #
Total comments: 2
Patch Set 4 : Removed unnecessary include #
Total comments: 4
Patch Set 5 : Addressed comments #
Total comments: 2
Patch Set 6 : Addressed more comments #
Total comments: 31
Patch Set 7 : Addressed further comments #
Total comments: 28
Patch Set 8 : Responded to more comments #Patch Set 9 : Further revisions #
Total comments: 4
Patch Set 10 : Additional changes #
Total comments: 2
Patch Set 11 : GN build modification #
Total comments: 1
Patch Set 12 : Fixed GN problem #
Messages
Total messages: 67 (21 generated)
Description was changed from ========== Added retry policy When hands-off zero-touch enrollment is invoked, various problems may be encountered. Examples include the TPM chip not yet being ready for enrollment, and network outages occurring before enrollment is finished. BUG=crbug:667613 TEST=none ========== to ========== Added retry policy When hands-off zero-touch enrollment is invoked, various problems may be encountered. Examples include the TPM chip not yet being ready for enrollment, and network outages occurring before enrollment is finished. BUG=crbug:667613 TEST=none ==========
kumarniranjan@google.com changed reviewers: + drcrash@chromium.org, joth@chromium.org, xiyuan@chromium.org
kumarniranjan@google.com changed reviewers: + alemate@chromium.org
https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:67: : BaseScreen(base_screen_delegate), actor_(actor), weak_ptr_factory_(this) { as a general guide use initializer list when you can. so here: , retry_backoff_(&retry_policy) { policy_.num_errors_to_ignore = 0; ... // etc } https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:71: policy_->multiply_factor = 2.0; given we're using this to paper over the TPM init, I'm inclined to have it not multiply quite so quickly. suggest: factor = 1.5 https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:73: policy_->maximum_backoff_ms = -1; Lets cap the maximum delay to 8 minutes https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:208: Show(); is it safe to call this multiple times? (just wondering if we can have multiple errors or user clicking "retry" resulting in multiple calls to this) Maybe you can make a unit test to cover this scenario https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:270: base::Bind(&EnrollmentScreen::OnRetry, I don't have a strong preference, but it maybe clearer to make a new "DoRetry" method that we call from here and from "OnRetry" as the latter is (AIUI?) intended as a UI button click handler so it's misleading to call it from the timer It'd be also nice to log when the retry is happening and why (user button press vs timer) https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:277: EnterpriseEnrollmentHelper::OtherError error) { Wonder if we should retry here too? https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/enrollment... The 2 errors look pretty fatal (code bug and device attempting to enroll to wrong domain) and "should" be impossible in rialto. otoh it'd probably not be harmful to retry anyway. (Maybe the domain mismatched can be fixed in the CPanel and the retry would eventually succeed) https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right): https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.h:162: base::OneShotTimer timer_; This object can only be used for one thing at a time, so lets call it retry_timer_ https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.h:165: net::BackoffEntry::Policy* policy_; suggest grouping the timer with these, and prefix all the names -- retry_backoff_ retry_policy_ retry_timer_ https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.h:165: net::BackoffEntry::Policy* policy_; Don't use naked pointers here, either use unique_ptr to ensure deletion, or even simpler just put the objects directly in as members of the class: net::BackoffEntry::Policy retry_policy_; net::BackoffEntry retry_backoff_; (policy then needs to be before the backoff entry, to ensure it is constructed before it)
https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right): https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.h:165: net::BackoffEntry::Policy* policy_; I agree re: using objects here. Also it seems that policy_ is only used in the constructor to build backoff_. If so, are you able to actually build the policy there and transfer ownership to backoff?
On 2016/11/23 23:00:16, The one and only Dr. Crash wrote: > https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... > File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right): > > https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... > chrome/browser/chromeos/login/enrollment/enrollment_screen.h:165: > net::BackoffEntry::Policy* policy_; > I agree re: using objects here. > > Also it seems that policy_ is only used in the constructor to build backoff_. If > so, are you able to actually build the policy there and transfer ownership to > backoff? It seems this would require changing the back off class, which might be good but too much for your needs. In any case, could we make the variable names a bit less generic, maybe something like retry_backoff_ and retry_policy_?
On 2016/11/23 23:39:45, The one and only Dr. Crash wrote: > On 2016/11/23 23:00:16, The one and only Dr. Crash wrote: > > > https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... > > File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right): > > > > > https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... > > chrome/browser/chromeos/login/enrollment/enrollment_screen.h:165: > > net::BackoffEntry::Policy* policy_; > > I agree re: using objects here. > > > > Also it seems that policy_ is only used in the constructor to build backoff_. > If > > so, are you able to actually build the policy there and transfer ownership to > > backoff? > > It seems this would require changing the back off class, which might be good but > too much for your needs. > > In any case, could we make the variable names a bit less generic, maybe > something like retry_backoff_ and retry_policy_? I changed the variable names as you suggested. I'm okay either way with what we decide to do with retry_policy_, whether we keep it, or we create a new constructor for BackoffRetry where we allocate a BackoffRetry::Policy object on the heap and then call the existing BackoffRetry constructor, giving it a pointer to the Policy object. I will need to remember to free this Policy object in the BackoffRetry destructor. However, the destructor will not know which constructor was used initially, and it only needs to free the Policy object if my new constructor was used. So I will have to add an additional bool field to BackoffRetry to keep track of this. I'm okay whether we do this or not. Please let me know what you think.
On 2016/11/23 23:39:45, The one and only Dr. Crash wrote: > On 2016/11/23 23:00:16, The one and only Dr. Crash wrote: > > > https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... > > File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right): > > > > > https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... > > chrome/browser/chromeos/login/enrollment/enrollment_screen.h:165: > > net::BackoffEntry::Policy* policy_; > > I agree re: using objects here. > > > > Also it seems that policy_ is only used in the constructor to build backoff_. > If > > so, are you able to actually build the policy there and transfer ownership to > > backoff? > > It seems this would require changing the back off class, which might be good but > too much for your needs. > > In any case, could we make the variable names a bit less generic, maybe > something like retry_backoff_ and retry_policy_? I changed the variable names as you suggested. I'm okay either way with what we decide to do with retry_policy_, whether we keep it, or we create a new constructor for BackoffRetry where we allocate a BackoffRetry::Policy object on the heap and then call the existing BackoffRetry constructor, giving it a pointer to the Policy object. I will need to remember to free this Policy object in the BackoffRetry destructor. However, the destructor will not know which constructor was used initially, and it only needs to free the Policy object if my new constructor was used. So I will have to add an additional bool field to BackoffRetry to keep track of this. I'm okay whether we do this or not. Please let me know what you think.
https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:67: : BaseScreen(base_screen_delegate), actor_(actor), weak_ptr_factory_(this) { On 2016/11/23 22:24:02, joth wrote: > as a general guide use initializer list when you can. so here: > > , retry_backoff_(&retry_policy) { > policy_.num_errors_to_ignore = 0; > ... // etc > } Done. https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:71: policy_->multiply_factor = 2.0; On 2016/11/23 22:24:02, joth wrote: > given we're using this to paper over the TPM init, I'm inclined to have it not > multiply quite so quickly. suggest: > factor = 1.5 Done. https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:73: policy_->maximum_backoff_ms = -1; On 2016/11/23 22:24:03, joth wrote: > Lets cap the maximum delay to 8 minutes Done. https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:208: Show(); On 2016/11/23 22:24:02, joth wrote: > is it safe to call this multiple times? (just wondering if we can have multiple > errors or user clicking "retry" resulting in multiple calls to this) > Maybe you can make a unit test to cover this scenario The current UI without my changes allows for this to be called multiple times. I tried doing it manually, and could not observe any ill effects. However, I agree that a test is needed for this. https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:270: base::Bind(&EnrollmentScreen::OnRetry, On 2016/11/23 22:24:02, joth wrote: > I don't have a strong preference, but it maybe clearer to make a new "DoRetry" > method that we call from here and from "OnRetry" as the latter is (AIUI?) > intended as a UI button click handler so it's misleading to call it from the > timer > > It'd be also nice to log when the retry is happening and why (user button press > vs timer) I agree. I ended up calling the method "ProcessRetry" instead of "DoRetry" https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:277: EnterpriseEnrollmentHelper::OtherError error) { On 2016/11/23 22:24:02, joth wrote: > Wonder if we should retry here too? > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/enrollment... > > The 2 errors look pretty fatal (code bug and device attempting to enroll to > wrong domain) and "should" be impossible in rialto. otoh it'd probably not be > harmful to retry anyway. (Maybe the domain mismatched can be fixed in the CPanel > and the retry would eventually succeed) Considering we are in hands-off mode, it can't hurt to retry here, especially since the user can't do anything. I will go ahead and add that logic here. https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right): https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.h:162: base::OneShotTimer timer_; On 2016/11/23 22:24:03, joth wrote: > This object can only be used for one thing at a time, so lets call it > retry_timer_ Done. https://codereview.chromium.org/2526973002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/enrollment/enrollment_screen.h:165: net::BackoffEntry::Policy* policy_; On 2016/11/23 22:24:03, joth wrote: > Don't use naked pointers here, either use unique_ptr to ensure deletion, or even > simpler just put the objects directly in as members of the class: > net::BackoffEntry::Policy retry_policy_; > net::BackoffEntry retry_backoff_; > > (policy then needs to be before the backoff entry, to ensure it is constructed > before it) I decided to put the objects directly in as members of the class. I don't believe there is a way of building the policy in the constructor and transferring ownership to the BackoffEntry object without creating a new constructor for BackoffEntry
Could you add some unit tests please?
On 2016/11/28 19:38:30, The one and only Dr. Crash wrote: > Could you add some unit tests please? Of course I will. I just wanted to get everyone's feedback on the design. Then I will write the tests and re-upload the CL. I tried to do it this way because there have been times where I do everything (code and tests) and then the design gets completely changed, which required me to change the tests as well.
Overall looks good. https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:69: retry_backoff_(&retry_policy_), Can we use a unique_ptr and create |retry_backoff_| after policy is filled up, similar to [1] ? [1] https://cs.chromium.org/chromium/src/chromeos/network/portal_detector/network... https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:70: num_retries_(0), nit: move the initializer to header https://chromium-cpp.appspot.com/, check out "Non-Static Class Member Initializers" row. https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:213: num_retries_++; nit: ++num_retries_; https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:216: ProcessRetry(); retry_timer_.Stop() in case the timer is running? https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:219: void EnrollmentScreen::AutomaticRetry() { nit: AutomaticRetry -> StartRetryTimer https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:224: num_retries_++; nit: ditto https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:225: LOG(WARNING) << "Enrollment retry " << num_retries_ << " initiated by timer."; What is the purpose of |num_retries_| other than output in the log? Should we have an upper limit of the retry? https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:288: policy::ZeroTouchEnrollmentMode::HANDS_OFF) nit: wrap the condition into a helper function (e.g. ShouldAutoRetryOnError) since it is used in two places. https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:289: AutomaticRetry(); nit: wrap with {} since condition takes more than 1 line https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:299: AutomaticRetry(); nit: wrap with {}
https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:213: num_retries_++; On 2016/11/28 22:16:34, xiyuan wrote: > nit: ++num_retries_; Yes! (Sorry, I am a big fan of pre-increment/decrement). https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:289: AutomaticRetry(); On 2016/11/28 22:16:34, xiyuan wrote: > nit: wrap with {} since condition takes more than 1 line Probably unnecessary once a helper method is available. https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:299: AutomaticRetry(); On 2016/11/28 22:16:33, xiyuan wrote: > nit: wrap with {} Probably unnecessary once a helper method is available.
https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:78: retry_policy_.maximum_backoff_ms = 480000; You set a maximum backoff but you never call ShouldRejectRequest() so what effect does that backoff time have? Is that the maximum interval between two attempts at the very worst? Also maybe interesting to see how different your policy is from, say the one for chromoting: https://cs.chromium.org/chromium/src/remoting/host/chromoting_host.cc?rcl=148... How did you come up w/ the different numbers?
On 2016/11/30 23:30:08, The one and only Dr. Crash wrote: > https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): > > https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:78: > retry_policy_.maximum_backoff_ms = 480000; > You set a maximum backoff but you never call ShouldRejectRequest() so what > effect does that backoff time have? Is that the maximum interval between two > attempts at the very worst? Also maybe interesting to see how different your > policy is from, say the one for chromoting: > https://cs.chromium.org/chromium/src/remoting/host/chromoting_host.cc?rcl=148... > > > How did you come up w/ the different numbers? Nevermind my first question (I got mixed up) but I am curious about the numbers choices.
As Jonathan suggested, I am planning to use a mock clock or timer to test this. Please let me know if you have any suggestions regarding testing. https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:69: retry_backoff_(&retry_policy_), On 2016/11/28 22:16:34, xiyuan wrote: > Can we use a unique_ptr and create |retry_backoff_| after policy is filled up, > similar to [1] ? > > [1] > https://cs.chromium.org/chromium/src/chromeos/network/portal_detector/network... Done. https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:70: num_retries_(0), On 2016/11/28 22:16:34, xiyuan wrote: > nit: move the initializer to header > > https://chromium-cpp.appspot.com/, check out "Non-Static Class Member > Initializers" row. Done. https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:78: retry_policy_.maximum_backoff_ms = 480000; On 2016/11/30 23:30:08, The one and only Dr. Crash wrote: > You set a maximum backoff but you never call ShouldRejectRequest() so what > effect does that backoff time have? Is that the maximum interval between two > attempts at the very worst? Also maybe interesting to see how different your > policy is from, say the one for chromoting: > https://cs.chromium.org/chromium/src/remoting/host/chromoting_host.cc?rcl=148... > > > How did you come up w/ the different numbers? Jonathan wanted the maximum interval between retry attempts to be 8 min (480000 ms). He also wanted the multiplication factor to be 1.5. I came up with the 4 s initial delay. It works well when I try it on my device. If you think I should change these numbers, please let me know. https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:213: num_retries_++; On 2016/11/30 23:09:56, The one and only Dr. Crash wrote: > On 2016/11/28 22:16:34, xiyuan wrote: > > nit: ++num_retries_; > > Yes! (Sorry, I am a big fan of pre-increment/decrement). Done. https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:216: ProcessRetry(); On 2016/11/28 22:16:34, xiyuan wrote: > retry_timer_.Stop() in case the timer is running? Good catch! We don't want an automatic retry to happen while we are processing the user's retry request. https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:224: num_retries_++; On 2016/11/28 22:16:34, xiyuan wrote: > nit: ditto Done. https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:225: LOG(WARNING) << "Enrollment retry " << num_retries_ << " initiated by timer."; On 2016/11/28 22:16:34, xiyuan wrote: > What is the purpose of |num_retries_| other than output in the log? Should we > have an upper limit of the retry? It gives us the capability to set an upper limit on the number of retries in the future. This number will probably be quite large. Since there is no UI and the user's only recourse in the event of the problem is to call tech support, it makes sense to retry every 8 minutes for multiple days. The num_retries_ variable is also very useful for testing.
joth@google.com changed reviewers: + joth@google.com
https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:78: retry_policy_.maximum_backoff_ms = 480000; On 2016/12/05 22:04:52, kumarniranjan wrote: > On 2016/11/30 23:30:08, The one and only Dr. Crash wrote: > > You set a maximum backoff but you never call ShouldRejectRequest() so what > > effect does that backoff time have? Is that the maximum interval between two > > attempts at the very worst? Also maybe interesting to see how different your > > policy is from, say the one for chromoting: > > > https://cs.chromium.org/chromium/src/remoting/host/chromoting_host.cc?rcl=148... > > > > > > How did you come up w/ the different numbers? > > Jonathan wanted the maximum interval between retry attempts to be 8 min (480000 > ms). He also wanted the multiplication factor to be 1.5. I came up with the 4 s > initial delay. It works well when I try it on my device. If you think I should > change these numbers, please let me know. I gave a bit of reasoning in https://codereview.chromium.org/2526973002/#msg4 but basically: - x2 multiple is reasonable for network requests but for local operatoin I like the retry not to back quiet so quick as we expect it to normally be done within the initial delay, and if it wasn't then waiting a further 2x initial delay is a high penalty. - 8mins max retry interval is kind of arbitrary, but if the failure is due to network reception (likely case) then it's actually cheap to keep the retry bound relatively low (effectively each retry is a fail fast no-op) and connecting to a network (when it comes in range) may take ~5mins so a retry rate in this vicinity feels about right. If we let it back out to taking hours (or days) between retries, they become worthless.
A mock clock will work. I made it work here: https://codereview.chromium.org/2529743002/ On Mon, Dec 5, 2016 at 2:59 PM, <joth@google.com> wrote: > > https://codereview.chromium.org/2526973002/diff/20001/ > chrome/browser/chromeos/login/enrollment/enrollment_screen.cc > File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc > (right): > > https://codereview.chromium.org/2526973002/diff/20001/ > chrome/browser/chromeos/login/enrollment/enrollment_screen.cc#newcode78 > chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:78: > retry_policy_.maximum_backoff_ms = 480000; > On 2016/12/05 22:04:52, kumarniranjan wrote: > > On 2016/11/30 23:30:08, The one and only Dr. Crash wrote: > > > You set a maximum backoff but you never call ShouldRejectRequest() > so what > > > effect does that backoff time have? Is that the maximum interval > between two > > > attempts at the very worst? Also maybe interesting to see how > different your > > > policy is from, say the one for chromoting: > > > > > > https://cs.chromium.org/chromium/src/remoting/host/chromoting_host.cc?rcl= > 1480522801&l=38 > > > > > > > > > How did you come up w/ the different numbers? > > > > Jonathan wanted the maximum interval between retry attempts to be 8 > min (480000 > > ms). He also wanted the multiplication factor to be 1.5. I came up > with the 4 s > > initial delay. It works well when I try it on my device. If you think > I should > > change these numbers, please let me know. > > I gave a bit of reasoning in > https://codereview.chromium.org/2526973002/#msg4 but basically: > - x2 multiple is reasonable for network requests but for local operatoin > I like the retry not to back quiet so quick as we expect it to normally > be done within the initial delay, and if it wasn't then waiting a > further 2x initial delay is a high penalty. > > - 8mins max retry interval is kind of arbitrary, but if the failure is > due to network reception (likely case) then it's actually cheap to keep > the retry bound relatively low (effectively each retry is a fail fast > no-op) and connecting to a network (when it comes in range) may take > ~5mins so a retry rate in this vicinity feels about right. > If we let it back out to taking hours (or days) between retries, they > become worthless. > > https://codereview.chromium.org/2526973002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right): https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.h:170: base::OneShotTimer retry_timer_; Instead of using a timer, you can simply post delayed tasks to your message loop. See this CL as an example: https://codereview.chromium.org/2529743002/
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/06 22:24:34, The one and only Dr. Crash wrote: > https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right): > > https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/login/enrollment/enrollment_screen.h:170: > base::OneShotTimer retry_timer_; > Instead of using a timer, you can simply post delayed tasks to your message > loop. See this CL as an example: https://codereview.chromium.org/2529743002/ The issue I see with this is how I would cancel the delayed task when the user presses the retry button. The only way I know to do this is by deleting the weak pointer. But then, I won't be able to post another delayed task if this retry fails. Is there another way? Also, am I thinking about this incorrectly? Second, how would I test it? With timer, I can change the underlying clock to be a mock clock.
Another way would be to have a state that says whether the action of the delayed task still needs to be taken. As far as moving the clock, I do not know. I gave up trying to do it in my own CL and used the wall clock with a very small timeout. These are just possibilities. Pick the one that fits you best. On Mon, Dec 12, 2016, 14:15 <kumarniranjan@google.com> wrote: > On 2016/12/06 22:24:34, The one and only Dr. Crash wrote: > > > > https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... > > File chrome/browser/chromeos/login/enrollment/enrollment_screen.h > (right): > > > > > > https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/login/enrollment/enrollment_screen.h:170: > > base::OneShotTimer retry_timer_; > > Instead of using a timer, you can simply post delayed tasks to your > message > > loop. See this CL as an example: > https://codereview.chromium.org/2529743002/ > > The issue I see with this is how I would cancel the delayed task when the > user > presses the retry button. The only way I know to do this is by deleting > the weak > pointer. But then, I won't be able to post another delayed task if this > retry > fails. Is there another way? Also, am I thinking about this incorrectly? > > Second, how would I test it? With timer, I can change the underlying clock > to be > a mock clock. > > https://codereview.chromium.org/2526973002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/12/13 20:49:13, The one and only Dr. Crash wrote: > Another way would be to have a state that says whether the action of the > delayed task still needs to be taken. As far as moving the clock, I do not > know. I gave up trying to do it in my own CL and used the wall clock with a > very small timeout. > > These are just possibilities. Pick the one that fits you best. > > On Mon, Dec 12, 2016, 14:15 <mailto:kumarniranjan@google.com> wrote: > > > On 2016/12/06 22:24:34, The one and only Dr. Crash wrote: > > > > > > > > https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... > > > File chrome/browser/chromeos/login/enrollment/enrollment_screen.h > > (right): > > > > > > > > > > > https://codereview.chromium.org/2526973002/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/login/enrollment/enrollment_screen.h:170: > > > base::OneShotTimer retry_timer_; > > > Instead of using a timer, you can simply post delayed tasks to your > > message > > > loop. See this CL as an example: > > https://codereview.chromium.org/2529743002/ > > > > The issue I see with this is how I would cancel the delayed task when the > > user > > presses the retry button. The only way I know to do this is by deleting > > the weak > > pointer. But then, I won't be able to post another delayed task if this > > retry > > fails. Is there another way? Also, am I thinking about this incorrectly? > > > > Second, how would I test it? With timer, I can change the underlying clock > > to be > > a mock clock. > > > > https://codereview.chromium.org/2526973002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Based on my brief code search, if you're using MessageLoop::current()->PostDelayedTask(), it looks like unit tests would be best done via ScopedMockTimeMessageLoopTaskRunner (or MockTimeMessageLoopTaskRunner direct). existing unittests like incident_reporting_service_unittest.cc gives some examples.
kumarniranjan@google.com changed reviewers: + skylarc@google.com
Just a few nits, otherwise LGTM https://codereview.chromium.org/2526973002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/2526973002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:74: retry_policy_.maximum_backoff_ms = 480000; Please comment with conversions to minutes for easy readability. Optionally, some/all of these might be better served as constants. https://codereview.chromium.org/2526973002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc (right): https://codereview.chromium.org/2526973002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:96: EXPECT_TRUE(enrollment_screen->num_retries_ >= 4 || `&&` ? https://codereview.chromium.org/2526973002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:132: EXPECT_TRUE(enrollment_screen->num_retries_ >= 4 && Does EXPECT_TRUE() print the original argument with the variable names intact when it fails? Things like EXPECT_LE() and EXPECT_GE(), or even EXPECT_PRED(), can help when debugging tests as they'll tell you what the erroring value was (obviously you've already done the hard work here, just a note).
https://codereview.chromium.org/2526973002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/2526973002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:74: retry_policy_.maximum_backoff_ms = 480000; On 2017/02/01 00:47:47, can Skylar cook wrote: > Please comment with conversions to minutes for easy readability. Optionally, > some/all of these might be better served as constants. Done. https://codereview.chromium.org/2526973002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc (right): https://codereview.chromium.org/2526973002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:96: EXPECT_TRUE(enrollment_screen->num_retries_ >= 4 || On 2017/02/01 00:47:47, can Skylar cook wrote: > `&&` ? Done. https://codereview.chromium.org/2526973002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:132: EXPECT_TRUE(enrollment_screen->num_retries_ >= 4 && On 2017/02/01 00:47:47, can Skylar cook wrote: > Does EXPECT_TRUE() print the original argument with the variable names intact > when it fails? Things like EXPECT_LE() and EXPECT_GE(), or even EXPECT_PRED(), > can help when debugging tests as they'll tell you what the erroring value was > (obviously you've already done the hard work here, just a note). Thanks for the suggestion. I will try these from now on.
lgtm Needs @xiyuan to make the final approval I think? https://codereview.chromium.org/2526973002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right): https://codereview.chromium.org/2526973002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.h:187: static const int64_t MAX_DELAY = 480000; // 8 minutes nit: (unless others suggested otherwise) I like this style MAX_DELAY_MS = 8 * 60 * 1000; (note _MS or _MILLIS in the name too) also for INITIAL_DELAY optional: even more self documenting is: FOO = TimeDelta::FromMinutes(8).InMilliSeconds() but if going this route you'd want to move this expression into the .cc file (to avoid every #includer being pulled into seeing that)
https://codereview.chromium.org/2526973002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right): https://codereview.chromium.org/2526973002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.h:187: static const int64_t MAX_DELAY = 480000; // 8 minutes On 2017/02/01 21:51:33, joth wrote: > nit: (unless others suggested otherwise) I like this style > MAX_DELAY_MS = 8 * 60 * 1000; > > > (note _MS or _MILLIS in the name too) > > also for INITIAL_DELAY > > > optional: even more self documenting is: > FOO = TimeDelta::FromMinutes(8).InMilliSeconds() > > but if going this route you'd want to move this expression into the .cc file (to > avoid every #includer being pulled into seeing that) > Done.
https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/BUILD.gn:1583: "login/enrollment/mock_enrollment_screen.h", enterprise_enrollment_helper_mock.*, mock_enrollment_screen.* and mock_base_screen_delegate.* are included for browser_tests in chrome/test/BUILD.gn already. If we want to share the code, we should create a build target to include them instead of duplicating them. I am thinking: 1. Add a "test_support" static lib in chrome/browser/chromeos/BUILD.gn similar to the "arc_test_support" target there to include those shared files. 2. Make browser_tests target in chrome/test/BUILD.gn depends on it, i.e. add //chrome/browser/chromeos:test_support to the deps; 3. Make unit_tests target in chrome/browser/chromeos/BUILD.gn also depends on it; https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:203: LOG(WARNING) << "Enrollment retry " << ++num_retries_ Use a separate line for the increment instead of inlining it in a LOG line. LOG line could be compiled into a no-op in some build config. https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:215: LOG(WARNING) << "Enrollment retry " << ++num_retries_ smilar here, move ++num_retries_ out of LOG line. https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right): https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen.h:163: bool UsingHandsOffEnrollment(); nit: This can be moved to cc file as a helper func in anonymous namespace. https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen.h:187: static const int64_t MAX_DELAY_MS = 8 * 60 * 1000; // 8 minutes If those constants are not used outside the impl, move them into an anonymous namespace in cc file instead of here. https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc (right): https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:6: #include "base/lazy_instance.h" nit: unused ? https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:7: #include "base/logging.h" nit: unused ? https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:9: #include "base/run_loop.h" nit: unused ? https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:25: using testing::Invoke; Are these used? https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:37: static EnterpriseEnrollmentHelper* mock_enrollment_helper_creator( Move into anonymous namespace for static method when possible. Use CamelCase for function names. i.e. mock_enrollment_helper_creator -> MockEnrollmentHelperCreator https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:44: void SetUp() override { nit: Add a comment for overridden interface/class methods. e.g. // testing::Test: void SetUp() override {...} void TearDown() override {...} https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:54: static bool should_enroll; Is this used anywhere? https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:64: MockEnrollmentScreenActor mock_actor; Name of a class member var should end with "_" https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:69: TestingBrowserProcess::CreateInstance(); Why do we need to DeleteInstance then CreateInstance? https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:76: static_cast<pairing_chromeos::ControllerPairingController*>( Is static_cast necessary? https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:81: EnterpriseEnrollmentHelperMock::should_enroll = false; Can we avoid using static (or any global state) for this purpose? Is it possible to make the creator function expose the EnterpriseEnrollmentHelperMock object it creates to allow setting per-instance state? Or defer SetupEnrollmentHelperMock call here to set a creator that constructs a mock with desired behavior? https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:113: &fake_controller)); Line 104-113 repeats for every test case. Can they be moved into SetUp? https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc (right): https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc:10: #include "testing/gtest/include/gtest/gtest.h" The above two headers are already included in the mock's header. https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc:21: Invoke(this, &EnterpriseEnrollmentHelperMock::TryEnrollment)); This probably should be moved to the mock creator function in the test instead of here since it is only relevant to that test.
Forgot to mention that the bug line in CL description has wrong format. It should not have crbug: prefix. i.e. BUG=crbug:667613 => BUG=667613
https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc (right): https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:6: #include "base/lazy_instance.h" On 2017/02/01 23:01:51, xiyuan wrote: > nit: unused ? It was unused. I deleted it. Thanks for catching this! https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:7: #include "base/logging.h" On 2017/02/01 23:01:51, xiyuan wrote: > nit: unused ? Deleted https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:9: #include "base/run_loop.h" On 2017/02/01 23:01:51, xiyuan wrote: > nit: unused ? Deleted https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:25: using testing::Invoke; On 2017/02/01 23:01:51, xiyuan wrote: > Are these used? Yes https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:37: static EnterpriseEnrollmentHelper* mock_enrollment_helper_creator( On 2017/02/01 23:01:51, xiyuan wrote: > Move into anonymous namespace for static method when possible. > > Use CamelCase for function names. > i.e. > mock_enrollment_helper_creator -> MockEnrollmentHelperCreator Done. https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:44: void SetUp() override { On 2017/02/01 23:01:51, xiyuan wrote: > nit: Add a comment for overridden interface/class methods. > > e.g. > > // testing::Test: > void SetUp() override {...} > void TearDown() override {...} Done. https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:54: static bool should_enroll; On 2017/02/01 23:01:51, xiyuan wrote: > Is this used anywhere? No. Deleted now. https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:64: MockEnrollmentScreenActor mock_actor; On 2017/02/01 23:01:52, xiyuan wrote: > Name of a class member var should end with "_" Done. https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:69: TestingBrowserProcess::CreateInstance(); On 2017/02/01 23:01:51, xiyuan wrote: > Why do we need to DeleteInstance then CreateInstance? We want to replace the standard BrowserProcess with a TestingBrowserProcess. Before the TestingBrowserProcess can be created, the existing BrowserProcess must be deleted. There is a DCHECK which demands this: https://cs.chromium.org/chromium/src/chrome/test/base/testing_browser_process... https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:76: static_cast<pairing_chromeos::ControllerPairingController*>( On 2017/02/01 23:01:51, xiyuan wrote: > Is static_cast necessary? Actually, it is not needed. https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:81: EnterpriseEnrollmentHelperMock::should_enroll = false; On 2017/02/01 23:01:51, xiyuan wrote: > Can we avoid using static (or any global state) for this purpose? Is it possible > to make the creator function expose the EnterpriseEnrollmentHelperMock object it > creates to allow setting per-instance state? Or defer SetupEnrollmentHelperMock > call here to set a creator that constructs a mock with desired behavior? Yes. Done. https://codereview.chromium.org/2526973002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:113: &fake_controller)); On 2017/02/01 23:01:52, xiyuan wrote: > Line 104-113 repeats for every test case. Can they be moved into SetUp? Yes. Done. I also need a separate function SetUpEnrollmentScreen since there is one line in the middle (after certain revisions) that is different in each test.
https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/BUILD.gn:1461: "login/enrollment/mock_enrollment_screen.h", We have a duplicated mock_enrollment_screen.h. Nuke one. https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:56: const int INITIAL_DELAY_MS = 4 * 1000; // 4 seconds nit: constant should be name as kTheConstantName, like the above. And chromium code prefers to use constexpr in new code. i.e. constexpr int kInitialDelayMs = 4 * 1000; // 4 seconds https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:229: num_retries_++; nit: use pre-increment, i.e. ++num_retries_; https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:40: template <bool SHOULD_ENROLL> SHOULD_ENROLL -> should_enroll https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:64: TestingBrowserProcess::DeleteInstance(); Think DeleteInstance() is not necessary because there should be no BrowserProcess instance in a unittest. https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:75: // testing::Test: nit: Remove this. Only need one at the beginning of the overridden methods. https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:86: base::MessageLoop loop_; Make member vars private and provide getters to them, or at least make them protected: https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:97: }; DISALLOW_COPY_AND_ASSIGN(EnrollmentScreenUnitTest); https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc:24: static_cast<EnrollmentScreen*>(status_consumer()) static_cast should be avoided. Can you call status_consumer()->OnDeviceAttributeUpdatePermission(false) instead? https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.h (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.h:25: void FailEnrollment(); nit: SucceedEnrollment -> SimulateEnrommentSuccess FailEnrollment -> SimulateEnrollmentFailure https://codereview.chromium.org/2526973002/diff/120001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:2250: "../browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.h", These two and other files listed in //chrome/browser/chromeos:test_support should be removed from this list now.
Taking myself out of reviewers
kumarniranjan@google.com changed reviewers: - achuith@chromium.org, alemate@chromium.org
https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/BUILD.gn:1461: "login/enrollment/mock_enrollment_screen.h", On 2017/02/03 23:34:14, xiyuan wrote: > We have a duplicated mock_enrollment_screen.h. Nuke one. Done. https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:56: const int INITIAL_DELAY_MS = 4 * 1000; // 4 seconds On 2017/02/03 23:34:14, xiyuan wrote: > nit: constant should be name as kTheConstantName, like the above. And chromium > code prefers to use constexpr in new code. > > i.e. > constexpr int kInitialDelayMs = 4 * 1000; // 4 seconds Done. https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:229: num_retries_++; On 2017/02/03 23:34:14, xiyuan wrote: > nit: use pre-increment, > i.e. > ++num_retries_; Done. https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:40: template <bool SHOULD_ENROLL> On 2017/02/03 23:34:14, xiyuan wrote: > SHOULD_ENROLL -> should_enroll Done. https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:64: TestingBrowserProcess::DeleteInstance(); On 2017/02/03 23:34:14, xiyuan wrote: > Think DeleteInstance() is not necessary because there should be no > BrowserProcess instance in a unittest. I tried removing this line, but my test fails because the code hits a DCHECK. https://cs.chromium.org/chromium/src/chrome/test/base/testing_browser_process... It appears that there is a BrowserProcess, even in a unit test. https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:75: // testing::Test: On 2017/02/03 23:34:14, xiyuan wrote: > nit: Remove this. Only need one at the beginning of the overridden methods. Done. https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:86: base::MessageLoop loop_; On 2017/02/03 23:34:14, xiyuan wrote: > Make member vars private and provide getters to them, or at least make them > protected: Done. https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:97: }; On 2017/02/03 23:34:14, xiyuan wrote: > DISALLOW_COPY_AND_ASSIGN(EnrollmentScreenUnitTest); Done. https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc:24: static_cast<EnrollmentScreen*>(status_consumer()) On 2017/02/03 23:34:15, xiyuan wrote: > static_cast should be avoided. Can you call > status_consumer()->OnDeviceAttributeUpdatePermission(false) > > instead? The method OnDeviceAttributeUpdatePermission is not defined in EnrollmentStatusConsumer. It is only defined in EnrollmentScreen. So, unfortunately, I cannot remove the static cast. https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.h (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.h:25: void FailEnrollment(); On 2017/02/03 23:34:15, xiyuan wrote: > nit: > SucceedEnrollment -> SimulateEnrommentSuccess > FailEnrollment -> SimulateEnrollmentFailure Done. https://codereview.chromium.org/2526973002/diff/120001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:2250: "../browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.h", On 2017/02/03 23:34:15, xiyuan wrote: > These two and other files listed in //chrome/browser/chromeos:test_support > should be removed from this list now. Done.
https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:64: TestingBrowserProcess::DeleteInstance(); On 2017/02/06 21:38:24, kumarniranjan wrote: > On 2017/02/03 23:34:14, xiyuan wrote: > > Think DeleteInstance() is not necessary because there should be no > > BrowserProcess instance in a unittest. > > I tried removing this line, but my test fails because the code hits a DCHECK. > https://cs.chromium.org/chromium/src/chrome/test/base/testing_browser_process... > It appears that there is a BrowserProcess, even in a unit test. Can you remind me why we need to re-create TestingBrowserProcess ? Maybe we should not do anything. A unittest might just get TestingBrowserProcess by default. https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc:24: static_cast<EnrollmentScreen*>(status_consumer()) On 2017/02/06 21:38:25, kumarniranjan wrote: > On 2017/02/03 23:34:15, xiyuan wrote: > > static_cast should be avoided. Can you call > > status_consumer()->OnDeviceAttributeUpdatePermission(false) > > > > instead? > > The method OnDeviceAttributeUpdatePermission is not defined in > EnrollmentStatusConsumer. It is only defined in EnrollmentScreen. > So, unfortunately, I cannot remove the static cast. It seems to be part of EnrollmentStatusConsumer [1]. What did I miss? [1]: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/enrollment...
https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:64: TestingBrowserProcess::DeleteInstance(); On 2017/02/06 23:05:33, xiyuan wrote: > On 2017/02/06 21:38:24, kumarniranjan wrote: > > On 2017/02/03 23:34:14, xiyuan wrote: > > > Think DeleteInstance() is not necessary because there should be no > > > BrowserProcess instance in a unittest. > > > > I tried removing this line, but my test fails because the code hits a DCHECK. > > > https://cs.chromium.org/chromium/src/chrome/test/base/testing_browser_process... > > It appears that there is a BrowserProcess, even in a unit test. > > Can you remind me why we need to re-create TestingBrowserProcess ? Maybe we > should not do anything. A unittest might just get TestingBrowserProcess by > default. The only reason I put TestingBrowserProcess::DeleteInstance() into the code was that I got a DCHECK error when I did CreateInstance() When I remove both, the test passes! This indicates that you are correct in assuming that a Unit Test gets a TestingBrowserProcess. https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc:24: static_cast<EnrollmentScreen*>(status_consumer()) On 2017/02/06 23:05:33, xiyuan wrote: > On 2017/02/06 21:38:25, kumarniranjan wrote: > > On 2017/02/03 23:34:15, xiyuan wrote: > > > static_cast should be avoided. Can you call > > > status_consumer()->OnDeviceAttributeUpdatePermission(false) > > > > > > instead? > > > > The method OnDeviceAttributeUpdatePermission is not defined in > > EnrollmentStatusConsumer. It is only defined in EnrollmentScreen. > > So, unfortunately, I cannot remove the static cast. > > It seems to be part of EnrollmentStatusConsumer [1]. What did I miss? > > [1]: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/enrollment... Sorry, I meant that ShowEnrollmentStatusOnSuccess is only defined in EnrollmentScreen. However, when I try replacing ShowEnrollmentStatusOnSuccess() with OnDeviceAttributeUpdatePermission(false), I get a Segmentation fault. It originates from the StartupUtils call here https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/enrollment... StartupUtils probably doesn't work as expected since, in this unit test, I did not do the full initialization work that is done during normal Chrome browser startup.
https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc:24: static_cast<EnrollmentScreen*>(status_consumer()) On 2017/02/06 23:35:12, kumarniranjan wrote: > On 2017/02/06 23:05:33, xiyuan wrote: > > On 2017/02/06 21:38:25, kumarniranjan wrote: > > > On 2017/02/03 23:34:15, xiyuan wrote: > > > > static_cast should be avoided. Can you call > > > > status_consumer()->OnDeviceAttributeUpdatePermission(false) > > > > > > > > instead? > > > > > > The method OnDeviceAttributeUpdatePermission is not defined in > > > EnrollmentStatusConsumer. It is only defined in EnrollmentScreen. > > > So, unfortunately, I cannot remove the static cast. > > > > It seems to be part of EnrollmentStatusConsumer [1]. What did I miss? > > > > [1]: > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/enrollment... > > Sorry, I meant that ShowEnrollmentStatusOnSuccess is only defined in > EnrollmentScreen. > > However, when I try replacing ShowEnrollmentStatusOnSuccess() with > OnDeviceAttributeUpdatePermission(false), I get a Segmentation fault. > > It originates from the StartupUtils call here > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/enrollment... > > StartupUtils probably doesn't work as expected since, in this unit test, I did > not do the full initialization work that is done during normal Chrome browser > startup. Thank you for trying it out. static_cast makes EnterpriseEnrollmentHelperMock assume it must have a status consumer that could be casted to a EnrollmentScreen. This could be somewhat unexpected for future user of the mock class. How about deriving a mock class from EnterpriseEnrollmentHelperMock in your test? I am fine with doing the static_cast there since it has limited impact. https://codereview.chromium.org/2526973002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc (right): https://codereview.chromium.org/2526973002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:17: #include "chrome/test/base/testing_browser_process.h" This can also be removed now.
https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc (right): https://codereview.chromium.org/2526973002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.cc:24: static_cast<EnrollmentScreen*>(status_consumer()) On 2017/02/06 23:46:05, xiyuan wrote: > On 2017/02/06 23:35:12, kumarniranjan wrote: > > On 2017/02/06 23:05:33, xiyuan wrote: > > > On 2017/02/06 21:38:25, kumarniranjan wrote: > > > > On 2017/02/03 23:34:15, xiyuan wrote: > > > > > static_cast should be avoided. Can you call > > > > > status_consumer()->OnDeviceAttributeUpdatePermission(false) > > > > > > > > > > instead? > > > > > > > > The method OnDeviceAttributeUpdatePermission is not defined in > > > > EnrollmentStatusConsumer. It is only defined in EnrollmentScreen. > > > > So, unfortunately, I cannot remove the static cast. > > > > > > It seems to be part of EnrollmentStatusConsumer [1]. What did I miss? > > > > > > [1]: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/enrollment... > > > > Sorry, I meant that ShowEnrollmentStatusOnSuccess is only defined in > > EnrollmentScreen. > > > > However, when I try replacing ShowEnrollmentStatusOnSuccess() with > > OnDeviceAttributeUpdatePermission(false), I get a Segmentation fault. > > > > It originates from the StartupUtils call here > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/enrollment... > > > > StartupUtils probably doesn't work as expected since, in this unit test, I did > > not do the full initialization work that is done during normal Chrome browser > > startup. > > Thank you for trying it out. > > static_cast makes EnterpriseEnrollmentHelperMock assume it must have a status > consumer that could be casted to a EnrollmentScreen. This could be somewhat > unexpected for future user of the mock class. > > How about deriving a mock class from EnterpriseEnrollmentHelperMock in your > test? I am fine with doing the static_cast there since it has limited impact. I came up with a solution using anonymous functions. This way, I do not need to put unexpected constraints on users of EnterpriseEnrollmentHelperMock, while at the same time avoiding the creation of a whole new class just for one test. https://codereview.chromium.org/2526973002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc (right): https://codereview.chromium.org/2526973002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:17: #include "chrome/test/base/testing_browser_process.h" On 2017/02/06 23:46:05, xiyuan wrote: > This can also be removed now. I can't remove it because of TestingBrowserProcess::GetGlobal()->SetShuttingDown(true); which is executed during test tear-down.
Let me know when new patch is uploaded and I can take a look. Thanks. https://codereview.chromium.org/2526973002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc (right): https://codereview.chromium.org/2526973002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:17: #include "chrome/test/base/testing_browser_process.h" On 2017/02/07 21:26:00, kumarniranjan wrote: > On 2017/02/06 23:46:05, xiyuan wrote: > > This can also be removed now. > > I can't remove it because of > TestingBrowserProcess::GetGlobal()->SetShuttingDown(true); > which is executed during test tear-down. Acknowledged. https://codereview.chromium.org/2526973002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:126: TestingBrowserProcess::GetGlobal()->SetShuttingDown(true); Move to EnrollmentScreenUnitTest::TearDown since all test cases run this line.
lgtm Thank you for bearing with me and making the changes.
On 2017/02/07 22:03:17, xiyuan wrote: > lgtm > > Thank you for bearing with me and making the changes. I appreciate your detailed comments and feedback. I can now be confident that my code is good!
The CQ bit was checked by kumarniranjan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from skylarc@google.com, joth@chromium.org Link to the patchset: https://codereview.chromium.org/2526973002/#ps180001 (title: "Additional changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Added retry policy When hands-off zero-touch enrollment is invoked, various problems may be encountered. Examples include the TPM chip not yet being ready for enrollment, and network outages occurring before enrollment is finished. BUG=crbug:667613 TEST=none ========== to ========== Added retry policy When hands-off zero-touch enrollment is invoked, various problems may be encountered. Examples include the TPM chip not yet being ready for enrollment, and network outages occurring before enrollment is finished. BUG=667613 TEST=none ==========
https://codereview.chromium.org/2526973002/diff/180001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2526973002/diff/180001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:1238: "//chrome/browser/chromeos:test_support", We should move this to a deps += [...] inside the "if (is_chromeos)" block around line 1261 to fix the gn error.
https://codereview.chromium.org/2526973002/diff/180001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2526973002/diff/180001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:1238: "//chrome/browser/chromeos:test_support", On 2017/02/07 22:36:53, xiyuan wrote: > We should move this to a deps += [...] inside the "if (is_chromeos)" block > around line 1261 to fix the gn error. Done.
The CQ bit was checked by kumarniranjan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from joth@chromium.org, skylarc@google.com, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2526973002/#ps200001 (title: "GN build modification")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2526973002/diff/200001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2526973002/diff/200001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:1266: "//chrome/browser/chromeos:test_support", We need to add deps+= [ "//chrome/browser/chromeos:test_support", ] in this block instead of adding this to "data"
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kumarniranjan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from joth@chromium.org, skylarc@google.com, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2526973002/#ps220001 (title: "Fixed GN problem")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1486512341210240,
"parent_rev": "4928bd5160bb22433aff3dc42d2c2773e4f09b9f", "commit_rev":
"6349e12992c74e7cb830b76d9357716eba808796"}
Message was sent while issue was closed.
Description was changed from ========== Added retry policy When hands-off zero-touch enrollment is invoked, various problems may be encountered. Examples include the TPM chip not yet being ready for enrollment, and network outages occurring before enrollment is finished. BUG=667613 TEST=none ========== to ========== Added retry policy When hands-off zero-touch enrollment is invoked, various problems may be encountered. Examples include the TPM chip not yet being ready for enrollment, and network outages occurring before enrollment is finished. BUG=667613 TEST=none Review-Url: https://codereview.chromium.org/2526973002 Cr-Commit-Position: refs/heads/master@{#448860} Committed: https://chromium.googlesource.com/chromium/src/+/6349e12992c74e7cb830b76d9357... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/6349e12992c74e7cb830b76d9357... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
