Issue 2413503002: Cleanup mojo Wallpaper interfaces for mash.

msw

Description was changed from ========== Use mojo Wallpaper interfaces for both mash and classic ash. ...

4 years, 2 months ago (2016-10-11 21:56:31 UTC) #1

msw

Description was changed from ========== Use mojo Wallpaper interfaces for both mash and classic ash. ...

4 years, 2 months ago (2016-10-11 21:57:42 UTC) #3

msw

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

4 years, 2 months ago (2016-10-11 22:06:04 UTC) #5

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2413503002/20001

4 years, 2 months ago (2016-10-11 22:06:34 UTC) #6

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-11 22:45:18 UTC) #7

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-11 22:45:19 UTC) #8

James Cook

LGTM with nits. Ping me again if the test failure requires significant changes to fix. ...

4 years, 2 months ago (2016-10-11 23:21:49 UTC) #9

msw

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

4 years, 2 months ago (2016-10-14 20:47:28 UTC) #10

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2413503002/80001

4 years, 2 months ago (2016-10-14 20:48:07 UTC) #11

msw

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

4 years, 2 months ago (2016-10-14 21:03:26 UTC) #13

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2413503002/120001

4 years, 2 months ago (2016-10-14 21:04:18 UTC) #14

msw

Description was changed from ========== Use mojo Wallpaper interfaces for both mash and classic ash. ...

4 years, 2 months ago (2016-10-14 21:05:21 UTC) #15

msw

Hey James, please take another look. This CL no longer enables mojo codepaths for transporting ...

4 years, 2 months ago (2016-10-14 21:07:43 UTC) #16

Hey James, please take another look. This CL no longer enables mojo codepaths
for transporting wallpapers in classic ash production code, see my explanatory
comment below.

https://codereview.chromium.org/2413503002/diff/20001/ash/common/wallpaper/wa...
File ash/common/wallpaper/wallpaper_controller.cc (right):

https://codereview.chromium.org/2413503002/diff/20001/ash/common/wallpaper/wa...
ash/common/wallpaper/wallpaper_controller.cc:35: case
ash::mojom::WallpaperLayout::CENTER:
On 2016/10/11 23:21:49, James Cook wrote:
> nit: ash:: not needed?

Done.

https://codereview.chromium.org/2413503002/diff/20001/ash/common/wallpaper/wa...
ash/common/wallpaper/wallpaper_controller.cc:210: auto shell = WmShell::Get();
On 2016/10/11 23:21:49, James Cook wrote:
> nit: I think WmShell* is better than auto here. The type isn't long enough to
> make the shortening worth it.

Done.

https://codereview.chromium.org/2413503002/diff/20001/ash/common/wallpaper/wa...
ash/common/wallpaper/wallpaper_controller.cc:211: auto connector =
shell->delegate()->GetShellConnector();
On 2016/10/11 23:21:49, James Cook wrote:
> This one might be worth it for wrapping, but also consider "auto*" so it's
> obvious the !connector below is checking a pointer. Or just spell out the
type.

Done.

https://codereview.chromium.org/2413503002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_interface_factory.cc (right):

https://codereview.chromium.org/2413503002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_interface_factory.cc:124:
chromeos::WallpaperManager::Get()->BindRequest(std::move(request));
On 2016/10/11 23:21:49, James Cook wrote:
> nit: chromeos:: not needed?

Done.

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

https://codereview.chromium.org/2413503002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:187: //
TODO(crbug.com/655875): Optimize ash wallpaper transport; avoid sending
Because sending large wallpaper bitmaps over Mojo might cause UI jank (Robert
and I estimated ~500ms just to send ~100MB over a pipe in the login UI for some
lower spec devices with 4k support, etc.), I'm going to leave classic ash's
pattern of communicating directly with the WallpaperController here for now. We
can investigate the performance impact and/or optimize this separately before
enabling. (also note that enabling mojo here would require additional changes to
tests and SetDefaultWallpaperPath's use of WallpaperIsAlreadyLoaded that I
haven't quite resolved) This CL is still a worthwhile consolidation of logic and
addition of wallpaper enum traits.

James Cook

Some little things. https://codereview.chromium.org/2413503002/diff/120001/ash/common/wallpaper/wallpaper_controller.cc File ash/common/wallpaper/wallpaper_controller.cc (right): https://codereview.chromium.org/2413503002/diff/120001/ash/common/wallpaper/wallpaper_controller.cc#newcode212 ash/common/wallpaper/wallpaper_controller.cc:212: callback.Run(*GetWallpaper().bitmap(), GetWallpaperLayout()); nit: maybe comment here ...

4 years, 2 months ago (2016-10-14 21:58:02 UTC) #17

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-14 22:05:18 UTC) #18

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-10-14 22:05:19 UTC) #19

msw

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

4 years, 2 months ago (2016-10-17 21:36:01 UTC) #21

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2413503002/180001

4 years, 2 months ago (2016-10-17 21:36:52 UTC) #22

msw

Hey James, please take another look; thanks! As discussed offline, I removed the GetWallpaper functionality ...

4 years, 2 months ago (2016-10-17 22:22:48 UTC) #23

Hey James, please take another look; thanks! As discussed offline, I removed the
GetWallpaper functionality and restored the classic ash pattern of testing (to
match the current production pattern of communicating directly with the
wallpaper controller). We'll need to address the blocking perf bug before
actually switching over to mojo here.

https://codereview.chromium.org/2413503002/diff/120001/ash/common/wallpaper/w...
File ash/common/wallpaper/wallpaper_controller.cc (right):

https://codereview.chromium.org/2413503002/diff/120001/ash/common/wallpaper/w...
ash/common/wallpaper/wallpaper_controller.cc:212:
callback.Run(*GetWallpaper().bitmap(), GetWallpaperLayout());
On 2016/10/14 21:58:02, James Cook wrote:
> nit: maybe comment here also that this can be slow?

Moot; removed.

https://codereview.chromium.org/2413503002/diff/120001/ash/public/interfaces/...
File ash/public/interfaces/wallpaper.mojom (right):

https://codereview.chromium.org/2413503002/diff/120001/ash/public/interfaces/...
ash/public/interfaces/wallpaper.mojom:15: GetWallpaper() => (skia.mojom.Bitmap?
wallpaper, WallpaperLayout layout);
On 2016/10/14 21:58:02, James Cook wrote:
> Document all mojom functions. In this case the most interesting bit is what it
> means to receive a null bitmap.
> 
> (Also, if I'm just reading the interface it's a little odd that Chrome would
> need to read back the wallpaper.)

Done; documented Set and removed Get.

https://codereview.chromium.org/2413503002/diff/120001/ash/public/interfaces/...
ash/public/interfaces/wallpaper.mojom:19: interface WallpaperManager {
On 2016/10/14 21:58:02, James Cook wrote:
> aside: When reading other parts of the code it's not immediately apparent
which
> direction the communication is for WallpaperManager vs. WallpaperController.
> Maybe in another CL WallpaperManager should become WallpaperControllerClient?

Perhaps; the WallpaperManager class and its WallpaperManagerBase base class do
quite a lot; I'm not sure it makes sense to simply label the Chrome impl as a
client of the WallpaperController interface, but yeah, the naming isn't ideal.

https://codereview.chromium.org/2413503002/diff/120001/chrome/browser/chromeo...
File
chrome/browser/chromeos/customization/customization_wallpaper_downloader_browsertest.cc
(right):

https://codereview.chromium.org/2413503002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/customization/customization_wallpaper_downloader_browsertest.cc:106:
auto connector = connection ? connection->GetConnector() : nullptr;
On 2016/10/14 21:58:02, James Cook wrote:
> As before, I don't think these types are complex enough to justify "auto". It
> probably avoids line wrapping, but I don't think that's enough benefit. I
would
> rather have explicit types.

I reverted these test changes.

https://codereview.chromium.org/2413503002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/customization/customization_wallpaper_downloader_browsertest.cc:111:
// Under mash the WallpaperController interface is in the ash process. In
On 2016/10/14 21:58:02, James Cook wrote:
> So does this test no longer exercise the classic ash code path?

I restored the classic ash code path here.

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

https://codereview.chromium.org/2413503002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:178: auto
connector = connection ? connection->GetConnector() : nullptr;
On 2016/10/14 21:58:02, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/2413503002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:190: } else
if (ash::WmShell::HasInstance()) {
On 2016/10/14 21:58:02, James Cook wrote:
> I would keep the previous comment about tests. (In general I would expect
> !IsRunningInMash to imply we have a shell instance.)
> 
> Frankly I wonder if that comment is still true. It seems odd that we would run
> tests without a shell.

Running *Wallpaper* ash_unittests, browser_tests and interactive_ui_tests seems
to show that you're right. I've removed the two shell HasInstance checks and
test comments in this file; we'll see how the trybot runs go.

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-17 22:38:53 UTC) #24

commit-bot: I haz the power

Dry run: Exceeded global retry quota

4 years, 2 months ago (2016-10-17 22:38:54 UTC) #25

msw

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

4 years, 2 months ago (2016-10-17 23:31:19 UTC) #26

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2413503002/200001

4 years, 2 months ago (2016-10-17 23:32:33 UTC) #27

msw

Description was changed from ========== Cleanup mojo Wallpaper interfaces for mash. Implement WallpaperController in ash::WallpaperController. ...

4 years, 2 months ago (2016-10-17 23:48:59 UTC) #29

msw

msw@chromium.org changed reviewers: + sky@chromium.org, stevenjb@chromium.org, tsepez@chromium.org, yusukes@chromium.org

4 years, 2 months ago (2016-10-17 23:49:00 UTC) #30

msw

OWNERS, please take a look; thanks! tsepez: ash/public/interfaces/* stevenjb: c/b/chromeos/* and c/b/ui/webui/* sky: non-chromeos non-webui ...

4 years, 2 months ago (2016-10-17 23:53:38 UTC) #31

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-18 01:18:16 UTC) #34

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-10-18 01:18:17 UTC) #35

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2413503002/200001

4 years, 2 months ago (2016-10-18 20:47:06 UTC) #39

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-18 20:51:56 UTC) #40

commit-bot: I haz the power

Failed to apply patch for ash/common/mojo_interface_factory.cc: While running git apply --index -3 -p1; error: patch ...

4 years, 2 months ago (2016-10-18 20:51:59 UTC) #41

Failed to apply patch for ash/common/mojo_interface_factory.cc:
While running git apply --index -3 -p1;
  error: patch failed: ash/common/mojo_interface_factory.cc:6
  Falling back to three-way merge...
  Applied patch to 'ash/common/mojo_interface_factory.cc' with conflicts.
  U ash/common/mojo_interface_factory.cc

Patch:       ash/common/mojo_interface_factory.cc
Index: ash/common/mojo_interface_factory.cc
diff --git a/ash/common/mojo_interface_factory.cc
b/ash/common/mojo_interface_factory.cc
index
e4384fef951d561d0f2334d9a56afb3d4bb5188f..a3c4cc84f23eea374689e83c5202c61ab3a108dc
100644
--- a/ash/common/mojo_interface_factory.cc
+++ b/ash/common/mojo_interface_factory.cc
@@ -6,6 +6,7 @@
 
 #include "ash/common/shelf/shelf_controller.h"
 #include "ash/common/system/tray/system_tray_controller.h"
+#include "ash/common/wallpaper/wallpaper_controller.h"
 #include "ash/common/wm_shell.h"
 #include "base/bind.h"
 #include "services/service_manager/public/cpp/interface_registry.h"
@@ -14,8 +15,7 @@ namespace ash {
 
 namespace {
 
-void BindShelfControllerRequestOnMainThread(
-    mojom::ShelfControllerRequest request) {
+void BindShelfRequestOnMainThread(mojom::ShelfControllerRequest request) {
   WmShell::Get()->shelf_controller()->BindRequest(std::move(request));
 }
 
@@ -23,6 +23,11 @@ void
BindSystemTrayRequestOnMainThread(mojom::SystemTrayRequest request) {
   WmShell::Get()->system_tray_controller()->BindRequest(std::move(request));
 }
 
+void BindWallpaperRequestOnMainThread(
+    mojom::WallpaperControllerRequest request) {
+  WmShell::Get()->wallpaper_controller()->BindRequest(std::move(request));
+}
+
 }  // namespace
 
 namespace mojo_interface_factory {
@@ -30,10 +35,12 @@ namespace mojo_interface_factory {
 void RegisterInterfaces(
     service_manager::InterfaceRegistry* registry,
     scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner) {
-  registry->AddInterface(base::Bind(&BindShelfControllerRequestOnMainThread),
+  registry->AddInterface(base::Bind(&BindShelfRequestOnMainThread),
                          main_thread_task_runner);
   registry->AddInterface(base::Bind(&BindSystemTrayRequestOnMainThread),
                          main_thread_task_runner);
+  registry->AddInterface(base::Bind(&BindWallpaperRequestOnMainThread),
+                         main_thread_task_runner);
 }
 
 }  // namespace mojo_interface_factory

msw

Description was changed from ========== Cleanup mojo Wallpaper interfaces for mash. Implement WallpaperController in ash::WallpaperController. ...

4 years, 2 months ago (2016-10-18 21:30:10 UTC) #42

msw

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, jamescook@chromium.org, yusukes@chromium.org, sky@chromium.org, ...

4 years, 2 months ago (2016-10-18 21:30:19 UTC) #44

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2413503002/220001

4 years, 2 months ago (2016-10-18 21:30:52 UTC) #45

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-18 23:34:39 UTC) #46

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/297992)

4 years, 2 months ago (2016-10-18 23:34:40 UTC) #47

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2413503002/220001

4 years, 2 months ago (2016-10-19 00:02:50 UTC) #49

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-19 00:26:33 UTC) #50

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-19 00:26:35 UTC) #51

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2413503002/220001

4 years, 2 months ago (2016-10-19 01:33:31 UTC) #53

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-19 05:26:54 UTC) #54

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/298323)

4 years, 2 months ago (2016-10-19 05:26:56 UTC) #55

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2413503002/220001

4 years, 2 months ago (2016-10-19 15:15:58 UTC) #57

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-19 15:35:28 UTC) #58

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/299053)

4 years, 2 months ago (2016-10-19 15:35:30 UTC) #59

msw

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, jamescook@chromium.org, yusukes@chromium.org, sky@chromium.org, ...

4 years, 2 months ago (2016-10-19 16:04:21 UTC) #61

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2413503002/240001

4 years, 2 months ago (2016-10-19 16:04:59 UTC) #62

commit-bot: I haz the power

Description was changed from ========== Cleanup mojo Wallpaper interfaces for mash. Implement WallpaperController in ash::WallpaperController. ...

4 years, 2 months ago (2016-10-19 17:06:04 UTC) #63

commit-bot: I haz the power

Description was changed from ========== Cleanup mojo Wallpaper interfaces for mash. Implement WallpaperController in ash::WallpaperController. ...

4 years, 2 months ago (2016-10-21 13:09:33 UTC) #65

commit-bot: I haz the power

4 years, 2 months ago (2016-10-21 13:09:34 UTC) #66

Message was sent while issue was closed.

Patchset 11 (id:??) landed as
https://crrev.com/c691bf17a2b2fdee9c863be37134149d042787b4
Cr-Commit-Position: refs/heads/master@{#426227}

Issue 2413503002: Cleanup mojo Wallpaper interfaces for mash. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Cleanup. #

Patch Set 3 : Address comments; add WallpaperController::GetWallpaper; fixing tests. #

Patch Set 4 : Add row_bytes field to skia bitmap mojo struct. #

Patch Set 5 : Cleanup. #

Patch Set 6 : Sync and rebase; do not use mojo in classic ash. #

Patch Set 7 : Address comments; remove GetWallpaper and test path change. #

Patch Set 8 : Sync and rebase; cleanup the SetWallpaper helper. #

Patch Set 9 : Restore WmShell::HasInstance check in SetWallpaper for unit_tests. #

Patch Set 10 : Sync and rebase. #

Patch Set 11 : Sync and rebase again... #

Messages