Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1882713004:
Only create DockedBackgroundWidget as needed. (Closed)
DescriptionOnly create DockedBackgroundWidget as needed.
This util widget shows up as a user window shelf icon in mash.
Don't create this widget until it's needed (not used in mash).
We currently put kShellWindowId_DockedContainer windows in mash::wm::mojom::Container::USER_WINDOWS, which might be okay in the long run, but even if we put them in a separate container, we would need UserWindowControllerImpl to track that container's set of user windows (to show shelf icons); so we would need to single out this widget as a non-user-window somehow.
We currently don't support docking in mash, but if we choose to use ash's DockedWindowLayoutManager (and DockedBackgroundWidget), we'll need to avoid tracking this background widget as a user window.
BUG=603419
TEST="mojo_runner mojo:mash_session" doesn't show a mysterious shelf icon at startup.
R=sky@chromium.org
Committed: https://crrev.com/14816ec6589c41b5a34f7b534b1c1565c97fe2d7
Cr-Commit-Position: refs/heads/master@{#387182}
Committed: https://crrev.com/21a784910f87c59e1db7d8034088d34268753d9d
Cr-Commit-Position: refs/heads/master@{#387486}
Patch Set 1 #Patch Set 2 : Cleanup. #
Total comments: 2
Patch Set 3 : Make DockedBackgroundWidget the ShelfLayoutManagerObserver; init background type. #Patch Set 4 : Show and Hide after create, rather than re-create. #
Total comments: 6
Patch Set 5 : Address comment to set bounds before showing. #Patch Set 6 : Add comment about stacking. #Patch Set 7 : Use the DockedWindowLayoutManager's shelf pointer. #
Total comments: 5
Patch Set 8 : Revise comment. #Patch Set 9 : Use a more verbose comment. #
Messages
Total messages: 43 (19 generated)
Description was changed from ========== Only create DockedBackgroundWidget as needed. BUG=NONE ========== to ========== Only create DockedBackgroundWidget as needed. This util widget shows up as a user window shelf icon in mash. Don't create this widget until it's needed (not used in mash). We currently put kShellWindowId_DockedContainer windows in mash::wm::mojom::Container::USER_WINDOWS, which might be okay in the long run, but even if we put them in a separate container, we would need UserWindowControllerImpl to track that container's set of user windows (to show shelf icons); so we would need to single out this widget as a non-user-window somehow. We currently don't support docking in mash, but if we choose to use ash's DockedWindowLayoutManager (and DockedBackgroundWidget), we'll need to avoid tracking this background widget as a user window. BUG=NONE TEST="mojo_runner mojo:mash_session" doesn't show a mysterious shelf icon at startup. R=sky@chromium.org ==========
msw@chromium.org changed reviewers: + sky@chromium.org
Hey Scott, please take a look; thanks!
The CQ bit was checked by msw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882713004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882713004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1882713004/diff/20001/ash/wm/dock/docked_wind... File ash/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/1882713004/diff/20001/ash/wm/dock/docked_wind... ash/wm/dock/docked_window_layout_manager.cc:831: background_widget_->SetBackgroundType(background_type, change_type); If you lazily create background_widget_ after this is called won't the background type be out of sync? In other words does this need to cache the background_type/change_type and supply it to background_widget_ on creation?
Please take another look; thanks! https://codereview.chromium.org/1882713004/diff/20001/ash/wm/dock/docked_wind... File ash/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/1882713004/diff/20001/ash/wm/dock/docked_wind... ash/wm/dock/docked_window_layout_manager.cc:831: background_widget_->SetBackgroundType(background_type, change_type); On 2016/04/13 21:45:42, sky wrote: > If you lazily create background_widget_ after this is called won't the > background type be out of sync? In other words does this need to cache the > background_type/change_type and supply it to background_widget_ on creation? Good point; I made DockedBackgroundWidget init the background type in it's ctor; it's also now the ShelfLayoutManagerObserver. On ToT, afaict, the initial background is always shown immediately, it's not animated, so change_type is okay to init as BACKGROUND_CHANGE_IMMEDIATE.
The CQ bit was checked by msw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882713004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882713004/40001
The CQ bit was checked by msw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882713004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882713004/60001
https://codereview.chromium.org/1882713004/diff/60001/ash/wm/dock/docked_wind... File ash/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/1882713004/diff/60001/ash/wm/dock/docked_wind... ash/wm/dock/docked_window_layout_manager.cc:166: parent->StackChildAtBottom(GetNativeWindow()); Why do you need this change? https://codereview.chromium.org/1882713004/diff/60001/ash/wm/dock/docked_wind... ash/wm/dock/docked_window_layout_manager.cc:1298: background_widget_->SetBackgroundBounds(background_bounds, alignment_); Are you sure you don't want to do this before the Show()?
https://codereview.chromium.org/1882713004/diff/60001/ash/wm/dock/docked_wind... File ash/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/1882713004/diff/60001/ash/wm/dock/docked_wind... ash/wm/dock/docked_window_layout_manager.cc:166: parent->StackChildAtBottom(GetNativeWindow()); On 2016/04/13 23:47:00, sky wrote: > Why do you need this change? Previously, the background was created and added to the dock container before any user windows, which gave it a defacto bottom ordering. Now that I'm delaying creation, it's added after the first window, and paints over it without this call. I think the explicit call makes sense. https://codereview.chromium.org/1882713004/diff/60001/ash/wm/dock/docked_wind... ash/wm/dock/docked_window_layout_manager.cc:1298: background_widget_->SetBackgroundBounds(background_bounds, alignment_); On 2016/04/13 23:46:59, sky wrote: > Are you sure you don't want to do this before the Show()? Done.
LGTM https://codereview.chromium.org/1882713004/diff/60001/ash/wm/dock/docked_wind... File ash/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/1882713004/diff/60001/ash/wm/dock/docked_wind... ash/wm/dock/docked_window_layout_manager.cc:166: parent->StackChildAtBottom(GetNativeWindow()); On 2016/04/13 23:58:17, msw wrote: > On 2016/04/13 23:47:00, sky wrote: > > Why do you need this change? > > Previously, the background was created and added to the dock container before > any user windows, which gave it a defacto bottom ordering. Now that I'm delaying > creation, it's added after the first window, and paints over it without this > call. I think the explicit call makes sense. Got it. Please add a comment as it's subtle.
https://codereview.chromium.org/1882713004/diff/60001/ash/wm/dock/docked_wind... File ash/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/1882713004/diff/60001/ash/wm/dock/docked_wind... ash/wm/dock/docked_window_layout_manager.cc:166: parent->StackChildAtBottom(GetNativeWindow()); On 2016/04/14 00:01:13, sky wrote: > On 2016/04/13 23:58:17, msw wrote: > > On 2016/04/13 23:47:00, sky wrote: > > > Why do you need this change? > > > > Previously, the background was created and added to the dock container before > > any user windows, which gave it a defacto bottom ordering. Now that I'm > delaying > > creation, it's added after the first window, and paints over it without this > > call. I think the explicit call makes sense. > > Got it. Please add a comment as it's subtle. Done.
The CQ bit was checked by msw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1882713004/#ps100001 (title: "Add comment about stacking.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882713004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882713004/100001
Message was sent while issue was closed.
Description was changed from ========== Only create DockedBackgroundWidget as needed. This util widget shows up as a user window shelf icon in mash. Don't create this widget until it's needed (not used in mash). We currently put kShellWindowId_DockedContainer windows in mash::wm::mojom::Container::USER_WINDOWS, which might be okay in the long run, but even if we put them in a separate container, we would need UserWindowControllerImpl to track that container's set of user windows (to show shelf icons); so we would need to single out this widget as a non-user-window somehow. We currently don't support docking in mash, but if we choose to use ash's DockedWindowLayoutManager (and DockedBackgroundWidget), we'll need to avoid tracking this background widget as a user window. BUG=NONE TEST="mojo_runner mojo:mash_session" doesn't show a mysterious shelf icon at startup. R=sky@chromium.org ========== to ========== Only create DockedBackgroundWidget as needed. This util widget shows up as a user window shelf icon in mash. Don't create this widget until it's needed (not used in mash). We currently put kShellWindowId_DockedContainer windows in mash::wm::mojom::Container::USER_WINDOWS, which might be okay in the long run, but even if we put them in a separate container, we would need UserWindowControllerImpl to track that container's set of user windows (to show shelf icons); so we would need to single out this widget as a non-user-window somehow. We currently don't support docking in mash, but if we choose to use ash's DockedWindowLayoutManager (and DockedBackgroundWidget), we'll need to avoid tracking this background widget as a user window. BUG=NONE TEST="mojo_runner mojo:mash_session" doesn't show a mysterious shelf icon at startup. R=sky@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Only create DockedBackgroundWidget as needed. This util widget shows up as a user window shelf icon in mash. Don't create this widget until it's needed (not used in mash). We currently put kShellWindowId_DockedContainer windows in mash::wm::mojom::Container::USER_WINDOWS, which might be okay in the long run, but even if we put them in a separate container, we would need UserWindowControllerImpl to track that container's set of user windows (to show shelf icons); so we would need to single out this widget as a non-user-window somehow. We currently don't support docking in mash, but if we choose to use ash's DockedWindowLayoutManager (and DockedBackgroundWidget), we'll need to avoid tracking this background widget as a user window. BUG=NONE TEST="mojo_runner mojo:mash_session" doesn't show a mysterious shelf icon at startup. R=sky@chromium.org ========== to ========== Only create DockedBackgroundWidget as needed. This util widget shows up as a user window shelf icon in mash. Don't create this widget until it's needed (not used in mash). We currently put kShellWindowId_DockedContainer windows in mash::wm::mojom::Container::USER_WINDOWS, which might be okay in the long run, but even if we put them in a separate container, we would need UserWindowControllerImpl to track that container's set of user windows (to show shelf icons); so we would need to single out this widget as a non-user-window somehow. We currently don't support docking in mash, but if we choose to use ash's DockedWindowLayoutManager (and DockedBackgroundWidget), we'll need to avoid tracking this background widget as a user window. BUG=NONE TEST="mojo_runner mojo:mash_session" doesn't show a mysterious shelf icon at startup. R=sky@chromium.org Committed: https://crrev.com/14816ec6589c41b5a34f7b534b1c1565c97fe2d7 Cr-Commit-Position: refs/heads/master@{#387182} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/14816ec6589c41b5a34f7b534b1c1565c97fe2d7 Cr-Commit-Position: refs/heads/master@{#387182}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1888733002/ by vabr@chromium.org. The reason for reverting is: Broke ash_unittests. More info in http://crbug.com/603419. BUG=603419.
Message was sent while issue was closed.
Description was changed from ========== Only create DockedBackgroundWidget as needed. This util widget shows up as a user window shelf icon in mash. Don't create this widget until it's needed (not used in mash). We currently put kShellWindowId_DockedContainer windows in mash::wm::mojom::Container::USER_WINDOWS, which might be okay in the long run, but even if we put them in a separate container, we would need UserWindowControllerImpl to track that container's set of user windows (to show shelf icons); so we would need to single out this widget as a non-user-window somehow. We currently don't support docking in mash, but if we choose to use ash's DockedWindowLayoutManager (and DockedBackgroundWidget), we'll need to avoid tracking this background widget as a user window. BUG=NONE TEST="mojo_runner mojo:mash_session" doesn't show a mysterious shelf icon at startup. R=sky@chromium.org Committed: https://crrev.com/14816ec6589c41b5a34f7b534b1c1565c97fe2d7 Cr-Commit-Position: refs/heads/master@{#387182} ========== to ========== Only create DockedBackgroundWidget as needed. This util widget shows up as a user window shelf icon in mash. Don't create this widget until it's needed (not used in mash). We currently put kShellWindowId_DockedContainer windows in mash::wm::mojom::Container::USER_WINDOWS, which might be okay in the long run, but even if we put them in a separate container, we would need UserWindowControllerImpl to track that container's set of user windows (to show shelf icons); so we would need to single out this widget as a non-user-window somehow. We currently don't support docking in mash, but if we choose to use ash's DockedWindowLayoutManager (and DockedBackgroundWidget), we'll need to avoid tracking this background widget as a user window. BUG=NONE TEST="mojo_runner mojo:mash_session" doesn't show a mysterious shelf icon at startup. R=sky@chromium.org Committed: https://crrev.com/14816ec6589c41b5a34f7b534b1c1565c97fe2d7 Cr-Commit-Position: refs/heads/master@{#387182} ==========
Patchset #7 (id:120001) has been deleted
Scott, please take a look at patch set 7 to address the test crashes that caused the revert, thanks.
The CQ bit was checked by msw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882713004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882713004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1882713004/diff/140001/ash/wm/dock/docked_win... File ash/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/1882713004/diff/140001/ash/wm/dock/docked_win... ash/wm/dock/docked_window_layout_manager.cc:78: manager_->shelf()->shelf_layout_manager()->RemoveObserver(this); Are you sure you don't need the conditional tests here that were on 429 old? https://codereview.chromium.org/1882713004/diff/140001/ash/wm/dock/docked_win... ash/wm/dock/docked_window_layout_manager.cc:163: // Stack the background below any windows already in the dock container. You should document why this is necessary, not what the code does.
https://codereview.chromium.org/1882713004/diff/140001/ash/wm/dock/docked_win... File ash/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/1882713004/diff/140001/ash/wm/dock/docked_win... ash/wm/dock/docked_window_layout_manager.cc:78: manager_->shelf()->shelf_layout_manager()->RemoveObserver(this); On 2016/04/14 20:55:07, sky wrote: > Are you sure you don't need the conditional tests here that were on 429 old? This should be alright; line 443 (new) destroys this object with background_widget_.reset(), directly before setting |shelf_| to null (no other place does that). Also, RootWindowController::CloseChildWindows() calls that Shutdown function before shutting down the shelf and its layout manager: https://code.google.com/p/chromium/codesearch#chromium/src/ash/root_window_co... https://codereview.chromium.org/1882713004/diff/140001/ash/wm/dock/docked_win... ash/wm/dock/docked_window_layout_manager.cc:163: // Stack the background below any windows already in the dock container. On 2016/04/14 20:55:07, sky wrote: > You should document why this is necessary, not what the code does. Done; my new comment isn't much different, but I can add something more verbose if you prefer, maybe something like "This background should be explicitly stacked below any windows already in the dock, otherwise the z-order is set by the order in which windows were added to the container, and UpdateStacking only manages user windows, not the background widget."
Description was changed from ========== Only create DockedBackgroundWidget as needed. This util widget shows up as a user window shelf icon in mash. Don't create this widget until it's needed (not used in mash). We currently put kShellWindowId_DockedContainer windows in mash::wm::mojom::Container::USER_WINDOWS, which might be okay in the long run, but even if we put them in a separate container, we would need UserWindowControllerImpl to track that container's set of user windows (to show shelf icons); so we would need to single out this widget as a non-user-window somehow. We currently don't support docking in mash, but if we choose to use ash's DockedWindowLayoutManager (and DockedBackgroundWidget), we'll need to avoid tracking this background widget as a user window. BUG=NONE TEST="mojo_runner mojo:mash_session" doesn't show a mysterious shelf icon at startup. R=sky@chromium.org Committed: https://crrev.com/14816ec6589c41b5a34f7b534b1c1565c97fe2d7 Cr-Commit-Position: refs/heads/master@{#387182} ========== to ========== Only create DockedBackgroundWidget as needed. This util widget shows up as a user window shelf icon in mash. Don't create this widget until it's needed (not used in mash). We currently put kShellWindowId_DockedContainer windows in mash::wm::mojom::Container::USER_WINDOWS, which might be okay in the long run, but even if we put them in a separate container, we would need UserWindowControllerImpl to track that container's set of user windows (to show shelf icons); so we would need to single out this widget as a non-user-window somehow. We currently don't support docking in mash, but if we choose to use ash's DockedWindowLayoutManager (and DockedBackgroundWidget), we'll need to avoid tracking this background widget as a user window. BUG=603419 TEST="mojo_runner mojo:mash_session" doesn't show a mysterious shelf icon at startup. R=sky@chromium.org Committed: https://crrev.com/14816ec6589c41b5a34f7b534b1c1565c97fe2d7 Cr-Commit-Position: refs/heads/master@{#387182} ==========
LGTM https://codereview.chromium.org/1882713004/diff/140001/ash/wm/dock/docked_win... File ash/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/1882713004/diff/140001/ash/wm/dock/docked_win... ash/wm/dock/docked_window_layout_manager.cc:163: // Stack the background below any windows already in the dock container. On 2016/04/14 21:33:48, msw wrote: > On 2016/04/14 20:55:07, sky wrote: > > You should document why this is necessary, not what the code does. > > Done; my new comment isn't much different, but I can add something more verbose > if you prefer, maybe something like "This background should be explicitly > stacked below any windows already in the dock, otherwise the z-order is set by > the order in which windows were added to the container, and UpdateStacking only > manages user windows, not the background widget." New comment SGTM
The CQ bit was checked by msw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1882713004/#ps180001 (title: "Use a more verbose comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882713004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882713004/180001
Message was sent while issue was closed.
Description was changed from ========== Only create DockedBackgroundWidget as needed. This util widget shows up as a user window shelf icon in mash. Don't create this widget until it's needed (not used in mash). We currently put kShellWindowId_DockedContainer windows in mash::wm::mojom::Container::USER_WINDOWS, which might be okay in the long run, but even if we put them in a separate container, we would need UserWindowControllerImpl to track that container's set of user windows (to show shelf icons); so we would need to single out this widget as a non-user-window somehow. We currently don't support docking in mash, but if we choose to use ash's DockedWindowLayoutManager (and DockedBackgroundWidget), we'll need to avoid tracking this background widget as a user window. BUG=603419 TEST="mojo_runner mojo:mash_session" doesn't show a mysterious shelf icon at startup. R=sky@chromium.org Committed: https://crrev.com/14816ec6589c41b5a34f7b534b1c1565c97fe2d7 Cr-Commit-Position: refs/heads/master@{#387182} ========== to ========== Only create DockedBackgroundWidget as needed. This util widget shows up as a user window shelf icon in mash. Don't create this widget until it's needed (not used in mash). We currently put kShellWindowId_DockedContainer windows in mash::wm::mojom::Container::USER_WINDOWS, which might be okay in the long run, but even if we put them in a separate container, we would need UserWindowControllerImpl to track that container's set of user windows (to show shelf icons); so we would need to single out this widget as a non-user-window somehow. We currently don't support docking in mash, but if we choose to use ash's DockedWindowLayoutManager (and DockedBackgroundWidget), we'll need to avoid tracking this background widget as a user window. BUG=603419 TEST="mojo_runner mojo:mash_session" doesn't show a mysterious shelf icon at startup. R=sky@chromium.org Committed: https://crrev.com/14816ec6589c41b5a34f7b534b1c1565c97fe2d7 Cr-Commit-Position: refs/heads/master@{#387182} ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Only create DockedBackgroundWidget as needed. This util widget shows up as a user window shelf icon in mash. Don't create this widget until it's needed (not used in mash). We currently put kShellWindowId_DockedContainer windows in mash::wm::mojom::Container::USER_WINDOWS, which might be okay in the long run, but even if we put them in a separate container, we would need UserWindowControllerImpl to track that container's set of user windows (to show shelf icons); so we would need to single out this widget as a non-user-window somehow. We currently don't support docking in mash, but if we choose to use ash's DockedWindowLayoutManager (and DockedBackgroundWidget), we'll need to avoid tracking this background widget as a user window. BUG=603419 TEST="mojo_runner mojo:mash_session" doesn't show a mysterious shelf icon at startup. R=sky@chromium.org Committed: https://crrev.com/14816ec6589c41b5a34f7b534b1c1565c97fe2d7 Cr-Commit-Position: refs/heads/master@{#387182} ========== to ========== Only create DockedBackgroundWidget as needed. This util widget shows up as a user window shelf icon in mash. Don't create this widget until it's needed (not used in mash). We currently put kShellWindowId_DockedContainer windows in mash::wm::mojom::Container::USER_WINDOWS, which might be okay in the long run, but even if we put them in a separate container, we would need UserWindowControllerImpl to track that container's set of user windows (to show shelf icons); so we would need to single out this widget as a non-user-window somehow. We currently don't support docking in mash, but if we choose to use ash's DockedWindowLayoutManager (and DockedBackgroundWidget), we'll need to avoid tracking this background widget as a user window. BUG=603419 TEST="mojo_runner mojo:mash_session" doesn't show a mysterious shelf icon at startup. R=sky@chromium.org Committed: https://crrev.com/14816ec6589c41b5a34f7b534b1c1565c97fe2d7 Cr-Commit-Position: refs/heads/master@{#387182} Committed: https://crrev.com/21a784910f87c59e1db7d8034088d34268753d9d Cr-Commit-Position: refs/heads/master@{#387486} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/21a784910f87c59e1db7d8034088d34268753d9d Cr-Commit-Position: refs/heads/master@{#387486} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
