Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24625003:
Delay wallpaper load by 2 * average wallpaper load time. (Closed)
|
Created:
7 years, 2 months ago by Alexander Alekseev Modified:
7 years, 1 month ago Reviewers:
dzhioev (left Google) CC:
chromium-reviews, stevenjb+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDelay wallpaper load by 2 * average wallpaper load time.
BUG=279102
TEST=manual
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230138
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234596
Patch Set 1 #
Total comments: 20
Patch Set 2 : Catch WallpaperAnimationFinished event and inform JS. #
Total comments: 1
Patch Set 3 : Move WallpaperLoader to separate object, simplify. #
Total comments: 32
Patch Set 4 : Review comments fixed. #
Total comments: 7
Patch Set 5 : Remove unused variable. #Patch Set 6 : Fix clang build. #Patch Set 7 : Use user.username instead of user.emailAddress . #Patch Set 8 : Rebased. #
Messages
Total messages: 17 (0 generated)
Please review.
https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... File chrome/browser/resources/chromeos/login/screen_account_picker.js (right): https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/screen_account_picker.js:30: 'wallpaperLoaded' I think, this method should be renamed to "onWallpaperLoaded". https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... File chrome/browser/resources/chromeos/login/user_pod_row.js (right): https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:948: keyboardActivated_: false, keyboardActivated_ is not used anymore and can be deleted. https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:966: wallpaperLoadInProgress: {}, Please respect convention about naming private members ("_" suffix) https://codereview.chromium.org/24625003/diff/1/chrome/browser/ui/webui/chrom... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/24625003/diff/1/chrome/browser/ui/webui/chrom... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1286: CallJS("login.AccountPickerScreen.wallpaperLoaded", email); It should be called after wallpaper was loaded.
https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... File chrome/browser/resources/chromeos/login/user_pod_row.js (right): https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:982: totalTimeMs: 0 Are you sure that it is needed? I think it is premature optimization. https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:1338: wallpaperLoadAvg: function() { get*_ https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:1362: var elapsed = (started ? Brackets are excess here. https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:1363: finished.getUTCMilliseconds() - started.getUTCMilliseconds() : I believe that getUTCMilliseconds is not what you want as it returns number in 0-999. getTime should be used instead. Or implicit conversion: finished.getTime() - started.getTime() == fininshed - started. https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:1372: var nextAfter = new Date; You use "new Date" and "new Date()". Choose one form. https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:1381: this.scheduleLoadWallpaper(this.wallpaperLoadPending); This call is useless because scheduleLoadWallpaper starts with: if (this.wallpaperLoadPending && this.wallpaperLoadPending == email) return;
Please review again. https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... File chrome/browser/resources/chromeos/login/screen_account_picker.js (right): https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/screen_account_picker.js:30: 'wallpaperLoaded' On 2013/09/26 17:46:41, dzhioev wrote: > I think, this method should be renamed to "onWallpaperLoaded". Done. https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... File chrome/browser/resources/chromeos/login/user_pod_row.js (right): https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:948: keyboardActivated_: false, On 2013/09/26 17:46:41, dzhioev wrote: > keyboardActivated_ is not used anymore and can be deleted. Done. https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:966: wallpaperLoadInProgress: {}, On 2013/09/26 17:46:41, dzhioev wrote: > Please respect convention about naming private members ("_" suffix) Done. https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:1338: wallpaperLoadAvg: function() { On 2013/09/27 09:04:39, dzhioev wrote: > get*_ Done. https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:1363: finished.getUTCMilliseconds() - started.getUTCMilliseconds() : On 2013/09/27 09:04:39, dzhioev wrote: > I believe that getUTCMilliseconds is not what you want as it returns number in > 0-999. getTime should be used instead. Or implicit conversion: > finished.getTime() - started.getTime() == fininshed - started. Done. https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:1372: var nextAfter = new Date; On 2013/09/27 09:04:39, dzhioev wrote: > You use "new Date" and "new Date()". Choose one form. Done. https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:1381: this.scheduleLoadWallpaper(this.wallpaperLoadPending); On 2013/09/27 09:04:39, dzhioev wrote: > This call is useless because scheduleLoadWallpaper starts with: > if (this.wallpaperLoadPending && this.wallpaperLoadPending == email) > return; Done.
Please review. https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... File chrome/browser/resources/chromeos/login/user_pod_row.js (right): https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:982: totalTimeMs: 0 On 2013/09/27 09:04:39, dzhioev wrote: > Are you sure that it is needed? I think it is premature optimization. Done. https://codereview.chromium.org/24625003/diff/1/chrome/browser/resources/chro... chrome/browser/resources/chromeos/login/user_pod_row.js:1362: var elapsed = (started ? On 2013/09/27 09:04:39, dzhioev wrote: > Brackets are excess here. Done. https://codereview.chromium.org/24625003/diff/1/chrome/browser/ui/webui/chrom... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/24625003/diff/1/chrome/browser/ui/webui/chrom... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1286: CallJS("login.AccountPickerScreen.wallpaperLoaded", email); On 2013/09/26 17:46:41, dzhioev wrote: > It should be called after wallpaper was loaded. Done.
https://codereview.chromium.org/24625003/diff/9001/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/24625003/diff/9001/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/wallpaper_manager.h:89: typedef std::list<base::WeakPtr<Observer> > ObserverList; Use ObserverList from base/observer_list.h https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... File chrome/browser/resources/chromeos/login/wallpaper_loader.js (right): https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:56: wallpaperLoadInProgress_: null, We load only one wallpaper at time, so such dictionary is not needed. I propose wallpaperLoadInProgress_ to be object with fields 'name' and 'startTime'. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:65: wallpaperCurrent_: '', currentWallpaper_ https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:69: wallpaperLoadStats_: undefined, Just loadStats_ https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:71: reset: function() { You can delete 'wallpaperLoadPending_' in this method. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:71: reset: function() { Please add comment for method. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:72: if (this.loadWallpaperTimeout_) I think setTimeout can return 0 as a valid ID. So it's better to check explicitly that 'loadWallpaperTimeout_' is not 'null' ( !== null ) https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:73: clearTimeout(this.loadWallpaperTimeout_); window.clearTimeout https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:94: if (this.wallpaperLoadTryNextAfter_ && I think this if-block can be simplified to: if (this.wallpaperLoadTryNextAfter_) timeout = Math.max(timeout, this.wallpaperLoadTryNextAfter_ - Date.now()); https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:98: timeout = this.wallpaperLoadTryNextAfter_.getTime() - You can drop getTime() in arithmetic calculations with Date, because Dete implicitly converts to 'ms since epoch'. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:130: avg = this.wallpaperLoadStats_.reduce( Maybe it should be moved to separate method. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:131: function(previousValue, currentValue, index, array) { You can drop 'index' and 'array'. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:139: } Brackets not needed for one line body. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:157: WALLPAPER_LOAD_STATS_MAX_LENGTH) No need to split condition that fits in one line. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:163: this.wallpaperLoadTryNextAfter_ = nextAfter; I think we delay should be multiplied by 2 inside getWallpaperLoadDelay_ method. Also previous 3 lines can be simplified to: this.wallpaperLoadTryNextAfter_ = new Date(Date.now() + this.getWallpaperLoadDelay_()); https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:167: if (newWallpaperEmail == email) { How this can happen? I thought it's already checked in scheduleLoad. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/ui/webui/c... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/24625003/diff/24001/chrome/browser/ui/webui/c... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:559: WallpaperManager::Get()->AddObserver(this); Why do you add observer here? I see that |this| added as observer to network state informer in constructor. I don't know which way is correct though.
Please review. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... File chrome/browser/resources/chromeos/login/wallpaper_loader.js (right): https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:56: wallpaperLoadInProgress_: null, On 2013/10/17 12:47:49, dzhioev wrote: > We load only one wallpaper at time, so such dictionary is not needed. I propose > wallpaperLoadInProgress_ to be object with fields 'name' and 'startTime'. Done. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:65: wallpaperCurrent_: '', On 2013/10/17 12:47:49, dzhioev wrote: > currentWallpaper_ Done. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:69: wallpaperLoadStats_: undefined, On 2013/10/17 12:47:49, dzhioev wrote: > Just loadStats_ Done. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:71: reset: function() { On 2013/10/17 12:47:49, dzhioev wrote: > You can delete 'wallpaperLoadPending_' in this method. Done. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:71: reset: function() { On 2013/10/17 12:47:49, dzhioev wrote: > You can delete 'wallpaperLoadPending_' in this method. Done. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:72: if (this.loadWallpaperTimeout_) On 2013/10/17 12:47:49, dzhioev wrote: > I think setTimeout can return 0 as a valid ID. So it's better to check > explicitly that 'loadWallpaperTimeout_' is not 'null' ( !== null ) probably if (loadWallpaperTimeout_ != null) ? https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:73: clearTimeout(this.loadWallpaperTimeout_); On 2013/10/17 12:47:49, dzhioev wrote: > window.clearTimeout Done. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:94: if (this.wallpaperLoadTryNextAfter_ && On 2013/10/17 12:47:49, dzhioev wrote: > I think this if-block can be simplified to: > if (this.wallpaperLoadTryNextAfter_) > timeout = Math.max(timeout, this.wallpaperLoadTryNextAfter_ - Date.now()); Done. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:98: timeout = this.wallpaperLoadTryNextAfter_.getTime() - On 2013/10/17 12:47:49, dzhioev wrote: > You can drop getTime() in arithmetic calculations with Date, because Dete > implicitly converts to 'ms since epoch'. Done. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:130: avg = this.wallpaperLoadStats_.reduce( On 2013/10/17 12:47:49, dzhioev wrote: > Maybe it should be moved to separate method. Done. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:131: function(previousValue, currentValue, index, array) { On 2013/10/17 12:47:49, dzhioev wrote: > You can drop 'index' and 'array'. Done. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:139: } On 2013/10/17 12:47:49, dzhioev wrote: > Brackets not needed for one line body. Done. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:157: WALLPAPER_LOAD_STATS_MAX_LENGTH) On 2013/10/17 12:47:49, dzhioev wrote: > No need to split condition that fits in one line. Done. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:163: this.wallpaperLoadTryNextAfter_ = nextAfter; On 2013/10/17 12:47:49, dzhioev wrote: > I think we delay should be multiplied by 2 inside getWallpaperLoadDelay_ method. > Also previous 3 lines can be simplified to: > this.wallpaperLoadTryNextAfter_ = new Date(Date.now() + > this.getWallpaperLoadDelay_()); Done. I've renamed getWallpaperLoadDelay_ to getWallpaperLoadTime_ to clarify it's purpose. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:167: if (newWallpaperEmail == email) { On 2013/10/17 12:47:49, dzhioev wrote: > How this can happen? I thought it's already checked in scheduleLoad. Done. https://codereview.chromium.org/24625003/diff/24001/chrome/browser/ui/webui/c... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/24625003/diff/24001/chrome/browser/ui/webui/c... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:559: WallpaperManager::Get()->AddObserver(this); On 2013/10/17 12:47:49, dzhioev wrote: > Why do you add observer here? I see that |this| added as observer to network > state informer in constructor. I don't know which way is correct though. Hmmm... I don't reember why, but there was a reason. Probably the reason has gone. Moved to constructor.
LGTM with several remarks. https://codereview.chromium.org/24625003/diff/31001/chrome/browser/resources/... File chrome/browser/resources/chromeos/login/user_pod_row.js (right): https://codereview.chromium.org/24625003/diff/31001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/user_pod_row.js:5: <include src="wallpaper_loader.js"></include> As I know, closing tag is not needed. https://codereview.chromium.org/24625003/diff/31001/chrome/browser/resources/... File chrome/browser/resources/chromeos/login/wallpaper_loader.js (right): https://codereview.chromium.org/24625003/diff/31001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:44: this.wallpaperLoadInProgress_ = { Isn't it better to keep 'wallpaperLoadInProgress_' == null, if wallpaper is not loading? https://codereview.chromium.org/24625003/diff/31001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:46: date: undefined startDate https://codereview.chromium.org/24625003/diff/31001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:173: this.wallpaperLoadTryNextAfter_ = nextAfter; You can assign 'wallpaperLoadTryNextAfter_' directly, without using temporarily variable.
https://codereview.chromium.org/24625003/diff/31001/chrome/browser/resources/... File chrome/browser/resources/chromeos/login/user_pod_row.js (right): https://codereview.chromium.org/24625003/diff/31001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/user_pod_row.js:5: <include src="wallpaper_loader.js"></include> On 2013/10/22 00:01:19, dzhioev wrote: > As I know, closing tag is not needed. Nevertheless it seems it's always used with closing tag. https://codereview.chromium.org/24625003/diff/31001/chrome/browser/resources/... File chrome/browser/resources/chromeos/login/wallpaper_loader.js (right): https://codereview.chromium.org/24625003/diff/31001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:44: this.wallpaperLoadInProgress_ = { On 2013/10/22 00:01:19, dzhioev wrote: > Isn't it better to keep 'wallpaperLoadInProgress_' == null, if wallpaper is not > loading? This is to avoid double checks like: if (wallpaperLoadInProgress_ && wallpaperLoadInProgress_.name == email) https://codereview.chromium.org/24625003/diff/31001/chrome/browser/resources/... chrome/browser/resources/chromeos/login/wallpaper_loader.js:173: this.wallpaperLoadTryNextAfter_ = nextAfter; On 2013/10/22 00:01:19, dzhioev wrote: > You can assign 'wallpaperLoadTryNextAfter_' directly, without using temporarily > variable. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/24625003/91001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/24625003/301001
Message was sent while issue was closed.
Change committed as 230138
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/24625003/501001
Failed to apply patch for
chrome/browser/resources/chromeos/login/user_pod_row.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/resources/chromeos/login/user_pod_row.js
Hunk #8 FAILED at 1256.
Hunk #9 succeeded at 1544 with fuzz 1 (offset 11 lines).
1 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/resources/chromeos/login/user_pod_row.js.rej
Patch: chrome/browser/resources/chromeos/login/user_pod_row.js
Index: chrome/browser/resources/chromeos/login/user_pod_row.js
diff --git a/chrome/browser/resources/chromeos/login/user_pod_row.js
b/chrome/browser/resources/chromeos/login/user_pod_row.js
index
7969895a095c6c517b9d7d119978dcbb514bce38..2ee5b8ab34102112e20e79151172499a06f14dfb
100644
--- a/chrome/browser/resources/chromeos/login/user_pod_row.js
+++ b/chrome/browser/resources/chromeos/login/user_pod_row.js
@@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+<include src="wallpaper_loader.js"></include>
+
/**
* @fileoverview User pod row implementation.
*/
@@ -22,20 +24,6 @@ cr.define('login', function() {
var PRESELECT_FIRST_POD = true;
/**
- * Wallpaper load delay in milliseconds.
- * @type {number}
- * @const
- */
- var WALLPAPER_LOAD_DELAY_MS = 500;
-
- /**
- * Wallpaper load delay in milliseconds. TODO(nkostylev): Tune this constant.
- * @type {number}
- * @const
- */
- var WALLPAPER_BOOT_LOAD_DELAY_MS = 100;
-
- /**
* Maximum time for which the pod row remains hidden until all user images
* have been loaded.
* @type {number}
@@ -959,17 +947,9 @@ cr.define('login', function() {
// Whether this user pod row is shown for the first time.
firstShown_: true,
- // Whether the initial wallpaper load after boot has been requested. Used
- // only if |Oobe.getInstance().shouldLoadWallpaperOnBoot()| is true.
- bootWallpaperLoaded_: false,
-
// True if inside focusPod().
insideFocusPod_: false,
- // True if user pod has been activated with keyboard.
- // In case of activation with keyboard we delay wallpaper change.
- keyboardActivated_: false,
-
// Focused pod.
focusedPod_: undefined,
@@ -979,9 +959,8 @@ cr.define('login', function() {
// Pod that was most recently focused, if any.
lastFocusedPod_: undefined,
- // When moving through users quickly at login screen, set a timeout to
- // prevent loading intermediate wallpapers.
- loadWallpaperTimeout_: null,
+ // Note: created only in decorate() !
+ wallpaperLoader_: undefined,
// Pods whose initial images haven't been loaded yet.
podsWithPendingImages_: [],
@@ -998,6 +977,7 @@ cr.define('login', function() {
mousemove: [this.handleMouseMove_.bind(this), false],
keydown: [this.handleKeyDown.bind(this), false]
};
+ this.wallpaperLoader_ = new login.WallpaperLoader();
},
/**
@@ -1242,7 +1222,7 @@ cr.define('login', function() {
}
this.insideFocusPod_ = true;
- clearTimeout(this.loadWallpaperTimeout_);
+ this.wallpaperLoader_.reset();
for (var i = 0, pod; pod = this.pods[i]; ++i) {
if (!this.isSinglePod) {
pod.isActionBoxMenuActive = false;
@@ -1265,16 +1245,9 @@ cr.define('login', function() {
podToFocus.classList.remove('faded');
podToFocus.classList.add('focused');
podToFocus.reset(true); // Reset and give focus.
- chrome.send('focusPod', [podToFocus.user.emailAddress]);
- if (hadFocus && this.keyboardActivated_) {
- // Delay wallpaper loading to let user tab through pods without lag.
- this.loadWallpaperTimeout_ = window.setTimeout(
- this.loadWallpaper_.bind(this), WALLPAPER_LOAD_DELAY_MS);
- } else if (!this.firstShown_) {
- // Load wallpaper immediately if there no pod was focused
- // previously, and it is not a boot into user pod list case.
- this.loadWallpaper_();
- }
+ chrome.send('focusPod', [podToFocus.user.username]);
+
+ this.wallpaperLoader_.scheduleLoad(podToFocus.user.username);
this.firstShown_ = false;
this.lastFocusedPod_ = podToFocus;
}
@@ -1283,20 +1256,19 @@ cr.define('login', function() {
},
/**
- * Loads wallpaper for the active user pod, if any.
- * @private
+ * Resets wallpaper to the last active user's wallpaper, if any.
*/
- loadWallpaper_: function() {
- if (this.focusedPod_)
- chrome.send('loadWallpaper', [this.focusedPod_.user.username]);
+ loadLastWallpaper: function() {
+ if (this.lastFocusedPod_)
+ this.wallpaperLoader_.scheduleLoad(this.lastFocusedPod_.user.username);
},
/**
- * Resets wallpaper to the last active user's wallpaper, if any.
+ * Handles 'onWallpaperLoaded' event. Recalculates statistics and
+ * [re]schedules next wallpaper load.
*/
- loadLastWallpaper: function() {
- if (this.lastFocusedPod_)
- chrome.send('loadWallpaper', [this.lastFocusedPod_.user.username]);
+ onWallpaperLoaded: function(username) {
+ this.wallpaperLoader_.onWallpaperLoaded(username);
},
/**
@@ -1560,13 +1532,7 @@ cr.define('login', function() {
focusedPod.reset(true);
// Notify screen that it is ready.
screen.onShow();
- // Boot transition: load wallpaper.
- if (!self.bootWallpaperLoaded_ &&
- Oobe.getInstance().shouldLoadWallpaperOnBoot()) {
- self.loadWallpaperTimeout_ = window.setTimeout(
- self.loadWallpaper_.bind(self),
WALLPAPER_BOOT_LOAD_DELAY_MS);
- self.bootWallpaperLoaded_ = true;
- }
+ self.wallpaperLoader_.scheduleLoad(focusedPod.user.username);
}
});
}
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/24625003/601001
Message was sent while issue was closed.
Change committed as 234596 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
