Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2108933003:
Reorder browser list on workspace switch (Closed)
|
Created:
4 years, 5 months ago by Tom (Use chromium acct) Modified:
4 years, 5 months ago CC:
chromium-reviews, tfarina, yusukes+watch_chromium.org, derat+watch_chromium.org, danakj, girard Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinux: Reorder browser list on workspace switch
Opening a link from a 3rd party app picks the most recently
used profile/browser to open the new tab in. Whenever a user
changes workspaces, reorder the browser list such that the
browsers in the current workspace are moved to the top of the
list. This way, when a user opens a link, it will open in a
browser on the current workspace (if there are any browsers).
BUG=619673
Committed: https://crrev.com/edabf4d52846972d359457d563eb5e782bbf8a1d
Cr-Commit-Position: refs/heads/master@{#404544}
Patch Set 1 #Patch Set 2 : Revert unnecessary changes #Patch Set 3 : Moved code from desktop_screen_x11 into x11_desktop_handler #Patch Set 4 : Set a browser as last active on workspace change #
Total comments: 14
Patch Set 5 : Address comments from erg and thestig #
Total comments: 8
Patch Set 6 : Use LazyInstance. Add small API descriptions. #
Total comments: 10
Patch Set 7 : remove display::desktop #Patch Set 8 : Add Desktop class and remove global OnWorkspaceChanged #
Total comments: 1
Patch Set 9 : Rebase & add desktop.* :P #
Total comments: 1
Patch Set 10 : Moved code into ChromeBrowserMainExtraPartsX11 #
Total comments: 11
Patch Set 11 : Reverted ui/display #
Total comments: 13
Patch Set 12 : Cache workspace, address comments #
Total comments: 1
Patch Set 13 : s/PartitionByWorkspace/MoveBrowsersInWorkspaceToFront #Patch Set 14 : removed endl #Messages
Total messages: 65 (25 generated)
Description was changed from ========== formatted BUILD.gn Fixed the bug. Derp. compiles and links but does not work fix include order format links Compiles, but does not link BUG= ========== to ========== Opening a link from a 3rd party app picks the most recently used profile/browser to open the new tab in. Whenever a user changes workspaces, reorder the browser list such that the browsers in the current workspace are moved to the top of the list. This way, when a user opens a link, it will open in a browser on the current workspace (if there are any browsers). BUG=619673 ==========
thomasanderson@google.com changed reviewers: + erg@chromium.org, sky@chromium.org, thestig@chromium.org
Description was changed from ========== Opening a link from a 3rd party app picks the most recently used profile/browser to open the new tab in. Whenever a user changes workspaces, reorder the browser list such that the browsers in the current workspace are moved to the top of the list. This way, when a user opens a link, it will open in a browser on the current workspace (if there are any browsers). BUG=619673 ========== to ========== Opening a link from a 3rd party app picks the most recently used profile/browser to open the new tab in. Whenever a user changes workspaces, reorder the browser list such that the browsers in the current workspace are moved to the top of the list. This way, when a user opens a link, it will open in a browser on the current workspace (if there are any browsers). BUG=619673 ==========
The CQ bit was checked by thomasanderson@google.com 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...
https://codereview.chromium.org/2108933003/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/2108933003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.cc:264: BrowserList::~BrowserList() { Do you need a matched RemoveObserver? https://codereview.chromium.org/2108933003/diff/60001/ui/display/desktop_obse... File ui/display/desktop_observer.h (right): https://codereview.chromium.org/2108933003/diff/60001/ui/display/desktop_obse... ui/display/desktop_observer.h:25: g_desktop_observer_list; I'm pretty sure you don't want to have a raw ObserverList like this; I can only find one other instance in the chrome code base of doing this (ui/accessibility/platform/ax_platform_node_win.h).
Description was changed from ========== Opening a link from a 3rd party app picks the most recently used profile/browser to open the new tab in. Whenever a user changes workspaces, reorder the browser list such that the browsers in the current workspace are moved to the top of the list. This way, when a user opens a link, it will open in a browser on the current workspace (if there are any browsers). BUG=619673 ========== to ========== Reorder browser list on workspace switch Opening a link from a 3rd party app picks the most recently used profile/browser to open the new tab in. Whenever a user changes workspaces, reorder the browser list such that the browsers in the current workspace are moved to the top of the list. This way, when a user opens a link, it will open in a browser on the current workspace (if there are any browsers). BUG=619673 ==========
Description was changed from ========== Reorder browser list on workspace switch Opening a link from a 3rd party app picks the most recently used profile/browser to open the new tab in. Whenever a user changes workspaces, reorder the browser list such that the browsers in the current workspace are moved to the top of the list. This way, when a user opens a link, it will open in a browser on the current workspace (if there are any browsers). BUG=619673 ========== to ========== Linux: Reorder browser list on workspace switch Opening a link from a 3rd party app picks the most recently used profile/browser to open the new tab in. Whenever a user changes workspaces, reorder the browser list such that the browsers in the current workspace are moved to the top of the list. This way, when a user opens a link, it will open in a browser on the current workspace (if there are any browsers). BUG=619673 ==========
https://codereview.chromium.org/2108933003/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/2108933003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.cc:222: void BrowserList::OnWorkspaceChanged(const std::string& new_workspace) { Isn't BrowserList cross-platform? How does this affect other OSes? Does this not get called? https://codereview.chromium.org/2108933003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.cc:223: DCHECK_NE(new_workspace, ""); DCHECK(!new_workspace.empty()) ? https://codereview.chromium.org/2108933003/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2108933003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.h:98: // display::DesktopObserver: There's a bunch of non-static methods above. https://codereview.chromium.org/2108933003/diff/60001/ui/display/desktop_obse... File ui/display/desktop_observer.h (right): https://codereview.chromium.org/2108933003/diff/60001/ui/display/desktop_obse... ui/display/desktop_observer.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/2108933003/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/2108933003/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_handler.cc:24: const char* kAtomsToCache[] = { BTW, please make this: const char* const
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/2108933003/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/2108933003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.cc:222: void BrowserList::OnWorkspaceChanged(const std::string& new_workspace) { On 2016/06/29 18:30:53, Lei Zhang wrote: > Isn't BrowserList cross-platform? How does this affect other OSes? Does this not > get called? It won't get called, until we eventually add support for Windows or OS X workspaces. https://codereview.chromium.org/2108933003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.cc:223: DCHECK_NE(new_workspace, ""); On 2016/06/29 18:30:53, Lei Zhang wrote: > DCHECK(!new_workspace.empty()) ? Done. https://codereview.chromium.org/2108933003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.cc:264: BrowserList::~BrowserList() { On 2016/06/29 18:21:09, Elliot Glaysher wrote: > Do you need a matched RemoveObserver? Done. https://codereview.chromium.org/2108933003/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2108933003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.h:98: // display::DesktopObserver: On 2016/06/29 18:30:53, Lei Zhang wrote: > There's a bunch of non-static methods above. Done. (moved to private section) https://codereview.chromium.org/2108933003/diff/60001/ui/display/desktop_obse... File ui/display/desktop_observer.h (right): https://codereview.chromium.org/2108933003/diff/60001/ui/display/desktop_obse... ui/display/desktop_observer.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/06/29 18:30:53, Lei Zhang wrote: > no (c) Done. https://codereview.chromium.org/2108933003/diff/60001/ui/display/desktop_obse... ui/display/desktop_observer.h:25: g_desktop_observer_list; On 2016/06/29 18:21:09, Elliot Glaysher wrote: > I'm pretty sure you don't want to have a raw ObserverList like this; I can only > find one other instance in the chrome code base of doing this > (ui/accessibility/platform/ax_platform_node_win.h). Done. https://codereview.chromium.org/2108933003/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/2108933003/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_handler.cc:24: const char* kAtomsToCache[] = { On 2016/06/29 18:30:53, Lei Zhang wrote: > BTW, please make this: const char* const Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2108933003/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2108933003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.h:29: class BrowserList : private display::desktop::DesktopObserver { This stays public, but you can leave the overrides in the private section below. https://google.github.io/styleguide/cppguide.html#Inheritance https://codereview.chromium.org/2108933003/diff/80001/ui/display/desktop_obse... File ui/display/desktop_observer.cc (right): https://codereview.chromium.org/2108933003/diff/80001/ui/display/desktop_obse... ui/display/desktop_observer.cc:13: base::ObserverList<DesktopObserver> g_desktop_observer_list; This probably should be a LazyInstance or it will add a static initializer. https://codereview.chromium.org/2108933003/diff/80001/ui/display/desktop_obse... File ui/display/desktop_observer.h (right): https://codereview.chromium.org/2108933003/diff/80001/ui/display/desktop_obse... ui/display/desktop_observer.h:10: #include "base/observer_list.h" Move to .cc https://codereview.chromium.org/2108933003/diff/80001/ui/display/desktop_obse... ui/display/desktop_observer.h:20: virtual void OnWorkspaceChanged(const std::string& new_workspace) = 0; Please describe what the method(s) do.
https://codereview.chromium.org/2108933003/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2108933003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.h:29: class BrowserList : private display::desktop::DesktopObserver { On 2016/06/29 20:51:01, Lei Zhang wrote: > This stays public, but you can leave the overrides in the private section below. > https://google.github.io/styleguide/cppguide.html#Inheritance Done. https://codereview.chromium.org/2108933003/diff/80001/ui/display/desktop_obse... File ui/display/desktop_observer.cc (right): https://codereview.chromium.org/2108933003/diff/80001/ui/display/desktop_obse... ui/display/desktop_observer.cc:13: base::ObserverList<DesktopObserver> g_desktop_observer_list; On 2016/06/29 20:51:01, Lei Zhang wrote: > This probably should be a LazyInstance or it will add a static initializer. Done. https://codereview.chromium.org/2108933003/diff/80001/ui/display/desktop_obse... File ui/display/desktop_observer.h (right): https://codereview.chromium.org/2108933003/diff/80001/ui/display/desktop_obse... ui/display/desktop_observer.h:10: #include "base/observer_list.h" On 2016/06/29 20:51:01, Lei Zhang wrote: > Move to .cc Done. https://codereview.chromium.org/2108933003/diff/80001/ui/display/desktop_obse... ui/display/desktop_observer.h:20: virtual void OnWorkspaceChanged(const std::string& new_workspace) = 0; On 2016/06/29 20:51:01, Lei Zhang wrote: > Please describe what the method(s) do. Done.
lgtm
The CQ bit was checked by thomasanderson@google.com 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...
https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... File ui/display/desktop_observer.h (right): https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... ui/display/desktop_observer.h:14: namespace desktop { Don't use desktop here. See threads on namespace usage in chromium-dev for details. https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... ui/display/desktop_observer.h:17: class DISPLAY_EXPORT DesktopObserver { Is there a reason you didn't add this to DisplayObserver? X11DesktopHandler can talk to DesktopScreenX11 to notify appropriately.
https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... File ui/display/desktop_observer.h (right): https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... ui/display/desktop_observer.h:14: namespace desktop { On 2016/06/29 22:55:58, sky wrote: > Don't use desktop here. See threads on namespace usage in chromium-dev for > details. Can you point at a thread? The google style guide says to group things with a namespace instead of a class, which I learnt recently.
https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... File ui/display/desktop_observer.h (right): https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... ui/display/desktop_observer.h:14: namespace desktop { On 2016/06/29 22:55:58, sky wrote: > Don't use desktop here. See threads on namespace usage in chromium-dev for > details. Do you mean to create a display::Desktop class to encapsulate these functions or just get rid of display::desktop and have them in display? https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... ui/display/desktop_observer.h:14: namespace desktop { On 2016/06/29 22:56:51, danakj wrote: > On 2016/06/29 22:55:58, sky wrote: > > Don't use desktop here. See threads on namespace usage in chromium-dev for > > details. > > Can you point at a thread? The google style guide says to group things with a > namespace instead of a class, which I learnt recently. I'll hold off on making this change until sky/danakj agree on what's best. https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... ui/display/desktop_observer.h:17: class DISPLAY_EXPORT DesktopObserver { On 2016/06/29 22:55:58, sky wrote: > Is there a reason you didn't add this to DisplayObserver? X11DesktopHandler can > talk to DesktopScreenX11 to notify appropriately. I did that originally, but the desktop::Screen instance is initialized later during startup than BrowserList, so I was getting segfaults. I suppose I could have found where the instance is set, and just added the observer after that, but I wanted to register the observer in the constructor of BrowserList. Also (and this may just be nit-picking), I feel like the workspace isn't really a display or a screen property, but is more of a desktop property.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On Wed, Jun 29, 2016 at 3:56 PM, <danakj@chromium.org> wrote: > > https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... > File ui/display/desktop_observer.h (right): > > https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... > ui/display/desktop_observer.h:14: namespace desktop { > On 2016/06/29 22:55:58, sky wrote: >> Don't use desktop here. See threads on namespace usage in chromium-dev > for >> details. > > Can you point at a thread? The google style guide says to group things > with a namespace instead of a class, which I learnt recently. https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/namesp... -- 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.
https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... File ui/display/desktop_observer.h (right): https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... ui/display/desktop_observer.h:14: namespace desktop { On 2016/06/29 23:24:20, Tom Anderson wrote: > On 2016/06/29 22:55:58, sky wrote: > > Don't use desktop here. See threads on namespace usage in chromium-dev for > > details. > > Do you mean to create a display::Desktop class to encapsulate these functions or > just get rid of display::desktop and have them in display? Sorry for not being clear. I meant the later, nuke the desktop namespace. https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... ui/display/desktop_observer.h:17: class DISPLAY_EXPORT DesktopObserver { On 2016/06/29 23:24:20, Tom Anderson wrote: > On 2016/06/29 22:55:58, sky wrote: > > Is there a reason you didn't add this to DisplayObserver? X11DesktopHandler > can > > talk to DesktopScreenX11 to notify appropriately. > > I did that originally, but the desktop::Screen instance is initialized later > during startup than BrowserList, so I was getting segfaults. Can you move initialization earlier? > I suppose I could > have found where the instance is set, and just added the observer after that, > but I wanted to register the observer in the constructor of BrowserList. > > Also (and this may just be nit-picking), I feel like the workspace isn't really > a display or a screen property, but is more of a desktop property. I suggested moving workspace onto display/displayobserver as it seems to me the workspace is logically related to the screen..
https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... File ui/display/desktop_observer.h (right): https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... ui/display/desktop_observer.h:14: namespace desktop { On 2016/06/30 00:25:55, sky wrote: > On 2016/06/29 23:24:20, Tom Anderson wrote: > > On 2016/06/29 22:55:58, sky wrote: > > > Don't use desktop here. See threads on namespace usage in chromium-dev for > > > details. > > > > Do you mean to create a display::Desktop class to encapsulate these functions > or > > just get rid of display::desktop and have them in display? > > Sorry for not being clear. I meant the later, nuke the desktop namespace. Done. https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... ui/display/desktop_observer.h:17: class DISPLAY_EXPORT DesktopObserver { On 2016/06/30 00:25:55, sky wrote: > On 2016/06/29 23:24:20, Tom Anderson wrote: > > On 2016/06/29 22:55:58, sky wrote: > > > Is there a reason you didn't add this to DisplayObserver? X11DesktopHandler > > can > > > talk to DesktopScreenX11 to notify appropriately. > > > > I did that originally, but the desktop::Screen instance is initialized later > > during startup than BrowserList, so I was getting segfaults. > > Can you move initialization earlier? > > > I suppose I could > > have found where the instance is set, and just added the observer after that, > > but I wanted to register the observer in the constructor of BrowserList. > > > > Also (and this may just be nit-picking), I feel like the workspace isn't > really > > a display or a screen property, but is more of a desktop property. > > I suggested moving workspace onto display/displayobserver as it seems to me the > workspace is logically related to the screen.. If you really want, I can move it back into DisplayObserver like it was in patch set 1, and move the Screen initialization earlier
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I like DisplayObserver, but I'm also ok with introducing another class (maybe Desktop or Workspace) that you attach the observer too, can query for the active workspace and does not expose OnWorkspaceChanged(). But a public function that notifies the observers seems just wrong. -Scott On Thu, Jun 30, 2016 at 10:35 AM, <thomasanderson@google.com> wrote: > > https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... > File ui/display/desktop_observer.h (right): > > https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... > ui/display/desktop_observer.h:14: namespace desktop { > On 2016/06/30 00:25:55, sky wrote: >> On 2016/06/29 23:24:20, Tom Anderson wrote: >> > On 2016/06/29 22:55:58, sky wrote: >> > > Don't use desktop here. See threads on namespace usage in > chromium-dev for >> > > details. >> > >> > Do you mean to create a display::Desktop class to encapsulate these > functions >> or >> > just get rid of display::desktop and have them in display? >> >> Sorry for not being clear. I meant the later, nuke the desktop > namespace. > > Done. > > https://codereview.chromium.org/2108933003/diff/100001/ui/display/desktop_obs... > ui/display/desktop_observer.h:17: class DISPLAY_EXPORT DesktopObserver { > On 2016/06/30 00:25:55, sky wrote: >> On 2016/06/29 23:24:20, Tom Anderson wrote: >> > On 2016/06/29 22:55:58, sky wrote: >> > > Is there a reason you didn't add this to DisplayObserver? > X11DesktopHandler >> > can >> > > talk to DesktopScreenX11 to notify appropriately. >> > >> > I did that originally, but the desktop::Screen instance is > initialized later >> > during startup than BrowserList, so I was getting segfaults. >> >> Can you move initialization earlier? >> >> > I suppose I could >> > have found where the instance is set, and just added the observer > after that, >> > but I wanted to register the observer in the constructor of > BrowserList. >> > >> > Also (and this may just be nit-picking), I feel like the workspace > isn't >> really >> > a display or a screen property, but is more of a desktop property. >> >> I suggested moving workspace onto display/displayobserver as it seems > to me the >> workspace is logically related to the screen.. > > If you really want, I can move it back into DisplayObserver like it was > in patch set 1, and move the Screen initialization earlier > > https://codereview.chromium.org/2108933003/ -- 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.
Patchset #8 (id:140001) has been deleted
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/06/30 21:05:04, sky wrote: > I like DisplayObserver, but I'm also ok with introducing another class > (maybe Desktop or Workspace) that you attach the observer too, can > query for the active workspace and does not expose > OnWorkspaceChanged(). But a public function that notifies the > observers seems just wrong. > > -Scott > Done. Added display::Desktop and remove global OnWorkspaceChanged. display::Desktop doesn't yet get the current workspace. PLMK if you want this feature.
https://codereview.chromium.org/2108933003/diff/160001/ui/display/BUILD.gn File ui/display/BUILD.gn (right): https://codereview.chromium.org/2108933003/diff/160001/ui/display/BUILD.gn#ne... ui/display/BUILD.gn:36: "desktop.cc", I think you forgot the git add desktop.*
The CQ bit was checked by thomasanderson@google.com 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...
On 2016/07/01 19:43:39, sky wrote: > https://codereview.chromium.org/2108933003/diff/160001/ui/display/BUILD.gn > File ui/display/BUILD.gn (right): > > https://codereview.chromium.org/2108933003/diff/160001/ui/display/BUILD.gn#ne... > ui/display/BUILD.gn:36: "desktop.cc", > I think you forgot the git add desktop.* oops. Added :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Another option that I think is less code, and more focused to what you have here is to have x startup code (say ChromeBrowserMainPartsLinux, or somewhere similar) call X11DesktopHandler directly and add an observer to it. X11DesktopHandler already has a static getter. You then expose a public method in BrowserList to do the reordering. https://codereview.chromium.org/2108933003/diff/180001/ui/display/desktop.h File ui/display/desktop.h (right): https://codereview.chromium.org/2108933003/diff/180001/ui/display/desktop.h#n... ui/display/desktop.h:16: class DISPLAY_EXPORT Desktop { Sorry for not being clear. I was suggesting you have a Desktop::GetInstance() function and add/remove observer functions on said object (not static). Basically the same as in Screen. That way you have better object lifetime.
Moved code into ChromeBrowserMainExtraPartsX11 https://codereview.chromium.org/2108933003/diff/200001/ui/display/desktop.h File ui/display/desktop.h (right): https://codereview.chromium.org/2108933003/diff/200001/ui/display/desktop.h#n... ui/display/desktop.h:19: static Desktop* GetDesktop(); GetDesktop currently is not used anywhere, but ChromeBrowserMainExtraPartsX11 calls SetDesktopInstance Should we just get rid of these?
https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main_extra_parts_x11.h (right): https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_browser_main_extra_parts_x11.h:15: class ChromeBrowserMainExtraPartsX11 : public ChromeBrowserMainExtraParts, Sorry, I should have dug a bit more. We try to avoid chrome/browser depending upon specific ui implementations. Can you move this to ChromeBrowserMainExtraPartsViewsLinux? https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.cc:205: BrowserVector in_this_workspace; Isn't this the same as a sort with a comparator that looks at workspace? https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame.cc (right): https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame.cc:225: BrowserList::SetLastActive(browser); Might browser not be active at this point? https://codereview.chromium.org/2108933003/diff/200001/ui/display/desktop.h File ui/display/desktop.h (right): https://codereview.chromium.org/2108933003/diff/200001/ui/display/desktop.h#n... ui/display/desktop.h:19: static Desktop* GetDesktop(); On 2016/07/06 18:19:11, Tom Anderson wrote: > GetDesktop currently is not used anywhere, but ChromeBrowserMainExtraPartsX11 > calls SetDesktopInstance > > Should we just get rid of these? Yes. Here is what I recommend doing: . remove all changes to ui/display. . X11DesktopHandler gets an AddObserver function. The observer is X11DesktopHandlerObserver and defined in its own header in ui/views/widget/desktop_aura/x11_desktop_handler_observer.h. . Make ChromeBrowserMainExtraPartsViewsLinux implement X11DesktopHandlerObserver (or have it create a class that implement X11DesktopHandlerObserver , I'm fine either way).
Reverted ui/display This CL has gone through quite some evolution https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main_extra_parts_x11.h (right): https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_browser_main_extra_parts_x11.h:15: class ChromeBrowserMainExtraPartsX11 : public ChromeBrowserMainExtraParts, On 2016/07/06 20:33:05, sky wrote: > Sorry, I should have dug a bit more. We try to avoid chrome/browser depending > upon specific ui implementations. Can you move this to > ChromeBrowserMainExtraPartsViewsLinux? Done. https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.cc:205: BrowserVector in_this_workspace; On 2016/07/06 20:33:05, sky wrote: > Isn't this the same as a sort with a comparator that looks at workspace? Nice catch. stable_sort is much cleaner https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame.cc (right): https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame.cc:225: BrowserList::SetLastActive(browser); On 2016/07/06 20:33:05, sky wrote: > Might browser not be active at this point? It might not be. In cinnamon, you can "zoom-out" to see all workspaces, move a window from a different workspace into the current workspace (without activating it), and then zoom-in back to your workspace. We don't get a workspace change event in this case. Although you made me realize this code is wrong. We may move a browser to a workspace other than the current one. In this case, we don't want the browser to be SetLastActive. Even if we do move it to the current workspace, we want it to be below the active one. So I guess the correct thing to do is call BrowserList::ReorderAfterWorkspaceChange, but we have no way of giving it the current workspace now that display::Desktop is gone. https://codereview.chromium.org/2108933003/diff/200001/ui/display/desktop.h File ui/display/desktop.h (right): https://codereview.chromium.org/2108933003/diff/200001/ui/display/desktop.h#n... ui/display/desktop.h:19: static Desktop* GetDesktop(); On 2016/07/06 20:33:05, sky wrote: > On 2016/07/06 18:19:11, Tom Anderson wrote: > > GetDesktop currently is not used anywhere, but ChromeBrowserMainExtraPartsX11 > > calls SetDesktopInstance > > > > Should we just get rid of these? > > Yes. Here is what I recommend doing: > . remove all changes to ui/display. > . X11DesktopHandler gets an AddObserver function. The observer is > X11DesktopHandlerObserver and defined in its own header in > ui/views/widget/desktop_aura/x11_desktop_handler_observer.h. > . Make ChromeBrowserMainExtraPartsViewsLinux implement X11DesktopHandlerObserver > (or have it create a class that implement X11DesktopHandlerObserver , I'm fine > either way). Done. https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc (right): https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:73: ~ChromeBrowserMainExtraPartsViewsLinux() {} Do we need views::X11DesktopHandler::get()->AddObserver(this) here? X11DesktopHandler goes away when aura::Env does, but idk about ChromeBrowserMainExtraParts*
https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame.cc (right): https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame.cc:225: BrowserList::SetLastActive(browser); On 2016/07/06 22:29:16, Tom Anderson wrote: > On 2016/07/06 20:33:05, sky wrote: > > Might browser not be active at this point? > > It might not be. In cinnamon, you can "zoom-out" to see all workspaces, move a > window from a different workspace into the current workspace (without activating > it), and then zoom-in back to your workspace. We don't get a workspace change > event in this case. > > Although you made me realize this code is wrong. We may move a browser to a > workspace other than the current one. In this case, we don't want the browser > to be SetLastActive. Even if we do move it to the current workspace, we want it > to be below the active one. > > So I guess the correct thing to do is call > BrowserList::ReorderAfterWorkspaceChange, but we have no way of giving it the > current workspace now that display::Desktop is gone. An ifdef for linux and calling to x11desktop to get the workspace is fine here. https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.cc:207: return b1->window()->GetWorkspace() != Don't you want < ? https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:72: // Called when the user switches to workspace X. Moves all the browsers that 'workspace X' -> 'workspace |new_workspace|' or 'new workspace' https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:73: // show on workspace X to the end of the browser list (i.e. the browsers that similar comment. https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc (right): https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:73: ~ChromeBrowserMainExtraPartsViewsLinux() {} On 2016/07/06 22:29:16, Tom Anderson wrote: > Do we need views::X11DesktopHandler::get()->AddObserver(this) here? > > X11DesktopHandler goes away when aura::Env does, but idk about > ChromeBrowserMainExtraParts* I think you mean RemoveObserver. Not sure. You'll have to verify the shutdown order. Please add a comment if you can't add the Remove, and a DCHECK that env is gone. https://codereview.chromium.org/2108933003/diff/220001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/x11_desktop_handler_observer.cc (right): https://codereview.chromium.org/2108933003/diff/220001/ui/views/widget/deskto... ui/views/widget/desktop_aura/x11_desktop_handler_observer.cc:9: X11DesktopHandlerObserver::~X11DesktopHandlerObserver() {} inline this. https://codereview.chromium.org/2108933003/diff/220001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/x11_desktop_handler_observer.h (right): https://codereview.chromium.org/2108933003/diff/220001/ui/views/widget/deskto... ui/views/widget/desktop_aura/x11_desktop_handler_observer.h:24: } // namespace display display->views
thomasanderson@google.com changed reviewers: + sadrul@chromium.org
+ Sadrul for export in ui/base/x/x11_util.h Also cache the workspace since it requires a round trip to the X server, but we request it many times during a partition https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame.cc (right): https://codereview.chromium.org/2108933003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame.cc:225: BrowserList::SetLastActive(browser); On 2016/07/07 03:23:27, sky wrote: > On 2016/07/06 22:29:16, Tom Anderson wrote: > > On 2016/07/06 20:33:05, sky wrote: > > > Might browser not be active at this point? > > > > It might not be. In cinnamon, you can "zoom-out" to see all workspaces, move > a > > window from a different workspace into the current workspace (without > activating > > it), and then zoom-in back to your workspace. We don't get a workspace change > > event in this case. > > > > Although you made me realize this code is wrong. We may move a browser to a > > workspace other than the current one. In this case, we don't want the browser > > to be SetLastActive. Even if we do move it to the current workspace, we want > it > > to be below the active one. > > > > So I guess the correct thing to do is call > > BrowserList::ReorderAfterWorkspaceChange, but we have no way of giving it the > > current workspace now that display::Desktop is gone. > > An ifdef for linux and calling to x11desktop to get the workspace is fine here. Done. https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.cc:207: return b1->window()->GetWorkspace() != On 2016/07/07 03:23:27, sky wrote: > Don't you want < ? actually wanted stable_partition https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:72: // Called when the user switches to workspace X. Moves all the browsers that On 2016/07/07 03:23:27, sky wrote: > 'workspace X' -> 'workspace |new_workspace|' or 'new workspace' Done. https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:73: // show on workspace X to the end of the browser list (i.e. the browsers that On 2016/07/07 03:23:27, sky wrote: > similar comment. Done. https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc (right): https://codereview.chromium.org/2108933003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:73: ~ChromeBrowserMainExtraPartsViewsLinux() {} On 2016/07/07 03:23:27, sky wrote: > On 2016/07/06 22:29:16, Tom Anderson wrote: > > Do we need views::X11DesktopHandler::get()->AddObserver(this) here? > > > > X11DesktopHandler goes away when aura::Env does, but idk about > > ChromeBrowserMainExtraParts* > > I think you mean RemoveObserver. Not sure. You'll have to verify the shutdown > order. Please add a comment if you can't add the Remove, and a DCHECK that env > is gone. Done. https://codereview.chromium.org/2108933003/diff/220001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/x11_desktop_handler_observer.cc (right): https://codereview.chromium.org/2108933003/diff/220001/ui/views/widget/deskto... ui/views/widget/desktop_aura/x11_desktop_handler_observer.cc:9: X11DesktopHandlerObserver::~X11DesktopHandlerObserver() {} On 2016/07/07 03:23:27, sky wrote: > inline this. Done. https://codereview.chromium.org/2108933003/diff/220001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/x11_desktop_handler_observer.h (right): https://codereview.chromium.org/2108933003/diff/220001/ui/views/widget/deskto... ui/views/widget/desktop_aura/x11_desktop_handler_observer.h:24: } // namespace display On 2016/07/07 03:23:27, sky wrote: > display->views Done.
LGTM https://codereview.chromium.org/2108933003/diff/240001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2108933003/diff/240001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:74: static void PartitionByWorkspace(const std::string& new_workspace); nit: IMO partition is obscure and while it matches the function you're using it doesn't really indicate what this does. How about MoveBrowsersInWorkspaceToFront? I realize front is a bit confusing here, but it matches begin_last_active(), which returns the end.
The CQ bit was checked by thomasanderson@google.com 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.
lgtm
lgtm
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, sky@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2108933003/#ps280001 (title: "removed endl")
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.
Description was changed from ========== Linux: Reorder browser list on workspace switch Opening a link from a 3rd party app picks the most recently used profile/browser to open the new tab in. Whenever a user changes workspaces, reorder the browser list such that the browsers in the current workspace are moved to the top of the list. This way, when a user opens a link, it will open in a browser on the current workspace (if there are any browsers). BUG=619673 ========== to ========== Linux: Reorder browser list on workspace switch Opening a link from a 3rd party app picks the most recently used profile/browser to open the new tab in. Whenever a user changes workspaces, reorder the browser list such that the browsers in the current workspace are moved to the top of the list. This way, when a user opens a link, it will open in a browser on the current workspace (if there are any browsers). BUG=619673 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:280001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Linux: Reorder browser list on workspace switch Opening a link from a 3rd party app picks the most recently used profile/browser to open the new tab in. Whenever a user changes workspaces, reorder the browser list such that the browsers in the current workspace are moved to the top of the list. This way, when a user opens a link, it will open in a browser on the current workspace (if there are any browsers). BUG=619673 ========== to ========== Linux: Reorder browser list on workspace switch Opening a link from a 3rd party app picks the most recently used profile/browser to open the new tab in. Whenever a user changes workspaces, reorder the browser list such that the browsers in the current workspace are moved to the top of the list. This way, when a user opens a link, it will open in a browser on the current workspace (if there are any browsers). BUG=619673 Committed: https://crrev.com/edabf4d52846972d359457d563eb5e782bbf8a1d Cr-Commit-Position: refs/heads/master@{#404544} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/edabf4d52846972d359457d563eb5e782bbf8a1d Cr-Commit-Position: refs/heads/master@{#404544} |
