Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2155743003:
mash: Show status area widget in mojo:ash instead of mojo:sysui (Closed)
|
Created:
4 years, 5 months ago by James Cook Modified:
4 years, 5 months ago Reviewers:
msw CC:
chromium-reviews, kalyank, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmash: Initialize chromeos components and strings
This is required to show the status area widget in mojo:ash.
* Initialize various service singletons in WindowManagerApplication
* Add ash strings to the window manager pak file
BUG=619636
TEST=ash_unittests, mash_unittests
Committed: https://crrev.com/4d37f2228b46a8f0155e4130e33a7f3615e3192a
Cr-Commit-Position: refs/heads/master@{#406126}
Patch Set 1 #Patch Set 2 : tweak #
Total comments: 10
Patch Set 3 : review comments #Patch Set 4 : rebase #Patch Set 5 : nogncheck #Messages
Total messages: 30 (19 generated)
jamescook@chromium.org changed reviewers: + msw@chromium.org
msw, please take a look!
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== mash: Show status area widget in mojo:ash instead of mojo:sysui This is a step toward eliminating mojo:sysui in favor of combined wm/sysui. * Initialize various service singletons in WindowManagerApplication * Add ash strings to the window manager pak file * Create StatusAreaWidget in ash::mus::RootWindowController, which will become the place to access it (instead of via ShelfWidget) * Hide the widget in mojo:sysui with some in_mus checks TODO: Fix layout after root window resize. BUG=619636 TEST=ash_unittests, mash_unittests ========== to ========== mash: Show status area widget in mojo:ash instead of mojo:sysui This is a step toward eliminating mojo:sysui in favor of combined wm/sysui. * Initialize various service singletons in WindowManagerApplication * Add ash strings to the window manager pak file * Create StatusAreaWidget in ash::mus::RootWindowController, which will become the place to access it (instead of via ShelfWidget) * Hide the widget in mojo:sysui with some in_mus checks TODO: Fix layout after root window resize and shelf alignment change BUG=619636 TEST=ash_unittests, mash_unittests ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2155743003/diff/20001/ash/mus/BUILD.gn File ash/mus/BUILD.gn (right): https://codereview.chromium.org/2155743003/diff/20001/ash/mus/BUILD.gn#newcode85 ash/mus/BUILD.gn:85: "//device/bluetooth", nit: move to if (is_chromeos) like window_manager_application.cc's include? https://codereview.chromium.org/2155743003/diff/20001/ash/mus/BUILD.gn#newcod... ash/mus/BUILD.gn:118: "//chromeos:power_manager_proto", What is this needed for? https://codereview.chromium.org/2155743003/diff/20001/ash/mus/root_window_con... File ash/mus/root_window_controller.cc (right): https://codereview.chromium.org/2155743003/diff/20001/ash/mus/root_window_con... ash/mus/root_window_controller.cc:229: status_area_widget->Show(); nit: only show if (WmShell::Get()->GetSessionStateDelegate()->IsActiveUserSessionStarted()) like shelf_widget.cc? (maybe we do want to unconditionally show it here though...) https://codereview.chromium.org/2155743003/diff/20001/ash/mus/window_manager_... File ash/mus/window_manager_application.cc (right): https://codereview.chromium.org/2155743003/diff/20001/ash/mus/window_manager_... ash/mus/window_manager_application.cc:39: void InitializeSingletons() { nit: InitializeComponents (like ash_sysui)? ditto for Shutdown. https://codereview.chromium.org/2155743003/diff/20001/ash/shelf/shelf_widget.cc File ash/shelf/shelf_widget.cc (right): https://codereview.chromium.org/2155743003/diff/20001/ash/shelf/shelf_widget.... ash/shelf/shelf_widget.cc:596: status_area_widget_ = Should we still create this instance? I wonder what sorts of issues will come up with this deprecated instance still being accessed and used throughout ash_sysui instead of the new instance (besides the obvious layout defects). Should we actually land this (even as a short-term path), or is it a proof of concept?
Description was changed from ========== mash: Show status area widget in mojo:ash instead of mojo:sysui This is a step toward eliminating mojo:sysui in favor of combined wm/sysui. * Initialize various service singletons in WindowManagerApplication * Add ash strings to the window manager pak file * Create StatusAreaWidget in ash::mus::RootWindowController, which will become the place to access it (instead of via ShelfWidget) * Hide the widget in mojo:sysui with some in_mus checks TODO: Fix layout after root window resize and shelf alignment change BUG=619636 TEST=ash_unittests, mash_unittests ========== to ========== mash: Initialize chromeos components and strings This is required to show the status area widget in mojo:ash. * Initialize various service singletons in WindowManagerApplication * Add ash strings to the window manager pak file BUG=619636 TEST=ash_unittests, mash_unittests ==========
msw, please take another look https://codereview.chromium.org/2155743003/diff/20001/ash/mus/BUILD.gn File ash/mus/BUILD.gn (right): https://codereview.chromium.org/2155743003/diff/20001/ash/mus/BUILD.gn#newcode85 ash/mus/BUILD.gn:85: "//device/bluetooth", On 2016/07/18 17:20:46, msw wrote: > nit: move to if (is_chromeos) like window_manager_application.cc's include? Done. https://codereview.chromium.org/2155743003/diff/20001/ash/mus/BUILD.gn#newcod... ash/mus/BUILD.gn:118: "//chromeos:power_manager_proto", On 2016/07/18 17:20:46, msw wrote: > What is this needed for? It's required to generate this header, used by power_status.h: #include "chromeos/dbus/power_manager/power_supply_properties.pb.h" https://codereview.chromium.org/2155743003/diff/20001/ash/mus/root_window_con... File ash/mus/root_window_controller.cc (right): https://codereview.chromium.org/2155743003/diff/20001/ash/mus/root_window_con... ash/mus/root_window_controller.cc:229: status_area_widget->Show(); On 2016/07/18 17:20:46, msw wrote: > nit: only show if > (WmShell::Get()->GetSessionStateDelegate()->IsActiveUserSessionStarted()) like > shelf_widget.cc? (maybe we do want to unconditionally show it here though...) As discussed, reverted showing the widget. https://codereview.chromium.org/2155743003/diff/20001/ash/mus/window_manager_... File ash/mus/window_manager_application.cc (right): https://codereview.chromium.org/2155743003/diff/20001/ash/mus/window_manager_... ash/mus/window_manager_application.cc:39: void InitializeSingletons() { On 2016/07/18 17:20:46, msw wrote: > nit: InitializeComponents (like ash_sysui)? ditto for Shutdown. Done. https://codereview.chromium.org/2155743003/diff/20001/ash/shelf/shelf_widget.cc File ash/shelf/shelf_widget.cc (right): https://codereview.chromium.org/2155743003/diff/20001/ash/shelf/shelf_widget.... ash/shelf/shelf_widget.cc:596: status_area_widget_ = On 2016/07/18 17:20:46, msw wrote: > Should we still create this instance? I wonder what sorts of issues will come up > with this deprecated instance still being accessed and used throughout ash_sysui > instead of the new instance (besides the obvious layout defects). Should we > actually land this (even as a short-term path), or is it a proof of concept? As discussed, we're just going to land the cros init parts.
lgtm
The CQ bit was checked by jamescook@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2155743003/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jamescook@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2155743003/#ps80001 (title: "nogncheck")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jamescook@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== mash: Initialize chromeos components and strings This is required to show the status area widget in mojo:ash. * Initialize various service singletons in WindowManagerApplication * Add ash strings to the window manager pak file BUG=619636 TEST=ash_unittests, mash_unittests ========== to ========== mash: Initialize chromeos components and strings This is required to show the status area widget in mojo:ash. * Initialize various service singletons in WindowManagerApplication * Add ash strings to the window manager pak file BUG=619636 TEST=ash_unittests, mash_unittests Committed: https://crrev.com/4d37f2228b46a8f0155e4130e33a7f3615e3192a Cr-Commit-Position: refs/heads/master@{#406126} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4d37f2228b46a8f0155e4130e33a7f3615e3192a Cr-Commit-Position: refs/heads/master@{#406126} |
