kiosk: Network connectivity test during launch.

kiosk: Network connectivity test during launch.

- Add NetworkStateInformer and ErrorScreenActor to
  AppLaunchSplashScreenHandler;
- AppLaunchSplashScreenHandler sends network state
  to its delegate (AppLaunchController) during network
  check;
- AppLaunchController decides when to show network
  configure UI and call AppLaunchSplashScreenActor
  to show it (via ErrorScreenActor);
- Show network config UI for enterprise kiosk;
- Add a 'Reboot' button for kiosk network error;
- Add a PromptForNetworkWhenOffline boolean field to
  DeviceLocalAccountsProto to control whether to show
  network config UI for enterprise managed kiosk;

BUG=314710
TEST=KioskTest.LaunchAppNetworkDown/Portal

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238873

[zel, 2013-11-20 19:21:20]

I would also add a policy for letting kiosk users mess with network settings
(default: enabled)

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/app_launch_utils.cc (right):

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/app_launch_utils.cc:40:
startup_app_launcher_->ContinueWithNetworkReady();
what if we crashed before we managed to connect to the network?

[xiyuan, 2013-11-20 21:41:21]

On 2013/11/20 19:21:20, zel wrote:
> I would also add a policy for letting kiosk users mess with network settings
> (default: enabled)
> 

There is already http://crbug.com/241958 for having a device policy to control
that. Not sure about the status of that though.

>
https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/a...
> File chrome/browser/chromeos/app_mode/app_launch_utils.cc (right):
> 
> startup_app_launcher_->ContinueWithNetworkReady();
> what if we crashed before we managed to connect to the network?

We could move wait here for the online signal. It would be a little tricky to
bring up the login screen to show a UI. Would that be okay?

[Tim Song, 2013-11-21 01:12:16]

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/app_launch_utils.cc (right):

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/app_launch_utils.cc:40:
startup_app_launcher_->ContinueWithNetworkReady();
On 2013/11/20 19:21:21, zel wrote:
> what if we crashed before we managed to connect to the network?

When we are waiting on the network in the splash screen, the kiosk user is
logged in but the session is not started. What will crash recovery do in this
case?

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/startup_app_launcher.h (right):

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/startup_app_launcher.h:42: virtual void
OnInitializingNetwork() = 0;
Please add a comment for this function that StartupAppLauncher will block until
ContinueWithNetworkReady() is called.

It might also be a good idea to rename the observer to delegate and
OnInitializingNetwork to InitializeNetwork because we are now relying on the
this class for functionality.

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/app_launch_controller.cc (right):

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/app_launch_controller.cc:198: if (online)
If the network comes up automatically when the error screen is being shown,
OnNetworkStateChanged(true) will still be called, right? In this case, we need
to dismiss the error screen if it's still there.

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/app_launch_controller.cc:244: bool
AppLaunchController::CanConfigureNetwork() {
Won't this function always return true now?

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/app_launch_controller.cc:263: } else {
indent

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/app_launch_controller.cc:265:
app_launch_splash_screen_actor_->ShowNetworkConfigureUI();
Do we want to show the network UI immediately? One of the reasons for showing
the "Configure Network" button is so we can have a more aggressive time out
without severely disrupting the flow of the splash screen.

[xiyuan, 2013-11-21 22:39:39]

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/app_launch_utils.cc (right):

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/app_launch_utils.cc:40:
startup_app_launcher_->ContinueWithNetworkReady();
On 2013/11/21 01:12:17, Tim Song wrote:
> On 2013/11/20 19:21:21, zel wrote:
> > what if we crashed before we managed to connect to the network?
> 
> When we are waiting on the network in the splash screen, the kiosk user is
> logged in but the session is not started. What will crash recovery do in this
> case?

Talked to zel and we probably would get rid of the crash-restart path and always
restart the whole session for kiosk. Filed http://crbug.com/321858.

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/startup_app_launcher.h (right):

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/startup_app_launcher.h:42: virtual void
OnInitializingNetwork() = 0;
On 2013/11/21 01:12:17, Tim Song wrote:
> Please add a comment for this function that StartupAppLauncher will block
until
> ContinueWithNetworkReady() is called.
> 
> It might also be a good idea to rename the observer to delegate and
> OnInitializingNetwork to InitializeNetwork because we are now relying on the
> this class for functionality.

Done.
- OnInitializingNetwork -> InitializeNetwork and added comment.
- Observer -> Delegate

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/app_launch_controller.cc (right):

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/app_launch_controller.cc:198: if (online)
On 2013/11/21 01:12:17, Tim Song wrote:
> If the network comes up automatically when the error screen is being shown,
> OnNetworkStateChanged(true) will still be called, right? In this case, we need
> to dismiss the error screen if it's still there.

Yes, this will be called when network state changes, regardless of whether we
are showing the error screen. |showing_network_dialog_| is set to true when
error screen (or owner signin screen) is shown. And it will be dismissed in
AppLaunchController::OnInstallingApp.

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/app_launch_controller.cc:244: bool
AppLaunchController::CanConfigureNetwork() {
On 2013/11/21 01:12:17, Tim Song wrote:
> Won't this function always return true now?

Nope, it returns false for consumer kiosk mode without an owner. I'll add a
comment to wire with device policy when its ready here.

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/app_launch_controller.cc:263: } else {
On 2013/11/21 01:12:17, Tim Song wrote:
> indent

Done.

https://codereview.chromium.org/79113002/diff/30001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/app_launch_controller.cc:265:
app_launch_splash_screen_actor_->ShowNetworkConfigureUI();
On 2013/11/21 01:12:17, Tim Song wrote:
> Do we want to show the network UI immediately? One of the reasons for showing
> the "Configure Network" button is so we can have a more aggressive time out
> without severely disrupting the flow of the splash screen.

I can understand not showing up the owner signin screen automatically because
that would feel strange. But for error screen, I don't think it's too
disruptive. It's actually better because it saves user one click.

[Tim Song, 2013-11-22 18:47:44]

lgtm

[xiyuan, 2013-12-03 23:37:28]

Mattias, could you help review the policy change part?

chrome/app/policy/policy_templates.json
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc
chrome/browser/chromeos/settings/device_settings_provider.cc
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto

and AppLaunchController::CanConfigureNetwork where the policy is checked in
chrome/browser/chromeos/login/app_launch_controller.cc

Thanks.

[Mattias Nissler (ping if slow), 2013-12-04 10:15:07]

Enterprise stuff LGTM w/ nit, adding Joao for policy_templates.json

https://codereview.chromium.org/79113002/diff/340001/chrome/browser/policy/pr...
File chrome/browser/policy/proto/chromeos/chrome_device_policy.proto (right):

https://codereview.chromium.org/79113002/diff/340001/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:320: // Whether
to network configuration should be offered or not when the device
remove stray "to"

[Joao da Silva, 2013-12-04 10:45:19]

policy_templates.json looks good but I'd like to understand why some of the
public accounts policies are tagged 'future'.

https://codereview.chromium.org/79113002/diff/340001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/79113002/diff/340001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:652: Reboot
FWIW the button to restart after an update on the help page uses the label
"Restart" (IDS_RELAUNCH_BUTTON). How about using the same resource ID for
consistency? Or at least the same string?

https://codereview.chromium.org/79113002/diff/340001/chrome/app/policy/policy...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/79113002/diff/340001/chrome/app/policy/policy...
chrome/app/policy/policy_templates.json:3889: 'future': True,
Mattias, why is this?

https://codereview.chromium.org/79113002/diff/340001/chrome/app/policy/policy...
chrome/app/policy/policy_templates.json:3949: 'future': True,
Mattias, why is this?

https://codereview.chromium.org/79113002/diff/340001/chrome/app/policy/policy...
chrome/app/policy/policy_templates.json:3968: 'future': True,
I guess this was copied from the policy above, but I don't see why it's used.

[Mattias Nissler (ping if slow), 2013-12-04 11:33:34]

https://codereview.chromium.org/79113002/diff/340001/chrome/app/policy/policy...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/79113002/diff/340001/chrome/app/policy/policy...
chrome/app/policy/policy_templates.json:3889: 'future': True,
On 2013/12/04 10:45:19, Joao da Silva wrote:
> Mattias, why is this?

Should have been removed, but I forgot to do so. Xiyan, can you just remove this
line?

https://codereview.chromium.org/79113002/diff/340001/chrome/app/policy/policy...
chrome/app/policy/policy_templates.json:3949: 'future': True,
On 2013/12/04 10:45:19, Joao da Silva wrote:
> Mattias, why is this?

ditto

https://codereview.chromium.org/79113002/diff/340001/chrome/app/policy/policy...
chrome/app/policy/policy_templates.json:3968: 'future': True,
On 2013/12/04 10:45:19, Joao da Silva wrote:
> I guess this was copied from the policy above, but I don't see why it's used.

ditto, should be removed.

[Joao da Silva, 2013-12-04 12:42:49]

OK, policy_templates.json lgtm after removing the 'future' keys from the public
accounts policies.

Please consider the comment about the string too; I don't feel strongly about it
but I think it's good to aim for consistency in the UI.

[xiyuan, 2013-12-04 18:24:59]

https://codereview.chromium.org/79113002/diff/340001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/79113002/diff/340001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:652: Reboot
On 2013/12/04 10:45:19, Joao da Silva wrote:
> FWIW the button to restart after an update on the help page uses the label
> "Restart" (IDS_RELAUNCH_BUTTON). How about using the same resource ID for
> consistency? Or at least the same string?

Removed and use IDS_RELAUNCH_BUTTON for the button label.

https://codereview.chromium.org/79113002/diff/340001/chrome/app/policy/policy...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/79113002/diff/340001/chrome/app/policy/policy...
chrome/app/policy/policy_templates.json:3889: 'future': True,
On 2013/12/04 11:33:35, Mattias Nissler wrote:
> On 2013/12/04 10:45:19, Joao da Silva wrote:
> > Mattias, why is this?
> 
> Should have been removed, but I forgot to do so. Xiyan, can you just remove
this
> line?

Done.

https://codereview.chromium.org/79113002/diff/340001/chrome/app/policy/policy...
chrome/app/policy/policy_templates.json:3949: 'future': True,
On 2013/12/04 11:33:35, Mattias Nissler wrote:
> On 2013/12/04 10:45:19, Joao da Silva wrote:
> > Mattias, why is this?
> 
> ditto

Done.

https://codereview.chromium.org/79113002/diff/340001/chrome/app/policy/policy...
chrome/app/policy/policy_templates.json:3968: 'future': True,
On 2013/12/04 11:33:35, Mattias Nissler wrote:
> On 2013/12/04 10:45:19, Joao da Silva wrote:
> > I guess this was copied from the policy above, but I don't see why it's
used.
> 
> ditto, should be removed.

Done.

https://codereview.chromium.org/79113002/diff/340001/chrome/browser/policy/pr...
File chrome/browser/policy/proto/chromeos/chrome_device_policy.proto (right):

https://codereview.chromium.org/79113002/diff/340001/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:320: // Whether
to network configuration should be offered or not when the device
On 2013/12/04 10:15:07, Mattias Nissler wrote:
> remove stray "to"

Done.

[xiyuan, 2013-12-04 19:10:45]

asvitkine@, could you help with a owner's stamp on histograms.xml?

zel@, also need your stamp on /chromeos/* files.

Thanks.

[Alexei Svitkine (slow), 2013-12-04 19:12:30]

histograms lgtm

[zel, 2013-12-04 19:57:32]

lgtm

[commit-bot: I haz the power, 2013-12-04 22:11:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/79113002/380001

[commit-bot: I haz the power, 2013-12-04 22:18:45]

Retried try job too often on mac_rel for step(s) app_list_unittests,
cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
google_apis_unittests, gpu_unittests, ipc_tests, jingle_unittests,
media_unittests, message_center_unittests, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-04 23:16:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/79113002/380001

[commit-bot: I haz the power, 2013-12-05 03:26:02]

Change committed as 238873