Create OOBE screen for Voice Interaction value proposition

Create OOBE screen for Voice Interaction value proposition

- Add a new OOBE screen for Voice Interaction value prop, which will
  show up after user image selection on the first run.
- If user clicks "No Thanks", chromeos tutorial app will be launched
- If user clicks "Continue", an intent to launch Voice Interaction
  setup wizard will be fired.

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

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

[yueli, 2017-05-27 00:57:06]

Description was changed from

==========
Create OOBE screen for Voice Interaction value proposition

- Add a new OOBE screen for Voice Interaction value prop, which will
  show up after user image selection on the first run.
- If user clicks "No Thanks", chromeos tutorial app will be launched
- If user clicks "Continue", an intent to launch Voice Interaction
  setup wizard will be fired.

BUG=b/38197377
==========

to

==========
Create OOBE screen for Voice Interaction value proposition

- Add a new OOBE screen for Voice Interaction value prop, which will
  show up after user image selection on the first run.
- If user clicks "No Thanks", chromeos tutorial app will be launched
- If user clicks "Continue", an intent to launch Voice Interaction
  setup wizard will be fired.

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

[yueli, 2017-05-27 00:58:07]

Description was changed from

==========
Create OOBE screen for Voice Interaction value proposition

- Add a new OOBE screen for Voice Interaction value prop, which will
  show up after user image selection on the first run.
- If user clicks "No Thanks", chromeos tutorial app will be launched
- If user clicks "Continue", an intent to launch Voice Interaction
  setup wizard will be fired.

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

to

==========
Create OOBE screen for Voice Interaction value proposition

- Add a new OOBE screen for Voice Interaction value prop, which will
  show up after user image selection on the first run.
- If user clicks "No Thanks", chromeos tutorial app will be launched
- If user clicks "Continue", an intent to launch Voice Interaction
  setup wizard will be fired.

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

[yueli, 2017-05-27 00:58:07]

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

[yueli, 2017-05-30 17:48:54]

PTAL

[Luis Héctor Chávez, 2017-05-30 17:59:55]

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

[Luis Héctor Chávez, 2017-05-30 17:59:56]

drive-by

also, is there any way you can split this change? it's kinda huge :(

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc
(right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:300:
DCHECK(framework_instance);
You shouldn't DCHECK here since there will be a few Chrome builds where this
will be nullptr since the container is not uprevved immediately. Change this to

if (!framework_instance)
  return;

this already does its own logging, so you don't even need to log.

[xiyuan, 2017-05-30 19:26:22]

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/fir...
File chrome/browser/chromeos/first_run/first_run.cc (right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/fir...
chrome/browser/chromeos/first_run/first_run.cc:51:
prefs::kArcVoiceInteractionValuePropAccepted))
How would this play along with MaybeLaunchDialogAfterSessionStart() and
MaybeLaunchDialogImmediately() ? I wonder whether we should do the skipping at
where those two calls are made instead of here.

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

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/voice_interaction_value_prop_screen_view.h:8:
#include <string>
nit: unused ?

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

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:1516: const Profile* profile
= ProfileManager::GetActiveUserProfile();
nit: Move this after IsUserLoggedIn() test.

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:1547: }
What if we show ARC terms but user declined it ? Think we should call
IsArcPlayStoreEnabledForProfile() to skip if user declined ARC terms.

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.css
(right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.css:11:
text-align: start;
Can the above 3 rules be removed ?

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.html
(right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.html:14:
<webview id="valueView"
src="https://www.gstatic.com/opa-chromeos/oobe/en/value_proposition.html"></webview>
nit: valueView -> value-id to follow existing style

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.html:14:
<webview id="valueView"
src="https://www.gstatic.com/opa-chromeos/oobe/en/value_proposition.html"></webview>
Think we need to
- use a proper url for locales other than en;
- handle load error;
- clicking on the links inside the webview to ensure it does not become a
browser;

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

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.js:18:
chrome.send('login.VoiceInteractionValuePropScreen.userActed',
Existing polymer elements are firing events to let the hosting screen js to do
the chrome.send. Should we follow that pattern ?

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

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.cc:63:
if (!page_is_ready() || !show_on_init_)
page_is_ready() is not necessary. It should always return true when Initialize()
is called.

We might need to test |screen_| though since it could be nullptr when
Initialize() is called.

[xc, 2017-05-31 18:37:53]

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/fir...
File chrome/browser/chromeos/first_run/first_run.cc (right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/fir...
chrome/browser/chromeos/first_run/first_run.cc:51:
prefs::kArcVoiceInteractionValuePropAccepted))
+1 We may have to show this more than once even if user accepted it, e.g. user
accepted the value prop but declined the UDC later.

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

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:847: OnOobeFlowFinished();
I am not sure we want to call this here.  It would start the browser and at the
same time with starting voice interaction suw.

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:867:
UMA_HISTOGRAM_CUSTOM_TIMES("OOBE.BootToSignInCompleted", delta,
Not sure what the original definition of "OOBE.BootToSignInCompleted" histogram.
 Can you double check the comments in histogram xml and see if we have changed
the meaning?

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:1547: }
On 2017/05/30 19:26:21, xiyuan wrote:
> What if we show ARC terms but user declined it ? Think we should call
> IsArcPlayStoreEnabledForProfile() to skip if user declined ARC terms.

+1 IsArcPlayStoreEnabledForProfile() check should replace ShouldShowArcTerms()
check.

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.css
(right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.css:1:
/* Copyright 2013 The Chromium Authors. All rights reserved.
wrong date

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.html
(right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.html:1:
<div class="step faded hidden no-logo" id="voice-interaction-value-prop"
copyright header

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.html:2:
role="group" i18n-values="aria-label:resetScreenAccessibleTitle">
wrong label?

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

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.js:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
wrong date, also I think the new style of copy right header does not have (c)

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.html
(right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.html:14:
<webview id="valueView"
src="https://www.gstatic.com/opa-chromeos/oobe/en/value_proposition.html"></webview>
> - use a proper url for locales other than en;
Yue should be replacing "en" with proper locales.  But is there some conventions
to handle i18n for gstatic? currently the other team provided only language
specific directories

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

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.js:12:
/**
nit: newline above

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/ui/webui/chr...
File
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.h
(right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
date, and some others too.

[yueli, 2017-06-02 02:03:47]

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc
(right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:300:
DCHECK(framework_instance);
On 2017/05/30 17:59:55, Luis Héctor Chávez wrote:
> You shouldn't DCHECK here since there will be a few Chrome builds where this
> will be nullptr since the container is not uprevved immediately. Change this
to
> 
> if (!framework_instance)
>   return;
> 
> this already does its own logging, so you don't even need to log.

Done.

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/fir...
File chrome/browser/chromeos/first_run/first_run.cc (right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/fir...
chrome/browser/chromeos/first_run/first_run.cc:51:
prefs::kArcVoiceInteractionValuePropAccepted))
On 2017/05/31 18:37:52, xc wrote:
> +1 We may have to show this more than once even if user accepted it, e.g. user
> accepted the value prop but declined the UDC later. 

Done.

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

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/voice_interaction_value_prop_screen_view.h:8:
#include <string>
On 2017/05/30 19:26:21, xiyuan wrote:
> nit: unused ?

Done.

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

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:867:
UMA_HISTOGRAM_CUSTOM_TIMES("OOBE.BootToSignInCompleted", delta,
On 2017/05/31 18:37:52, xc wrote:
> Not sure what the original definition of "OOBE.BootToSignInCompleted"
histogram.
>  Can you double check the comments in histogram xml and see if we have changed
> the meaning?

Seems it log the time from boot to sign-in completed. And the only use case is
here

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:1516: const Profile* profile
= ProfileManager::GetActiveUserProfile();
On 2017/05/30 19:26:21, xiyuan wrote:
> nit: Move this after IsUserLoggedIn() test.

Done.

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:1547: }
On 2017/05/31 18:37:52, xc wrote:
> On 2017/05/30 19:26:21, xiyuan wrote:
> > What if we show ARC terms but user declined it ? Think we should call
> > IsArcPlayStoreEnabledForProfile() to skip if user declined ARC terms.
> 
> +1 IsArcPlayStoreEnabledForProfile() check should replace ShouldShowArcTerms()
> check.

Done.

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.css
(right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.css:1:
/* Copyright 2013 The Chromium Authors. All rights reserved.
On 2017/05/31 18:37:52, xc wrote:
> wrong date

Done.

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.css:11:
text-align: start;
On 2017/05/30 19:26:22, xiyuan wrote:
> Can the above 3 rules be removed ?

Done.

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.html
(right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.html:1:
<div class="step faded hidden no-logo" id="voice-interaction-value-prop"
On 2017/05/31 18:37:52, xc wrote:
> copyright header

Done.

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.html:2:
role="group" i18n-values="aria-label:resetScreenAccessibleTitle">
On 2017/05/31 18:37:52, xc wrote:
> wrong label?

Done.

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

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.js:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2017/05/31 18:37:52, xc wrote:
> wrong date, also I think the new style of copy right header does not have (c)

Done.

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.html
(right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.html:14:
<webview id="valueView"
src="https://www.gstatic.com/opa-chromeos/oobe/en/value_proposition.html"></webview>
On 2017/05/30 19:26:22, xiyuan wrote:
> nit: valueView -> value-id to follow existing style

Done.

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.html:14:
<webview id="valueView"
src="https://www.gstatic.com/opa-chromeos/oobe/en/value_proposition.html"></webview>
On 2017/05/30 19:26:22, xiyuan wrote:
> Think we need to
> - use a proper url for locales other than en;
> - handle load error;
> - clicking on the links inside the webview to ensure it does not become a
> browser;

How should I disable links in webview?

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

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.js:12:
/**
On 2017/05/31 18:37:52, xc wrote:
> nit: newline above

Done.

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.js:18:
chrome.send('login.VoiceInteractionValuePropScreen.userActed',
On 2017/05/30 19:26:22, xiyuan wrote:
> Existing polymer elements are firing events to let the hosting screen js to do
> the chrome.send. Should we follow that pattern ?

Seems the use pattern is mixed -- some polymer elements directly call
chrome.send such as oobe_reset.js and oobe_eula.js

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

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.cc:63:
if (!page_is_ready() || !show_on_init_)
On 2017/05/30 19:26:22, xiyuan wrote:
> page_is_ready() is not necessary. It should always return true when
Initialize()
> is called.
> 
> We might need to test |screen_| though since it could be nullptr when
> Initialize() is called.

Done.

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/ui/webui/chr...
File
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.h
(right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/05/31 18:37:53, xc wrote:
> date, and some others too.

Done.

[xc, 2017-06-02 18:53:49]

lgtm

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.js
(right):

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.js:24:
'https://www.gstatic.com/opa-chromeos/oobe/en/value_proposition.html';
if the "en" url were also 404, which should never happen, this is going to be an
infinite loop, right?  Let's check for that and show error for now.  Add a TODO
that we will provide some static content later for the final fallback.

[xiyuan, 2017-06-02 22:12:34]

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.html
(right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.html:14:
<webview id="valueView"
src="https://www.gstatic.com/opa-chromeos/oobe/en/value_proposition.html"></webview>
On 2017/06/02 02:03:47, yueli wrote:
> On 2017/05/30 19:26:22, xiyuan wrote:
> > Think we need to
> > - use a proper url for locales other than en;
> > - handle load error;
> > - clicking on the links inside the webview to ensure it does not become a
> > browser;
> 
> How should I disable links in webview?

webview.addContentScripts can inject script that runs in the loaded document.
For now, we can do a simple stripping after document is loaded.

Something like:

webview.addContentScripts([
  {
    name: 'stripLinks',
    matches: ['<all_urls>'],
    js: { code:
"document.querySelectorAll('a').forEach(function(anchor){anchor.href='javascript:void(0)';})"
},
    run_at: 'document_end'
  }]);

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

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.js:18:
chrome.send('login.VoiceInteractionValuePropScreen.userActed',
On 2017/06/02 02:03:47, yueli wrote:
> On 2017/05/30 19:26:22, xiyuan wrote:
> > Existing polymer elements are firing events to let the hosting screen js to
do
> > the chrome.send. Should we follow that pattern ?
> 
> Seems the use pattern is mixed -- some polymer elements directly call
> chrome.send such as oobe_reset.js and oobe_eula.js

I would argue that chrome.send and the messages sent are kind of anti polymer
pattern and makes the element not really re-usable.

But I guess we will never reuse it anywhere else. So this is optional and you
can leave as-is.

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.js
(right):

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.js:16:
*/
nit: This can be just 
  /** @override */

for overridden mthods.

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.js:21:
valueView.request.onHeadersReceived.addListener(function(details) {
Should we use onErrorOccurred to catch all errors ?

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.js:22:
if (details.statusLine.match(/^HTTP.*404/)) {
details.statusCode instead of manually parsing. And we only care of 404 ?

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.js:24:
'https://www.gstatic.com/opa-chromeos/oobe/en/value_proposition.html';
On 2017/06/02 18:53:48, xc wrote:
> if the "en" url were also 404, which should never happen, this is going to be
an
> infinite loop, right?  Let's check for that and show error for now.  Add a
TODO
> that we will provide some static content later for the final fallback.

Good point. Please do. The simplest way is to check valueView.src and only set
to "en" url if it is not.

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.js:26:
}, {urls: ['<all_urls>']});
Think we only care about main frame loading problems. We should add
  types: ['main_frame']
to the filter. Otherwise, subframe or image loading failure would cause the
webview to reload.

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

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.cc:65:
if (!screen_ || !show_on_init_)
We would need to touch a couple of more places:

1. In VoiceInteractionValuePropScreenHandler::Bind(), we should add the
following at the end:

  if (page_is_ready())
    Initialize()

so that skipped Show() would be handled.

2. Add |screen_| check in VoiceInteractionValuePropScreenHandler::Show(),
i.e. 
  if (!page_is_ready()) ->
  if (!page_is_ready() || !screen_) {

[dcheng, 2017-06-05 19:38:43]

mojom lgtm

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/voice_interaction_value_prop_screen.h
(right):

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/voice_interaction_value_prop_screen.h:25:
void OnViewDestroyed(VoiceInteractionValuePropScreenView* view_);
Nit: |view_| should be named |view|

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:632: VLOG(1) << "Showing
voice interaction value prop screen.";
Consider using DVLOG() in this file, it lets us drop the strings in official
builds. Ditto for all the other VLOGs

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:877:
base::Unretained(UserSessionManager::GetInstance()),
Nit: it's usually nice to document why this it's safe to use Unretained here
(maybe this is self-documenting enough for people who are familiar with the code
though?) *shrug*

[yueli, 2017-06-06 17:38:20]

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:632: VLOG(1) << "Showing
voice interaction value prop screen.";
On 2017/06/05 19:38:42, dcheng wrote:
> Consider using DVLOG() in this file, it lets us drop the strings in official
> builds. Ditto for all the other VLOGs

I use VLOG() since this file seems to use it everywhere. So did you mean we
should change all these VLOG() to DVLOG()?

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:877:
base::Unretained(UserSessionManager::GetInstance()),
On 2017/06/05 19:38:42, dcheng wrote:
> Nit: it's usually nice to document why this it's safe to use Unretained here
> (maybe this is self-documenting enough for people who are familiar with the
code
> though?) *shrug*

This code block is refactored from line 766-783 in the original file. So
actually I did not know the reason.

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.js
(right):

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.js:16:
*/
On 2017/06/02 22:12:33, xiyuan wrote:
> nit: This can be just 
>   /** @override */
> 
> for overridden mthods.

Done.

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.js:24:
'https://www.gstatic.com/opa-chromeos/oobe/en/value_proposition.html';
On 2017/06/02 22:12:34, xiyuan wrote:
> On 2017/06/02 18:53:48, xc wrote:
> > if the "en" url were also 404, which should never happen, this is going to
be
> an
> > infinite loop, right?  Let's check for that and show error for now.  Add a
> TODO
> > that we will provide some static content later for the final fallback.
> 
> Good point. Please do. The simplest way is to check valueView.src and only set
> to "en" url if it is not.

Done.

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_voice_interaction_value_prop.js:26:
}, {urls: ['<all_urls>']});
On 2017/06/02 22:12:33, xiyuan wrote:
> Think we only care about main frame loading problems. We should add
>   types: ['main_frame']
> to the filter. Otherwise, subframe or image loading failure would cause the
> webview to reload.

Done.

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

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.cc:65:
if (!screen_ || !show_on_init_)
On 2017/06/02 22:12:34, xiyuan wrote:
> We would need to touch a couple of more places:
> 
> 1. In VoiceInteractionValuePropScreenHandler::Bind(), we should add the
> following at the end:
> 
>   if (page_is_ready())
>     Initialize()
> 
> so that skipped Show() would be handled.
> 
> 2. Add |screen_| check in VoiceInteractionValuePropScreenHandler::Show(),
> i.e. 
>   if (!page_is_ready()) ->
>   if (!page_is_ready() || !screen_) {

Done.

[Luis Héctor Chávez, 2017-06-06 17:55:40]

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2912593002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:877:
base::Unretained(UserSessionManager::GetInstance()),
On 2017/06/06 17:38:19, yueli wrote:
> On 2017/06/05 19:38:42, dcheng wrote:
> > Nit: it's usually nice to document why this it's safe to use Unretained here
> > (maybe this is self-documenting enough for people who are familiar with the
> code
> > though?) *shrug*
> 
> This code block is refactored from line 766-783 in the original file. So
> actually I did not know the reason.

This is safe because it's a Singleton and outlives all threads.

[yueli, 2017-06-07 18:31:50]

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.html
(right):

https://codereview.chromium.org/2912593002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_voice_interaction_value_prop.html:14:
<webview id="valueView"
src="https://www.gstatic.com/opa-chromeos/oobe/en/value_proposition.html"></webview>
On 2017/06/02 22:12:33, xiyuan wrote:
> On 2017/06/02 02:03:47, yueli wrote:
> > On 2017/05/30 19:26:22, xiyuan wrote:
> > > Think we need to
> > > - use a proper url for locales other than en;
> > > - handle load error;
> > > - clicking on the links inside the webview to ensure it does not become a
> > > browser;
> > 
> > How should I disable links in webview?
> 
> webview.addContentScripts can inject script that runs in the loaded document.
> For now, we can do a simple stripping after document is loaded.
> 
> Something like:
> 
> webview.addContentScripts([
>   {
>     name: 'stripLinks',
>     matches: ['<all_urls>'],
>     js: { code:
>
"document.querySelectorAll('a').forEach(function(anchor){anchor.href='javascript:void(0)';})"
> },
>     run_at: 'document_end'
>   }]);

Done.

[xiyuan, 2017-06-07 18:45:06]

https://codereview.chromium.org/2912593002/diff/60001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.cc
(right):

https://codereview.chromium.org/2912593002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.cc:45:
Initialize();
Think we should reverse the call order. At least, move this after setting
|screen_|.

I would recommend calling super class methods first.

So something like this:
  BaseScreenHandler::SetBaseScreen(screen_);
  screen_ = screen;
  if (page_is_ready())
    Initialize();

[yueli, 2017-06-07 18:52:42]

https://codereview.chromium.org/2912593002/diff/60001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.cc
(right):

https://codereview.chromium.org/2912593002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.cc:45:
Initialize();
On 2017/06/07 18:45:06, xiyuan wrote:
> Think we should reverse the call order. At least, move this after setting
> |screen_|.
> 
> I would recommend calling super class methods first.
> 
> So something like this:
>   BaseScreenHandler::SetBaseScreen(screen_);
>   screen_ = screen;
>   if (page_is_ready())
>     Initialize();

Did you mean SetBaseScreen(screen)? Otherwise we may set it to nullptr.

[xiyuan, 2017-06-07 19:04:20]

https://codereview.chromium.org/2912593002/diff/60001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.cc
(right):

https://codereview.chromium.org/2912593002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.cc:45:
Initialize();
On 2017/06/07 18:52:42, yueli wrote:
> On 2017/06/07 18:45:06, xiyuan wrote:
> > Think we should reverse the call order. At least, move this after setting
> > |screen_|.
> > 
> > I would recommend calling super class methods first.
> > 
> > So something like this:
> >   BaseScreenHandler::SetBaseScreen(screen_);
> >   screen_ = screen;
> >   if (page_is_ready())
> >     Initialize();
> 
> Did you mean SetBaseScreen(screen)? Otherwise we may set it to nullptr.

Correct. My bad... My finger is faster than my brain.

[yueli, 2017-06-07 19:17:35]

https://codereview.chromium.org/2912593002/diff/60001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.cc
(right):

https://codereview.chromium.org/2912593002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/voice_interaction_value_prop_screen_handler.cc:45:
Initialize();
On 2017/06/07 18:45:06, xiyuan wrote:
> Think we should reverse the call order. At least, move this after setting
> |screen_|.
> 
> I would recommend calling super class methods first.
> 
> So something like this:
>   BaseScreenHandler::SetBaseScreen(screen_);
>   screen_ = screen;
>   if (page_is_ready())
>     Initialize();

Done.

[xiyuan, 2017-06-07 19:39:53]

lgtm

[yueli, 2017-06-07 20:10:48]

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

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

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-07 20:15:24]

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

[commit-bot: I haz the power, 2017-06-07 20:15:26]

Dry run: Try jobs failed on following builders:
  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...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[yueli, 2017-06-07 20:50:59]

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

[commit-bot: I haz the power, 2017-06-07 20:51: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-07 20:52:18]

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

[commit-bot: I haz the power, 2017-06-07 20:52:19]

Dry run: CQ cannot get SignCLA result. Please try later.

[yueli, 2017-06-07 21:22:02]

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

[commit-bot: I haz the power, 2017-06-07 21:22:57]

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-07 21:38:59]

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

[commit-bot: I haz the power, 2017-06-07 21:39:01]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[yueli, 2017-06-07 23:33:49]

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

[commit-bot: I haz the power, 2017-06-07 23:34:26]

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-08 01:36:50]

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

[commit-bot: I haz the power, 2017-06-08 01:36:52]

Dry run: This issue passed the CQ dry run.

[yueli, 2017-06-08 02:04:27]

The CQ bit was checked by updowndota@chromium.org

[yueli, 2017-06-08 02:04:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
xiaohuic@chromium.org, dcheng@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2912593002/#ps120001
(title: "Create OOBE screen for Voice Interaction value proposition")

[commit-bot: I haz the power, 2017-06-08 02:04:49]

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-08 02:11:22]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496887467290250,
"parent_rev": "cb098170439737719a7f7e9ae825c6365027523e", "commit_rev":
"ec498067cc5b7e656957722e4c7a45d696164b64"}

[commit-bot: I haz the power, 2017-06-08 02:11:37]

Description was changed from

==========
Create OOBE screen for Voice Interaction value proposition

- Add a new OOBE screen for Voice Interaction value prop, which will
  show up after user image selection on the first run.
- If user clicks "No Thanks", chromeos tutorial app will be launched
- If user clicks "Continue", an intent to launch Voice Interaction
  setup wizard will be fired.

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

to

==========
Create OOBE screen for Voice Interaction value proposition

- Add a new OOBE screen for Voice Interaction value prop, which will
  show up after user image selection on the first run.
- If user clicks "No Thanks", chromeos tutorial app will be launched
- If user clicks "Continue", an intent to launch Voice Interaction
  setup wizard will be fired.

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

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

[commit-bot: I haz the power, 2017-06-08 02:11:40]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ec498067cc5b7e656957722e4c7a...