Create OOBE screen for Waiting for Container ready

Create OOBE screen for Waiting for Container ready

- Add a new OOBE screen for waiting for container ready.
- The screen will be shown after user click Continue in the value prop
  screen.
- The screen will show a progress animation while container is
  initializing.
- The OOBE flow will end after container is ready or a one
  minute timeout is reached.
- A temporary 5 second delay is added to avoid potential racing issue.

BUG=b/62549680
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[yueli, 2017-06-16 23:40:15]

Description was changed from

==========
Create OOBE screen for Waiting for Container ready

- Add a new OOBE screen for waiting for container ready.
- The screen will be shown after user click Continue in the value prop
  screen.
- The screen will show a progress animation while container is
  initializing.
- The OOBE flow will end after container is ready or a one
  minute timeout is reached.
- A temporary 5 second delay is added to avoid potential racing issue.

BUG=b/62549680
==========

to

==========
Create OOBE screen for Waiting for Container ready

- Add a new OOBE screen for waiting for container ready.
- The screen will be shown after user click Continue in the value prop
  screen.
- The screen will show a progress animation while container is
  initializing.
- The OOBE flow will end after container is ready or a one
  minute timeout is reached.
- A temporary 5 second delay is added to avoid potential racing issue.

BUG=b/62549680
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[yueli, 2017-06-16 23:42:52]

updowndota@chromium.org changed reviewers:
+ xiaohuic@chromium.org, xiyuan@chromium.org

[yueli, 2017-06-16 23:42:52]

PTAL

[xc, 2017-06-17 00:35:22]

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
File
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc
(right):

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:39:
builder->Add("locale", g_browser_process->GetApplicationLocale());
where is locale used?

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:67:
screen_->OnContainerReady();
Why not return here and skip the timer and show screen?  Otherwise isn't the
timer going to override eachother?

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:77:
void WaitForContainerReadyScreenHandler::Hide() {}
why we don't handle hide?

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:101:
if (screen_)
add a todo for adding a histogram.

[xiyuan, 2017-06-19 16:36:02]

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

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/wait_for_container_ready_screen.h:30: void
OnContainerReady();
nit: Insert an emtpy line before since this is part of BaseScreen. And add a
brief comment for it.

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_wait_for_container_ready.js
(right):

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_wait_for_container_ready.js:16:
getElement: function(id) {
We should try to avoid this kind of method on a polymer element. This exposes
the impl details and we should only use it as a last resort. And this particular
one seems not used.

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
File
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc
(right):

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:16:
const char kJsScreenPath[] = "login.WaitForContainerReadyScreen";
nit: const -> constexpr

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:26:
arc::ArcSessionManager::Get()->AddObserver(this);
nit: is_container_ready_ = arc::ArcSessionManager::Get()->IsSessionRunning(),
just in case.

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:88:
base::Bind(&WaitForContainerReadyScreenHandler::OnTimeout,
nit: Suggest to have a NotifyContainerReady method and wire that instead of
OnTimeout.

[yueli, 2017-06-19 18:49:19]

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

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/wait_for_container_ready_screen.h:30: void
OnContainerReady();
On 2017/06/19 16:36:01, xiyuan wrote:
> nit: Insert an emtpy line before since this is part of BaseScreen. And add a
> brief comment for it.

Done.

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_wait_for_container_ready.js
(right):

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_wait_for_container_ready.js:16:
getElement: function(id) {
On 2017/06/19 16:36:02, xiyuan wrote:
> We should try to avoid this kind of method on a polymer element. This exposes
> the impl details and we should only use it as a last resort. And this
particular
> one seems not used.

Done.

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
File
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc
(right):

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:16:
const char kJsScreenPath[] = "login.WaitForContainerReadyScreen";
On 2017/06/19 16:36:02, xiyuan wrote:
> nit: const -> constexpr

Done.

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:26:
arc::ArcSessionManager::Get()->AddObserver(this);
On 2017/06/19 16:36:02, xiyuan wrote:
> nit: is_container_ready_ = arc::ArcSessionManager::Get()->IsSessionRunning(),
> just in case.

Done.

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:39:
builder->Add("locale", g_browser_process->GetApplicationLocale());
On 2017/06/17 00:35:22, xc wrote:
> where is locale used?

Done.

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:67:
screen_->OnContainerReady();
On 2017/06/17 00:35:21, xc wrote:
> Why not return here and skip the timer and show screen?  Otherwise isn't the
> timer going to override eachother?

Done.

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:88:
base::Bind(&WaitForContainerReadyScreenHandler::OnTimeout,
On 2017/06/19 16:36:02, xiyuan wrote:
> nit: Suggest to have a NotifyContainerReady method and wire that instead of
> OnTimeout.

Done.

https://codereview.chromium.org/2947483002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:101:
if (screen_)
On 2017/06/17 00:35:21, xc wrote:
> add a todo for adding a histogram.

Done.

[xiyuan, 2017-06-19 21:04:09]

https://codereview.chromium.org/2947483002/diff/20001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc
(right):

https://codereview.chromium.org/2947483002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:67:
screen_->OnContainerReady();
Call NotifyContainerReady so that we have one place to send back notification.

https://codereview.chromium.org/2947483002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:73:
base::Bind(&WaitForContainerReadyScreenHandler::NotifyContainerReady,
Sorry, I should have been more clear.

I would like to separate the two time outs. This one is the intended to have a
max wait time. We should keep the previous OnTimeOut (or use a better name, e.g.
OnMaxContainerWaitTimeOut) for this. And the 2nd timeout is to avoid ARC start
racing, it can be wired directly to NotifyContainerReady.

The purpose is to make the intention of code more clear.

[yueli, 2017-06-19 21:23:45]

https://codereview.chromium.org/2947483002/diff/20001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc
(right):

https://codereview.chromium.org/2947483002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:67:
screen_->OnContainerReady();
On 2017/06/19 21:04:09, xiyuan wrote:
> Call NotifyContainerReady so that we have one place to send back notification.

Done.

https://codereview.chromium.org/2947483002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/wait_for_container_ready_screen_handler.cc:73:
base::Bind(&WaitForContainerReadyScreenHandler::NotifyContainerReady,
On 2017/06/19 21:04:09, xiyuan wrote:
> Sorry, I should have been more clear.
> 
> I would like to separate the two time outs. This one is the intended to have a
> max wait time. We should keep the previous OnTimeOut (or use a better name,
e.g.
> OnMaxContainerWaitTimeOut) for this. And the 2nd timeout is to avoid ARC start
> racing, it can be wired directly to NotifyContainerReady.
> 
> The purpose is to make the intention of code more clear.

Oh! I get it, that make sense.

[xiyuan, 2017-06-19 21:29:33]

lgtm

Thanks.

[yueli, 2017-06-19 21:30:22]

The CQ bit was checked by updowndota@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 21:30:56]

Dry run: CQ is trying da patch.

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

[xiaohuic, 2017-06-19 22:21:47]

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

[xiaohuic, 2017-06-19 22:21:48]

lgtm

[commit-bot: I haz the power, 2017-06-19 23:09:27]

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

[commit-bot: I haz the power, 2017-06-19 23:09:29]

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

[yueli, 2017-06-19 23:18:23]

The CQ bit was checked by updowndota@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 23:18:47]

Dry run: 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-06-20 01:01:21]

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

[commit-bot: I haz the power, 2017-06-20 01:01:22]

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

[yueli, 2017-06-20 18:15:30]

The CQ bit was checked by updowndota@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 18:16:03]

Dry run: 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-06-20 20:10:06]

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

[commit-bot: I haz the power, 2017-06-20 20:10:08]

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

[yueli, 2017-06-20 21:07:01]

The CQ bit was checked by updowndota@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 21:07:35]

Dry run: 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-06-20 22:46:12]

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

[commit-bot: I haz the power, 2017-06-20 22:46:13]

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

[yueli, 2017-06-20 22:54:16]

The CQ bit was checked by updowndota@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 22:54:45]

Dry run: 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-06-20 23:43:02]

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

[commit-bot: I haz the power, 2017-06-20 23:43:03]

Dry run: This issue passed the CQ dry run.

[yueli, 2017-06-20 23:44:28]

The CQ bit was checked by updowndota@chromium.org

[yueli, 2017-06-20 23:44:28]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
xiaohuic@google.com
Link to the patchset: https://codereview.chromium.org/2947483002/#ps80001
(title: "Rebase and run js format")

[commit-bot: I haz the power, 2017-06-20 23:44:56]

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-06-20 23:50:21]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498002268052760,
"parent_rev": "a50819a86ab1ebb6c0c22b2f60c47119534c58a9", "commit_rev":
"c2e0203b34c677befb95b244de7cc4da07089497"}

[commit-bot: I haz the power, 2017-06-20 23:50:36]

Description was changed from

==========
Create OOBE screen for Waiting for Container ready

- Add a new OOBE screen for waiting for container ready.
- The screen will be shown after user click Continue in the value prop
  screen.
- The screen will show a progress animation while container is
  initializing.
- The OOBE flow will end after container is ready or a one
  minute timeout is reached.
- A temporary 5 second delay is added to avoid potential racing issue.

BUG=b/62549680
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Create OOBE screen for Waiting for Container ready

- Add a new OOBE screen for waiting for container ready.
- The screen will be shown after user click Continue in the value prop
  screen.
- The screen will show a progress animation while container is
  initializing.
- The OOBE flow will end after container is ready or a one
  minute timeout is reached.
- A temporary 5 second delay is added to avoid potential racing issue.

BUG=b/62549680
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2017-06-20 23:50:38]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c2e0203b34c677befb95b244de7c...