Added UI to enable debugging features on CrOS

Added UI to enable debugging features on CrOS

BUG=403165
TEST=EnableDebuggingNonDevTest.NoShowInNonDevMode, EnableDebuggingTest.ShowAndCancelRemoveProtection, EnableDebuggingTest.ShowAndRemoveProtection, EnableDebuggingTest.ShowSetup, WizardControllerEnableDebuggingTest.ShowAndCancelEnableDebugging
TBR=mnissler

Committed: https://crrev.com/459b11b10f0f18c4111bcfa973be5244171f1412
Cr-Commit-Position: refs/heads/master@{#304161}

[zel, 2014-10-30 02:09:49]

zelidrag@chromium.org changed reviewers:
+ nkostylev@chromium.org, xiyuan@chromium.org

[xiyuan, 2014-10-30 03:52:09]

LGTM with nits

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/enable_debugging_screen.h (right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/enable_debugging_screen.h:11: #include
"base/compiler_specific.h"
nit: Include "base/macros.h" for DISALLOW_COPY_AND_ASSIGN or just get rid of
this since basictypes.h include macros.h.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/enable_debugging_screen.h:17: //
Representation independent class that controls screen showing reset to users.
nit: update this comment?

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/enable_debugging_screen_actor.h
(right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/enable_debugging_screen_actor.h:12: //
Interface between reset screen and its representation.
nit: Update comment.

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

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:483: 
nit: nuke one empty line.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.html
(right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.html:25:
i18n-content="enableDebuggingPasswordLengthNote">
nit: shift left 2 spaces.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js
(right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:35:
'keyup', this.onPasswordChanged_.bind(this));
nit: There is a 'input' event for <input> and <textarea> that we can use. It is
fired after the value of the element is changed.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:134:
$('enable-debugging-help-link').setAttribute('hidden', true);
nit: we can just do 
$('enable-debugging-help-link').hidden = true;

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:149: *
@param {string} state.
nit: |state| seems to be an number.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:168: }
nit: we could use classList.toggle to simplify a bit.

setDialogView_: function(state) {
  this.state_ = state;
  this.classList.toggle('remove-protection-view', state ==
this.UI_STATE.REMOVE_PROTECTION);
  this.classList.toggle('setup-view', state == this.UI_STATE.SETUP);
  this.classList.toggle('wait-view', state == this.UI_STATE.WAIT);
  this.classList.toggle('done-view', state == this.UI_STATE.DONE);
}

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:44: if
(event.keyCode == 32)
Seems like we need a button that responds to space key rather than a link. We
used to have css to make a button looks like a link but somehow I am no longer
able to find it. :(

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:54:
this.classList.add('connect-debugging-view');
nit: this.classList.toggle('connect-debugging-view', 'isDeveloperMode' in data
&& data['isDeveloperMode']);

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.css (right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_reset.css:38:
.enable-debugging-text {
Seems unintentional. revert?

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc
(right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:10:
#include "base/metrics/histogram.h"
nit: seems not used.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:16:
#include "chrome/browser/chromeos/reset/metrics.h"
nit: seems not needed.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:25:
#include "chromeos/dbus/session_manager_client.h"
nit: seems not used

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:26:
#include "content/public/browser/browser_thread.h"
nit: seems not used.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:32: //
Reset screen id.
nit: update comment or just get rid of it.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:57:
prefs->SetBoolean(prefs::kDebuggingFeaturesRequested, false);
nit: how about call ClearPref()?

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.h
(right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.h:14:
#include "content/public/browser/web_ui.h"
nit: seems not used?

https://codereview.chromium.org/539273002/diff/100001/chromeos/dbus/debug_dae...
File chromeos/dbus/debug_daemon_client.cc (right):

https://codereview.chromium.org/539273002/diff/100001/chromeos/dbus/debug_dae...
chromeos/dbus/debug_daemon_client.cc:43: }
nit: }  // namespace debugd

https://codereview.chromium.org/539273002/diff/100001/chromeos/dbus/debug_dae...
chromeos/dbus/debug_daemon_client.cc:328: virtual void RemoveRootfsVerification(
nit: move this before EnableDebuggingFeatures to match the order in header.

[zel, 2014-10-31 01:22:27]

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/enable_debugging_screen.h (right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/enable_debugging_screen.h:11: #include
"base/compiler_specific.h"
On 2014/10/30 03:52:08, xiyuan wrote:
> nit: Include "base/macros.h" for DISALLOW_COPY_AND_ASSIGN or just get rid of
> this since basictypes.h include macros.h.

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/enable_debugging_screen.h:17: //
Representation independent class that controls screen showing reset to users.
On 2014/10/30 03:52:08, xiyuan wrote:
> nit: update this comment?

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/enable_debugging_screen_actor.h
(right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/enable_debugging_screen_actor.h:12: //
Interface between reset screen and its representation.
On 2014/10/30 03:52:08, xiyuan wrote:
> nit: Update comment.

Done.

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

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:483: 
On 2014/10/30 03:52:08, xiyuan wrote:
> nit: nuke one empty line.

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.html
(right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.html:25:
i18n-content="enableDebuggingPasswordLengthNote">
On 2014/10/30 03:52:08, xiyuan wrote:
> nit: shift left 2 spaces.

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js
(right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:35:
'keyup', this.onPasswordChanged_.bind(this));
On 2014/10/30 03:52:08, xiyuan wrote:
> nit: There is a 'input' event for <input> and <textarea> that we can use. It
is
> fired after the value of the element is changed.

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:134:
$('enable-debugging-help-link').setAttribute('hidden', true);
On 2014/10/30 03:52:08, xiyuan wrote:
> nit: we can just do 
> $('enable-debugging-help-link').hidden = true;

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:149: *
@param {string} state.
On 2014/10/30 03:52:08, xiyuan wrote:
> nit: |state| seems to be an number.

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:168: }
On 2014/10/30 03:52:08, xiyuan wrote:
> nit: we could use classList.toggle to simplify a bit.
> 
> setDialogView_: function(state) {
>   this.state_ = state;
>   this.classList.toggle('remove-protection-view', state ==
> this.UI_STATE.REMOVE_PROTECTION);
>   this.classList.toggle('setup-view', state == this.UI_STATE.SETUP);
>   this.classList.toggle('wait-view', state == this.UI_STATE.WAIT);
>   this.classList.toggle('done-view', state == this.UI_STATE.DONE);
> }

neat. done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:44: if
(event.keyCode == 32)
On 2014/10/30 03:52:08, xiyuan wrote:
> Seems like we need a button that responds to space key rather than a link. We
> used to have css to make a button looks like a link but somehow I am no longer
> able to find it. :(

I've just copies what we already do for accessibility link.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:54:
this.classList.add('connect-debugging-view');
On 2014/10/30 03:52:08, xiyuan wrote:
> nit: this.classList.toggle('connect-debugging-view', 'isDeveloperMode' in data
> && data['isDeveloperMode']);

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.css (right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_reset.css:38:
.enable-debugging-text {
On 2014/10/30 03:52:08, xiyuan wrote:
> Seems unintentional. revert? 

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc
(right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:10:
#include "base/metrics/histogram.h"
On 2014/10/30 03:52:09, xiyuan wrote:
> nit: seems not used.

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:16:
#include "chrome/browser/chromeos/reset/metrics.h"
On 2014/10/30 03:52:09, xiyuan wrote:
> nit: seems not needed.

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:25:
#include "chromeos/dbus/session_manager_client.h"
On 2014/10/30 03:52:09, xiyuan wrote:
> nit: seems not used

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:26:
#include "content/public/browser/browser_thread.h"
On 2014/10/30 03:52:08, xiyuan wrote:
> nit: seems not used.

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:32: //
Reset screen id.
On 2014/10/30 03:52:08, xiyuan wrote:
> nit: update comment or just get rid of it.

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:57:
prefs->SetBoolean(prefs::kDebuggingFeaturesRequested, false);
On 2014/10/30 03:52:08, xiyuan wrote:
> nit: how about call ClearPref()?

Done.

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.h
(right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.h:14:
#include "content/public/browser/web_ui.h"
On 2014/10/30 03:52:09, xiyuan wrote:
> nit: seems not used?

Done.

https://codereview.chromium.org/539273002/diff/100001/chromeos/dbus/debug_dae...
File chromeos/dbus/debug_daemon_client.cc (right):

https://codereview.chromium.org/539273002/diff/100001/chromeos/dbus/debug_dae...
chromeos/dbus/debug_daemon_client.cc:43: }
On 2014/10/30 03:52:09, xiyuan wrote:
> nit: }  // namespace debugd

Done.

https://codereview.chromium.org/539273002/diff/100001/chromeos/dbus/debug_dae...
chromeos/dbus/debug_daemon_client.cc:328: virtual void RemoveRootfsVerification(
On 2014/10/30 03:52:09, xiyuan wrote:
> nit: move this before EnableDebuggingFeatures to match the order in header.

Done.

[xiyuan, 2014-10-31 03:36:01]

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

https://codereview.chromium.org/539273002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:44: if
(event.keyCode == 32)
On 2014/10/31 01:22:26, zel wrote:
> On 2014/10/30 03:52:08, xiyuan wrote:
> > Seems like we need a button that responds to space key rather than a link.
We
> > used to have css to make a button looks like a link but somehow I am no
longer
> > able to find it. :(
> 
> I've just copies what we already do for accessibility link.

Yep, let's just keep it as is.

https://codereview.chromium.org/539273002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc
(right):

https://codereview.chromium.org/539273002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:81:
ready_for_setup_ = prefs->GetBoolean(prefs::kDebuggingFeaturesRequested);
prefs::kDebuggingFeaturesRequested and |ready_for_setup_| seems not really being
used since we have QueryDebuggingFeatures() now.

https://codereview.chromium.org/539273002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:87: }
weak_ptr_factory_.InvalidateWeakPtrs() to cancel pending callbacks when the
screen hides?

https://codereview.chromium.org/539273002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:121:
IDS_ENABLE_DEBUGGING_ERROR_MESSAGE,
nit: wrong indent.

https://codereview.chromium.org/539273002/diff/120001/chromeos/dbus/fake_debu...
File chromeos/dbus/fake_debug_daemon_client.cc (right):

https://codereview.chromium.org/539273002/diff/120001/chromeos/dbus/fake_debu...
chromeos/dbus/fake_debug_daemon_client.cc:141: supported ?
DebugDaemonClient::DEV_FEATURE_ROOTFS_VERIFICATION_REMOVED :
nit: line too long.

[zel, 2014-10-31 23:23:43]

https://codereview.chromium.org/539273002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc
(right):

https://codereview.chromium.org/539273002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:81:
ready_for_setup_ = prefs->GetBoolean(prefs::kDebuggingFeaturesRequested);
On 2014/10/31 03:36:01, xiyuan wrote:
> prefs::kDebuggingFeaturesRequested and |ready_for_setup_| seems not really
being
> used since we have QueryDebuggingFeatures() now.

Done.

https://codereview.chromium.org/539273002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:87: }
On 2014/10/31 03:36:01, xiyuan wrote:
> weak_ptr_factory_.InvalidateWeakPtrs() to cancel pending callbacks when the
> screen hides?

good idea, done

https://codereview.chromium.org/539273002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:121:
IDS_ENABLE_DEBUGGING_ERROR_MESSAGE,
On 2014/10/31 03:36:01, xiyuan wrote:
> nit: wrong indent.

Done.

https://codereview.chromium.org/539273002/diff/120001/chromeos/dbus/fake_debu...
File chromeos/dbus/fake_debug_daemon_client.cc (right):

https://codereview.chromium.org/539273002/diff/120001/chromeos/dbus/fake_debu...
chromeos/dbus/fake_debug_daemon_client.cc:141: supported ?
DebugDaemonClient::DEV_FEATURE_ROOTFS_VERIFICATION_REMOVED :
On 2014/10/31 03:36:01, xiyuan wrote:
> nit: line too long.

Done.

[xiyuan, 2014-11-01 00:41:24]

LGTM!

[dpursell, 2014-11-04 19:33:01]

A few notes, but I'm not super familiar with this code so feel free to ignore
anything if I've misunderstood what's going on.

https://codereview.chromium.org/539273002/diff/180001/chromeos/dbus/debug_dae...
File chromeos/dbus/debug_daemon_client.cc (right):

https://codereview.chromium.org/539273002/diff/180001/chromeos/dbus/debug_dae...
chromeos/dbus/debug_daemon_client.cc:543: if (!response ||
!dbus::MessageReader(response).PopBool(&success)) {
The RemoveRootfsVerification function doesn't return a bool, success/failure is
indicated by whether the D-Bus call completes or errors out (like
OnEnableDebuggingFeatures() above)

https://codereview.chromium.org/539273002/diff/180001/chromeos/dbus/debug_dae...
File chromeos/dbus/debug_daemon_client.h (right):

https://codereview.chromium.org/539273002/diff/180001/chromeos/dbus/debug_dae...
chromeos/dbus/debug_daemon_client.h:169: DEV_FEATURE_DEV_MODE_ROOT_PASSWORD_SET,
You may want DEV_FEATURES_SYSTEM_ROOT_PASSWORD_SET here instead of or in
addition to DEV_FEATURE_DEV_MODE_ROOT_PASSWORD_SET, depending on what we want to
consider the full feature set.

The system password is required for password-based SSH access whereas dev mode
password only provides local shell login access, so using the current
definition, the system could think all features are enabled when password-based
SSH access isn't.

It likely won't make a practical difference since both passwords are enabled at
once, unless setting the system password fails for some reason.

[Nikita (slow), 2014-11-05 17:33:15]

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/c...
File chrome/browser/chromeos/login/help_app_launcher.h (right):

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/c...
chrome/browser/chromeos/login/help_app_launcher.h:43: HELP_ENABLE_DEBUGGING =
183084,
nit: Please make sure that there's an issue filed for that.

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/c...
File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right):

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/c...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:1195:
COMPILE_ASSERT(BaseScreenDelegate::EXIT_CODES_COUNT == 26,
nit: I suggest adding simple test cases that cover new wizard transitions.

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/u...
File chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc
(right):

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/u...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc:242:
help_app_->ShowHelpTopic(HelpAppLauncher::HELP_ENABLE_DEBUGGING);
I suggest commenting out this line till real help article ID is added.

https://chromiumcodereview.appspot.com/539273002/diff/180001/ui/login/display...
File ui/login/display_manager.js (right):

https://chromiumcodereview.appspot.com/539273002/diff/180001/ui/login/display...
ui/login/display_manager.js:145: SCREEN_ERROR_MESSAGE,
SCREEN_ERROR_MESSAGE may be shown even when there're users on the device i.e.
out-of-box is completed. Are you ok with exposing this functionality outside of
out-of-box flow?

https://code.google.com/p/chromium/issues/detail?id=403165#c13 says
"I think we should start by OOBE-only and then iterate from there."

[Nikita (slow), 2014-11-05 17:51:28]

lgtm

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/r...
File chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js
(right):

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/r...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:12: /*
Possible UI states of the reset screen. */
nit: debugging screen

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/r...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:40:
loadTimeData.getString('enableDebuggingPasswordLabel');
nit: 4 spaces indent

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/r...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:42:
loadTimeData.getString('enableDebuggingConfirmPasswordLabel');
nit: 4 spaces indent

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/r...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:83:
loadTimeData.getString('enableDebuggingCancelButton');
nit: 4 spaces indent

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/r...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:118: *
Cancels the reset and drops the user back to the login screen.
nit: Cancels the debugging screen

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/u...
File chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.h
(right):

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/u...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.h:39:
void OnRollbackCheck(bool can_rollback);
nit: Drop unused method.

[zel, 2014-11-11 01:02:28]

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/c...
File chrome/browser/chromeos/login/help_app_launcher.h (right):

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/c...
chrome/browser/chromeos/login/help_app_launcher.h:43: HELP_ENABLE_DEBUGGING =
183084,
On 2014/11/05 17:33:15, Nikita wrote:
> nit: Please make sure that there's an issue filed for that.

Done. The link will stay hidden until we create this article.

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/r...
File chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js
(right):

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/r...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:12: /*
Possible UI states of the reset screen. */
On 2014/11/05 17:51:27, Nikita wrote:
> nit: debugging screen

Done.

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/r...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:40:
loadTimeData.getString('enableDebuggingPasswordLabel');
On 2014/11/05 17:51:27, Nikita wrote:
> nit: 4 spaces indent

Done.

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/r...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:42:
loadTimeData.getString('enableDebuggingConfirmPasswordLabel');
On 2014/11/05 17:51:27, Nikita wrote:
> nit: 4 spaces indent

Done.

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/r...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:83:
loadTimeData.getString('enableDebuggingCancelButton');
On 2014/11/05 17:51:27, Nikita wrote:
> nit: 4 spaces indent

Done.

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/r...
chrome/browser/resources/chromeos/login/oobe_screen_enable_debugging.js:118: *
Cancels the reset and drops the user back to the login screen.
On 2014/11/05 17:51:27, Nikita wrote:
> nit: Cancels the debugging screen

Done.

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/u...
File chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.h
(right):

https://chromiumcodereview.appspot.com/539273002/diff/180001/chrome/browser/u...
chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.h:39:
void OnRollbackCheck(bool can_rollback);
On 2014/11/05 17:51:27, Nikita wrote:
> nit: Drop unused method.

Done.

https://chromiumcodereview.appspot.com/539273002/diff/180001/chromeos/dbus/de...
File chromeos/dbus/debug_daemon_client.cc (right):

https://chromiumcodereview.appspot.com/539273002/diff/180001/chromeos/dbus/de...
chromeos/dbus/debug_daemon_client.cc:543: if (!response ||
!dbus::MessageReader(response).PopBool(&success)) {
On 2014/11/04 19:33:00, dpursell wrote:
> The RemoveRootfsVerification function doesn't return a bool, success/failure
is
> indicated by whether the D-Bus call completes or errors out (like
> OnEnableDebuggingFeatures() above)

Done.

https://chromiumcodereview.appspot.com/539273002/diff/180001/chromeos/dbus/de...
File chromeos/dbus/debug_daemon_client.h (right):

https://chromiumcodereview.appspot.com/539273002/diff/180001/chromeos/dbus/de...
chromeos/dbus/debug_daemon_client.h:169: DEV_FEATURE_DEV_MODE_ROOT_PASSWORD_SET,
On 2014/11/04 19:33:00, dpursell wrote:
> You may want DEV_FEATURES_SYSTEM_ROOT_PASSWORD_SET here instead of or in
> addition to DEV_FEATURE_DEV_MODE_ROOT_PASSWORD_SET, depending on what we want
to
> consider the full feature set.
> 
> The system password is required for password-based SSH access whereas dev mode
> password only provides local shell login access, so using the current
> definition, the system could think all features are enabled when
password-based
> SSH access isn't.
> 
> It likely won't make a practical difference since both passwords are enabled
at
> once, unless setting the system password fails for some reason.

Should I then add both DEV_FEATURES_SYSTEM_ROOT_PASSWORD_SET and
DEV_FEATURE_DEV_MODE_ROOT_PASSWORD_SET here?

https://chromiumcodereview.appspot.com/539273002/diff/180001/ui/login/display...
File ui/login/display_manager.js (right):

https://chromiumcodereview.appspot.com/539273002/diff/180001/ui/login/display...
ui/login/display_manager.js:145: SCREEN_ERROR_MESSAGE,
On 2014/11/05 17:33:15, Nikita wrote:
> SCREEN_ERROR_MESSAGE may be shown even when there're users on the device i.e.
> out-of-box is completed. Are you ok with exposing this functionality outside
of
> out-of-box flow?
> 
> https://code.google.com/p/chromium/issues/detail?id=403165#c13 says
> "I think we should start by OOBE-only and then iterate from there."

Removed.

[dpursell, 2014-11-11 22:53:49]

https://chromiumcodereview.appspot.com/539273002/diff/180001/chromeos/dbus/de...
File chromeos/dbus/debug_daemon_client.h (right):

https://chromiumcodereview.appspot.com/539273002/diff/180001/chromeos/dbus/de...
chromeos/dbus/debug_daemon_client.h:169: DEV_FEATURE_DEV_MODE_ROOT_PASSWORD_SET,
On 2014/11/11 01:02:28, zel wrote:
> On 2014/11/04 19:33:00, dpursell wrote:
> > You may want DEV_FEATURES_SYSTEM_ROOT_PASSWORD_SET here instead of or in
> > addition to DEV_FEATURE_DEV_MODE_ROOT_PASSWORD_SET, depending on what we
want
> to
> > consider the full feature set.
> > 
> > The system password is required for password-based SSH access whereas dev
mode
> > password only provides local shell login access, so using the current
> > definition, the system could think all features are enabled when
> password-based
> > SSH access isn't.
> > 
> > It likely won't make a practical difference since both passwords are enabled
> at
> > once, unless setting the system password fails for some reason.
> 
> Should I then add both DEV_FEATURES_SYSTEM_ROOT_PASSWORD_SET and
> DEV_FEATURE_DEV_MODE_ROOT_PASSWORD_SET here?


Yeah that would be my recommendation, that way we won't ever mistakenly think
the whole feature set is enabled if one of these is missing.

[zel, 2014-11-12 16:04:40]

https://chromiumcodereview.appspot.com/539273002/diff/180001/chromeos/dbus/de...
File chromeos/dbus/debug_daemon_client.h (right):

https://chromiumcodereview.appspot.com/539273002/diff/180001/chromeos/dbus/de...
chromeos/dbus/debug_daemon_client.h:169: DEV_FEATURE_DEV_MODE_ROOT_PASSWORD_SET,
On 2014/11/11 22:53:49, dpursell wrote:
> On 2014/11/11 01:02:28, zel wrote:
> > On 2014/11/04 19:33:00, dpursell wrote:
> > > You may want DEV_FEATURES_SYSTEM_ROOT_PASSWORD_SET here instead of or in
> > > addition to DEV_FEATURE_DEV_MODE_ROOT_PASSWORD_SET, depending on what we
> want
> > to
> > > consider the full feature set.
> > > 
> > > The system password is required for password-based SSH access whereas dev
> mode
> > > password only provides local shell login access, so using the current
> > > definition, the system could think all features are enabled when
> > password-based
> > > SSH access isn't.
> > > 
> > > It likely won't make a practical difference since both passwords are
enabled
> > at
> > > once, unless setting the system password fails for some reason.
> > 
> > Should I then add both DEV_FEATURES_SYSTEM_ROOT_PASSWORD_SET and
> > DEV_FEATURE_DEV_MODE_ROOT_PASSWORD_SET here?
> 
> 
> Yeah that would be my recommendation, that way we won't ever mistakenly think
> the whole feature set is enabled if one of these is missing.

Done.

[zel, 2014-11-12 16:04:59]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-11-12 16:06:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/539273002/280001

[commit-bot: I haz the power, 2014-11-12 17:54:57]

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

[commit-bot: I haz the power, 2014-11-12 17:54:58]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zel, 2014-11-14 00:15:04]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-11-14 00:17:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/539273002/320001

[commit-bot: I haz the power, 2014-11-14 01:49:33]

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

[commit-bot: I haz the power, 2014-11-14 01:49:35]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zel, 2014-11-14 02:17:36]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-11-14 02:19:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/539273002/340001

[commit-bot: I haz the power, 2014-11-14 04:06:03]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2014-11-14 04:06:36]

Patchset 18 (id:??) landed as
https://crrev.com/459b11b10f0f18c4111bcfa973be5244171f1412
Cr-Commit-Position: refs/heads/master@{#304161}