Reset Screen moved to ScreenContext.

Reset Screen moved to ScreenContext.

BUG=456778
TBR=jonross@chromium.org

Committed: https://crrev.com/4b77f613917020e58b31975355e4ca2347f216e2
Cr-Commit-Position: refs/heads/master@{#316212}

Committed: https://crrev.com/ba85904895874b5d82d78bcb499c3c5ee7388e34
Cr-Commit-Position: refs/heads/master@{#316232}

Committed: https://crrev.com/2ea8cb65f0280ff774b05a6f6745a45257888ad8
Cr-Commit-Position: refs/heads/master@{#316441}

[merkulova, 2015-02-09 15:43:45]

merkulova@chromium.org changed reviewers:
+ dzhioev@chromium.org

[dzhioev (left Google), 2015-02-11 12:38:30]

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

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/reset_browsertest.cc:64: LOG(ERROR) << "1";
Remove this.

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

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/reset_model.cc:19: const char
ResetModel::kContextKeyIsRestartRequired[] = "restart-required";
Could you please use contextKeyName-style for context keys names? We want to be
able to access context fields using obj.contextKeyName in JS.

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

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/reset_model.h:38: // Called when user
pushes cancel or ESC button.
Do we need to reveal all the On* methods in ResetModel interface?

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

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/reset_screen.h:10: #include
"base/basictypes.h"
What this include is needed for?

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/reset_screen.h:15: #include
"chrome/browser/chromeos/login/screens/base_screen.h"
Not needed.

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/reset_screen.h:66:
scoped_refptr<HelpAppLauncher> help_app_;
Do you now why help_app_ is a scoped_refptr, and not a simple scoped_ptr?

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:43:
$('powerwash-help-link').addEventListener('click', function(event) {
this.declareUserAction($('powerwash-help-link'),
                       { action_id: USER_ACTION_LEARN_MORE_PRESSED,
                         event: 'click'
                       });

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:103:
restartButton.addEventListener('click', function(e) {
Use declareUserAction

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:119:
self.context.set(CONTEXT_KEY_IS_CONFIRMATIONAL_VIEW, true);
This should be done on C++ side as a reaction on USER_ACTION_SHOW_CONFIRMATION
action.

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:129:
cancelButton.textContent = loadTimeData.getString('cancelButton');
Use declareUserAction

[merkulova, 2015-02-11 15:07:17]

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

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/reset_browsertest.cc:64: LOG(ERROR) << "1";
On 2015/02/11 12:38:29, dzhioev wrote:
> Remove this.

Done.

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/reset_browsertest.cc:64: LOG(ERROR) << "1";
On 2015/02/11 12:38:29, dzhioev wrote:
> Remove this.

Done.

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

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/reset_model.cc:19: const char
ResetModel::kContextKeyIsRestartRequired[] = "restart-required";
On 2015/02/11 12:38:29, dzhioev wrote:
> Could you please use contextKeyName-style for context keys names? We want to
be
> able to access context fields using obj.contextKeyName in JS.

As discussed offline, it's an issue for a separate CL. As many *_model.cc files
are affected.

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

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/reset_model.h:38: // Called when user
pushes cancel or ESC button.
On 2015/02/11 12:38:29, dzhioev wrote:
> Do we need to reveal all the On* methods in ResetModel interface?

Done.

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

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/reset_screen.h:10: #include
"base/basictypes.h"
On 2015/02/11 12:38:29, dzhioev wrote:
> What this include is needed for?

Removed.

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/reset_screen.h:15: #include
"chrome/browser/chromeos/login/screens/base_screen.h"
On 2015/02/11 12:38:29, dzhioev wrote:
> Not needed. 

Done.

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/reset_screen.h:66:
scoped_refptr<HelpAppLauncher> help_app_;
On 2015/02/11 12:38:29, dzhioev wrote:
> Do you now why help_app_ is a scoped_refptr, and not a simple scoped_ptr?

Because some other screens might use it.

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:43:
$('powerwash-help-link').addEventListener('click', function(event) {
On 2015/02/11 12:38:29, dzhioev wrote:
> this.declareUserAction($('powerwash-help-link'),
>                        { action_id: USER_ACTION_LEARN_MORE_PRESSED,
>                          event: 'click'
>                        });

Done.

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:103:
restartButton.addEventListener('click', function(e) {
On 2015/02/11 12:38:29, dzhioev wrote:
> Use declareUserAction

Done.

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:119:
self.context.set(CONTEXT_KEY_IS_CONFIRMATIONAL_VIEW, true);
On 2015/02/11 12:38:29, dzhioev wrote:
> This should be done on C++ side as a reaction on USER_ACTION_SHOW_CONFIRMATION
> action.

Done.

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:129:
cancelButton.textContent = loadTimeData.getString('cancelButton');
On 2015/02/11 12:38:29, dzhioev wrote:
> Use declareUserAction

Done.

[dzhioev (left Google), 2015-02-12 12:51:58]

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

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/reset_screen.h:66:
scoped_refptr<HelpAppLauncher> help_app_;
On 2015/02/11 15:07:17, merkulova wrote:
> On 2015/02/11 12:38:29, dzhioev wrote:
> > Do you now why help_app_ is a scoped_refptr, and not a simple scoped_ptr?
> 
> Because some other screens might use it.

First of all, help_app_ is a private field without any getters. Second, 
HelpAppLauncher is an utility class, so any screen can create its own copy.
Please, change to scoped_ptr.

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:119:
self.context.set(CONTEXT_KEY_IS_CONFIRMATIONAL_VIEW, true);
On 2015/02/11 15:07:17, merkulova wrote:
> On 2015/02/11 12:38:29, dzhioev wrote:
> > This should be done on C++ side as a reaction on
USER_ACTION_SHOW_CONFIRMATION
> > action.
> 
> Done.

Nothing has changed.

[merkulova, 2015-02-12 13:37:38]

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

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/reset_screen.h:66:
scoped_refptr<HelpAppLauncher> help_app_;
On 2015/02/12 12:51:58, dzhioev wrote:
> On 2015/02/11 15:07:17, merkulova wrote:
> > On 2015/02/11 12:38:29, dzhioev wrote:
> > > Do you now why help_app_ is a scoped_refptr, and not a simple scoped_ptr?
> > 
> > Because some other screens might use it.
> 
> First of all, help_app_ is a private field without any getters. Second, 
> HelpAppLauncher is an utility class, so any screen can create its own copy.
> Please, change to scoped_ptr.

is_refcounted_type_and_needs_scoped_refptr as HelpAppLauncher is inheriting
base::RefCountedThreadSafe<HelpAppLauncher>

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

https://codereview.chromium.org/904163003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:119:
self.context.set(CONTEXT_KEY_IS_CONFIRMATIONAL_VIEW, true);
On 2015/02/12 12:51:58, dzhioev wrote:
> On 2015/02/11 15:07:17, merkulova wrote:
> > On 2015/02/11 12:38:29, dzhioev wrote:
> > > This should be done on C++ side as a reaction on
> USER_ACTION_SHOW_CONFIRMATION
> > > action.
> > 
> > Done.
> 
> Nothing has changed.
Sorry, missed the change.

[dzhioev (left Google), 2015-02-13 12:40:23]

On 2015/02/12 14:27:27, merkulova wrote:

LGTM

[merkulova, 2015-02-13 12:41:44]

The CQ bit was checked by merkulova@chromium.org

[merkulova, 2015-02-13 12:41:56]

The CQ bit was unchecked by merkulova@chromium.org

[merkulova, 2015-02-13 12:52:57]

New patchsets have been uploaded after l-g-t-m from dzhioev@chromium.org

[merkulova, 2015-02-13 12:53:24]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2015-02-13 12:53:36]

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

[commit-bot: I haz the power, 2015-02-13 13:42:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-13 13:42:58]

Patchset 6 (id:??) landed as
https://crrev.com/4b77f613917020e58b31975355e4ca2347f216e2
Cr-Commit-Position: refs/heads/master@{#316212}

[merkulova, 2015-02-13 13:57:58]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/917613005/ by merkulova@chromium.org.

The reason for reverting is: Conflict with
https://codereview.chromium.org/872633008/.

[merkulova, 2015-02-13 16:08:11]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2015-02-13 16:08:59]

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

[commit-bot: I haz the power, 2015-02-13 17:30:13]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-13 17:30:38]

Patchset 7 (id:??) landed as
https://crrev.com/ba85904895874b5d82d78bcb499c3c5ee7388e34
Cr-Commit-Position: refs/heads/master@{#316232}

[jonross, 2015-02-13 17:44:34]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/928513002/ by jonross@chromium.org.

The reason for reverting is: This is faling a compile on ChromeOS bots:

http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Chrome....

[jonross, 2015-02-13 17:49:03]

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

[jonross, 2015-02-13 17:49:04]

https://codereview.chromium.org/904163003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/reset_screen.cc (right):

https://codereview.chromium.org/904163003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/reset_screen.cc:208: help_app_ = new
HelpAppLauncher(GetNativeWindow());
This line appears to be not compiling on a Chrome OS bot. It's likely that the
configuration doesn't have the includes that you are expecting. And none of the
trybots seem to testing this before submission.

Sorry about that!

[merkulova, 2015-02-16 08:25:10]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2015-02-16 08:25:24]

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

[merkulova, 2015-02-16 08:27:28]

merkulova@chromium.org changed reviewers:
- jonross@chromium.org

[commit-bot: I haz the power, 2015-02-16 09:05:53]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-16 09:06:39]

Patchset 8 (id:??) landed as
https://crrev.com/2ea8cb65f0280ff774b05a6f6745a45257888ad8
Cr-Commit-Position: refs/heads/master@{#316441}