Enable wallpaper boot animation for boot into sign in too. Add flag to disable it.

Enable wallpaper boot animation for boot into sign in too. Add flag to disable it.

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

[bshe, 2012-07-31 16:04:23]

lgtm

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right):

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:247: // should trigger
wallpaper load on boot.
nit: more words can be fitted to previous line?

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:249:
switches::kDisableBootAnimation)) {
nit: indent is off here.

[Nikita (slow), 2012-07-31 16:06:58]

We're still not sure that this change should be committed.
If user selects custom wallpaper (considering it resizes to device resolution)
we delay login WebUI by the time it takes to load that custom wallpaper from
disk. I've seen difference in 3..4.5 seconds.
That should be async loading (as it is right now) I guess.

[bshe, 2012-07-31 16:28:20]

On 2012/07/31 16:06:58, Nikita Kostylev wrote:
> We're still not sure that this change should be committed.
> If user selects custom wallpaper (considering it resizes to device resolution)
> we delay login WebUI by the time it takes to load that custom wallpaper from
> disk. I've seen difference in 3..4.5 seconds.
> That should be async loading (as it is right now) I guess.

The custom wallpaper is async loading. On which device you tested the custom
wallpaper loading? Do you also include Ivan's cl for removing the unnecessary
png encoding in UserImage class? This is the most time consuming part.

[Nikita (slow), 2012-07-31 17:06:26]

This CL makes sign in WebUI wait for wallpaper animation on boot.

Good point about Ivan's CL, I haven't patched it.
31.07.2012 9:28 пользователь <bshe@chromium.org> написал:

> On 2012/07/31 16:06:58, Nikita Kostylev wrote:
>
>> We're still not sure that this change should be committed.
>> If user selects custom wallpaper (considering it resizes to device
>> resolution)
>> we delay login WebUI by the time it takes to load that custom wallpaper
>> from
>> disk. I've seen difference in 3..4.5 seconds.
>> That should be async loading (as it is right now) I guess.
>>
>
> The custom wallpaper is async loading. On which device you tested the
> custom
> wallpaper loading? Do you also include Ivan's cl for removing the
> unnecessary
> png encoding in UserImage class? This is the most time consuming part.
>
>
http://codereview.chromium.**org/10834081/<http://codereview.chromium.org/108...
>

[Ivan Korotkov, 2012-07-31 22:20:26]

lgtm

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/chromeos/bac...
File chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc (right):

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/chromeos/bac...
chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc:28: class
UserWallpaperDelegate: public ash::UserWallpaperDelegate {
space before :

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/chromeos/bac...
chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc:47: bool
disable_boot_animation =
Do you need a separate variable?

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/chromeos/bac...
chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc:76: // Setting
this variable would make sure that boot animation type
// Make sure that...

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/display_manager.js (right):

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/display_manager.js:369: return
document.documentElement.getAttribute('bootWallpaper') == 'on';
Since this is a JS-only flag and CSS stylization isn't needed, maybe just
extract it from localized strings and not add to html element?

[Nikita (slow), 2012-07-31 23:58:16]

On 2012/07/31 16:28:20, bshe wrote:
> Do you also include Ivan's cl for removing the unnecessary
> png encoding in UserImage class? This is the most time consuming part.

Yes, that was the case. Now waiting for custom wallpaper animation takes almost
the same time (difference is in 200-300ms) on boot.

[Nikita (slow), 2012-08-01 02:09:06]

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/chromeos/bac...
File chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc (right):

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/chromeos/bac...
chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc:28: class
UserWallpaperDelegate: public ash::UserWallpaperDelegate {
On 2012/07/31 22:20:26, Ivan Korotkov wrote:
> space before :

Done.

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/chromeos/bac...
chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc:47: bool
disable_boot_animation =
On 2012/07/31 22:20:26, Ivan Korotkov wrote:
> Do you need a separate variable?

I think it is a bit more readable this way.

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/chromeos/bac...
chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc:76: // Setting
this variable would make sure that boot animation type
On 2012/07/31 22:20:26, Ivan Korotkov wrote:
> // Make sure that...

Done.

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/display_manager.js (right):

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/display_manager.js:369: return
document.documentElement.getAttribute('bootWallpaper') == 'on';
On 2012/07/31 22:20:26, Ivan Korotkov wrote:
> Since this is a JS-only flag and CSS stylization isn't needed, maybe just
> extract it from localized strings and not add to html element?

Done.

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right):

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:247: // should trigger
wallpaper load on boot.
On 2012/07/31 16:04:23, bshe wrote:
> nit: more words can be fitted to previous line?

Done.

http://codereview.chromium.org/10834081/diff/7001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:249:
switches::kDisableBootAnimation)) {
On 2012/07/31 16:04:23, bshe wrote:
> nit: indent is off here.

I think it is not as it is essentially the same statement so should have 4
spaces indent.

[Nikita (slow), 2012-08-01 04:28:09]

+sky
Please do owners review for ash / chrome

[Nikita (slow), 2012-08-01 20:33:24]

-sky
+ben

Ben, could you please do owners review for
ash/
chrome/

[Nikita (slow), 2012-08-01 23:13:23]

+sky

[Ben Goodger (Google), 2012-08-02 15:38:39]

lgtm

http://codereview.chromium.org/10834081/diff/5004/chrome/browser/chromeos/bac...
File chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc (right):

http://codereview.chromium.org/10834081/diff/5004/chrome/browser/chromeos/bac...
chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc:37: if
(chromeos::UserManager::Get()->IsUserLoggedIn() ||
It's becoming hard to discern the intent of this function given the complexity
of the conditions. It'd be nice if we could somehow write this using anonymous
helper functions that express intent... 

e.g. 

if (NormalChange())
  return FADE;
elif (FirstBoot())
  return GRAYSCALE;

etc. Up to you. As it stands, it's a little hard to grok all the conditions
here, and seems like it might be easy to inadvertedly break.

http://codereview.chromium.org/10834081/diff/5004/chrome/browser/chromeos/bac...
chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc:54: return
ash::WINDOW_VISIBILITY_ANIMATION_TYPE_BRIGHTNESS_GRAYSCALE;
no else after return.

http://codereview.chromium.org/10834081/diff/5004/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/webui_login_display_host.cc (right):

http://codereview.chromium.org/10834081/diff/5004/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/webui_login_display_host.cc:66:
CommandLine::ForCurrentProcess()->
up to prev line

http://codereview.chromium.org/10834081/diff/5004/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/webui_login_display_host.cc:181:
user_wallpaper_delegate()->OnWallpaperBootAnimationFinished();
move the user_wallpaper.. up to the previous line.

[Nikita (slow), 2012-08-02 17:58:57]

http://codereview.chromium.org/10834081/diff/5004/chrome/browser/chromeos/bac...
File chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc (right):

http://codereview.chromium.org/10834081/diff/5004/chrome/browser/chromeos/bac...
chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc:37: if
(chromeos::UserManager::Get()->IsUserLoggedIn() ||
On 2012/08/02 15:38:39, Ben Goodger (Google) wrote:
> It's becoming hard to discern the intent of this function given the complexity
> of the conditions. It'd be nice if we could somehow write this using anonymous
> helper functions that express intent... 
> 
> e.g. 
> 
> if (NormalChange())
>   return FADE;
> elif (FirstBoot())
>   return GRAYSCALE;
> 
> etc. Up to you. As it stands, it's a little hard to grok all the conditions
> here, and seems like it might be easy to inadvertedly break.

Done.

http://codereview.chromium.org/10834081/diff/5004/chrome/browser/chromeos/bac...
chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc:54: return
ash::WINDOW_VISIBILITY_ANIMATION_TYPE_BRIGHTNESS_GRAYSCALE;
On 2012/08/02 15:38:39, Ben Goodger (Google) wrote:
> no else after return.

Done.

http://codereview.chromium.org/10834081/diff/5004/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/webui_login_display_host.cc (right):

http://codereview.chromium.org/10834081/diff/5004/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/webui_login_display_host.cc:66:
CommandLine::ForCurrentProcess()->
On 2012/08/02 15:38:39, Ben Goodger (Google) wrote:
> up to prev line

Done.

http://codereview.chromium.org/10834081/diff/5004/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/webui_login_display_host.cc:181:
user_wallpaper_delegate()->OnWallpaperBootAnimationFinished();
On 2012/08/02 15:38:39, Ben Goodger (Google) wrote:
> move the user_wallpaper.. up to the previous line.

Done.

[Ben Goodger (Google), 2012-08-02 20:39:12]

cool lgtm

[commit-bot: I haz the power, 2012-08-03 20:35:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nkostylev@chromium.org/10834081/19001

[commit-bot: I haz the power, 2012-08-03 20:35:13]

Presubmit check for 10834081-19001 failed and returned exit status 1.

Running presubmit commit checks ...
Traceback (most recent call last):
  File "/b/commit-queue/verification/presubmit_shim.py", line 43, in <module>
    sys.exit(presubmit_support.Main(argv))
  File "/b/depot_tools/presubmit_support.py", line 1257, in Main
    rietveld_obj)
  File "/b/depot_tools/presubmit_support.py", line 1105, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/b/depot_tools/presubmit_support.py", line 1022, in ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 531, in CheckChangeOnCommit
  File "<string>", line 435, in _CommonChecks
  File "/b/depot_tools/presubmit_canned_checks.py", line 920, in
PanProjectChecks
    input_api, output_api, source_file_filter=None))
  File "/b/depot_tools/presubmit_canned_checks.py", line 754, in CheckOwners
    approval_needed=input_api.is_committing)
  File "/b/depot_tools/presubmit_canned_checks.py", line 792, in
_RietveldOwnerAndReviewers
    int(input_api.change.issue), True)
  File "/b/depot_tools/rietveld.py", line 88, in get_issue_properties
    return json.loads(self.get(url))
  File "/b/depot_tools/rietveld.py", line 305, in get
    return self._send(request_path, **kwargs)
  File "/b/depot_tools/rietveld.py", line 330, in _send
    result = self.rpc_server.Send(request_path, **kwargs)
  File "/b/depot_tools/third_party/upload.py", line 409, in Send
    ErrorExit(e.read())
  File "/b/depot_tools/rietveld.py", line 322, in trap_http_500
    raise urllib2.HTTPError(request_path, m.group(1), msg, None, None)
urllib2.HTTPError: HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2012-08-03 21:07:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nkostylev@chromium.org/10834081/19001

[commit-bot: I haz the power, 2012-08-03 21:07:16]

Failed to apply patch for chrome/browser/chromeos/login/wallpaper_manager.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/login/wallpaper_manager.cc
Hunk #1 succeeded at 198 with fuzz 2.
Hunk #2 FAILED at 219.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/login/wallpaper_manager.cc.rej

[commit-bot: I haz the power, 2012-08-03 22:30:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nkostylev@chromium.org/10834081/19003

[commit-bot: I haz the power, 2012-08-03 22:30:09]

Failed to apply patch for chrome/browser/chromeos/login/wallpaper_manager.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/login/wallpaper_manager.cc
Hunk #1 succeeded at 198 with fuzz 2.
Hunk #2 FAILED at 219.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/login/wallpaper_manager.cc.rej

[commit-bot: I haz the power, 2012-08-03 23:54:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nkostylev@chromium.org/10834081/19003

[commit-bot: I haz the power, 2012-08-04 00:14:14]

Try job failure for 10834081-19003 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...