Fix autoupdate_EndToEndTest on veyron_rialto

Fix autoupdate_EndToEndTest on veyron_rialto

Adds a 3 min delay before starting zero-touch enrollment during OOBE.

BUG=chromium:710716
TEST=browser test

Review-Url: https://codereview.chromium.org/2824283003
Cr-Commit-Position: refs/heads/master@{#468086}
Committed: https://chromium.googlesource.com/chromium/src/+/cb477af46a98d9415f1491f613d43920003f4f86

[xiyuan, 2017-04-18 22:45:25]

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

[xiyuan, 2017-04-18 22:45:26]

https://codereview.chromium.org/2824283003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/network_screen_browsertest.cc
(right):

https://codereview.chromium.org/2824283003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen_browsertest.cc:176:
base::TestMockTimeTaskRunner::ScopedContext scoped_context(
We need to make ScopedContext a member var of the test so that it will have the
same life time of the test. The original task runner is restored when
ScopedContext is destructed (i.e. go out of scope). That is probably why
FastFoward does not work.

[xiyuan, 2017-04-27 19:34:22]

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc
(right):

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:10:
#include "base/strings/utf_string_conversions.h"
Is this needed?

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/mock_network_state_helper.cc (right):

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/mock_network_state_helper.cc:1: // Copyright (c)
2017 The Chromium Authors. All rights reserved.
nit: (c) is not needed

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/mock_network_state_helper.cc:8: 
nit: nuke the empty line

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/mock_network_state_helper.cc:15: 
nit: nuke the empty line

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/mock_network_state_helper.h (right):

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/mock_network_state_helper.h:18:
~MockNetworkStateHelper();
override

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/mock_model_view_channel.h (right):

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/mock_model_view_channel.h:17:
~MockModelViewChannel();
override

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

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_browsertest.cc:12: #include
"base/test/test_mock_time_task_runner.h"
unused?

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

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_unittest.cc:7: #include
"base/strings/utf_string_conversions.h"
unused?

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_unittest.cc:14: #include
"chrome/browser/chromeos/login/screens/mock_network_screen.h"
unused?

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_unittest.cc:20: #include
"chromeos/system/fake_statistics_provider.h"
unused?

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_unittest.cc:67:
CrosSettings::Get()->ClearSettingsObservers(kSystemTimezone);
Why do we need to do this? Wouldn't network_screen_.reset() above be good
enough?

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_unittest.cc:79:
MockModelViewChannel mock_channel_;
nit: |mock_view_| and |mock_channel_| can be private.

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_unittest.cc:80:
login::MockNetworkStateHelper* mock_network_state_helper_;
nit: = nullptr;

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_unittest.cc:84:
base::ScopedMockTimeMessageLoopTaskRunner runner_;
nit: prefer to make the two members private and add an accessor for |runner_|.

https://codereview.chromium.org/2824283003/diff/100001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2824283003/diff/100001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:2276:
"../browser/chromeos/login/mock_network_state_helper.h",
Why do we need to add them? Would deps in [1] be good enough since we added
these files to chromeos:test_support?

[1]:
https://cs.chromium.org/chromium/src/chrome/test/BUILD.gn?rcl=7a4b49e0f05c61a...

https://codereview.chromium.org/2824283003/diff/100001/ui/base/ime/chromeos/m...
File ui/base/ime/chromeos/mock_component_extension_ime_manager.h (right):

https://codereview.chromium.org/2824283003/diff/100001/ui/base/ime/chromeos/m...
ui/base/ime/chromeos/mock_component_extension_ime_manager.h:15:
input_method::InputMethodDescriptors GetXkbIMEAsInputMethodDescriptor() {
GetXkbIMEAsInputMethodDescriptor is not virtual so this might cause problems in
the future. If we want to mock it, we need to make it virtual in base class.

Since test in this CL does not really care about IME, why do we need to mock
this? Can we just call InputMethodManager::Initialize and Shutdown directly and
avoid this code?

[kumarniranjan, 2017-04-27 22:59:03]

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc
(right):

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:10:
#include "base/strings/utf_string_conversions.h"
On 2017/04/27 19:34:21, xiyuan wrote:
> Is this needed?

Thanks for catching this. It is not needed. I will remove it.

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/mock_network_state_helper.cc (right):

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/mock_network_state_helper.cc:1: // Copyright (c)
2017 The Chromium Authors. All rights reserved.
On 2017/04/27 19:34:21, xiyuan wrote:
> nit: (c) is not needed

Done.

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/mock_network_state_helper.cc:8: 
On 2017/04/27 19:34:21, xiyuan wrote:
> nit: nuke the empty line

Done.

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/mock_network_state_helper.cc:15: 
On 2017/04/27 19:34:21, xiyuan wrote:
> nit: nuke the empty line

Done.

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/mock_network_state_helper.h (right):

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/mock_network_state_helper.h:18:
~MockNetworkStateHelper();
On 2017/04/27 19:34:21, xiyuan wrote:
> override

Done.

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/mock_model_view_channel.h (right):

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/mock_model_view_channel.h:17:
~MockModelViewChannel();
On 2017/04/27 19:34:21, xiyuan wrote:
> override

Done.

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

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_browsertest.cc:11: #include
"base/strings/utf_string_conversions.h"
This one is also unused and I have removed it as well.

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_browsertest.cc:12: #include
"base/test/test_mock_time_task_runner.h"
On 2017/04/27 19:34:21, xiyuan wrote:
> unused?

Yes. It is unused and I have removed it.

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

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_unittest.cc:7: #include
"base/strings/utf_string_conversions.h"
On 2017/04/27 19:34:21, xiyuan wrote:
> unused?

It is used on line 131 and line 146.

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_unittest.cc:14: #include
"chrome/browser/chromeos/login/screens/mock_network_screen.h"
On 2017/04/27 19:34:21, xiyuan wrote:
> unused?

It is used to import MockNetworkView (see line 78)

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_unittest.cc:20: #include
"chromeos/system/fake_statistics_provider.h"
On 2017/04/27 19:34:22, xiyuan wrote:
> unused?

It is needed for ScopedFakeStatisticsProvider (see line 90)

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_unittest.cc:67:
CrosSettings::Get()->ClearSettingsObservers(kSystemTimezone);
On 2017/04/27 19:34:21, xiyuan wrote:
> Why do we need to do this? Wouldn't network_screen_.reset() above be good
> enough?

I needed to do it earlier during development, but now, with the
network_screen_.reset(), you are right; it is no longer needed. Removing it.

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_unittest.cc:79:
MockModelViewChannel mock_channel_;
On 2017/04/27 19:34:21, xiyuan wrote:
> nit: |mock_view_| and |mock_channel_| can be private.

Done.

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_unittest.cc:80:
login::MockNetworkStateHelper* mock_network_state_helper_;
On 2017/04/27 19:34:21, xiyuan wrote:
> nit: = nullptr;

Done.

https://codereview.chromium.org/2824283003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen_unittest.cc:84:
base::ScopedMockTimeMessageLoopTaskRunner runner_;
On 2017/04/27 19:34:22, xiyuan wrote:
> nit: prefer to make the two members private and add an accessor for |runner_|.

Done.

https://codereview.chromium.org/2824283003/diff/100001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2824283003/diff/100001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:2276:
"../browser/chromeos/login/mock_network_state_helper.h",
On 2017/04/27 19:34:22, xiyuan wrote:
> Why do we need to add them? Would deps in [1] be good enough since we added
> these files to chromeos:test_support?
> 
> [1]:
>
https://cs.chromium.org/chromium/src/chrome/test/BUILD.gn?rcl=7a4b49e0f05c61a...

You're right: they are not needed. Removing.

https://codereview.chromium.org/2824283003/diff/100001/ui/base/ime/chromeos/m...
File ui/base/ime/chromeos/mock_component_extension_ime_manager.h (right):

https://codereview.chromium.org/2824283003/diff/100001/ui/base/ime/chromeos/m...
ui/base/ime/chromeos/mock_component_extension_ime_manager.h:15:
input_method::InputMethodDescriptors GetXkbIMEAsInputMethodDescriptor() {
On 2017/04/27 19:34:22, xiyuan wrote:
> GetXkbIMEAsInputMethodDescriptor is not virtual so this might cause problems
in
> the future. If we want to mock it, we need to make it virtual in base class.
> 
> Since test in this CL does not really care about IME, why do we need to mock
> this? Can we just call InputMethodManager::Initialize and Shutdown directly
and
> avoid this code?
There are two levels of mocking: one where I create MockInputMethodManager, and
a second level where I make this mock object use a
MockComponentExtensionIMEManager. I need both levels of mocking.

If I use InputMethodManager instead of MockInputMethodManager, the code runs
into a Segmentation fault in this function:
https://cs.chromium.org/chromium/src/ui/base/ime/chromeos/component_extension...

If I use MockInputMethodManager as I have done, but don't use a
MockComponentExtensionIMEManager, the code runs into a Segmentation fault in
this function:
https://cs.chromium.org/chromium/src/ui/base/ime/chromeos/component_extension...

So that is why I have to do both levels of mocking. I have changed
ComponentExtensionIMEManager::GetXkbIMEAsInputMethodDescriptor to be virtual.

[xiyuan, 2017-04-27 23:29:47]

lgtm

You would need owner stamp for one file changes.

https://codereview.chromium.org/2824283003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/mock_network_state_helper.h (right):

https://codereview.chromium.org/2824283003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/mock_network_state_helper.h:1: // Copyright (c)
2017 The Chromium Authors. All rights reserved.
nit: no (c) here either and all the other files

https://codereview.chromium.org/2824283003/diff/120001/ui/base/ime/chromeos/m...
File ui/base/ime/chromeos/mock_component_extension_ime_manager.h (right):

https://codereview.chromium.org/2824283003/diff/120001/ui/base/ime/chromeos/m...
ui/base/ime/chromeos/mock_component_extension_ime_manager.h:20:
MockComponentExtensionIMEManager::GetXkbIMEAsInputMethodDescriptor() {
put this in a cc file or inline it as in previous ps.

[kumarniranjan, 2017-04-28 16:48:09]

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

[kumarniranjan, 2017-04-28 16:48:10]

https://codereview.chromium.org/2824283003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/mock_network_state_helper.h (right):

https://codereview.chromium.org/2824283003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/mock_network_state_helper.h:1: // Copyright (c)
2017 The Chromium Authors. All rights reserved.
On 2017/04/27 23:29:47, xiyuan wrote:
> nit: no (c) here either and all the other files

Done.

https://codereview.chromium.org/2824283003/diff/120001/ui/base/ime/chromeos/m...
File ui/base/ime/chromeos/mock_component_extension_ime_manager.h (right):

https://codereview.chromium.org/2824283003/diff/120001/ui/base/ime/chromeos/m...
ui/base/ime/chromeos/mock_component_extension_ime_manager.h:20:
MockComponentExtensionIMEManager::GetXkbIMEAsInputMethodDescriptor() {
On 2017/04/27 23:29:47, xiyuan wrote:
> put this in a cc file or inline it as in previous ps.

Once I made ComponentExtensionIMEManager::GetXkbIMEAsInputMethodDescriptor
virtual, the style checker complained if I inlined this method. So I will have
to make a cc file.

[kumarniranjan, 2017-04-28 16:55:41]

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

[nona, 2017-04-28 16:56:39]

nona@google.com changed reviewers:
+ nona@google.com

[nona, 2017-04-28 16:56:40]

base/ime lgtm

[kumarniranjan, 2017-04-28 17:14:31]

The CQ bit was checked by kumarniranjan@google.com

[kumarniranjan, 2017-04-28 17:14:32]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2824283003/#ps140001
(title: "Addressed more comments")

[commit-bot: I haz the power, 2017-04-28 17:15:00]

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-04-28 17:29:24]

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

[commit-bot: I haz the power, 2017-04-28 17:29:25]

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

[Seigo Nonaka, 2017-04-28 18:58:30]

lgtm from chromium.org account.

[kumarniranjan, 2017-04-28 18:59:14]

The CQ bit was checked by kumarniranjan@google.com

[commit-bot: I haz the power, 2017-04-28 19:00:48]

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-04-28 19:11:31]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493405954916780,
"parent_rev": "aa5f200c624aeb429e0d8084a11670feccf4f540", "commit_rev":
"cb477af46a98d9415f1491f613d43920003f4f86"}

[commit-bot: I haz the power, 2017-04-28 19:12:11]

Description was changed from

==========
Fix autoupdate_EndToEndTest on veyron_rialto

Adds a 3 min delay before starting zero-touch enrollment during OOBE.

BUG=chromium:710716
TEST=browser test
==========

to

==========
Fix autoupdate_EndToEndTest on veyron_rialto

Adds a 3 min delay before starting zero-touch enrollment during OOBE.

BUG=chromium:710716
TEST=browser test

Review-Url: https://codereview.chromium.org/2824283003
Cr-Commit-Position: refs/heads/master@{#468086}
Committed:
https://chromium.googlesource.com/chromium/src/+/cb477af46a98d9415f1491f613d4...
==========

[commit-bot: I haz the power, 2017-04-28 19:12:12]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/cb477af46a98d9415f1491f613d4...