Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2931063004:
Extract colors from wallpaper and dynamically update login screen overlay (Closed)
|
Created:
3 years, 6 months ago by Wenzhao (Colin) Zang Modified:
3 years, 6 months ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, rkc, Alexander Alekseev Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtract colors from wallpaper and dynamically update login screen overlay
In SigninScreenHandler there's already a 'loadWallpaper' handler. We can
add additional color extraction calculation after the wallpaper is
fetched and is being loaded.
Plaese note:
1) SigninScreenHandler is an observer for WallpaperManager, and
WallpaperManager is an observer for WallpaperColorCalculator.
2) The calculation is specific to the login screen. The interface
is not made more general because it's unlikely the color needed by the
login screen will change, nor the function needs to be reused.
3) Codes for caching color extraction results can be added if necessary.
There's no performance issue for an eve device.
BUG=718156
Review-Url: https://codereview.chromium.org/2931063004
Cr-Commit-Position: refs/heads/master@{#480616}
Committed: https://chromium.googlesource.com/chromium/src/+/11d5ed105384e572c6a2617126426980874a15a4
Patch Set 1 #Patch Set 2 : Add image-loading class to small pods container #
Total comments: 18
Patch Set 3 : Address comments #
Total comments: 32
Patch Set 4 : Address comments #Patch Set 5 : Move unrelated style changes to a separate CL #
Total comments: 17
Patch Set 6 : Address comments #
Total comments: 2
Patch Set 7 : Address comments and rebase with master #
Total comments: 4
Patch Set 8 : Add RemoveObserver() and TODO #Patch Set 9 : Rebase with master, no other changes #Patch Set 10 : Check nullptr before removing observer #Patch Set 11 : Delete RemoveObserver #Patch Set 12 : Add nullptr checking #Patch Set 13 : Replace background with backgroundColor in JS #Patch Set 14 : Test with AddObserver #Patch Set 15 : Observe WallpaperController instead of WallpaperManager #Patch Set 16 : Change back to WallpaperManagerBase::Observer #Patch Set 17 : Will clean up this CL when crbug.com/733709 is fixed #Messages
Total messages: 89 (67 generated)
Description was changed from ========== Extract colors from wallpaper and dynamically update login screen overlay BUG=718156 ========== to ========== Extract colors from wallpaper and dynamically update login screen overlay In SigninScreenHandler there's already a 'loadWallpaper' handler. We can add additional color extraction calculation after the wallpaper is fetched and is being loaded. Plaese note: 1) We avoid creating a new 'WallpaperManagerObserver' class by using the SigninScreenHandler directly as the observer for the color calculator. 2) The calculation is specific to the login screen. The interface is not made more general because it's unlikely the color needed by the login screen will be changed, nor the function needs to be reused. 3) Codes for caching color extraction results can be added if deemed necessary. There's no performance issue for an eve device. BUG=718156 ==========
wzang@chromium.org changed reviewers: + jdufault@chromium.org
Description was changed from ========== Extract colors from wallpaper and dynamically update login screen overlay In SigninScreenHandler there's already a 'loadWallpaper' handler. We can add additional color extraction calculation after the wallpaper is fetched and is being loaded. Plaese note: 1) We avoid creating a new 'WallpaperManagerObserver' class by using the SigninScreenHandler directly as the observer for the color calculator. 2) The calculation is specific to the login screen. The interface is not made more general because it's unlikely the color needed by the login screen will be changed, nor the function needs to be reused. 3) Codes for caching color extraction results can be added if deemed necessary. There's no performance issue for an eve device. BUG=718156 ========== to ========== Extract colors from wallpaper and dynamically update login screen overlay In SigninScreenHandler there's already a 'loadWallpaper' handler. We can add additional color extraction calculation after the wallpaper is fetched and is being loaded. Plaese note: 1) We avoid creating a new 'WallpaperManagerObserver' class by using the SigninScreenHandler directly as the observer for the color calculator. 2) The calculation is specific to the login screen. The interface is not made more general because it's unlikely the color needed by the login screen will be changed, nor the function needs to be reused. 3) Codes for caching color extraction results can be added if deemed necessary. There's no performance issue for an eve device. BUG=718156 ==========
Description was changed from ========== Extract colors from wallpaper and dynamically update login screen overlay In SigninScreenHandler there's already a 'loadWallpaper' handler. We can add additional color extraction calculation after the wallpaper is fetched and is being loaded. Plaese note: 1) We avoid creating a new 'WallpaperManagerObserver' class by using the SigninScreenHandler directly as the observer for the color calculator. 2) The calculation is specific to the login screen. The interface is not made more general because it's unlikely the color needed by the login screen will be changed, nor the function needs to be reused. 3) Codes for caching color extraction results can be added if deemed necessary. There's no performance issue for an eve device. BUG=718156 ========== to ========== Extract colors from wallpaper and dynamically update login screen overlay In SigninScreenHandler there's already a 'loadWallpaper' handler. We can add additional color extraction calculation after the wallpaper is fetched and is being loaded. Plaese note: 1) We avoid creating a new 'WallpaperManagerObserver' class by using the SigninScreenHandler directly as the observer for the color calculator. 2) The calculation is specific to the login screen. The interface is not made more general because it's unlikely the color needed by the login screen will be changed, nor the function needs to be reused. 3) Codes for caching color extraction results can be added if deemed necessary. There's no performance issue for an eve device. BUG=718156 ==========
Description was changed from ========== Extract colors from wallpaper and dynamically update login screen overlay In SigninScreenHandler there's already a 'loadWallpaper' handler. We can add additional color extraction calculation after the wallpaper is fetched and is being loaded. Plaese note: 1) We avoid creating a new 'WallpaperManagerObserver' class by using the SigninScreenHandler directly as the observer for the color calculator. 2) The calculation is specific to the login screen. The interface is not made more general because it's unlikely the color needed by the login screen will be changed, nor the function needs to be reused. 3) Codes for caching color extraction results can be added if deemed necessary. There's no performance issue for an eve device. BUG=718156 ========== to ========== Extract colors from wallpaper and dynamically update login screen overlay In SigninScreenHandler there's already a 'loadWallpaper' handler. We can add additional color extraction calculation after the wallpaper is fetched and is being loaded. Plaese note: 1) We avoid creating a new 'WallpaperManagerObserver' class by using the SigninScreenHandler directly as the observer for the color calculator. 2) The calculation is specific to the login screen. The interface is not made more general because it's unlikely the color needed by the login screen will change, nor the function needs to be reused. 3) Codes for caching color extraction results can be added if necessary. There's no performance issue for an eve device. BUG=718156 ==========
https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:1502: *dm_color = color_utils::SkColorToRgbaString(dm_sk_color); Output the SkColor values and have the string transformation happen at the call site when sending to JS/HTML. https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h (right): https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:120: // Setting a non-NULL observer indicates that color calculation is needed nit: non-null https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:121: // when a wallpaper is set. Generally, try to avoid encoding state information in a null/non-null value. Use base::Optional for that instead, as it conveys more semantic meaning. https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:122: void SetLoginOverlayCalculatorObserver( These functions should go above the overrides (overrides go last). https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:122: void SetLoginOverlayCalculatorObserver( This should follow the standard AddObserver/RemoveObserver pattern. See base::ObserverList. Register SigninScreenHandler as an observer on construction, and deregister in SigninScreenHandler dtor. https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:123: wallpaper::WallpaperColorCalculatorObserver*); Set a variable name here. https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:127: void getLoginOverlayColors(std::string* dm_color, nit: get -> Get https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:127: void getLoginOverlayColors(std::string* dm_color, nit: Out parameters should have out_ prefix. https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:177: // If the calculator observer is not NULL, calculate a prominent color based nit: NULL -> null
Description was changed from ========== Extract colors from wallpaper and dynamically update login screen overlay In SigninScreenHandler there's already a 'loadWallpaper' handler. We can add additional color extraction calculation after the wallpaper is fetched and is being loaded. Plaese note: 1) We avoid creating a new 'WallpaperManagerObserver' class by using the SigninScreenHandler directly as the observer for the color calculator. 2) The calculation is specific to the login screen. The interface is not made more general because it's unlikely the color needed by the login screen will change, nor the function needs to be reused. 3) Codes for caching color extraction results can be added if necessary. There's no performance issue for an eve device. BUG=718156 ========== to ========== Extract colors from wallpaper and dynamically update login screen overlay In SigninScreenHandler there's already a 'loadWallpaper' handler. We can add additional color extraction calculation after the wallpaper is fetched and is being loaded. Plaese note: 1) SigninScreenHandler is an observer for WallpaperManager, and WallpaperManager is an observer for WallpaperColorCalculator. 2) The calculation is specific to the login screen. The interface is not made more general because it's unlikely the color needed by the login screen will change, nor the function needs to be reused. 3) Codes for caching color extraction results can be added if necessary. There's no performance issue for an eve device. BUG=718156 ==========
https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:1502: *dm_color = color_utils::SkColorToRgbaString(dm_sk_color); On 2017/06/10 03:05:59, jdufault wrote: > Output the SkColor values and have the string transformation happen at the call > site when sending to JS/HTML. Done. https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h (right): https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:120: // Setting a non-NULL observer indicates that color calculation is needed On 2017/06/10 03:05:59, jdufault wrote: > nit: non-null Done. https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:121: // when a wallpaper is set. On 2017/06/10 03:05:59, jdufault wrote: > Generally, try to avoid encoding state information in a null/non-null value. Use > base::Optional for that instead, as it conveys more semantic meaning. Done. https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:122: void SetLoginOverlayCalculatorObserver( On 2017/06/10 03:05:59, jdufault wrote: > These functions should go above the overrides (overrides go last). Done. https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:122: void SetLoginOverlayCalculatorObserver( On 2017/06/10 03:05:59, jdufault wrote: > This should follow the standard AddObserver/RemoveObserver pattern. See > base::ObserverList. Register SigninScreenHandler as an observer on construction, > and deregister in SigninScreenHandler dtor. Done. https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:123: wallpaper::WallpaperColorCalculatorObserver*); On 2017/06/10 03:05:59, jdufault wrote: > Set a variable name here. Done. https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:127: void getLoginOverlayColors(std::string* dm_color, On 2017/06/10 03:05:59, jdufault wrote: > nit: Out parameters should have out_ prefix. Done. https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:127: void getLoginOverlayColors(std::string* dm_color, On 2017/06/10 03:05:59, jdufault wrote: > nit: get -> Get Done. https://codereview.chromium.org/2931063004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:177: // If the calculator observer is not NULL, calculate a prominent color based On 2017/06/10 03:05:59, jdufault wrote: > nit: NULL -> null Done.
https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:433: color_calculator_->RemoveObserver(this); Add a comment along the lines of // Cancel any in-flight color calculations https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:437: color_utils::LumaRange luma = color_utils::LumaRange::DARK; Eliminate the |luma| and |saturation| temporaries. They don't provide additional semantic value and the typing reduction is minimal. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:933: color_calculator_.reset(); Add a comment explaining why you cache color instead of just using color_calculator_->prominent_color() directly, ie, SetProminentColor(color_calculator_->prominent_color()); color_calculator_.reset(). https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h (right): https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:258: // SK_ColorTRANSPARENT is used if color extraction fails. Use base::Optional<>. Try to avoid using magic values for failure state, better to encode it into the type system. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:983: void SigninScreenHandler::SetSigninScreenColors(const SkColor& dm_color) { Just pass a SkColor dm_color parameter and mutate it. FYI, SkColor is an int so no performance penalty. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:985: SkColor dm_sk_color = dm_color; avoid non-obvious acronyms (dm). It makes the code harder to quickly parse/read. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:986: dm_sk_color = SkColorSetA(dm_sk_color, 0xFF); FYI: naming here is confusing; what is the difference between dm_sk_color and dm_color? This should get cleaned up by mutating param directly. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:991: SkColor background_sk_color = background_sk_color -> background_color https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:994: // moved to a separate file when views-based signin screen is being Add TODO and file a bug. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:996: constexpr int kLoginTranslucentAlpha = 76; move to anonymous namespace https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:997: SkColor scroll_sk_color = SkColorSetA(base_sk_color, kLoginTranslucentAlpha); scroll_sk_color -> scroll_color https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:998: // Transform SkColor values to rgba strings. Remove comment, it is immediately obvious. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right): https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:282: // WallpaperManager::Observer implementation. This doesn't look like the right type. https://codereview.chromium.org/2931063004/diff/40001/ui/login/account_picker... File ui/login/account_picker/md_screen_account_picker.js (right): https://codereview.chromium.org/2931063004/diff/40001/ui/login/account_picker... ui/login/account_picker/md_screen_account_picker.js:92: setOverlayColors: function(dmColor, scrollColor, backgroundColor) { expand dm acronym https://codereview.chromium.org/2931063004/diff/40001/ui/login/account_picker... File ui/login/account_picker/md_user_pod_row.js (right): https://codereview.chromium.org/2931063004/diff/40001/ui/login/account_picker... ui/login/account_picker/md_user_pod_row.js:2814: overlayColors_: {dmColor: undefined, scrollColor: undefined}, expand dm acronym https://codereview.chromium.org/2931063004/diff/40001/ui/login/account_picker... ui/login/account_picker/md_user_pod_row.js:3705: if (this.overlayColors_.scrollColor) nit: {}
https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:433: color_calculator_->RemoveObserver(this); On 2017/06/12 21:37:28, jdufault wrote: > Add a comment along the lines of > > // Cancel any in-flight color calculations Done. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:437: color_utils::LumaRange luma = color_utils::LumaRange::DARK; On 2017/06/12 21:37:28, jdufault wrote: > Eliminate the |luma| and |saturation| temporaries. They don't provide additional > semantic value and the typing reduction is minimal. Done. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:933: color_calculator_.reset(); On 2017/06/12 21:37:28, jdufault wrote: > Add a comment explaining why you cache color instead of just using > color_calculator_->prominent_color() directly, ie, > > SetProminentColor(color_calculator_->prominent_color()); > color_calculator_.reset(). Changed to using the color directly. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h (right): https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:258: // SK_ColorTRANSPARENT is used if color extraction fails. On 2017/06/12 21:37:28, jdufault wrote: > Use base::Optional<>. Try to avoid using magic values for failure state, better > to encode it into the type system. Done. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:983: void SigninScreenHandler::SetSigninScreenColors(const SkColor& dm_color) { On 2017/06/12 21:37:28, jdufault wrote: > Just pass a SkColor dm_color parameter and mutate it. FYI, SkColor is an int so > no performance penalty. Done. Originally I wanted to prevent mistakenly changing this value but it seems not necessary. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:985: SkColor dm_sk_color = dm_color; On 2017/06/12 21:37:29, jdufault wrote: > avoid non-obvious acronyms (dm). It makes the code harder to quickly parse/read. Done. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:986: dm_sk_color = SkColorSetA(dm_sk_color, 0xFF); On 2017/06/12 21:37:29, jdufault wrote: > FYI: naming here is confusing; what is the difference between dm_sk_color and > dm_color? This should get cleaned up by mutating param directly. Done. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:991: SkColor background_sk_color = On 2017/06/12 21:37:29, jdufault wrote: > background_sk_color -> background_color Done. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:994: // moved to a separate file when views-based signin screen is being On 2017/06/12 21:37:29, jdufault wrote: > Add TODO and file a bug. Done. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:996: constexpr int kLoginTranslucentAlpha = 76; On 2017/06/12 21:37:29, jdufault wrote: > move to anonymous namespace Done. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:997: SkColor scroll_sk_color = SkColorSetA(base_sk_color, kLoginTranslucentAlpha); On 2017/06/12 21:37:29, jdufault wrote: > scroll_sk_color -> scroll_color Done. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:998: // Transform SkColor values to rgba strings. On 2017/06/12 21:37:28, jdufault wrote: > Remove comment, it is immediately obvious. Done. https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right): https://codereview.chromium.org/2931063004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:282: // WallpaperManager::Observer implementation. On 2017/06/12 21:37:29, jdufault wrote: > This doesn't look like the right type. I think it's the right type. Other WallpaperManager observers, such as TestWallpaperObserver and TestObserver have the same type. https://codereview.chromium.org/2931063004/diff/40001/ui/login/account_picker... File ui/login/account_picker/md_screen_account_picker.js (right): https://codereview.chromium.org/2931063004/diff/40001/ui/login/account_picker... ui/login/account_picker/md_screen_account_picker.js:92: setOverlayColors: function(dmColor, scrollColor, backgroundColor) { On 2017/06/12 21:37:29, jdufault wrote: > expand dm acronym I changed it to maskColor which may make more sense for the user pod row. https://codereview.chromium.org/2931063004/diff/40001/ui/login/account_picker... File ui/login/account_picker/md_user_pod_row.js (right): https://codereview.chromium.org/2931063004/diff/40001/ui/login/account_picker... ui/login/account_picker/md_user_pod_row.js:2814: overlayColors_: {dmColor: undefined, scrollColor: undefined}, On 2017/06/12 21:37:29, jdufault wrote: > expand dm acronym Done. https://codereview.chromium.org/2931063004/diff/40001/ui/login/account_picker... ui/login/account_picker/md_user_pod_row.js:3705: if (this.overlayColors_.scrollColor) On 2017/06/12 21:37:29, jdufault wrote: > nit: {} Done.
wzang@chromium.org changed reviewers: + xdai@chromium.org, xiyuan@chromium.org
xdai@, please look at wallpaper_manager_base.h and wallpaper_manager.cc. Thanks so much. xiyuan@, please take a look at the other files. Thanks so much. This CL does signin screen colorization based on wallpaper, in a similar way with the shelf. These codes will likely be rewritten when the views-based signin screen is being developed.
See the inline comments. https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:439: image, color_utils::LumaRange::DARK, color_utils::SaturationRange::MUTED, Is the color scheme for the login screen fixed? Can we use the same color scheme as in the shelf? If so, maybe they can share codes? https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:442: if (!color_calculator_->StartCalculation()) { If the calculation fails to be initiated, why do we need to inform the observer the complete event? https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right): https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:244: public WallpaperManager::Observer { WallpaperManagerBase::Observer? https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:286: void OnWallpaperAnimationFinished(const AccountId& account_id) override; Any reason it was made a pure virtual function in WallpaperManagerBase::Observer? Can we make it not pure virtual function?
https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:935: return; |color_calculator_|.reset() is skipped when this happens. https://codereview.chromium.org/2931063004/diff/80001/components/wallpaper/wa... File components/wallpaper/wallpaper_manager_base.h (right): https://codereview.chromium.org/2931063004/diff/80001/components/wallpaper/wa... components/wallpaper/wallpaper_manager_base.h:198: virtual void OnColorCalculationComplete() {} Document this and other methods in this interface. OnColorCalculationComplete by the name itself is not clear when it is invoked, e.g. it is not obvious that this would be called as a result of SetWallpaper call.
Thanks for the comments. https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:439: image, color_utils::LumaRange::DARK, color_utils::SaturationRange::MUTED, On 2017/06/13 23:04:24, xdai1 wrote: > Is the color scheme for the login screen fixed? Can we use the same color scheme > as in the shelf? If so, maybe they can share codes? The color scheme for the shelf is derived from the command line, as in (please open in a new tab) https://cs.chromium.org/chromium/src/ash/wallpaper/wallpaper_controller.cc?dr... However the login screen always has a fixed color scheme, so they are not the same. https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:442: if (!color_calculator_->StartCalculation()) { On 2017/06/13 23:04:24, xdai1 wrote: > If the calculation fails to be initiated, why do we need to inform the observer > the complete event? Because the front end needs to be notified of the failure, and it will check whether the optional value exists to determine the status. The function name may wrongly imply that the calculation always completes, so the name is changed. https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:935: return; On 2017/06/13 23:23:48, xiyuan wrote: > |color_calculator_|.reset() is skipped when this happens. Done. Thanks for pointing it out. https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right): https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:244: public WallpaperManager::Observer { On 2017/06/13 23:04:24, xdai1 wrote: > WallpaperManagerBase::Observer? It's changed. Thanks. For some reason other classes use WallpaperManager::Observer though, e.g. https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/wall... https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:286: void OnWallpaperAnimationFinished(const AccountId& account_id) override; On 2017/06/13 23:04:24, xdai1 wrote: > Any reason it was made a pure virtual function in > WallpaperManagerBase::Observer? Can we make it not pure virtual function? I'm also wondering. I see all other subclasses have an empty implementation as well. I just feel changing that is beyond the scope of this CL. https://codereview.chromium.org/2931063004/diff/80001/components/wallpaper/wa... File components/wallpaper/wallpaper_manager_base.h (right): https://codereview.chromium.org/2931063004/diff/80001/components/wallpaper/wa... components/wallpaper/wallpaper_manager_base.h:198: virtual void OnColorCalculationComplete() {} On 2017/06/13 23:23:48, xiyuan wrote: > Document this and other methods in this interface. OnColorCalculationComplete by > the name itself is not clear when it is invoked, e.g. it is not obvious that > this would be called as a result of SetWallpaper call. I've tried my best to add comments to the other three methods, please modify if needed. Thanks.
https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:439: image, color_utils::LumaRange::DARK, color_utils::SaturationRange::MUTED, On 2017/06/14 00:44:50, Wenzhao (Colin) Zang wrote: > On 2017/06/13 23:04:24, xdai1 wrote: > > Is the color scheme for the login screen fixed? Can we use the same color > scheme > > as in the shelf? If so, maybe they can share codes? > > The color scheme for the shelf is derived from the command line, as in > (please open in a new tab) > > https://cs.chromium.org/chromium/src/ash/wallpaper/wallpaper_controller.cc?dr... > > However the login screen always has a fixed color scheme, so they are not the > same. The code logic of WallpaperColorCalculator here is almost the same as in WallpaperController, except the color scheme (even for the color scheme, the command switch in the shelf might be only used for developing/testing purpose. I don't think we set different shelf color scheme for different device board). And the color seems calculated twice in the same function SetWallpaper() (WallpaperController::CalculateWallpaperColors() is called in the sub-function of SetWallpaper()), which seems redundant? So instead of making SigninScreenHandler an observer of WallpaperManagerBase, it might be better to make it an observer of WallpaperController? https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right): https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:286: void OnWallpaperAnimationFinished(const AccountId& account_id) override; On 2017/06/14 00:44:50, Wenzhao (Colin) Zang wrote: > On 2017/06/13 23:04:24, xdai1 wrote: > > Any reason it was made a pure virtual function in > > WallpaperManagerBase::Observer? Can we make it not pure virtual function? > > I'm also wondering. I see all other subclasses have an empty implementation as > well. I just feel changing that is beyond the scope of this CL. Acknowledged. Could you address this in a separate CL? Btw: you don't need "// wallpaper::WallpaperManagerBase::Observer implementation." twice. Remove the one in line 285 and remove the blank line in 284. https://codereview.chromium.org/2931063004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): https://codereview.chromium.org/2931063004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:399: user_manager::UserManager::Get()->RemoveSessionStateObserver(this); Please also remove itself as an observer of WallpaperColorCalculator in Wallpaper destructor
https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:439: image, color_utils::LumaRange::DARK, color_utils::SaturationRange::MUTED, On 2017/06/14 18:23:40, xdai1 wrote: > On 2017/06/14 00:44:50, Wenzhao (Colin) Zang wrote: > > On 2017/06/13 23:04:24, xdai1 wrote: > > > Is the color scheme for the login screen fixed? Can we use the same color > > scheme > > > as in the shelf? If so, maybe they can share codes? > > > > The color scheme for the shelf is derived from the command line, as in > > (please open in a new tab) > > > > > https://cs.chromium.org/chromium/src/ash/wallpaper/wallpaper_controller.cc?dr... > > > > However the login screen always has a fixed color scheme, so they are not the > > same. > > The code logic of WallpaperColorCalculator here is almost the same as in > WallpaperController, except the color scheme (even for the color scheme, the > command switch in the shelf might be only used for developing/testing purpose. I > don't think we set different shelf color scheme for different device board). And > the color seems calculated twice in the same function SetWallpaper() > (WallpaperController::CalculateWallpaperColors() is called in the sub-function > of SetWallpaper()), which seems redundant? > > So instead of making SigninScreenHandler an observer of WallpaperManagerBase, it > might be better to make it an observer of WallpaperController? WallpaperController is part of ash. If we do that, we need to do it via mojo to be ready for mash.
https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:439: image, color_utils::LumaRange::DARK, color_utils::SaturationRange::MUTED, On 2017/06/14 18:28:26, xiyuan wrote: > On 2017/06/14 18:23:40, xdai1 wrote: > > On 2017/06/14 00:44:50, Wenzhao (Colin) Zang wrote: > > > On 2017/06/13 23:04:24, xdai1 wrote: > > > > Is the color scheme for the login screen fixed? Can we use the same color > > > scheme > > > > as in the shelf? If so, maybe they can share codes? > > > > > > The color scheme for the shelf is derived from the command line, as in > > > (please open in a new tab) > > > > > > > > > https://cs.chromium.org/chromium/src/ash/wallpaper/wallpaper_controller.cc?dr... > > > > > > However the login screen always has a fixed color scheme, so they are not > the > > > same. > > > > The code logic of WallpaperColorCalculator here is almost the same as in > > WallpaperController, except the color scheme (even for the color scheme, the > > command switch in the shelf might be only used for developing/testing purpose. > I > > don't think we set different shelf color scheme for different device board). > And > > the color seems calculated twice in the same function SetWallpaper() > > (WallpaperController::CalculateWallpaperColors() is called in the > sub-function > > of SetWallpaper()), which seems redundant? > > > > So instead of making SigninScreenHandler an observer of WallpaperManagerBase, > it > > might be better to make it an observer of WallpaperController? > > WallpaperController is part of ash. If we do that, we need to do it via mojo to > be ready for mash. I understand the concerns. Before writing this CL I already realized it's better to make it an observer of WallpaperController. So please advise on the following options: 1) We may not need to fix crbug.com/733391 for now, because this CL, including most other codes related to login screen will be discarded very soon, since the login screen will be completely re-implemented. So this CL and some other CLs I'm working on are temporary codes in order to meet the hard deadline - M61. This is the last major chunk of codes before completing the MD login screen re-design, and the UI review team already complains about the lack of color extraction. So are these codes acceptable for just two months? 2) If the codes are unacceptable, and we want to start working on crbug.com/733391 now (but the codes are likely to be discarded in two months too), I really need an alternative way to extract colors which can be used before the mojo calls are ready. Are there any suggestions? Because the background color is an integral part of the login screen, the earlier the UI-review team can see the color extraction results the more time I'll have to fix other potential issues before M61. Thanks.
On 2017/06/15 05:02:08, Wenzhao (Colin) Zang wrote: > https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): > > https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/chromeos... > chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:439: image, > color_utils::LumaRange::DARK, color_utils::SaturationRange::MUTED, > On 2017/06/14 18:28:26, xiyuan wrote: > > On 2017/06/14 18:23:40, xdai1 wrote: > > > On 2017/06/14 00:44:50, Wenzhao (Colin) Zang wrote: > > > > On 2017/06/13 23:04:24, xdai1 wrote: > > > > > Is the color scheme for the login screen fixed? Can we use the same > color > > > > scheme > > > > > as in the shelf? If so, maybe they can share codes? > > > > > > > > The color scheme for the shelf is derived from the command line, as in > > > > (please open in a new tab) > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/ash/wallpaper/wallpaper_controller.cc?dr... > > > > > > > > However the login screen always has a fixed color scheme, so they are not > > the > > > > same. > > > > > > The code logic of WallpaperColorCalculator here is almost the same as in > > > WallpaperController, except the color scheme (even for the color scheme, the > > > command switch in the shelf might be only used for developing/testing > purpose. > > I > > > don't think we set different shelf color scheme for different device board). > > And > > > the color seems calculated twice in the same function SetWallpaper() > > > (WallpaperController::CalculateWallpaperColors() is called in the > > sub-function > > > of SetWallpaper()), which seems redundant? > > > > > > So instead of making SigninScreenHandler an observer of > WallpaperManagerBase, > > it > > > might be better to make it an observer of WallpaperController? > > > > WallpaperController is part of ash. If we do that, we need to do it via mojo > to > > be ready for mash. > > > I understand the concerns. Before writing this CL I already realized it's better > to make it an observer of WallpaperController. So please advise on the following > options: > > 1) We may not need to fix crbug.com/733391 for now, because this CL, including > most other codes related to login screen will be discarded very soon, since the > login screen will be completely re-implemented. So this CL and some other CLs > I'm working on are temporary codes in order to meet the hard deadline - M61. > This is the last major chunk of codes before completing the MD login screen > re-design, and the UI review team already complains about the lack of color > extraction. So are these codes acceptable for just two months? > > 2) If the codes are unacceptable, and we want to start working on > crbug.com/733391 now (but the codes are likely to be discarded in two months > too), I really need an alternative way to extract colors which can be used > before the mojo calls are ready. Are there any suggestions? Because the > background color is an integral part of the login screen, the earlier the > UI-review team can see the color extraction results the more time I'll have to > fix other potential issues before M61. > > Thanks. If it's temporary only for 2 months then the current workaround is fine with me. Please file a bug and assign to yourself and clean the code up after the old login screen is deprecated.
The comments are addressed, and the bug is filed and a separate CL on the pure virtual function has been sent. Is there anything else? https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right): https://codereview.chromium.org/2931063004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:286: void OnWallpaperAnimationFinished(const AccountId& account_id) override; On 2017/06/14 18:23:40, xdai1 wrote: > On 2017/06/14 00:44:50, Wenzhao (Colin) Zang wrote: > > On 2017/06/13 23:04:24, xdai1 wrote: > > > Any reason it was made a pure virtual function in > > > WallpaperManagerBase::Observer? Can we make it not pure virtual function? > > > > I'm also wondering. I see all other subclasses have an empty implementation as > > well. I just feel changing that is beyond the scope of this CL. > > Acknowledged. Could you address this in a separate CL? > > Btw: you don't need "// wallpaper::WallpaperManagerBase::Observer > implementation." twice. Remove the one in line 285 and remove the blank line in > 284. Done. https://codereview.chromium.org/2931063004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): https://codereview.chromium.org/2931063004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:399: user_manager::UserManager::Get()->RemoveSessionStateObserver(this); On 2017/06/14 18:23:40, xdai1 wrote: > Please also remove itself as an observer of WallpaperColorCalculator in > Wallpaper destructor Done.
wallpaper_manager_base.h and wallpaper_manager.cc &.h lgtm https://codereview.chromium.org/2931063004/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): https://codereview.chromium.org/2931063004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:445: color_calculator_.reset(); Remove WallpaperManager as an observer before reset the color_calculator_?
lgtm
https://codereview.chromium.org/2931063004/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h (right): https://codereview.chromium.org/2931063004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:258: std::unique_ptr<wallpaper::WallpaperColorCalculator> color_calculator_; Sorry. one more comment: Could you also add a comment and TODO here to point to crbug.com/733709?
https://codereview.chromium.org/2931063004/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right): https://codereview.chromium.org/2931063004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:445: color_calculator_.reset(); On 2017/06/15 18:27:51, xdai1 wrote: > Remove WallpaperManager as an observer before reset the color_calculator_? Done. Then RemoveObserver() should also be added in WallpaperController. https://codereview.chromium.org/2931063004/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h (right): https://codereview.chromium.org/2931063004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h:258: std::unique_ptr<wallpaper::WallpaperColorCalculator> color_calculator_; On 2017/06/15 18:36:55, xdai1 wrote: > Sorry. one more comment: Could you also add a comment and TODO here to point to > crbug.com/733709? Done.
The CQ bit was checked by wzang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, xdai@chromium.org Link to the patchset: https://codereview.chromium.org/2931063004/#ps140001 (title: "Add RemoveObserver() and TODO")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by wzang@chromium.org
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wzang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, xdai@chromium.org Link to the patchset: https://codereview.chromium.org/2931063004/#ps320001 (title: "Will clean up this CL when crbug.com/733709 is fixed")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1497912478298140,
"parent_rev": "af5b11de25d789bc272ff88137de0df645977c51", "commit_rev":
"11d5ed105384e572c6a2617126426980874a15a4"}
Message was sent while issue was closed.
Description was changed from ========== Extract colors from wallpaper and dynamically update login screen overlay In SigninScreenHandler there's already a 'loadWallpaper' handler. We can add additional color extraction calculation after the wallpaper is fetched and is being loaded. Plaese note: 1) SigninScreenHandler is an observer for WallpaperManager, and WallpaperManager is an observer for WallpaperColorCalculator. 2) The calculation is specific to the login screen. The interface is not made more general because it's unlikely the color needed by the login screen will change, nor the function needs to be reused. 3) Codes for caching color extraction results can be added if necessary. There's no performance issue for an eve device. BUG=718156 ========== to ========== Extract colors from wallpaper and dynamically update login screen overlay In SigninScreenHandler there's already a 'loadWallpaper' handler. We can add additional color extraction calculation after the wallpaper is fetched and is being loaded. Plaese note: 1) SigninScreenHandler is an observer for WallpaperManager, and WallpaperManager is an observer for WallpaperColorCalculator. 2) The calculation is specific to the login screen. The interface is not made more general because it's unlikely the color needed by the login screen will change, nor the function needs to be reused. 3) Codes for caching color extraction results can be added if necessary. There's no performance issue for an eve device. BUG=718156 Review-Url: https://codereview.chromium.org/2931063004 Cr-Commit-Position: refs/heads/master@{#480616} Committed: https://chromium.googlesource.com/chromium/src/+/11d5ed105384e572c6a261712642... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/11d5ed105384e572c6a261712642... |
