[cros] Change how we track momemt to emit login-prompt-visible signal.

[cros] Change how we track momemt to emit login-prompt-visible signal.

We could not rely on NOTIFICATION_RENDER_WIDGET_HOST_DID_PAINT in case of accelerated compositing.

Also fix few JS style issues in screen_gaia_signin.js

BUG=127517
TEST=login-prompt-visible is emitted with or without accelerated compositing enabled.

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

[Daniel Erat, 2012-06-02 14:20:58]

http://codereview.chromium.org/10476002/diff/9/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/webui_login_view.h (right):

http://codereview.chromium.org/10476002/diff/9/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/webui_login_view.h:114: // Performs series of
action when login prompt is considered
nit: s/action/actions/

http://codereview.chromium.org/10476002/diff/9/chrome/common/chrome_notificat...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/10476002/diff/9/chrome/common/chrome_notificat...
chrome/common/chrome_notification_types.h:896: // 1.
NOTIFICATION_LOGIN_USER_IMAGES_LOADED
Can you delete some of these, too?  I only added the _USER_IMAGES_LOADED
notification so we could tell when the WebUI is visible -- if it's not needed in
order to send the _WEBUI_VISIBLE notification, I think that it can be removed.

[Nikita (slow), 2012-06-03 20:47:22]

Daniel, are these the only comments to this CL?
Could you please issue sticky l gtm if possible with these issues fixed?

[Daniel Erat, 2012-06-03 20:53:43]

On 2012/06/03 20:47:22, Nikita Kostylev wrote:
> Daniel, are these the only comments to this CL?
> Could you please issue sticky l gtm if possible with these issues fixed?

LGTM

Sure, if this works as described in both the accelerated and unaccelerated
cases, I'm fine with it going in after the dead notifications are removed.  We
have a bunch of similarly-named notifications around login now and the order in
which they're called is unintuitive, so I'd be in favor of removing everything
that doesn't have any observers, along with the supporting code for e.g. JS
notifying Chrome that the images have been loaded.  I'm happy to take another
look after those changes are in.

[Nikita (slow), 2012-06-04 09:03:21]

http://codereview.chromium.org/10476002/diff/9/chrome/common/chrome_notificat...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/10476002/diff/9/chrome/common/chrome_notificat...
chrome/common/chrome_notification_types.h:896: // 1.
NOTIFICATION_LOGIN_USER_IMAGES_LOADED
On 2012/06/02 14:20:59, Daniel Erat wrote:
> Can you delete some of these, too?  I only added the _USER_IMAGES_LOADED
> notification so we could tell when the WebUI is visible -- if it's not needed
in
> order to send the _WEBUI_VISIBLE notification, I think that it can be removed.

I think it makes sense to leave this _USER_IMAGES_LOADED notification.

They are used by automation proxy.

Even though they may be seen as duplicating _WEBUI_VISIBLE but in fact
_WEBUI_VISIBLE just says that one of the parts of WebUI login is ready/visible,
those 4 others say specifically which one is ready to be used and was shown.

Current usage in automation proxy is when on of the
_USER_IMAGES_LOADED, _LOGIN_WEBUI_READY, _WIZARD_FIRST_SCREEN_SHOWN is firedm
report event "WebUI ready". But we may as well improve on that and require
specific notification.

For instance when we're booting into image w/o users we're waiting for
_LOGIN_WEBUI_READY, not for _USER_IMAGES_LOADED.

[Nikita (slow), 2012-06-04 09:18:25]

http://codereview.chromium.org/10476002/diff/9/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/webui_login_view.h (right):

http://codereview.chromium.org/10476002/diff/9/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/webui_login_view.h:114: // Performs series of
action when login prompt is considered
On 2012/06/02 14:20:59, Daniel Erat wrote:
> nit: s/action/actions/

Done.

[Nikita (slow), 2012-06-04 12:12:17]

Tested on stumpy/alex with accelerated compositing enabled/disabled:

Boot into OOBE/GAIA frame/user list/error screen (no network).
Sign out into GAIA frame/user list.

[Daniel Erat, 2012-06-04 14:49:35]

On 2012/06/04 09:03:21, Nikita Kostylev wrote:
>
http://codereview.chromium.org/10476002/diff/9/chrome/common/chrome_notificat...
> File chrome/common/chrome_notification_types.h (right):
> 
>
http://codereview.chromium.org/10476002/diff/9/chrome/common/chrome_notificat...
> chrome/common/chrome_notification_types.h:896: // 1.
> NOTIFICATION_LOGIN_USER_IMAGES_LOADED
> On 2012/06/02 14:20:59, Daniel Erat wrote:
> > Can you delete some of these, too?  I only added the _USER_IMAGES_LOADED
> > notification so we could tell when the WebUI is visible -- if it's not
needed
> in
> > order to send the _WEBUI_VISIBLE notification, I think that it can be
removed.
> 
> I think it makes sense to leave this _USER_IMAGES_LOADED notification.
> 
> They are used by automation proxy.
> 
> Even though they may be seen as duplicating _WEBUI_VISIBLE but in fact
> _WEBUI_VISIBLE just says that one of the parts of WebUI login is
ready/visible,
> those 4 others say specifically which one is ready to be used and was shown.
> 
> Current usage in automation proxy is when on of the
> _USER_IMAGES_LOADED, _LOGIN_WEBUI_READY, _WIZARD_FIRST_SCREEN_SHOWN is firedm
> report event "WebUI ready". But we may as well improve on that and require
> specific notification.
> 
> For instance when we're booting into image w/o users we're waiting for
> _LOGIN_WEBUI_READY, not for _USER_IMAGES_LOADED.

The automation proxy should just be using the new signal that you're adding, no?
 The code in LoginWebuiReadyObserver looks like it's just a rough copy-and-paste
of WebUILoginView.  This code should be made simpler if possible (by deletion)
-- is there any reason why the automation code needs to know anything more than
whether we're ready to permit login or not?

[Nikita (slow), 2012-06-04 15:26:05]

On 2012/06/04 14:49:35, Daniel Erat wrote:
> On 2012/06/04 09:03:21, Nikita Kostylev wrote:
> >
>
http://codereview.chromium.org/10476002/diff/9/chrome/common/chrome_notificat...
> > File chrome/common/chrome_notification_types.h (right):
> > 
> >
>
http://codereview.chromium.org/10476002/diff/9/chrome/common/chrome_notificat...
> > chrome/common/chrome_notification_types.h:896: // 1.
> > NOTIFICATION_LOGIN_USER_IMAGES_LOADED
> > On 2012/06/02 14:20:59, Daniel Erat wrote:
> > > Can you delete some of these, too?  I only added the _USER_IMAGES_LOADED
> > > notification so we could tell when the WebUI is visible -- if it's not
> needed
> > in
> > > order to send the _WEBUI_VISIBLE notification, I think that it can be
> removed.
> > 
> > I think it makes sense to leave this _USER_IMAGES_LOADED notification.
> > 
> > They are used by automation proxy.
> > 
> > Even though they may be seen as duplicating _WEBUI_VISIBLE but in fact
> > _WEBUI_VISIBLE just says that one of the parts of WebUI login is
> ready/visible,
> > those 4 others say specifically which one is ready to be used and was shown.
> > 
> > Current usage in automation proxy is when on of the
> > _USER_IMAGES_LOADED, _LOGIN_WEBUI_READY, _WIZARD_FIRST_SCREEN_SHOWN is
firedm
> > report event "WebUI ready". But we may as well improve on that and require
> > specific notification.
> > 
> > For instance when we're booting into image w/o users we're waiting for
> > _LOGIN_WEBUI_READY, not for _USER_IMAGES_LOADED.
> 
> The automation proxy should just be using the new signal that you're adding,
no?
>  The code in LoginWebuiReadyObserver looks like it's just a rough
copy-and-paste
> of WebUILoginView.  This code should be made simpler if possible (by deletion)
> -- is there any reason why the automation code needs to know anything more
than
> whether we're ready to permit login or not?

I think automation should still have a way to distinguish these states.
For instance test that waits for GAIA frame should wait for _LOGIN_WEBUI_READY.
If there're network issues it will get  _NETWORK_ERROR_SHOWN + _LOGIN_VISIBLE
notifications. So "login screen" UI is visible but not in the state we're
expecting it to be.

[Daniel Erat, 2012-06-04 15:34:51]

On 2012/06/04 15:26:05, Nikita Kostylev wrote:
> On 2012/06/04 14:49:35, Daniel Erat wrote:
> > On 2012/06/04 09:03:21, Nikita Kostylev wrote:
> > >
> >
>
http://codereview.chromium.org/10476002/diff/9/chrome/common/chrome_notificat...
> > > File chrome/common/chrome_notification_types.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10476002/diff/9/chrome/common/chrome_notificat...
> > > chrome/common/chrome_notification_types.h:896: // 1.
> > > NOTIFICATION_LOGIN_USER_IMAGES_LOADED
> > > On 2012/06/02 14:20:59, Daniel Erat wrote:
> > > > Can you delete some of these, too?  I only added the _USER_IMAGES_LOADED
> > > > notification so we could tell when the WebUI is visible -- if it's not
> > needed
> > > in
> > > > order to send the _WEBUI_VISIBLE notification, I think that it can be
> > removed.
> > > 
> > > I think it makes sense to leave this _USER_IMAGES_LOADED notification.
> > > 
> > > They are used by automation proxy.
> > > 
> > > Even though they may be seen as duplicating _WEBUI_VISIBLE but in fact
> > > _WEBUI_VISIBLE just says that one of the parts of WebUI login is
> > ready/visible,
> > > those 4 others say specifically which one is ready to be used and was
shown.
> > > 
> > > Current usage in automation proxy is when on of the
> > > _USER_IMAGES_LOADED, _LOGIN_WEBUI_READY, _WIZARD_FIRST_SCREEN_SHOWN is
> firedm
> > > report event "WebUI ready". But we may as well improve on that and require
> > > specific notification.
> > > 
> > > For instance when we're booting into image w/o users we're waiting for
> > > _LOGIN_WEBUI_READY, not for _USER_IMAGES_LOADED.
> > 
> > The automation proxy should just be using the new signal that you're adding,
> no?
> >  The code in LoginWebuiReadyObserver looks like it's just a rough
> copy-and-paste
> > of WebUILoginView.  This code should be made simpler if possible (by
deletion)
> > -- is there any reason why the automation code needs to know anything more
> than
> > whether we're ready to permit login or not?
> 
> I think automation should still have a way to distinguish these states.
> For instance test that waits for GAIA frame should wait for
_LOGIN_WEBUI_READY.
> If there're network issues it will get  _NETWORK_ERROR_SHOWN + _LOGIN_VISIBLE
> notifications. So "login screen" UI is visible but not in the state we're
> expecting it to be.

Perhaps these notifications could just be more clearly named, and the order in
which they're emitted documented somewhere, then.  I found this stuff pretty
difficult to keep straight the last time I was working on it.

[Nikita (slow), 2012-06-05 08:25:02]

> Perhaps these notifications could just be more clearly named, and the order in
> which they're emitted documented somewhere, then.  I found this stuff pretty
> difficult to keep straight the last time I was working on it.

http://codereview.chromium.org/10536003/