Hook up Chrome's wallpaper picker for mash.

Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix picker regression from codereview.chromium.org/2009853002
TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405, 613657, 613707
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org

Committed: https://crrev.com/4d780247a697f62d0e27a6eb5dfb309807f40881
Cr-Commit-Position: refs/heads/master@{#397739}

[msw, 2016-05-19 19:14:03]

Description was changed from

==========
Hook up Chrome's wallpaper picker for mash.

BUG=557405
TEST=Right click mash desktop -> change wallpaper.
R=sky@chromium.org
==========

to

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.

BUG=557405
TEST=Right click mash desktop -> change wallpaper.
R=sky@chromium.org
==========

[msw, 2016-05-19 19:22:11]

Description was changed from

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.

BUG=557405
TEST=Right click mash desktop -> change wallpaper.
R=sky@chromium.org
==========

to

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=sky@chromium.org
==========

[msw, 2016-05-19 19:25:53]

Description was changed from

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=sky@chromium.org
==========

to

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org
==========

[msw, 2016-05-19 19:25:54]

msw@chromium.org changed reviewers:
+ jamescook@chromium.org
- sky@chromium.org

[msw, 2016-05-19 19:28:51]

Hey James, please take a look; thanks!

https://codereview.chromium.org/1984433002/diff/180001/ash/mus/user_wallpaper...
File ash/mus/user_wallpaper_delegate_mus.cc (right):

https://codereview.chromium.org/1984433002/diff/180001/ash/mus/user_wallpaper...
ash/mus/user_wallpaper_delegate_mus.cc:40: // No action required;
ChromeBrowserMainPartsChromeos inits WallpaperManager.
This is sufficient from my local testing, but I could hook up an
ash_sysui->chrome init call for redundancy / explicit behavior if desired.
Here's the existing codepath that initialize the wallpaper correctly (afaict) in
mash with or without --login-manager:

#1 chromeos::WallpaperManager::PendingWallpaper::PendingWallpaper()
#2 chromeos::WallpaperManager::GetPendingWallpaper()
#3 chromeos::WallpaperManager::ScheduleSetUserWallpaper()
#4 wallpaper::WallpaperManagerBase::SetUserWallpaperNow()
#5 chromeos::ChromeUserManagerImpl::RegularUserLoggedInAsEphemeral()
#6 user_manager::UserManagerBase::UserLoggedIn()
#7 chromeos::ChromeBrowserMainPartsChromeos::PreProfileInit()
#8 ChromeBrowserMainParts::PreMainMessageLoopRunImpl()
#9 ChromeBrowserMainParts::PreMainMessageLoopRun()
#10 chromeos::ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun()
#11 content::BrowserMainLoop::PreMainMessageLoopRun()

[msw, 2016-05-19 19:29:00]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-19 19:29:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984433002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984433002/180001

[commit-bot: I haz the power, 2016-05-19 19:35:23]

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

[commit-bot: I haz the power, 2016-05-19 19:35:24]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-05-19 19:44:24]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-19 19:45:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984433002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984433002/200001

[sadrul, 2016-05-19 19:45:22]

drive-by

https://codereview.chromium.org/1984433002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://codereview.chromium.org/1984433002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:172: if
(!ash::Shell::HasInstance() || ash::Shell::GetInstance()->in_mus()) {
If there's a Shell instance, it should never have in_mus() set here. i.e. the
in_mus() check shouldn't be needed.

You could also check chrome::IsRunningInMash() instead.

[commit-bot: I haz the power, 2016-05-19 20:03:13]

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

[commit-bot: I haz the power, 2016-05-19 20:03:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-05-19 20:19:48]

Patchset #12 (id:220001) has been deleted

[msw, 2016-05-19 20:22:17]

https://codereview.chromium.org/1984433002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://codereview.chromium.org/1984433002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:172: if
(!ash::Shell::HasInstance() || ash::Shell::GetInstance()->in_mus()) {
On 2016/05/19 19:45:22, sadrul wrote:
> If there's a Shell instance, it should never have in_mus() set here. i.e. the
> in_mus() check shouldn't be needed.
> 
> You could also check chrome::IsRunningInMash() instead.

Done.

[msw, 2016-05-19 20:22:55]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-19 20:23:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984433002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984433002/240001

[commit-bot: I haz the power, 2016-05-19 21:03:43]

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

[commit-bot: I haz the power, 2016-05-19 21:03:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2016-05-19 22:30:00]

It's a bummer we have to ship big bitmaps back and forth, but I can't think of
any other way to do it, since Chrome owns the downloading/decoding. But I think
it's worth making sure we don't have two copies of a screen-size bitmap in RAM.

https://codereview.chromium.org/1984433002/diff/240001/ash/desktop_background...
File ash/desktop_background/desktop_background_controller.cc (right):

https://codereview.chromium.org/1984433002/diff/240001/ash/desktop_background...
ash/desktop_background/desktop_background_controller.cc:174: // TODO(msw): Fix
this for Mash/Mus.
So this will fail on high DPI, because iter->size() is DIPs? If so, might be
worth filing a tracking bug for this, and reference it here, since we'll dogfood
on high DPI devices.

https://codereview.chromium.org/1984433002/diff/240001/ash/mus/sysui_applicat...
File ash/mus/sysui_application.cc (left):

https://codereview.chromium.org/1984433002/diff/240001/ash/mus/sysui_applicat...
ash/mus/sysui_application.cc:269: void SetupWallpaper(SkColor color) {
So nice to see this going away!

https://codereview.chromium.org/1984433002/diff/240001/ash/mus/user_wallpaper...
File ash/mus/user_wallpaper_delegate_mus.cc (right):

https://codereview.chromium.org/1984433002/diff/240001/ash/mus/user_wallpaper...
ash/mus/user_wallpaper_delegate_mus.cc:51: return true;
Maybe a TODO here about login status?

https://codereview.chromium.org/1984433002/diff/240001/ash/mus/user_wallpaper...
File ash/mus/user_wallpaper_delegate_mus.h (right):

https://codereview.chromium.org/1984433002/diff/240001/ash/mus/user_wallpaper...
ash/mus/user_wallpaper_delegate_mus.h:10: #include "base/compiler_specific.h"
not needed

https://codereview.chromium.org/1984433002/diff/240001/ash/mus/user_wallpaper...
ash/mus/user_wallpaper_delegate_mus.h:20: UserWallpaperDelegateMus() {}
nit: stick the {} implementations in the .cc file

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_interface_factory.h (right):

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_interface_factory.h:29: public
shell::InterfaceFactory<keyboard::mojom::Keyboard> {
optional: While you're here, maybe move Keyboard up to after Listener, so it
matches the order of the functions and member variables in this file?

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (left):

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:601: // Some
browser tests do not have a shell instance. As no wallpaper is needed
Maybe this should be unit tests? It looks like some unit tests are crashing and
this function is in the backtrace.

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:198:
ash::Shell::GetInstance()->desktop_background_controller()->SetWallpaperImage(
Do you need to do this in the mash case? In particular, will we end up with
Chrome/Ash holding one copy of a screen-size bitmap and ash_sysui also holding a
copy?

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_wallpaper_helper.cc (right):

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_wallpaper_helper.cc:8: #include
"chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h"
Do you need this?

https://codereview.chromium.org/1984433002/diff/240001/mash/public/interfaces...
File mash/public/interfaces/wallpaper.mojom (right):

https://codereview.chromium.org/1984433002/diff/240001/mash/public/interfaces...
mash/public/interfaces/wallpaper.mojom:9: // Used by Chrome to set the wallpaper
displayed by Mash.
nit: s/Mash/ash_sysui/ (I'm a big fan of documenting exactly who uses these
interfaces, unless they are something general purpose.)

https://codereview.chromium.org/1984433002/diff/240001/mash/public/interfaces...
mash/public/interfaces/wallpaper.mojom:11: SetWallpaper(skia.mojom.Bitmap
wallpaper, int32 layout);
document the layout values

https://codereview.chromium.org/1984433002/diff/240001/mash/public/interfaces...
mash/public/interfaces/wallpaper.mojom:14: // Used by Mash to trigger Chrome's
wallpaper picker functionality.
ditto

[msw, 2016-05-20 19:51:51]

Description was changed from

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org
==========

to

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org
==========

[msw, 2016-05-20 19:51:52]

msw@chromium.org changed reviewers:
+ sky@chromium.org

[msw, 2016-05-20 19:53:05]

Please take another look, thanks! +Scott for OWNERS.

https://codereview.chromium.org/1984433002/diff/240001/ash/desktop_background...
File ash/desktop_background/desktop_background_controller.cc (right):

https://codereview.chromium.org/1984433002/diff/240001/ash/desktop_background...
ash/desktop_background/desktop_background_controller.cc:174: // TODO(msw): Fix
this for Mash/Mus.
On 2016/05/19 22:29:59, James Cook wrote:
> So this will fail on high DPI, because iter->size() is DIPs? If so, might be
> worth filing a tracking bug for this, and reference it here, since we'll
dogfood
> on high DPI devices.

Done.

https://codereview.chromium.org/1984433002/diff/240001/ash/mus/sysui_applicat...
File ash/mus/sysui_application.cc (left):

https://codereview.chromium.org/1984433002/diff/240001/ash/mus/sysui_applicat...
ash/mus/sysui_application.cc:269: void SetupWallpaper(SkColor color) {
On 2016/05/19 22:29:59, James Cook wrote:
> So nice to see this going away!

Acknowledged.

https://codereview.chromium.org/1984433002/diff/240001/ash/mus/user_wallpaper...
File ash/mus/user_wallpaper_delegate_mus.cc (right):

https://codereview.chromium.org/1984433002/diff/240001/ash/mus/user_wallpaper...
ash/mus/user_wallpaper_delegate_mus.cc:51: return true;
On 2016/05/19 22:29:59, James Cook wrote:
> Maybe a TODO here about login status?

Done.

https://codereview.chromium.org/1984433002/diff/240001/ash/mus/user_wallpaper...
File ash/mus/user_wallpaper_delegate_mus.h (right):

https://codereview.chromium.org/1984433002/diff/240001/ash/mus/user_wallpaper...
ash/mus/user_wallpaper_delegate_mus.h:10: #include "base/compiler_specific.h"
On 2016/05/19 22:29:59, James Cook wrote:
> not needed

Done.

https://codereview.chromium.org/1984433002/diff/240001/ash/mus/user_wallpaper...
ash/mus/user_wallpaper_delegate_mus.h:20: UserWallpaperDelegateMus() {}
On 2016/05/19 22:30:00, James Cook wrote:
> nit: stick the {} implementations in the .cc file

Done.

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_interface_factory.h (right):

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_interface_factory.h:29: public
shell::InterfaceFactory<keyboard::mojom::Keyboard> {
On 2016/05/19 22:30:00, James Cook wrote:
> optional: While you're here, maybe move Keyboard up to after Listener, so it
> matches the order of the functions and member variables in this file?

Done.

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (left):

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:601: // Some
browser tests do not have a shell instance. As no wallpaper is needed
On 2016/05/19 22:30:00, James Cook wrote:
> Maybe this should be unit tests? It looks like some unit tests are crashing
and
> this function is in the backtrace.

Done; added GetMaxDisplaySizeInNative and SetWallpaper early returns.

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:198:
ash::Shell::GetInstance()->desktop_background_controller()->SetWallpaperImage(
On 2016/05/19 22:30:00, James Cook wrote:
> Do you need to do this in the mash case? In particular, will we end up with
> Chrome/Ash holding one copy of a screen-size bitmap and ash_sysui also holding
a
> copy?

Perhaps you missed the early return on line 195, or I'm missing something; lmk!
It's possible that we could destroy |user_wallpaper_| (and other images used to
call this function) after this call, but I'll leave optimizations for later.

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_wallpaper_helper.cc (right):

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_wallpaper_helper.cc:8: #include
"chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h"
On 2016/05/19 22:30:00, James Cook wrote:
> Do you need this?

Done.

https://codereview.chromium.org/1984433002/diff/240001/mash/public/interfaces...
File mash/public/interfaces/wallpaper.mojom (right):

https://codereview.chromium.org/1984433002/diff/240001/mash/public/interfaces...
mash/public/interfaces/wallpaper.mojom:9: // Used by Chrome to set the wallpaper
displayed by Mash.
On 2016/05/19 22:30:00, James Cook wrote:
> nit: s/Mash/ash_sysui/ (I'm a big fan of documenting exactly who uses these
> interfaces, unless they are something general purpose.)

Done.

https://codereview.chromium.org/1984433002/diff/240001/mash/public/interfaces...
mash/public/interfaces/wallpaper.mojom:11: SetWallpaper(skia.mojom.Bitmap
wallpaper, int32 layout);
On 2016/05/19 22:30:00, James Cook wrote:
> document the layout values

Done.

https://codereview.chromium.org/1984433002/diff/240001/mash/public/interfaces...
mash/public/interfaces/wallpaper.mojom:14: // Used by Mash to trigger Chrome's
wallpaper picker functionality.
On 2016/05/19 22:30:00, James Cook wrote:
> ditto

Done.

[msw, 2016-05-20 19:53:20]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[msw, 2016-05-20 19:53:28]

Description was changed from

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org
==========

to

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405,613657
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org
==========

[commit-bot: I haz the power, 2016-05-20 19:53:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984433002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984433002/260001

[James Cook, 2016-05-20 20:47:13]

LGTM with a question

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:198:
ash::Shell::GetInstance()->desktop_background_controller()->SetWallpaperImage(
On 2016/05/20 19:53:05, msw wrote:
> On 2016/05/19 22:30:00, James Cook wrote:
> > Do you need to do this in the mash case? In particular, will we end up with
> > Chrome/Ash holding one copy of a screen-size bitmap and ash_sysui also
holding
> a
> > copy?
> 
> Perhaps you missed the early return on line 195, or I'm missing something;
lmk!
> It's possible that we could destroy |user_wallpaper_| (and other images used
to
> call this function) after this call, but I'll leave optimizations for later.

Yeah, I missed the early return.

I think it's worth filing a bug about the duplication of bitmap memory between
ash_sysui and chrome, but it looks non-trivial to sort out, so I wouldn't do it
in this CL.

https://codereview.chromium.org/1984433002/diff/260001/ash/desktop_background...
File ash/desktop_background/desktop_background_controller.cc (right):

https://codereview.chromium.org/1984433002/diff/260001/ash/desktop_background...
ash/desktop_background/desktop_background_controller.cc:164: // Return an empty
size for test environments where the screen is null.
Is this for mus-only tests? The old code would crash is Screen::GetScreen() was
null.

https://codereview.chromium.org/1984433002/diff/260001/ash/desktop_background...
ash/desktop_background/desktop_background_controller.cc:172: for (auto display :
display::Screen::GetScreen()->GetAllDisplays()) {
optional: Did you want to copy each Display object or use auto& ? Actually, this
might be clearer without auto, since you know the type and it isn't a long
iterator type.

(Oh, and I think the following code is much clearer with the |display| variable,
thanks.)

[msw, 2016-05-20 21:31:49]

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/1984433002/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:198:
ash::Shell::GetInstance()->desktop_background_controller()->SetWallpaperImage(
On 2016/05/20 20:47:13, James Cook wrote:
> On 2016/05/20 19:53:05, msw wrote:
> > On 2016/05/19 22:30:00, James Cook wrote:
> > > Do you need to do this in the mash case? In particular, will we end up
with
> > > Chrome/Ash holding one copy of a screen-size bitmap and ash_sysui also
> holding
> > a
> > > copy?
> > 
> > Perhaps you missed the early return on line 195, or I'm missing something;
> lmk!
> > It's possible that we could destroy |user_wallpaper_| (and other images used
> to
> > call this function) after this call, but I'll leave optimizations for later.
> 
> Yeah, I missed the early return.
> 
> I think it's worth filing a bug about the duplication of bitmap memory between
> ash_sysui and chrome, but it looks non-trivial to sort out, so I wouldn't do
it
> in this CL.

Done. I filed http://crbug.com/613707

https://codereview.chromium.org/1984433002/diff/260001/ash/desktop_background...
File ash/desktop_background/desktop_background_controller.cc (right):

https://codereview.chromium.org/1984433002/diff/260001/ash/desktop_background...
ash/desktop_background/desktop_background_controller.cc:164: // Return an empty
size for test environments where the screen is null.
On 2016/05/20 20:47:13, James Cook wrote:
> Is this for mus-only tests? The old code would crash is Screen::GetScreen()
was
> null.

No, it's not mus-only tests. This is the crash we were seeing in the prior patch
set; it was only exposed because I removed some other early returns when there's
no ash::Shell instance. We want to support the lack of an ash::Shell instance
here for uses by Chrome in mash, so I found that the lack of a display::Screen
instance was a better indicator for tests (hopefully for mus and non-mus
tests...)

https://codereview.chromium.org/1984433002/diff/260001/ash/desktop_background...
ash/desktop_background/desktop_background_controller.cc:172: for (auto display :
display::Screen::GetScreen()->GetAllDisplays()) {
On 2016/05/20 20:47:13, James Cook wrote:
> optional: Did you want to copy each Display object or use auto& ? Actually,
this
> might be clearer without auto, since you know the type and it isn't a long
> iterator type.
> 
> (Oh, and I think the following code is much clearer with the |display|
variable,
> thanks.)

Changed to auto&, display::Display& adds a line break :p

[msw, 2016-05-20 21:31:55]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-20 21:32:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984433002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984433002/280001

[msw, 2016-05-20 21:33:34]

Description was changed from

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405,613657
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org
==========

to

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405,613657,613707
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org
==========

[sky, 2016-05-20 22:32:19]

https://codereview.chromium.org/1984433002/diff/280001/mash/public/interfaces...
File mash/public/interfaces/wallpaper.mojom (right):

https://codereview.chromium.org/1984433002/diff/280001/mash/public/interfaces...
mash/public/interfaces/wallpaper.mojom:5: module mash.mojom;
The interfaces in here are intended for use by all mash apps, but the interfaces
you have here are specific to the sysui. I think they should move to
ash/mus/public/interfaces. ash/mus should really be in mash/sysui, which would
make this cleaner. I filed 613727 to rename ash/mus to ash/sysui, or bite the
bullet and move it to mash/sysui.

https://codereview.chromium.org/1984433002/diff/280001/mash/public/interfaces...
mash/public/interfaces/wallpaper.mojom:12: SetWallpaper(skia.mojom.Bitmap
wallpaper, int32 layout);
Don't we want to define WallpaperLayout in mojom?

https://codereview.chromium.org/1984433002/diff/280001/mash/public/interfaces...
mash/public/interfaces/wallpaper.mojom:16: interface WallpaperHelper {
Should the interface be named WallpaperManager?

[commit-bot: I haz the power, 2016-05-20 23:15:39]

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

[commit-bot: I haz the power, 2016-05-20 23:15:40]

Dry run: Try jobs failed on following builders:
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[msw, 2016-05-21 00:30:00]

Patchset #15 (id:300001) has been deleted

[msw, 2016-05-21 00:30:35]

Please take another look; thanks!

https://codereview.chromium.org/1984433002/diff/280001/mash/public/interfaces...
File mash/public/interfaces/wallpaper.mojom (right):

https://codereview.chromium.org/1984433002/diff/280001/mash/public/interfaces...
mash/public/interfaces/wallpaper.mojom:5: module mash.mojom;
On 2016/05/20 22:32:19, sky wrote:
> The interfaces in here are intended for use by all mash apps, but the
interfaces
> you have here are specific to the sysui. I think they should move to
> ash/mus/public/interfaces. ash/mus should really be in mash/sysui, which would
> make this cleaner. I filed 613727 to rename ash/mus to ash/sysui, or bite the
> bullet and move it to mash/sysui.

Moved to ash/mus/public/interfaces... Maybe you can let me land this before we
move ash/mus, so I don't have to rebase?

https://codereview.chromium.org/1984433002/diff/280001/mash/public/interfaces...
mash/public/interfaces/wallpaper.mojom:12: SetWallpaper(skia.mojom.Bitmap
wallpaper, int32 layout);
On 2016/05/20 22:32:19, sky wrote:
> Don't we want to define WallpaperLayout in mojom?

I guess; Done.

https://codereview.chromium.org/1984433002/diff/280001/mash/public/interfaces...
mash/public/interfaces/wallpaper.mojom:16: interface WallpaperHelper {
On 2016/05/20 22:32:19, sky wrote:
> Should the interface be named WallpaperManager?

Heh, I had that but then though it was silly; Done.

[msw, 2016-05-21 00:30:42]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-21 00:31:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984433002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984433002/320001

[commit-bot: I haz the power, 2016-05-21 01:33:50]

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

[commit-bot: I haz the power, 2016-05-21 01:33:51]

Dry run: This issue passed the CQ dry run.

[sky, 2016-05-21 16:09:21]

LGTM - be sure and get a security review of the mojom before landing though.

https://codereview.chromium.org/1984433002/diff/280001/mash/public/interfaces...
File mash/public/interfaces/wallpaper.mojom (right):

https://codereview.chromium.org/1984433002/diff/280001/mash/public/interfaces...
mash/public/interfaces/wallpaper.mojom:5: module mash.mojom;
On 2016/05/21 00:30:34, msw wrote:
> On 2016/05/20 22:32:19, sky wrote:
> > The interfaces in here are intended for use by all mash apps, but the
> interfaces
> > you have here are specific to the sysui. I think they should move to
> > ash/mus/public/interfaces. ash/mus should really be in mash/sysui, which
would
> > make this cleaner. I filed 613727 to rename ash/mus to ash/sysui, or bite
the
> > bullet and move it to mash/sysui.
> 
> Moved to ash/mus/public/interfaces... Maybe you can let me land this before we
> move ash/mus, so I don't have to rebase?

Yes, sorry, I didn't mean you need to move everything before landing this. The
rename/move can happen at a later time.

https://codereview.chromium.org/1984433002/diff/320001/ash/mus/public/interfa...
File ash/mus/public/interfaces/wallpaper.mojom (right):

https://codereview.chromium.org/1984433002/diff/320001/ash/mus/public/interfa...
ash/mus/public/interfaces/wallpaper.mojom:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
Please add an OWNERS file for this directory that has all the security guys as
owners for the mojom files. See
https://chromium.googlesource.com/chromium/src/+/master/components/mus/public...
for an example of what I mean.

https://codereview.chromium.org/1984433002/diff/320001/ash/mus/public/interfa...
ash/mus/public/interfaces/wallpaper.mojom:19: OpenWallpaperManager();
nit: The 'WallpaperManager' here is a bit redundant. How about Open()?

[msw, 2016-05-24 18:44:16]

Description was changed from

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405,613657,613707
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org
==========

to

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405,613657,613707
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org
==========

[msw, 2016-05-24 18:44:16]

msw@chromium.org changed reviewers:
+ dcheng@chromium.org

[msw, 2016-05-24 18:45:42]

Daniel please security review ash/mus/public; thanks!

https://codereview.chromium.org/1984433002/diff/320001/ash/mus/public/interfa...
File ash/mus/public/interfaces/wallpaper.mojom (right):

https://codereview.chromium.org/1984433002/diff/320001/ash/mus/public/interfa...
ash/mus/public/interfaces/wallpaper.mojom:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/05/21 16:09:20, sky wrote:
> Please add an OWNERS file for this directory that has all the security guys as
> owners for the mojom files. See
>
https://chromium.googlesource.com/chromium/src/+/master/components/mus/public...
> for an example of what I mean.

Done.

https://codereview.chromium.org/1984433002/diff/320001/ash/mus/public/interfa...
ash/mus/public/interfaces/wallpaper.mojom:19: OpenWallpaperManager();
On 2016/05/21 16:09:20, sky wrote:
> nit: The 'WallpaperManager' here is a bit redundant. How about Open()?

Done.

[msw, 2016-05-24 18:46:02]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-24 18:46:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984433002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984433002/360001

[dcheng, 2016-05-24 18:53:37]

https://codereview.chromium.org/1984433002/diff/360001/ash/desktop_background...
File ash/desktop_background/desktop_background_controller.cc (right):

https://codereview.chromium.org/1984433002/diff/360001/ash/desktop_background...
ash/desktop_background/desktop_background_controller.cc:172: for (auto& display
: display::Screen::GetScreen()->GetAllDisplays()) {
Nit: const auto&?

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:193:
skia::mojom::Bitmap::From(*image.bitmap()),
Can we do this with a StructTraits instead? My worry about adding further
dependencies on the existing TypeConverter is that it's going to make it harder
to remove down the road.

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:194:
static_cast<ash::mus::mojom::WallpaperLayout>(layout));
Similarly, do StructTraits work for enums yet?

[commit-bot: I haz the power, 2016-05-24 20:11:05]

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

[commit-bot: I haz the power, 2016-05-24 20:11:07]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-05-24 22:04:41]

yzshen@chromium.org changed reviewers:
+ yzshen@chromium.org

[yzshen1, 2016-05-24 22:04:42]

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:194:
static_cast<ash::mus::mojom::WallpaperLayout>(layout));
On 2016/05/24 18:53:37, dcheng wrote:
> Similarly, do StructTraits work for enums yet?

Yes. For example:

module mojom;
enum SomeEnum {};
struct Foo {
  SomeEnum value;
};

The struct traits will be:

template<>
class StructTraits<mojom::Foo, CustomFoo> {
  // The getter of an enum field needs to return the exact mojo enum type.
  static mojom::SomeEnum value(const CustomFoo& input) {
    ...
  }
};

[msw, 2016-05-24 22:06:37]

Please let me know how you'd like me to proceed, thanks!

https://codereview.chromium.org/1984433002/diff/360001/ash/desktop_background...
File ash/desktop_background/desktop_background_controller.cc (right):

https://codereview.chromium.org/1984433002/diff/360001/ash/desktop_background...
ash/desktop_background/desktop_background_controller.cc:172: for (auto& display
: display::Screen::GetScreen()->GetAllDisplays()) {
On 2016/05/24 18:53:37, dcheng wrote:
> Nit: const auto&?

Done.

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:193:
skia::mojom::Bitmap::From(*image.bitmap()),
On 2016/05/24 18:53:37, dcheng wrote:
> Can we do this with a StructTraits instead? My worry about adding further
> dependencies on the existing TypeConverter is that it's going to make it
harder
> to remove down the road.

Yeah, Yuzhu gave me an intro to StructTraits, and it definitely seems possible.
Since I'm not adding a new type converter, but just making use of an existing
one, I wonder if it's necessary for this CL. Would you like me to add struct
traits and remove all existing skia bitmap type conversion uses and the
converter itself: (1) separately as a prerequisite to this CL, (2) done as part
of this CL (I hope not) or (3) left for a follow-up change?

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:194:
static_cast<ash::mus::mojom::WallpaperLayout>(layout));
On 2016/05/24 18:53:37, dcheng wrote:
> Similarly, do StructTraits work for enums yet?

We can support enums within a struct, but I don't think we have EnumTraits or
similar, let me know if there's some other approach you suggest.

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:194:
static_cast<ash::mus::mojom::WallpaperLayout>(layout));
On 2016/05/24 22:04:42, yzshen1 wrote:
> On 2016/05/24 18:53:37, dcheng wrote:
> > Similarly, do StructTraits work for enums yet?
> 
> Yes. For example:
> 
> module mojom;
> enum SomeEnum {};
> struct Foo {
>   SomeEnum value;
> };
> 
> The struct traits will be:
> 
> template<>
> class StructTraits<mojom::Foo, CustomFoo> {
>   // The getter of an enum field needs to return the exact mojo enum type.
>   static mojom::SomeEnum value(const CustomFoo& input) {
>     ...
>   }
> };
> 

Right, but in this case, I'm passing an enum value to a mojom function, not
encapsulating it within a struct...

[dcheng, 2016-05-25 17:31:26]

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:193:
skia::mojom::Bitmap::From(*image.bitmap()),
On 2016/05/24 at 22:06:37, msw wrote:
> On 2016/05/24 18:53:37, dcheng wrote:
> > Can we do this with a StructTraits instead? My worry about adding further
> > dependencies on the existing TypeConverter is that it's going to make it
harder
> > to remove down the road.
> 
> Yeah, Yuzhu gave me an intro to StructTraits, and it definitely seems
possible. Since I'm not adding a new type converter, but just making use of an
existing one, I wonder if it's necessary for this CL. Would you like me to add
struct traits and remove all existing skia bitmap type conversion uses and the
converter itself: (1) separately as a prerequisite to this CL, (2) done as part
of this CL (I hope not) or (3) left for a follow-up change?

We can add the StructTraits without removing all uses of the old one in this CL,
I think. That way, we can start incremental migration over.

[msw, 2016-05-25 18:16:41]

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:193:
skia::mojom::Bitmap::From(*image.bitmap()),
On 2016/05/25 17:31:26, dcheng wrote:
> On 2016/05/24 at 22:06:37, msw wrote:
> > On 2016/05/24 18:53:37, dcheng wrote:
> > > Can we do this with a StructTraits instead? My worry about adding further
> > > dependencies on the existing TypeConverter is that it's going to make it
> harder
> > > to remove down the road.
> > 
> > Yeah, Yuzhu gave me an intro to StructTraits, and it definitely seems
> possible. Since I'm not adding a new type converter, but just making use of an
> existing one, I wonder if it's necessary for this CL. Would you like me to add
> struct traits and remove all existing skia bitmap type conversion uses and the
> converter itself: (1) separately as a prerequisite to this CL, (2) done as
part
> of this CL (I hope not) or (3) left for a follow-up change?
> 
> We can add the StructTraits without removing all uses of the old one in this
CL,
> I think. That way, we can start incremental migration over.

I'd rather not pollute this wallpaper CL with the addition of skia bitmap
StructTraits. I'll put this CL on hold and implement that in a separate
prerequisite CL if you think it's a blocker.

[dcheng, 2016-05-25 19:56:40]

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:193:
skia::mojom::Bitmap::From(*image.bitmap()),
On 2016/05/25 at 18:16:41, msw wrote:
> On 2016/05/25 17:31:26, dcheng wrote:
> > On 2016/05/24 at 22:06:37, msw wrote:
> > > On 2016/05/24 18:53:37, dcheng wrote:
> > > > Can we do this with a StructTraits instead? My worry about adding
further
> > > > dependencies on the existing TypeConverter is that it's going to make it
> > harder
> > > > to remove down the road.
> > > 
> > > Yeah, Yuzhu gave me an intro to StructTraits, and it definitely seems
> > possible. Since I'm not adding a new type converter, but just making use of
an
> > existing one, I wonder if it's necessary for this CL. Would you like me to
add
> > struct traits and remove all existing skia bitmap type conversion uses and
the
> > converter itself: (1) separately as a prerequisite to this CL, (2) done as
part
> > of this CL (I hope not) or (3) left for a follow-up change?
> > 
> > We can add the StructTraits without removing all uses of the old one in this
CL,
> > I think. That way, we can start incremental migration over.
> 
> I'd rather not pollute this wallpaper CL with the addition of skia bitmap
StructTraits. I'll put this CL on hold and implement that in a separate
prerequisite CL if you think it's a blocker.

I'd strongly prefer to see that. I thought it'd be easier since this CL would be
defacto proof that the StructTraits works, but if you feel it would be better in
a separate CL, that's fine with me.

https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:194:
static_cast<ash::mus::mojom::WallpaperLayout>(layout));
On 2016/05/24 at 22:06:37, msw wrote:
> On 2016/05/24 22:04:42, yzshen1 wrote:
> > On 2016/05/24 18:53:37, dcheng wrote:
> > > Similarly, do StructTraits work for enums yet?
> > 
> > Yes. For example:
> > 
> > module mojom;
> > enum SomeEnum {};
> > struct Foo {
> >   SomeEnum value;
> > };
> > 
> > The struct traits will be:
> > 
> > template<>
> > class StructTraits<mojom::Foo, CustomFoo> {
> >   // The getter of an enum field needs to return the exact mojo enum type.
> >   static mojom::SomeEnum value(const CustomFoo& input) {
> >     ...
> >   }
> > };
> > 
> 
> Right, but in this case, I'm passing an enum value to a mojom function, not
encapsulating it within a struct...

and maybe in the meantime we can figure out something better for this? How does
StructTraits handle nested mojo structs, for example?

[yzshen1, 2016-05-25 20:13:13]

On 2016/05/25 19:56:40, dcheng wrote:
>
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
(right):
> 
>
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:193:
> skia::mojom::Bitmap::From(*image.bitmap()),
> On 2016/05/25 at 18:16:41, msw wrote:
> > On 2016/05/25 17:31:26, dcheng wrote:
> > > On 2016/05/24 at 22:06:37, msw wrote:
> > > > On 2016/05/24 18:53:37, dcheng wrote:
> > > > > Can we do this with a StructTraits instead? My worry about adding
> further
> > > > > dependencies on the existing TypeConverter is that it's going to make
it
> > > harder
> > > > > to remove down the road.
> > > > 
> > > > Yeah, Yuzhu gave me an intro to StructTraits, and it definitely seems
> > > possible. Since I'm not adding a new type converter, but just making use
of
> an
> > > existing one, I wonder if it's necessary for this CL. Would you like me to
> add
> > > struct traits and remove all existing skia bitmap type conversion uses and
> the
> > > converter itself: (1) separately as a prerequisite to this CL, (2) done as
> part
> > > of this CL (I hope not) or (3) left for a follow-up change?
> > > 
> > > We can add the StructTraits without removing all uses of the old one in
this
> CL,
> > > I think. That way, we can start incremental migration over.
> > 
> > I'd rather not pollute this wallpaper CL with the addition of skia bitmap
> StructTraits. I'll put this CL on hold and implement that in a separate
> prerequisite CL if you think it's a blocker.
> 
> I'd strongly prefer to see that. I thought it'd be easier since this CL would
be
> defacto proof that the StructTraits works, but if you feel it would be better
in
> a separate CL, that's fine with me.
> 
>
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:194:
> static_cast<ash::mus::mojom::WallpaperLayout>(layout));
> On 2016/05/24 at 22:06:37, msw wrote:
> > On 2016/05/24 22:04:42, yzshen1 wrote:
> > > On 2016/05/24 18:53:37, dcheng wrote:
> > > > Similarly, do StructTraits work for enums yet?
> > > 
> > > Yes. For example:
> > > 
> > > module mojom;
> > > enum SomeEnum {};
> > > struct Foo {
> > >   SomeEnum value;
> > > };
> > > 
> > > The struct traits will be:
> > > 
> > > template<>
> > > class StructTraits<mojom::Foo, CustomFoo> {
> > >   // The getter of an enum field needs to return the exact mojo enum type.
> > >   static mojom::SomeEnum value(const CustomFoo& input) {
> > >     ...
> > >   }
> > > };
> > > 
> > 
> > Right, but in this case, I'm passing an enum value to a mojom function, not
> encapsulating it within a struct...
> 
> and maybe in the meantime we can figure out something better for this? How
does
> StructTraits handle nested mojo structs, for example?

Yeah, one possible way is to follow the approach that we take for struct:
- support EnumTraits, so that you can return a custom enum value from a
StructTraits field getter and the bindings will know how to convert it to the
corresponding mojom enum.
- support defining typemap config (in .typemap file) to map a mojom enum to a
custom enum; that solves the problem of passing an enum value to a mojom
function.

As I said in another review thread. It shouldn't be difficult to support that.
But I wonder whether we should just suggest users to move their enum definition
into mojom, instead of trying to maintain two enums (a custom enum and a mojom
enum).

Custom typemapping makes sense for structs because custom structs may have extra
validation/methods attached to them. But in the case of enum, there isn't much
difference between a custom enum and a mojom enum.

WDYT? Thanks!

[dcheng, 2016-05-25 20:21:14]

On 2016/05/25 at 20:13:13, yzshen wrote:
> On 2016/05/25 19:56:40, dcheng wrote:
> >
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> > File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
(right):
> > 
> >
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> > chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:193:
> > skia::mojom::Bitmap::From(*image.bitmap()),
> > On 2016/05/25 at 18:16:41, msw wrote:
> > > On 2016/05/25 17:31:26, dcheng wrote:
> > > > On 2016/05/24 at 22:06:37, msw wrote:
> > > > > On 2016/05/24 18:53:37, dcheng wrote:
> > > > > > Can we do this with a StructTraits instead? My worry about adding
> > further
> > > > > > dependencies on the existing TypeConverter is that it's going to
make it
> > > > harder
> > > > > > to remove down the road.
> > > > > 
> > > > > Yeah, Yuzhu gave me an intro to StructTraits, and it definitely seems
> > > > possible. Since I'm not adding a new type converter, but just making use
of
> > an
> > > > existing one, I wonder if it's necessary for this CL. Would you like me
to
> > add
> > > > struct traits and remove all existing skia bitmap type conversion uses
and
> > the
> > > > converter itself: (1) separately as a prerequisite to this CL, (2) done
as
> > part
> > > > of this CL (I hope not) or (3) left for a follow-up change?
> > > > 
> > > > We can add the StructTraits without removing all uses of the old one in
this
> > CL,
> > > > I think. That way, we can start incremental migration over.
> > > 
> > > I'd rather not pollute this wallpaper CL with the addition of skia bitmap
> > StructTraits. I'll put this CL on hold and implement that in a separate
> > prerequisite CL if you think it's a blocker.
> > 
> > I'd strongly prefer to see that. I thought it'd be easier since this CL
would be
> > defacto proof that the StructTraits works, but if you feel it would be
better in
> > a separate CL, that's fine with me.
> > 
> >
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> > chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:194:
> > static_cast<ash::mus::mojom::WallpaperLayout>(layout));
> > On 2016/05/24 at 22:06:37, msw wrote:
> > > On 2016/05/24 22:04:42, yzshen1 wrote:
> > > > On 2016/05/24 18:53:37, dcheng wrote:
> > > > > Similarly, do StructTraits work for enums yet?
> > > > 
> > > > Yes. For example:
> > > > 
> > > > module mojom;
> > > > enum SomeEnum {};
> > > > struct Foo {
> > > >   SomeEnum value;
> > > > };
> > > > 
> > > > The struct traits will be:
> > > > 
> > > > template<>
> > > > class StructTraits<mojom::Foo, CustomFoo> {
> > > >   // The getter of an enum field needs to return the exact mojo enum
type.
> > > >   static mojom::SomeEnum value(const CustomFoo& input) {
> > > >     ...
> > > >   }
> > > > };
> > > > 
> > > 
> > > Right, but in this case, I'm passing an enum value to a mojom function,
not
> > encapsulating it within a struct...
> > 
> > and maybe in the meantime we can figure out something better for this? How
does
> > StructTraits handle nested mojo structs, for example?
> 
> Yeah, one possible way is to follow the approach that we take for struct:
> - support EnumTraits, so that you can return a custom enum value from a
StructTraits field getter and the bindings will know how to convert it to the
corresponding mojom enum.
> - support defining typemap config (in .typemap file) to map a mojom enum to a
custom enum; that solves the problem of passing an enum value to a mojom
function.
> 
> As I said in another review thread. It shouldn't be difficult to support that.
> But I wonder whether we should just suggest users to move their enum
definition into mojom, instead of trying to maintain two enums (a custom enum
and a mojom enum).
> 
> Custom typemapping makes sense for structs because custom structs may have
extra validation/methods attached to them. But in the case of enum, there isn't
much difference between a custom enum and a mojom enum.
> 
> WDYT? Thanks!

I expect devs have been duplicating enums to avoid the 'core' code (e.g.
//media) from depending on Mojo. While I don't like having to declare enums
twice either, is there a good way to address that concern?

[yzshen1, 2016-05-25 20:26:48]

On 2016/05/25 20:21:14, dcheng wrote:
> On 2016/05/25 at 20:13:13, yzshen wrote:
> > On 2016/05/25 19:56:40, dcheng wrote:
> > >
>
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> > > File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
> (right):
> > > 
> > >
>
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> > > chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:193:
> > > skia::mojom::Bitmap::From(*image.bitmap()),
> > > On 2016/05/25 at 18:16:41, msw wrote:
> > > > On 2016/05/25 17:31:26, dcheng wrote:
> > > > > On 2016/05/24 at 22:06:37, msw wrote:
> > > > > > On 2016/05/24 18:53:37, dcheng wrote:
> > > > > > > Can we do this with a StructTraits instead? My worry about adding
> > > further
> > > > > > > dependencies on the existing TypeConverter is that it's going to
> make it
> > > > > harder
> > > > > > > to remove down the road.
> > > > > > 
> > > > > > Yeah, Yuzhu gave me an intro to StructTraits, and it definitely
seems
> > > > > possible. Since I'm not adding a new type converter, but just making
use
> of
> > > an
> > > > > existing one, I wonder if it's necessary for this CL. Would you like
me
> to
> > > add
> > > > > struct traits and remove all existing skia bitmap type conversion uses
> and
> > > the
> > > > > converter itself: (1) separately as a prerequisite to this CL, (2)
done
> as
> > > part
> > > > > of this CL (I hope not) or (3) left for a follow-up change?
> > > > > 
> > > > > We can add the StructTraits without removing all uses of the old one
in
> this
> > > CL,
> > > > > I think. That way, we can start incremental migration over.
> > > > 
> > > > I'd rather not pollute this wallpaper CL with the addition of skia
bitmap
> > > StructTraits. I'll put this CL on hold and implement that in a separate
> > > prerequisite CL if you think it's a blocker.
> > > 
> > > I'd strongly prefer to see that. I thought it'd be easier since this CL
> would be
> > > defacto proof that the StructTraits works, but if you feel it would be
> better in
> > > a separate CL, that's fine with me.
> > > 
> > >
>
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> > > chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:194:
> > > static_cast<ash::mus::mojom::WallpaperLayout>(layout));
> > > On 2016/05/24 at 22:06:37, msw wrote:
> > > > On 2016/05/24 22:04:42, yzshen1 wrote:
> > > > > On 2016/05/24 18:53:37, dcheng wrote:
> > > > > > Similarly, do StructTraits work for enums yet?
> > > > > 
> > > > > Yes. For example:
> > > > > 
> > > > > module mojom;
> > > > > enum SomeEnum {};
> > > > > struct Foo {
> > > > >   SomeEnum value;
> > > > > };
> > > > > 
> > > > > The struct traits will be:
> > > > > 
> > > > > template<>
> > > > > class StructTraits<mojom::Foo, CustomFoo> {
> > > > >   // The getter of an enum field needs to return the exact mojo enum
> type.
> > > > >   static mojom::SomeEnum value(const CustomFoo& input) {
> > > > >     ...
> > > > >   }
> > > > > };
> > > > > 
> > > > 
> > > > Right, but in this case, I'm passing an enum value to a mojom function,
> not
> > > encapsulating it within a struct...
> > > 
> > > and maybe in the meantime we can figure out something better for this? How
> does
> > > StructTraits handle nested mojo structs, for example?
> > 
> > Yeah, one possible way is to follow the approach that we take for struct:
> > - support EnumTraits, so that you can return a custom enum value from a
> StructTraits field getter and the bindings will know how to convert it to the
> corresponding mojom enum.
> > - support defining typemap config (in .typemap file) to map a mojom enum to
a
> custom enum; that solves the problem of passing an enum value to a mojom
> function.
> > 
> > As I said in another review thread. It shouldn't be difficult to support
that.
> > But I wonder whether we should just suggest users to move their enum
> definition into mojom, instead of trying to maintain two enums (a custom enum
> and a mojom enum).
> > 
> > Custom typemapping makes sense for structs because custom structs may have
> extra validation/methods attached to them. But in the case of enum, there
isn't
> much difference between a custom enum and a mojom enum.
> > 
> > WDYT? Thanks!
> 
> I expect devs have been duplicating enums to avoid the 'core' code (e.g.
> //media) from depending on Mojo.

Hmm... I can see your point. I don't have a better idea than these two choices.

> While I don't like having to declare enums
> twice either, is there a good way to address that concern?

[sky, 2016-05-25 23:52:53]

Any chance Mike could land this and we straighten out the traits issue
separately? AFAIK Mike isn't doing anything we don't already have.

  -Scott

On Wed, May 25, 2016 at 1:26 PM,  <yzshen@chromium.org> wrote:
> On 2016/05/25 20:21:14, dcheng wrote:
>> On 2016/05/25 at 20:13:13, yzshen wrote:
>> > On 2016/05/25 19:56:40, dcheng wrote:
>> > >
>>
>
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
>> > > File
>> > > chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
>> (right):
>> > >
>> > >
>>
>
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
>> > >
>> > > chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:193:
>> > > skia::mojom::Bitmap::From(*image.bitmap()),
>> > > On 2016/05/25 at 18:16:41, msw wrote:
>> > > > On 2016/05/25 17:31:26, dcheng wrote:
>> > > > > On 2016/05/24 at 22:06:37, msw wrote:
>> > > > > > On 2016/05/24 18:53:37, dcheng wrote:
>> > > > > > > Can we do this with a StructTraits instead? My worry about
>> > > > > > > adding
>> > > further
>> > > > > > > dependencies on the existing TypeConverter is that it's going
>> > > > > > > to
>> make it
>> > > > > harder
>> > > > > > > to remove down the road.
>> > > > > >
>> > > > > > Yeah, Yuzhu gave me an intro to StructTraits, and it definitely
> seems
>> > > > > possible. Since I'm not adding a new type converter, but just
>> > > > > making
> use
>> of
>> > > an
>> > > > > existing one, I wonder if it's necessary for this CL. Would you
>> > > > > like
> me
>> to
>> > > add
>> > > > > struct traits and remove all existing skia bitmap type conversion
>> > > > > uses
>> and
>> > > the
>> > > > > converter itself: (1) separately as a prerequisite to this CL, (2)
> done
>> as
>> > > part
>> > > > > of this CL (I hope not) or (3) left for a follow-up change?
>> > > > >
>> > > > > We can add the StructTraits without removing all uses of the old
>> > > > > one
> in
>> this
>> > > CL,
>> > > > > I think. That way, we can start incremental migration over.
>> > > >
>> > > > I'd rather not pollute this wallpaper CL with the addition of skia
> bitmap
>> > > StructTraits. I'll put this CL on hold and implement that in a
>> > > separate
>> > > prerequisite CL if you think it's a blocker.
>> > >
>> > > I'd strongly prefer to see that. I thought it'd be easier since this
>> > > CL
>> would be
>> > > defacto proof that the StructTraits works, but if you feel it would be
>> better in
>> > > a separate CL, that's fine with me.
>> > >
>> > >
>>
>
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
>> > >
>> > > chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:194:
>> > > static_cast<ash::mus::mojom::WallpaperLayout>(layout));
>> > > On 2016/05/24 at 22:06:37, msw wrote:
>> > > > On 2016/05/24 22:04:42, yzshen1 wrote:
>> > > > > On 2016/05/24 18:53:37, dcheng wrote:
>> > > > > > Similarly, do StructTraits work for enums yet?
>> > > > >
>> > > > > Yes. For example:
>> > > > >
>> > > > > module mojom;
>> > > > > enum SomeEnum {};
>> > > > > struct Foo {
>> > > > > SomeEnum value;
>> > > > > };
>> > > > >
>> > > > > The struct traits will be:
>> > > > >
>> > > > > template<>
>> > > > > class StructTraits<mojom::Foo, CustomFoo> {
>> > > > > // The getter of an enum field needs to return the exact mojo enum
>> type.
>> > > > > static mojom::SomeEnum value(const CustomFoo& input) {
>> > > > > ...
>> > > > > }
>> > > > > };
>> > > > >
>> > > >
>> > > > Right, but in this case, I'm passing an enum value to a mojom
>> > > > function,
>> not
>> > > encapsulating it within a struct...
>> > >
>> > > and maybe in the meantime we can figure out something better for this?
>> > > How
>> does
>> > > StructTraits handle nested mojo structs, for example?
>> >
>> > Yeah, one possible way is to follow the approach that we take for
>> > struct:
>> > - support EnumTraits, so that you can return a custom enum value from a
>> StructTraits field getter and the bindings will know how to convert it to
>> the
>> corresponding mojom enum.
>> > - support defining typemap config (in .typemap file) to map a mojom enum
>> > to
> a
>> custom enum; that solves the problem of passing an enum value to a mojom
>> function.
>> >
>> > As I said in another review thread. It shouldn't be difficult to support
> that.
>> > But I wonder whether we should just suggest users to move their enum
>> definition into mojom, instead of trying to maintain two enums (a custom
>> enum
>> and a mojom enum).
>> >
>> > Custom typemapping makes sense for structs because custom structs may
>> > have
>> extra validation/methods attached to them. But in the case of enum, there
> isn't
>> much difference between a custom enum and a mojom enum.
>> >
>> > WDYT? Thanks!
>>
>> I expect devs have been duplicating enums to avoid the 'core' code (e.g.
>> //media) from depending on Mojo.
>
> Hmm... I can see your point. I don't have a better idea than these two
> choices.
>
>> While I don't like having to declare enums
>> twice either, is there a good way to address that concern?
>
>
>
> https://codereview.chromium.org/1984433002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2016-05-25 23:58:57]

On 2016/05/25 23:52:53, sky wrote:
> Any chance Mike could land this and we straighten out the traits issue
> separately? AFAIK Mike isn't doing anything we don't already have.
> 
>   -Scott
> 
> On Wed, May 25, 2016 at 1:26 PM,  <mailto:yzshen@chromium.org> wrote:
> > On 2016/05/25 20:21:14, dcheng wrote:
> >> On 2016/05/25 at 20:13:13, yzshen wrote:
> >> > On 2016/05/25 19:56:40, dcheng wrote:
> >> > >
> >>
> >
>
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> >> > > File
> >> > > chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
> >> (right):
> >> > >
> >> > >
> >>
> >
>
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> >> > >
> >> > > chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:193:
> >> > > skia::mojom::Bitmap::From(*image.bitmap()),
> >> > > On 2016/05/25 at 18:16:41, msw wrote:
> >> > > > On 2016/05/25 17:31:26, dcheng wrote:
> >> > > > > On 2016/05/24 at 22:06:37, msw wrote:
> >> > > > > > On 2016/05/24 18:53:37, dcheng wrote:
> >> > > > > > > Can we do this with a StructTraits instead? My worry about
> >> > > > > > > adding
> >> > > further
> >> > > > > > > dependencies on the existing TypeConverter is that it's going
> >> > > > > > > to
> >> make it
> >> > > > > harder
> >> > > > > > > to remove down the road.
> >> > > > > >
> >> > > > > > Yeah, Yuzhu gave me an intro to StructTraits, and it definitely
> > seems
> >> > > > > possible. Since I'm not adding a new type converter, but just
> >> > > > > making
> > use
> >> of
> >> > > an
> >> > > > > existing one, I wonder if it's necessary for this CL. Would you
> >> > > > > like
> > me
> >> to
> >> > > add
> >> > > > > struct traits and remove all existing skia bitmap type conversion
> >> > > > > uses
> >> and
> >> > > the
> >> > > > > converter itself: (1) separately as a prerequisite to this CL, (2)
> > done
> >> as
> >> > > part
> >> > > > > of this CL (I hope not) or (3) left for a follow-up change?
> >> > > > >
> >> > > > > We can add the StructTraits without removing all uses of the old
> >> > > > > one
> > in
> >> this
> >> > > CL,
> >> > > > > I think. That way, we can start incremental migration over.
> >> > > >
> >> > > > I'd rather not pollute this wallpaper CL with the addition of skia
> > bitmap
> >> > > StructTraits. I'll put this CL on hold and implement that in a
> >> > > separate
> >> > > prerequisite CL if you think it's a blocker.
> >> > >
> >> > > I'd strongly prefer to see that. I thought it'd be easier since this
> >> > > CL
> >> would be
> >> > > defacto proof that the StructTraits works, but if you feel it would be
> >> better in
> >> > > a separate CL, that's fine with me.
> >> > >
> >> > >
> >>
> >
>
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> >> > >
> >> > > chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:194:
> >> > > static_cast<ash::mus::mojom::WallpaperLayout>(layout));
> >> > > On 2016/05/24 at 22:06:37, msw wrote:
> >> > > > On 2016/05/24 22:04:42, yzshen1 wrote:
> >> > > > > On 2016/05/24 18:53:37, dcheng wrote:
> >> > > > > > Similarly, do StructTraits work for enums yet?
> >> > > > >
> >> > > > > Yes. For example:
> >> > > > >
> >> > > > > module mojom;
> >> > > > > enum SomeEnum {};
> >> > > > > struct Foo {
> >> > > > > SomeEnum value;
> >> > > > > };
> >> > > > >
> >> > > > > The struct traits will be:
> >> > > > >
> >> > > > > template<>
> >> > > > > class StructTraits<mojom::Foo, CustomFoo> {
> >> > > > > // The getter of an enum field needs to return the exact mojo enum
> >> type.
> >> > > > > static mojom::SomeEnum value(const CustomFoo& input) {
> >> > > > > ...
> >> > > > > }
> >> > > > > };
> >> > > > >
> >> > > >
> >> > > > Right, but in this case, I'm passing an enum value to a mojom
> >> > > > function,
> >> not
> >> > > encapsulating it within a struct...
> >> > >
> >> > > and maybe in the meantime we can figure out something better for this?
> >> > > How
> >> does
> >> > > StructTraits handle nested mojo structs, for example?
> >> >
> >> > Yeah, one possible way is to follow the approach that we take for
> >> > struct:
> >> > - support EnumTraits, so that you can return a custom enum value from a
> >> StructTraits field getter and the bindings will know how to convert it to
> >> the
> >> corresponding mojom enum.
> >> > - support defining typemap config (in .typemap file) to map a mojom enum
> >> > to
> > a
> >> custom enum; that solves the problem of passing an enum value to a mojom
> >> function.
> >> >
> >> > As I said in another review thread. It shouldn't be difficult to support
> > that.
> >> > But I wonder whether we should just suggest users to move their enum
> >> definition into mojom, instead of trying to maintain two enums (a custom
> >> enum
> >> and a mojom enum).
> >> >
> >> > Custom typemapping makes sense for structs because custom structs may
> >> > have
> >> extra validation/methods attached to them. But in the case of enum, there
> > isn't
> >> much difference between a custom enum and a mojom enum.
> >> >
> >> > WDYT? Thanks!
> >>
> >> I expect devs have been duplicating enums to avoid the 'core' code (e.g.
> >> //media) from depending on Mojo.
> >
> > Hmm... I can see your point. I don't have a better idea than these two
> > choices.
> >
> >> While I don't like having to declare enums
> >> twice either, is there a good way to address that concern?
> >
> >
> >
> > https://codereview.chromium.org/1984433002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

No objections. LGTM

[dcheng, 2016-05-26 01:07:56]

It's just going to make things even harder down the road though. I'd rather we
establish a precedent for doing things with StructTraits now that we have it: if
there's some other fundamental reason why a simple StructTraits won't work here
(e.g. the current StructTraits implementation is missing some feature that will
be required for SkBitmap serialization/deserialization), then it makes sense to
punt to a followup CL... but I don't think that's the case?

On 2016/05/25 at 23:52:53, sky wrote:
> Any chance Mike could land this and we straighten out the traits issue
> separately? AFAIK Mike isn't doing anything we don't already have.
> 
>   -Scott
> 
> On Wed, May 25, 2016 at 1:26 PM,  <yzshen@chromium.org> wrote:
> > On 2016/05/25 20:21:14, dcheng wrote:
> >> On 2016/05/25 at 20:13:13, yzshen wrote:
> >> > On 2016/05/25 19:56:40, dcheng wrote:
> >> > >
> >>
> >
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> >> > > File
> >> > > chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
> >> (right):
> >> > >
> >> > >
> >>
> >
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> >> > >
> >> > > chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:193:
> >> > > skia::mojom::Bitmap::From(*image.bitmap()),
> >> > > On 2016/05/25 at 18:16:41, msw wrote:
> >> > > > On 2016/05/25 17:31:26, dcheng wrote:
> >> > > > > On 2016/05/24 at 22:06:37, msw wrote:
> >> > > > > > On 2016/05/24 18:53:37, dcheng wrote:
> >> > > > > > > Can we do this with a StructTraits instead? My worry about
> >> > > > > > > adding
> >> > > further
> >> > > > > > > dependencies on the existing TypeConverter is that it's going
> >> > > > > > > to
> >> make it
> >> > > > > harder
> >> > > > > > > to remove down the road.
> >> > > > > >
> >> > > > > > Yeah, Yuzhu gave me an intro to StructTraits, and it definitely
> > seems
> >> > > > > possible. Since I'm not adding a new type converter, but just
> >> > > > > making
> > use
> >> of
> >> > > an
> >> > > > > existing one, I wonder if it's necessary for this CL. Would you
> >> > > > > like
> > me
> >> to
> >> > > add
> >> > > > > struct traits and remove all existing skia bitmap type conversion
> >> > > > > uses
> >> and
> >> > > the
> >> > > > > converter itself: (1) separately as a prerequisite to this CL, (2)
> > done
> >> as
> >> > > part
> >> > > > > of this CL (I hope not) or (3) left for a follow-up change?
> >> > > > >
> >> > > > > We can add the StructTraits without removing all uses of the old
> >> > > > > one
> > in
> >> this
> >> > > CL,
> >> > > > > I think. That way, we can start incremental migration over.
> >> > > >
> >> > > > I'd rather not pollute this wallpaper CL with the addition of skia
> > bitmap
> >> > > StructTraits. I'll put this CL on hold and implement that in a
> >> > > separate
> >> > > prerequisite CL if you think it's a blocker.
> >> > >
> >> > > I'd strongly prefer to see that. I thought it'd be easier since this
> >> > > CL
> >> would be
> >> > > defacto proof that the StructTraits works, but if you feel it would be
> >> better in
> >> > > a separate CL, that's fine with me.
> >> > >
> >> > >
> >>
> >
https://codereview.chromium.org/1984433002/diff/360001/chrome/browser/chromeo...
> >> > >
> >> > > chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:194:
> >> > > static_cast<ash::mus::mojom::WallpaperLayout>(layout));
> >> > > On 2016/05/24 at 22:06:37, msw wrote:
> >> > > > On 2016/05/24 22:04:42, yzshen1 wrote:
> >> > > > > On 2016/05/24 18:53:37, dcheng wrote:
> >> > > > > > Similarly, do StructTraits work for enums yet?
> >> > > > >
> >> > > > > Yes. For example:
> >> > > > >
> >> > > > > module mojom;
> >> > > > > enum SomeEnum {};
> >> > > > > struct Foo {
> >> > > > > SomeEnum value;
> >> > > > > };
> >> > > > >
> >> > > > > The struct traits will be:
> >> > > > >
> >> > > > > template<>
> >> > > > > class StructTraits<mojom::Foo, CustomFoo> {
> >> > > > > // The getter of an enum field needs to return the exact mojo enum
> >> type.
> >> > > > > static mojom::SomeEnum value(const CustomFoo& input) {
> >> > > > > ...
> >> > > > > }
> >> > > > > };
> >> > > > >
> >> > > >
> >> > > > Right, but in this case, I'm passing an enum value to a mojom
> >> > > > function,
> >> not
> >> > > encapsulating it within a struct...
> >> > >
> >> > > and maybe in the meantime we can figure out something better for this?
> >> > > How
> >> does
> >> > > StructTraits handle nested mojo structs, for example?
> >> >
> >> > Yeah, one possible way is to follow the approach that we take for
> >> > struct:
> >> > - support EnumTraits, so that you can return a custom enum value from a
> >> StructTraits field getter and the bindings will know how to convert it to
> >> the
> >> corresponding mojom enum.
> >> > - support defining typemap config (in .typemap file) to map a mojom enum
> >> > to
> > a
> >> custom enum; that solves the problem of passing an enum value to a mojom
> >> function.
> >> >
> >> > As I said in another review thread. It shouldn't be difficult to support
> > that.
> >> > But I wonder whether we should just suggest users to move their enum
> >> definition into mojom, instead of trying to maintain two enums (a custom
> >> enum
> >> and a mojom enum).
> >> >
> >> > Custom typemapping makes sense for structs because custom structs may
> >> > have
> >> extra validation/methods attached to them. But in the case of enum, there
> > isn't
> >> much difference between a custom enum and a mojom enum.
> >> >
> >> > WDYT? Thanks!
> >>
> >> I expect devs have been duplicating enums to avoid the 'core' code (e.g.
> >> //media) from depending on Mojo.
> >
> > Hmm... I can see your point. I don't have a better idea than these two
> > choices.
> >
> >> While I don't like having to declare enums
> >> twice either, is there a good way to address that concern?
> >
> >
> >
> > https://codereview.chromium.org/1984433002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 
>

[msw, 2016-05-26 04:55:43]

I'm working on a CL... slowly: https://codereview.chromium.org/2014013002
I'll have to spend an extra day or so figuring it out; StructTraits is new to
me.
It's not possible to use for a single case, afaict, it affects all users at
once.
Also, I'm unaware of any benefits; it just looks like a type converter to me!

On 2016/05/26 01:07:56, dcheng wrote:
> It's just going to make things even harder down the road though. I'd rather we
> establish a precedent for doing things with StructTraits now that we have it:
if
> there's some other fundamental reason why a simple StructTraits won't work
here
> (e.g. the current StructTraits implementation is missing some feature that
will
> be required for SkBitmap serialization/deserialization), then it makes sense
to
> punt to a followup CL... but I don't think that's the case?
> 
> On 2016/05/25 at 23:52:53, sky wrote:
> > Any chance Mike could land this and we straighten out the traits issue
> > separately? AFAIK Mike isn't doing anything we don't already have.
> > 
> >   -Scott

[dcheng, 2016-05-26 06:46:07]

On 2016/05/26 at 04:55:43, msw wrote:
> I'm working on a CL... slowly: https://codereview.chromium.org/2014013002
> I'll have to spend an extra day or so figuring it out; StructTraits is new to
me.
> It's not possible to use for a single case, afaict, it affects all users at
once.
> Also, I'm unaware of any benefits; it just looks like a type converter to me!

A TypeConverter doesn't provide any way to validate the input. If a message
sender passes a bogus skia::mojom::BitmapPtr across the wire (e.g.
pixel_data.size() != SkBitmap::getSize(), we silently return a null SkBitmap
rather than just dropping the message and closing the pipe.

> 
> On 2016/05/26 01:07:56, dcheng wrote:
> > It's just going to make things even harder down the road though. I'd rather
we
> > establish a precedent for doing things with StructTraits now that we have
it: if
> > there's some other fundamental reason why a simple StructTraits won't work
here
> > (e.g. the current StructTraits implementation is missing some feature that
will
> > be required for SkBitmap serialization/deserialization), then it makes sense
to
> > punt to a followup CL... but I don't think that's the case?
> > 
> > On 2016/05/25 at 23:52:53, sky wrote:
> > > Any chance Mike could land this and we straighten out the traits issue
> > > separately? AFAIK Mike isn't doing anything we don't already have.
> > > 
> > >   -Scott

[msw, 2016-06-01 19:38:09]

Patchset #23 (id:480001) has been deleted

[msw, 2016-06-01 20:12:35]

Patchset #24 (id:520001) has been deleted

[msw, 2016-06-01 20:12:49]

Patchset #23 (id:500001) has been deleted

[msw, 2016-06-01 20:17:58]

Patchset #23 (id:540001) has been deleted

[msw, 2016-06-01 20:21:13]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 20:21:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984433002/560001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984433002/560001

[commit-bot: I haz the power, 2016-06-01 20:25:11]

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

[commit-bot: I haz the power, 2016-06-01 20:25:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msw, 2016-06-01 20:26:59]

Description was changed from

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405,613657,613707
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org
==========

to

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

Depends on: https://codereview.chromium.org/2014013002

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405,613657,613707
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org
==========

[msw, 2016-06-02 21:29:09]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[msw, 2016-06-02 21:30:07]

Description was changed from

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

Depends on: https://codereview.chromium.org/2014013002

TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405,613657,613707
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org
==========

to

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix picker regression from codereview.chromium.org/2009853002
TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405,613657,613707
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org
==========

[commit-bot: I haz the power, 2016-06-02 21:30:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984433002/580001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984433002/580001

[msw, 2016-06-02 21:31:17]

Hey Daniel, please take another look. This CL now uses the SkBitmap struct
traits I implemented in https://codereview.chromium.org/2014013002

[dcheng, 2016-06-02 21:36:51]

Enum integration seems to be a repeated pain point...

https://codereview.chromium.org/1984433002/diff/580001/ash/sysui/user_wallpap...
File ash/sysui/user_wallpaper_delegate_mus.cc (right):

https://codereview.chromium.org/1984433002/diff/580001/ash/sysui/user_wallpap...
ash/sysui/user_wallpaper_delegate_mus.cc:68: image,
static_cast<wallpaper::WallpaperLayout>(layout));
=/

What guarantees that the enums line up?

[commit-bot: I haz the power, 2016-06-02 21:39:15]

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

[commit-bot: I haz the power, 2016-06-02 21:39:17]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msw, 2016-06-02 22:19:10]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[msw, 2016-06-02 22:19:16]

https://codereview.chromium.org/1984433002/diff/580001/ash/sysui/user_wallpap...
File ash/sysui/user_wallpaper_delegate_mus.cc (right):

https://codereview.chromium.org/1984433002/diff/580001/ash/sysui/user_wallpap...
ash/sysui/user_wallpaper_delegate_mus.cc:68: image,
static_cast<wallpaper::WallpaperLayout>(layout));
On 2016/06/02 21:36:51, dcheng wrote:
> =/
> 
> What guarantees that the enums line up?

I added explicit conversion functions.

[commit-bot: I haz the power, 2016-06-02 22:20:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984433002/620001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984433002/620001

[dcheng, 2016-06-02 22:27:34]

On 2016/06/02 at 22:19:16, msw wrote:
>
https://codereview.chromium.org/1984433002/diff/580001/ash/sysui/user_wallpap...
> File ash/sysui/user_wallpaper_delegate_mus.cc (right):
> 
>
https://codereview.chromium.org/1984433002/diff/580001/ash/sysui/user_wallpap...
> ash/sysui/user_wallpaper_delegate_mus.cc:68: image,
static_cast<wallpaper::WallpaperLayout>(layout));
> On 2016/06/02 21:36:51, dcheng wrote:
> > =/
> > 
> > What guarantees that the enums line up?
> 
> I added explicit conversion functions.

I think it was OK as is, we just need some static asserts or something...

But the problem of converting between mojo and non-mojo enums has been appearing
in quite a few CLs now, so I think we need to figure out something better for
the long run. I know yzshen@ mentioned not wanting to add EnumTraits, but this
seems strictly worse for people writing code that works with Mojo.

[dcheng, 2016-06-02 22:27:44]

(Also, LGTM)

[commit-bot: I haz the power, 2016-06-03 01:03:46]

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

[commit-bot: I haz the power, 2016-06-03 01:03:49]

Dry run: This issue passed the CQ dry run.

[msw, 2016-06-03 02:22:04]

The CQ bit was checked by msw@chromium.org

[msw, 2016-06-03 02:22:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, sky@chromium.org, yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1984433002/#ps620001
(title: "Add explicit WallpaperLayout enum conversion functions.")

[commit-bot: I haz the power, 2016-06-03 02:23:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984433002/620001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984433002/620001

[commit-bot: I haz the power, 2016-06-03 02:34:24]

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

[commit-bot: I haz the power, 2016-06-03 02:34:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yzshen1, 2016-06-03 15:22:09]

On 2016/06/02 22:27:34, dcheng wrote:
> On 2016/06/02 at 22:19:16, msw wrote:
> >
>
https://codereview.chromium.org/1984433002/diff/580001/ash/sysui/user_wallpap...
> > File ash/sysui/user_wallpaper_delegate_mus.cc (right):
> > 
> >
>
https://codereview.chromium.org/1984433002/diff/580001/ash/sysui/user_wallpap...
> > ash/sysui/user_wallpaper_delegate_mus.cc:68: image,
> static_cast<wallpaper::WallpaperLayout>(layout));
> > On 2016/06/02 21:36:51, dcheng wrote:
> > > =/
> > > 
> > > What guarantees that the enums line up?
> > 
> > I added explicit conversion functions.
> 
> I think it was OK as is, we just need some static asserts or something...
> 
> But the problem of converting between mojo and non-mojo enums has been
appearing
> in quite a few CLs now, so I think we need to figure out something better for
> the long run. I know yzshen@ mentioned not wanting to add EnumTraits, but this
> seems strictly worse for people writing code that works with Mojo.

Hi, Daniel.

After some discussion, I think I am persuaded to add custom type mapping for
enums and native-only enum support.
I will start working on it next week.

[bshe, 2016-06-03 17:10:06]

On 2016/06/03 15:22:09, yzshen1 wrote:
> On 2016/06/02 22:27:34, dcheng wrote:
> > On 2016/06/02 at 22:19:16, msw wrote:
> > >
> >
>
https://codereview.chromium.org/1984433002/diff/580001/ash/sysui/user_wallpap...
> > > File ash/sysui/user_wallpaper_delegate_mus.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1984433002/diff/580001/ash/sysui/user_wallpap...
> > > ash/sysui/user_wallpaper_delegate_mus.cc:68: image,
> > static_cast<wallpaper::WallpaperLayout>(layout));
> > > On 2016/06/02 21:36:51, dcheng wrote:
> > > > =/
> > > > 
> > > > What guarantees that the enums line up?
> > > 
> > > I added explicit conversion functions.
> > 
> > I think it was OK as is, we just need some static asserts or something...
> > 
> > But the problem of converting between mojo and non-mojo enums has been
> appearing
> > in quite a few CLs now, so I think we need to figure out something better
for
> > the long run. I know yzshen@ mentioned not wanting to add EnumTraits, but
this
> > seems strictly worse for people writing code that works with Mojo.
> 
> Hi, Daniel.
> 
> After some discussion, I think I am persuaded to add custom type mapping for
> enums and native-only enum support.
> I will start working on it next week.

components/wallpaper dep lgtm

[msw, 2016-06-03 17:12:44]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2016-06-03 17:13:22]

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

[commit-bot: I haz the power, 2016-06-03 17:19:48]

Description was changed from

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix picker regression from codereview.chromium.org/2009853002
TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405,613657,613707
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org
==========

to

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix picker regression from codereview.chromium.org/2009853002
TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405,613657,613707
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org
==========

[commit-bot: I haz the power, 2016-06-03 17:19:51]

Committed patchset #26 (id:620001)

[commit-bot: I haz the power, 2016-06-03 17:21:45]

Description was changed from

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix picker regression from codereview.chromium.org/2009853002
TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405,613657,613707
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org
==========

to

==========
Hook up Chrome's wallpaper picker for mash.

Add wallpaper.mojom for ash_sysui<->chrome interaction.
Implement WallpaperController in ash_sysui UserWallpaperDelegateMus.
Implement minimal new ChromeWallpaperHelper in chrome.
(lets chrome set mash wallpaper, mash trigger chrome wallpaper picker)

Add SetWallpaper helper for WallpaperManager to work in mash too.
Remove sysui_application.cc debug wallpaper initialization.
Bail early on WindowStateManager::[Minimize|Restore]InactiveWindows.
Workaround DesktopBackgroundController shell usage.

TODO: Fix picker regression from codereview.chromium.org/2009853002
TODO: Fix wallpaper_manager.cc's other Shell::GetInstance uses.
TODO: Fix DesktopBackgroundController display size checks in Mash.
TODO: Implement window hide/restore for wallpaper picker.
TODO: Implement missing UserWallpaperDelegateMus functionality.

BUG=557405,613657,613707
TEST=Chrome OS wallpaper works with or without --mash flag; right click mash
desktop -> "Set wallpaper..." works.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org

Committed: https://crrev.com/4d780247a697f62d0e27a6eb5dfb309807f40881
Cr-Commit-Position: refs/heads/master@{#397739}
==========

[commit-bot: I haz the power, 2016-06-03 17:21:48]

Patchset 26 (id:??) landed as
https://crrev.com/4d780247a697f62d0e27a6eb5dfb309807f40881
Cr-Commit-Position: refs/heads/master@{#397739}