Added usage of ScreenContext in EulaScreen.

Added usage of ScreenContext in EulaScreen.

* implemented usage of screen context in EulaScreen
* added possibility to add key observers to ScreenContext
* changes in screen context are passed automatically to EulaScreen.

BUG=433797
TEST=manual, existing browser_tests and unit_tests

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

[ygorshenin1, 2014-11-25 12:56:03]

ygorshenin@chromium.org changed reviewers:
+ antrim@chromium.org

[ygorshenin1, 2014-11-25 12:56:04]

Denis, could you please take a look? Crucial changes are in
EulaScreen, EulaView, EulaModel, BaseScreen and BaseScreenHandler.

[Denis Kuznetsov (DE-MUC), 2014-11-25 17:31:17]

https://chromiumcodereview.appspot.com/755203002/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/screens/base_screen.cc (right):

https://chromiumcodereview.appspot.com/755203002/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/screens/base_screen.cc:40: return GetName();
not in scope, but as you touch this file, please add todo here to eliminate
intermediate GetName() once it is possible.

https://chromiumcodereview.appspot.com/755203002/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/screens/base_screen.h (right):

https://chromiumcodereview.appspot.com/755203002/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/screens/base_screen.h:78: // counterpart.
Please keep it protected, use friend classes instead.

https://chromiumcodereview.appspot.com/755203002/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/screens/eula_model.cc (right):

https://chromiumcodereview.appspot.com/755203002/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/screens/eula_model.cc:13: EulaModel::~EulaModel()
{
move GetName() implementation here.

https://chromiumcodereview.appspot.com/755203002/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/screens/eula_screen.h (right):

https://chromiumcodereview.appspot.com/755203002/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/screens/eula_screen.h:23: class Delegate {
Do we still need this delegate?

https://chromiumcodereview.appspot.com/755203002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/webui/chromeos/login/eula_screen_handler.cc (right):

https://chromiumcodereview.appspot.com/755203002/diff/20001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/eula_screen_handler.cc:108: void
EulaScreenHandler::SetModel(EulaModel* model) {
Could you rename this to Bind/Unbind?

https://chromiumcodereview.appspot.com/755203002/diff/20001/ui/login/screen.js
File ui/login/screen.js (right):

https://chromiumcodereview.appspot.com/755203002/diff/20001/ui/login/screen.j...
ui/login/screen.js:323: screen.initialize();
I'd say that initialize() should follow decorate(), not vice versa.

[ygorshenin1, 2014-11-26 11:45:39]

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

[ygorshenin1, 2014-11-26 11:45:40]

PTAL

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

https://codereview.chromium.org/755203002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/base_screen.cc:40: return GetName();
On 2014/11/25 17:31:16, Denis Kuznetsov wrote:
> not in scope, but as you touch this file, please add todo here to eliminate
> intermediate GetName() once it is possible.

Done.

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

https://codereview.chromium.org/755203002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/base_screen.h:78: // counterpart.
On 2014/11/25 17:31:16, Denis Kuznetsov wrote:
> Please keep it protected, use friend classes instead.

Done.

https://codereview.chromium.org/755203002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/base_screen.h:78: // counterpart.
On 2014/11/25 17:31:16, Denis Kuznetsov wrote:
> Please keep it protected, use friend classes instead.

Done.

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

https://codereview.chromium.org/755203002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/eula_model.cc:13: EulaModel::~EulaModel()
{
On 2014/11/25 17:31:17, Denis Kuznetsov wrote:
> move GetName() implementation here.

Done.

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

https://codereview.chromium.org/755203002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/eula_screen.h:23: class Delegate {
On 2014/11/25 17:31:17, Denis Kuznetsov wrote:
> Do we still need this delegate?

Yes, because only WizardController knows how to Set|Get Usage Statistics
Reporting.

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

https://codereview.chromium.org/755203002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/eula_screen_handler.cc:108: void
EulaScreenHandler::SetModel(EulaModel* model) {
On 2014/11/25 17:31:17, Denis Kuznetsov wrote:
> Could you rename this to Bind/Unbind?

Done.

https://codereview.chromium.org/755203002/diff/20001/ui/login/screen.js
File ui/login/screen.js (right):

https://codereview.chromium.org/755203002/diff/20001/ui/login/screen.js#newco...
ui/login/screen.js:323: screen.initialize();
On 2014/11/25 17:31:17, Denis Kuznetsov wrote:
> I'd say that initialize() should follow decorate(), not vice versa.

Done.

[dzhioev (left Google), 2014-11-27 11:54:02]

https://codereview.chromium.org/755203002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/base_screen.h (right):

https://codereview.chromium.org/755203002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/base_screen.h:107: // Called when context
for the currenct screen was
*current

https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/base_screen.cc (right):

https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/base_screen.cc:58: LOG(WARNING) <<
"BaseScreen::OnContextKeyUpdated(): key=" << key;
Please make the message more meaningful. For example, "Unhandled context change.
Key=...".

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

https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/base_screen.h:86: // side. Should be
implemented in subclasses.
This comment needs to be updated. Implementation of this method is optional.

https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/base_screen.h:87: virtual void
OnContextKeyUpdated(const ::login::ScreenContext::KeyType& key);
I think we need another method, notifying about all changes at once. Something
like "OnContextUpdated(const std::vector<Type>& keys)".

https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/base_screen.h:107: // Called when context
for the currenct screen was
*current

https://codereview.chromium.org/755203002/diff/100001/ui/login/screen.js
File ui/login/screen.js (right):

https://codereview.chromium.org/755203002/diff/100001/ui/login/screen.js#newc...
ui/login/screen.js:324: screen.initialize();
Please remove "screen.initialize()" from
chrome/browser/resources/chromeos/login/oobe-screen.js:70

[ygorshenin1, 2014-11-27 13:07:33]

PTAL

https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/base_screen.cc (right):

https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/base_screen.cc:58: LOG(WARNING) <<
"BaseScreen::OnContextKeyUpdated(): key=" << key;
On 2014/11/27 11:54:02, dzhioev wrote:
> Please make the message more meaningful. For example, "Unhandled context
change.
> Key=...". 

Done.

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

https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/base_screen.h:86: // side. Should be
implemented in subclasses.
On 2014/11/27 11:54:02, dzhioev wrote:
> This comment needs to be updated. Implementation of this method is optional.

Done.

https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/base_screen.h:87: virtual void
OnContextKeyUpdated(const ::login::ScreenContext::KeyType& key);
On 2014/11/27 11:54:02, dzhioev wrote:
> I think we need another method, notifying about all changes at once. Something
> like "OnContextUpdated(const std::vector<Type>& keys)".

From my point of view it will be quite easy to implement OnContextUpdated() when
we'll have screens wanting the method.

https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/base_screen.h:107: // Called when context
for the currenct screen was
On 2014/11/27 11:54:02, dzhioev wrote:
> *current

Done.

https://codereview.chromium.org/755203002/diff/100001/ui/login/screen.js
File ui/login/screen.js (right):

https://codereview.chromium.org/755203002/diff/100001/ui/login/screen.js#newc...
ui/login/screen.js:324: screen.initialize();
On 2014/11/27 11:54:02, dzhioev wrote:
> Please remove "screen.initialize()" from
> chrome/browser/resources/chromeos/login/oobe-screen.js:70

Done.

[Denis Kuznetsov (DE-MUC), 2014-11-27 13:47:38]

lgtm on my side.

https://chromiumcodereview.appspot.com/755203002/diff/120001/chrome/browser/c...
File chrome/browser/chromeos/login/screens/base_screen.cc (right):

https://chromiumcodereview.appspot.com/755203002/diff/120001/chrome/browser/c...
chrome/browser/chromeos/login/screens/base_screen.cc:53: LOG(WARNING) <<
"Unhandled button click: button_id=" << button_id;
note: I agree that naming can be better: like OnUserAction(...), but it can be
done out of the this CL scope.

[ygorshenin1, 2014-11-27 14:12:06]

https://chromiumcodereview.appspot.com/755203002/diff/120001/chrome/browser/c...
File chrome/browser/chromeos/login/screens/base_screen.cc (right):

https://chromiumcodereview.appspot.com/755203002/diff/120001/chrome/browser/c...
chrome/browser/chromeos/login/screens/base_screen.cc:53: LOG(WARNING) <<
"Unhandled button click: button_id=" << button_id;
On 2014/11/27 13:47:38, Denis Kuznetsov wrote:
> note: I agree that naming can be better: like OnUserAction(...), but it can be
> done out of the this CL scope.

I renamed it in https://codereview.chromium.org/763563002/.

[dzhioev (left Google), 2014-11-27 15:38:25]

On 2014/11/27 13:07:33, ygorshenin1 wrote:
> PTAL
> 
>
https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
> File chrome/browser/chromeos/login/screens/base_screen.cc (right):
> 
>
https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
> chrome/browser/chromeos/login/screens/base_screen.cc:58: LOG(WARNING) <<
> "BaseScreen::OnContextKeyUpdated(): key=" << key;
> On 2014/11/27 11:54:02, dzhioev wrote:
> > Please make the message more meaningful. For example, "Unhandled context
> change.
> > Key=...". 
> 
> Done.
> 
>
https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
> File chrome/browser/chromeos/login/screens/base_screen.h (right):
> 
>
https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
> chrome/browser/chromeos/login/screens/base_screen.h:86: // side. Should be
> implemented in subclasses.
> On 2014/11/27 11:54:02, dzhioev wrote:
> > This comment needs to be updated. Implementation of this method is optional.
> 
> Done.
> 
>
https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
> chrome/browser/chromeos/login/screens/base_screen.h:87: virtual void
> OnContextKeyUpdated(const ::login::ScreenContext::KeyType& key);
> On 2014/11/27 11:54:02, dzhioev wrote:
> > I think we need another method, notifying about all changes at once.
Something
> > like "OnContextUpdated(const std::vector<Type>& keys)".
> 
> From my point of view it will be quite easy to implement OnContextUpdated()
when
> we'll have screens wanting the method.
> 
>
https://codereview.chromium.org/755203002/diff/100001/chrome/browser/chromeos...
> chrome/browser/chromeos/login/screens/base_screen.h:107: // Called when
context
> for the currenct screen was
> On 2014/11/27 11:54:02, dzhioev wrote:
> > *current
> 
> Done.
> 
> https://codereview.chromium.org/755203002/diff/100001/ui/login/screen.js
> File ui/login/screen.js (right):
> 
>
https://codereview.chromium.org/755203002/diff/100001/ui/login/screen.js#newc...
> ui/login/screen.js:324: screen.initialize();
> On 2014/11/27 11:54:02, dzhioev wrote:
> > Please remove "screen.initialize()" from
> > chrome/browser/resources/chromeos/login/oobe-screen.js:70
> 
> Done.

LGTM

[ygorshenin1, 2014-11-27 16:00:33]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-11-27 16:01:05]

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

[commit-bot: I haz the power, 2014-11-27 17:03:04]

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

[commit-bot: I haz the power, 2014-11-27 17:03:05]

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

[ygorshenin1, 2014-11-28 13:18:50]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-11-28 13:19:31]

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

[commit-bot: I haz the power, 2014-11-28 14:08:16]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-28 14:09:00]

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

[ygorshenin1, 2014-11-28 15:09:57]

Many thanks!