Make Hands-Off Zero-Touch Enrollment compatibile with tests

Make Hands-Off Zero-Touch Enrollment compatibile with tests

Specifically, the automatic nature of Hands-Off Enrollment leads to
the DUT performing an update check after reboot which interferes
with autoupdate_EndToEndTest. This CL fixes that issue.
It does so by instructing the UpdateScreen to skip checking for updates
if both of the following conditions hold:
1. The previous update check initiated by the UpdateScreen did not result
in an update.
2. This previous update check occurred during the last hour.

BUG=chromium:710716
TEST=unit test

Review-Url: https://codereview.chromium.org/2870203003
Cr-Commit-Position: refs/heads/master@{#473065}
Committed: https://chromium.googlesource.com/chromium/src/+/0e9353d37e60a871304084a1b34222062c4803e9

[xiyuan, 2017-05-09 22:03:52]

xiyuan@chromium.org changed reviewers:
+ xiyuan@chromium.org

[xiyuan, 2017-05-09 22:03:53]

early comments

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/base_screen.cc (right):

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/base_screen.cc:129: bool
BaseScreen::UsingHandsOffEnrollment() {
This does not feel belong here. Can we put it somewhere else? Maybe in
startup_utils ? or a static method of WizardController ?

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/update_screen.cc (right):

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/update_screen.cc:97: const int
kUpdateCheckRecencyThresholdInHours = 1;
nit: We can move this into where it is used since it is used only in one place.

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/update_screen.cc:650: return
(base::Time::Now() -
Wall clock could go backward. Let's do a compare before calculating delta.

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/update_screen.cc:651:
StartupUtils::GetTimeOfLastUpdateCheckWithoutUpdate()) >
GetTimeOfLastUpdateCheckWithoutUpdate() might not have a valid base::Time, e.g.
on the very first run. Let's handle that explicitly.

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/update_screen.h (right):

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/update_screen.h:134: bool
RecentUpdateCheckWithoutUpdate();
nit: RecentUpdateCheckWithoutUpdate -> ShouldCheckUpdate

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/startup_utils.cc (right):

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/startup_utils.cc:100:
registry->RegisterBooleanPref(prefs::kOobeControllerDetected, false);
You need to register kTimeOfLastUpdateCheckWithoutUpdate here.

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/startup_utils.cc:211:
SaveInt64PreferenceForced(prefs::kTimeOfLastUpdateCheckWithoutUpdate,
Where is kTimeOfLastUpdateCheckWithoutUpdate defined?

[kumarniranjan, 2017-05-09 22:49:47]

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/base_screen.cc (right):

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/base_screen.cc:129: bool
BaseScreen::UsingHandsOffEnrollment() {
On 2017/05/09 22:03:52, xiyuan wrote:
> This does not feel belong here. Can we put it somewhere else? Maybe in
> startup_utils ? or a static method of WizardController ?

Done.

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/update_screen.cc (right):

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/update_screen.cc:97: const int
kUpdateCheckRecencyThresholdInHours = 1;
On 2017/05/09 22:03:52, xiyuan wrote:
> nit: We can move this into where it is used since it is used only in one
place.

Done.

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/update_screen.cc:650: return
(base::Time::Now() -
On 2017/05/09 22:03:52, xiyuan wrote:
> Wall clock could go backward. Let's do a compare before calculating delta.

Done.

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/update_screen.cc:651:
StartupUtils::GetTimeOfLastUpdateCheckWithoutUpdate()) >
On 2017/05/09 22:03:52, xiyuan wrote:
> GetTimeOfLastUpdateCheckWithoutUpdate() might not have a valid base::Time,
e.g.
> on the very first run. Let's handle that explicitly.

Is adding it to the PrefsRegistry enough?
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/startup_ut...

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/update_screen.h (right):

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/update_screen.h:134: bool
RecentUpdateCheckWithoutUpdate();
On 2017/05/09 22:03:52, xiyuan wrote:
> nit: RecentUpdateCheckWithoutUpdate -> ShouldCheckUpdate

Done.

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/startup_utils.cc (right):

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/startup_utils.cc:100:
registry->RegisterBooleanPref(prefs::kOobeControllerDetected, false);
On 2017/05/09 22:03:52, xiyuan wrote:
> You need to register kTimeOfLastUpdateCheckWithoutUpdate here.

Done.

https://codereview.chromium.org/2870203003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/startup_utils.cc:211:
SaveInt64PreferenceForced(prefs::kTimeOfLastUpdateCheckWithoutUpdate,
On 2017/05/09 22:03:52, xiyuan wrote:
> Where is kTimeOfLastUpdateCheckWithoutUpdate defined?

Done.

[xiyuan, 2017-05-10 16:29:20]

Mostly nits.

https://codereview.chromium.org/2870203003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/update_screen.cc (right):

https://codereview.chromium.org/2870203003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen.cc:205: // a noncritical
update available. No need to update during OOBE.
nit: "No need to update during OOBE." -> "Record the time to block OOBE update
for sometime."

https://codereview.chromium.org/2870203003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen.cc:647: const int
kUpdateCheckRecencyThresholdInHours = 1;
nit: since we are in a function scoped, we can use base::TimeDelta directly.

i.e.
  const base::TimeDelta kUpdateCheckRecencyThreshold
      = base::TimeDelta::FromHours(1);

https://codereview.chromium.org/2870203003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen.cc:657: return false;
I think we should "return true" to allow update check if time goes backward. The
system might not be in a good shape when it happens. So we would want update
check to happen.

https://codereview.chromium.org/2870203003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen.cc:660: return true;
nit: move this to the beginning of the function and bail out earlier.

i.e.

bool UpdateScreen::ShouldCheckForUpdate() {
  // Always run update check for non hands-off enrollment.
  if (!WizardController::UsingHandsOffEnrollment())
    return true;
  
  ...
}

https://codereview.chromium.org/2870203003/diff/40001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2870203003/diff/40001/chrome/common/pref_name...
chrome/common/pref_names.cc:1921: const char
kTimeOfLastUpdateCheckWithoutUpdate[] =
nit: Document the purpose of this pref similar to other prefs.

https://codereview.chromium.org/2870203003/diff/40001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/2870203003/diff/40001/chrome/common/pref_name...
chrome/common/pref_names.h:684: extern const char
kTimeOfLastUpdateCheckWithoutUpdate[];
nit: Let's add Oobe prefix and move it close to where other Oobe prefs.

i.e.
  const char kOobeTimeOfLastUpdateCheckWithoutUpdate[] =
      "OobeTimeOfLastUpdateCheckWithoutUpdate";

[kumarniranjan, 2017-05-13 04:42:35]

kumarniranjan@google.com changed reviewers:
+ alemate@chromium.org

[kumarniranjan, 2017-05-13 04:42:36]

https://codereview.chromium.org/2870203003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/update_screen.cc (right):

https://codereview.chromium.org/2870203003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen.cc:205: // a noncritical
update available. No need to update during OOBE.
On 2017/05/10 16:29:19, xiyuan wrote:
> nit: "No need to update during OOBE." -> "Record the time to block OOBE update
> for sometime."

Done.

https://codereview.chromium.org/2870203003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen.cc:647: const int
kUpdateCheckRecencyThresholdInHours = 1;
On 2017/05/10 16:29:19, xiyuan wrote:
> nit: since we are in a function scoped, we can use base::TimeDelta directly.
> 
> i.e.
>   const base::TimeDelta kUpdateCheckRecencyThreshold
>       = base::TimeDelta::FromHours(1);

Done.

https://codereview.chromium.org/2870203003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen.cc:657: return false;
On 2017/05/10 16:29:19, xiyuan wrote:
> I think we should "return true" to allow update check if time goes backward.
The
> system might not be in a good shape when it happens. So we would want update
> check to happen.

Done.

https://codereview.chromium.org/2870203003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen.cc:660: return true;
On 2017/05/10 16:29:20, xiyuan wrote:
> nit: move this to the beginning of the function and bail out earlier.
> 
> i.e.
> 
> bool UpdateScreen::ShouldCheckForUpdate() {
>   // Always run update check for non hands-off enrollment.
>   if (!WizardController::UsingHandsOffEnrollment())
>     return true;
>   
>   ...
> }

Done.

https://codereview.chromium.org/2870203003/diff/40001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2870203003/diff/40001/chrome/common/pref_name...
chrome/common/pref_names.cc:1921: const char
kTimeOfLastUpdateCheckWithoutUpdate[] =
On 2017/05/10 16:29:20, xiyuan wrote:
> nit: Document the purpose of this pref similar to other prefs.

Done.

https://codereview.chromium.org/2870203003/diff/40001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/2870203003/diff/40001/chrome/common/pref_name...
chrome/common/pref_names.h:684: extern const char
kTimeOfLastUpdateCheckWithoutUpdate[];
On 2017/05/10 16:29:20, xiyuan wrote:
> nit: Let's add Oobe prefix and move it close to where other Oobe prefs.
> 
> i.e.
>   const char kOobeTimeOfLastUpdateCheckWithoutUpdate[] =
>       "OobeTimeOfLastUpdateCheckWithoutUpdate";

Done.

[xiyuan, 2017-05-15 17:24:23]

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/BUILD.gn (right):

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/BUILD.gn:1536: "login/screens/mock_update_screen.h",
We should remove mock_error_screen.* and mock_update_screen.* from 
chrome/test/BUILD.gn since we added them here.

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/screen_exit_code.h (right):

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/screen_exit_code.h:30: // "Critical"
updates are those which require the device to reboot.
nit: "Critical" updates are those that *have a deadline* and require the device
to reboot.

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/update_screen.cc (right):

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen.cc:643: void
UpdateScreen::RecordUpdateCheckResultingInUpdate() {
This name is confusing. How about call it ClearUpdateCheckNoUpdateTime() ? And
expose an explicit clear mthod from StartupUtils instead of writing a 0.

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen.cc:664: } else {
nit: get rid of "else"

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/update_screen_unittest.cc (right):

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:40:
runner_.task_runner()->FastForwardBy(time);
Is this relevant?

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:41: base::Time
last = StartupUtils::GetTimeOfLastUpdateCheckWithoutUpdate();
ASSERT_FALSE(last.is_null());

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:49: void
SimulateUpdateAvailable(bool available, bool critical = false) {
Avoid using default arg.

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:106: delete
mock_error_screen_;
Use std::uniqe_ptr<> and call .reset() here instead.

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:170:
second_update_screen_->StartNetworkCheck();
Looks like |first_update_screen_| and |second_update_screen_| can be locals of
the test case instead of being member of the test fixture.

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:176:
SimulateUpdateAvailable(false);
Is this call necessary? There is no follow up test code. Why do we need it?

https://codereview.chromium.org/2870203003/diff/80001/chromeos/network/portal...
File chromeos/network/portal_detector/network_portal_detector.h (right):

https://codereview.chromium.org/2870203003/diff/80001/chromeos/network/portal...
chromeos/network/portal_detector/network_portal_detector.h:148: CHROMEOS_EXPORT
void EndTest();
This is not necessary and resetting the flag separately is error prone. If
leaving the flag unchanged makes you feel uncomfortable, we can reset it in
Shutdown().

[kumarniranjan, 2017-05-15 22:08:22]

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/BUILD.gn (right):

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/BUILD.gn:1536: "login/screens/mock_update_screen.h",
On 2017/05/15 17:24:22, xiyuan wrote:
> We should remove mock_error_screen.* and mock_update_screen.* from 
> chrome/test/BUILD.gn since we added them here.

Done. Also removed mock_network_screen.*

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/screen_exit_code.h (right):

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/screen_exit_code.h:30: // "Critical"
updates are those which require the device to reboot.
On 2017/05/15 17:24:22, xiyuan wrote:
> nit: "Critical" updates are those that *have a deadline* and require the
device
> to reboot.

Done.

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/update_screen.cc (right):

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen.cc:643: void
UpdateScreen::RecordUpdateCheckResultingInUpdate() {
On 2017/05/15 17:24:22, xiyuan wrote:
> This name is confusing. How about call it ClearUpdateCheckNoUpdateTime() ? And
> expose an explicit clear mthod from StartupUtils instead of writing a 0.

Done.

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen.cc:664: } else {
On 2017/05/15 17:24:22, xiyuan wrote:
> nit: get rid of "else"

Done.

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/update_screen_unittest.cc (right):

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:40:
runner_.task_runner()->FastForwardBy(time);
On 2017/05/15 17:24:23, xiyuan wrote:
> Is this relevant? 

Good catch! It's no longer needed.

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:41: base::Time
last = StartupUtils::GetTimeOfLastUpdateCheckWithoutUpdate();
On 2017/05/15 17:24:23, xiyuan wrote:
> ASSERT_FALSE(last.is_null());

In StartupUtils::RegisterPrefs, I set this pref to have initial value "0" (since
Time is encoded as int64_t which is in turn encoded as a string). So,
technically, this does translate to a real time (when I was debugging the test
with print statements, I saw that this translated to sometime in the year 1601).
But adding the assert can't hurt, so I put it in anyway.

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:49: void
SimulateUpdateAvailable(bool available, bool critical = false) {
On 2017/05/15 17:24:23, xiyuan wrote:
> Avoid using default arg.

Done.

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:106: delete
mock_error_screen_;
On 2017/05/15 17:24:23, xiyuan wrote:
> Use std::uniqe_ptr<> and call .reset() here instead.

Done.

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:170:
second_update_screen_->StartNetworkCheck();
On 2017/05/15 17:24:23, xiyuan wrote:
> Looks like |first_update_screen_| and |second_update_screen_| can be locals of
> the test case instead of being member of the test fixture.

Done.

https://codereview.chromium.org/2870203003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:176:
SimulateUpdateAvailable(false);
On 2017/05/15 17:24:22, xiyuan wrote:
> Is this call necessary? There is no follow up test code. Why do we need it?

It is necessary to satisfy the expectation defined at the end of SetUp:
  96     // Later verifies that UpdateScreen successfully exits both times.
  97     EXPECT_CALL(mock_base_screen_delegate_,
  98                 OnExit(_, ScreenExitCode::UPDATE_NOUPDATE, _))
  99         .Times(2);

It is also more similar to the scenario the UpdateScreen will face in real use.

https://codereview.chromium.org/2870203003/diff/80001/chromeos/network/portal...
File chromeos/network/portal_detector/network_portal_detector.h (right):

https://codereview.chromium.org/2870203003/diff/80001/chromeos/network/portal...
chromeos/network/portal_detector/network_portal_detector.h:148: CHROMEOS_EXPORT
void EndTest();
On 2017/05/15 17:24:23, xiyuan wrote:
> This is not necessary and resetting the flag separately is error prone. If
> leaving the flag unchanged makes you feel uncomfortable, we can reset it in
> Shutdown().

Thanks for catching this. I think I used to need it at some point during
development but forgot to delete it.

Do you think we should modify network_portal_detector::Shutdown to reset this
flag? I'm okay either way.

[xiyuan, 2017-05-15 22:12:54]

Please upload a new PS before asking for review again.

[kumarniranjan, 2017-05-15 22:15:04]

On 2017/05/15 22:12:54, xiyuan wrote:
> Please upload a new PS before asking for review again.

Sorry. I uploaded it now

[xiyuan, 2017-05-15 22:30:20]

Have you get a chance to try this CL against the auto test?

https://codereview.chromium.org/2870203003/diff/80001/chromeos/network/portal...
File chromeos/network/portal_detector/network_portal_detector.h (right):

https://codereview.chromium.org/2870203003/diff/80001/chromeos/network/portal...
chromeos/network/portal_detector/network_portal_detector.h:148: CHROMEOS_EXPORT
void EndTest();
On 2017/05/15 22:08:22, kumarniranjan wrote:
> On 2017/05/15 17:24:23, xiyuan wrote:
> > This is not necessary and resetting the flag separately is error prone. If
> > leaving the flag unchanged makes you feel uncomfortable, we can reset it in
> > Shutdown().
> 
> Thanks for catching this. I think I used to need it at some point during
> development but forgot to delete it.
> 
> Do you think we should modify network_portal_detector::Shutdown to reset this
> flag? I'm okay either way.

I am okay leaving it as-is.

https://codereview.chromium.org/2870203003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/update_screen_unittest.cc (right):

https://codereview.chromium.org/2870203003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:52: const
std::unique_ptr<UpdateScreen>& update_screen,
We can pass UpdateScreen* as long as we can guarantee |update_screen| does not
go away while the function runs.

https://codereview.chromium.org/2870203003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:145:
&mock_base_screen_delegate_, &mock_view_, fake_controller_));
nit: std::unique_ptr<UpdateScreen> first_update_screen =
    base::MakeUnique<UpdateScreen>(...);

Chromium coding style prefers base::MakeUnique over using "new" directly.

Actually, do we have to new? I mean, could we just use a var on stack, 

e.g. 
  UpdateScreen first_update_screen(...);

https://codereview.chromium.org/2870203003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:152:
SimulateUpdateAvailable(first_update_screen, true, false);
nit: document the booleans

i.e.
  SimulateUpdateAvailable(first_update_screen,
      true /* available */, false /* critical */);

or
  const bool kAvailable = true;
  const bool kNotCritical = false;
  SimulateUpdateAvailable(first_update_screen, kAvailable, kNotCritical);

[kumarniranjan, 2017-05-15 23:21:37]

On 2017/05/15 22:30:20, xiyuan wrote:
> Have you get a chance to try this CL against the auto test?
No, since I am unable to run the autoupdate_EndToEndTest with a locally built
image. I am currently talking to dhaddock@ to figure out how I can do this.
Until he helps us find a way, our current procedure is to land the CL and look
at the test results published on Goldeneye to evaluate whether the code fulfills
its purpose. I have verified that devices work fine with this code, so there is
no risk of something breaking.
> 
>
https://codereview.chromium.org/2870203003/diff/80001/chromeos/network/portal...
> File chromeos/network/portal_detector/network_portal_detector.h (right):
> 
>
https://codereview.chromium.org/2870203003/diff/80001/chromeos/network/portal...
> chromeos/network/portal_detector/network_portal_detector.h:148:
CHROMEOS_EXPORT
> void EndTest();
> On 2017/05/15 22:08:22, kumarniranjan wrote:
> > On 2017/05/15 17:24:23, xiyuan wrote:
> > > This is not necessary and resetting the flag separately is error prone. If
> > > leaving the flag unchanged makes you feel uncomfortable, we can reset it
in
> > > Shutdown().
> > 
> > Thanks for catching this. I think I used to need it at some point during
> > development but forgot to delete it.
> > 
> > Do you think we should modify network_portal_detector::Shutdown to reset
this
> > flag? I'm okay either way.
> 
> I am okay leaving it as-is.
> 
>
https://codereview.chromium.org/2870203003/diff/100001/chrome/browser/chromeo...
> File chrome/browser/chromeos/login/screens/update_screen_unittest.cc (right):
> 
>
https://codereview.chromium.org/2870203003/diff/100001/chrome/browser/chromeo...
> chrome/browser/chromeos/login/screens/update_screen_unittest.cc:52: const
> std::unique_ptr<UpdateScreen>& update_screen,
> We can pass UpdateScreen* as long as we can guarantee |update_screen| does not
> go away while the function runs.
> 
>
https://codereview.chromium.org/2870203003/diff/100001/chrome/browser/chromeo...
> chrome/browser/chromeos/login/screens/update_screen_unittest.cc:145:
> &mock_base_screen_delegate_, &mock_view_, fake_controller_));
> nit: std::unique_ptr<UpdateScreen> first_update_screen =
>     base::MakeUnique<UpdateScreen>(...);
> 
> Chromium coding style prefers base::MakeUnique over using "new" directly.
> 
> Actually, do we have to new? I mean, could we just use a var on stack, 
> 
> e.g. 
>   UpdateScreen first_update_screen(...);
> 
>
https://codereview.chromium.org/2870203003/diff/100001/chrome/browser/chromeo...
> chrome/browser/chromeos/login/screens/update_screen_unittest.cc:152:
> SimulateUpdateAvailable(first_update_screen, true, false);
> nit: document the booleans
> 
> i.e.
>   SimulateUpdateAvailable(first_update_screen,
>       true /* available */, false /* critical */);
> 
> or
>   const bool kAvailable = true;
>   const bool kNotCritical = false;
>   SimulateUpdateAvailable(first_update_screen, kAvailable, kNotCritical);

[kumarniranjan, 2017-05-15 23:21:53]

https://codereview.chromium.org/2870203003/diff/80001/chromeos/network/portal...
File chromeos/network/portal_detector/network_portal_detector.h (right):

https://codereview.chromium.org/2870203003/diff/80001/chromeos/network/portal...
chromeos/network/portal_detector/network_portal_detector.h:148: CHROMEOS_EXPORT
void EndTest();
On 2017/05/15 22:30:20, xiyuan wrote:
> On 2017/05/15 22:08:22, kumarniranjan wrote:
> > On 2017/05/15 17:24:23, xiyuan wrote:
> > > This is not necessary and resetting the flag separately is error prone. If
> > > leaving the flag unchanged makes you feel uncomfortable, we can reset it
in
> > > Shutdown().
> > 
> > Thanks for catching this. I think I used to need it at some point during
> > development but forgot to delete it.
> > 
> > Do you think we should modify network_portal_detector::Shutdown to reset
this
> > flag? I'm okay either way.
> 
> I am okay leaving it as-is.

Okay, I will leave it as-is. Smaller CLs are better.

https://codereview.chromium.org/2870203003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/update_screen_unittest.cc (right):

https://codereview.chromium.org/2870203003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:52: const
std::unique_ptr<UpdateScreen>& update_screen,
On 2017/05/15 22:30:20, xiyuan wrote:
> We can pass UpdateScreen* as long as we can guarantee |update_screen| does not
> go away while the function runs.

Done.

https://codereview.chromium.org/2870203003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:145:
&mock_base_screen_delegate_, &mock_view_, fake_controller_));
On 2017/05/15 22:30:20, xiyuan wrote:
> nit: std::unique_ptr<UpdateScreen> first_update_screen =
>     base::MakeUnique<UpdateScreen>(...);
> 
> Chromium coding style prefers base::MakeUnique over using "new" directly.
> 
> Actually, do we have to new? I mean, could we just use a var on stack, 
> 
> e.g. 
>   UpdateScreen first_update_screen(...);
>   

True. I changed it to use var on stack

https://codereview.chromium.org/2870203003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/update_screen_unittest.cc:152:
SimulateUpdateAvailable(first_update_screen, true, false);
On 2017/05/15 22:30:20, xiyuan wrote:
> nit: document the booleans
> 
> i.e.
>   SimulateUpdateAvailable(first_update_screen,
>       true /* available */, false /* critical */);
> 
> or
>   const bool kAvailable = true;
>   const bool kNotCritical = false;
>   SimulateUpdateAvailable(first_update_screen, kAvailable, kNotCritical);

Done.

[xiyuan, 2017-05-16 04:37:33]

lgtm

You probably need additional owner approval for the CL. Try "git cl owners" to
figure out which files and who are the owners.

[Alexander Alekseev, 2017-05-17 22:30:41]

lgtm

https://codereview.chromium.org/2870203003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/screen_exit_code.h (right):

https://codereview.chromium.org/2870203003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/screen_exit_code.h:31: // to reboot.
It basically says "no update to be installed for whatever reason".
I don't agree with "despite" ;) But it's up to you.

[kumarniranjan, 2017-05-17 22:33:21]

https://codereview.chromium.org/2870203003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/screen_exit_code.h (right):

https://codereview.chromium.org/2870203003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/screen_exit_code.h:31: // to reboot.
On 2017/05/17 22:30:40, Alexander Alekseev wrote:
> It basically says "no update to be installed for whatever reason".
> I don't agree with "despite" ;) But it's up to you.

I'll remove the "despite"

[kumarniranjan, 2017-05-17 22:36:38]

The CQ bit was checked by kumarniranjan@google.com

[kumarniranjan, 2017-05-17 22:36:39]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
alemate@chromium.org
Link to the patchset: https://codereview.chromium.org/2870203003/#ps140001
(title: "done")

[commit-bot: I haz the power, 2017-05-17 22:38:04]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 00:45:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 00:45:43]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kumarniranjan, 2017-05-18 20:59:06]

The CQ bit was checked by kumarniranjan@google.com

[kumarniranjan, 2017-05-18 20:59:07]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
alemate@chromium.org
Link to the patchset: https://codereview.chromium.org/2870203003/#ps160001
(title: "Make Hands-Off Zero-Touch Enrollment compatibile with tests")

[commit-bot: I haz the power, 2017-05-18 21:00:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 22:45:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 22:45:38]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kumarniranjan, 2017-05-18 23:27:31]

The CQ bit was checked by kumarniranjan@google.com

[kumarniranjan, 2017-05-18 23:27:32]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
alemate@chromium.org
Link to the patchset: https://codereview.chromium.org/2870203003/#ps180001
(title: "Make Hands-Off Zero-Touch Enrollment compatibile with tests")

[commit-bot: I haz the power, 2017-05-18 23:29:17]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 03:44:42]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495150051794490,
"parent_rev": "6f30dee87e57b95a04b9d22b182c8269c2a39d62", "commit_rev":
"0e9353d37e60a871304084a1b34222062c4803e9"}

[commit-bot: I haz the power, 2017-05-19 03:44:53]

Description was changed from

==========
Make Hands-Off Zero-Touch Enrollment compatibile with tests

Specifically, the automatic nature of Hands-Off Enrollment leads to
the DUT performing an update check after reboot which interferes
with autoupdate_EndToEndTest. This CL fixes that issue.
It does so by instructing the UpdateScreen to skip checking for updates
if both of the following conditions hold:
1. The previous update check initiated by the UpdateScreen did not result
in an update.
2. This previous update check occurred during the last hour.

BUG=chromium:710716
TEST=unit test
==========

to

==========
Make Hands-Off Zero-Touch Enrollment compatibile with tests

Specifically, the automatic nature of Hands-Off Enrollment leads to
the DUT performing an update check after reboot which interferes
with autoupdate_EndToEndTest. This CL fixes that issue.
It does so by instructing the UpdateScreen to skip checking for updates
if both of the following conditions hold:
1. The previous update check initiated by the UpdateScreen did not result
in an update.
2. This previous update check occurred during the last hour.

BUG=chromium:710716
TEST=unit test

Review-Url: https://codereview.chromium.org/2870203003
Cr-Commit-Position: refs/heads/master@{#473065}
Committed:
https://chromium.googlesource.com/chromium/src/+/0e9353d37e60a871304084a1b342...
==========

[commit-bot: I haz the power, 2017-05-19 03:44:54]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/0e9353d37e60a871304084a1b342...

[Marc Treib, 2017-05-19 09:27:10]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2889383002/ by treib@chromium.org.

The reason for reverting is: UpdateScreenUnitTest is failing on ASan LSan:

https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chrom....

[findit-for-me, 2017-05-19 12:47:27]

Findit (https://goo.gl/kROfz5) identified this CL at revision 473065 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...