Add support for X11 workspaces

Add support for X11 workspaces

The core changes introduced:
- Add new observers that will get notified when a window's workspace
changes.
- Save the browser window workspace whenever the workspace changes
- Restore the browser workspace for a restored browser session

BUG=18596
Committed: https://crrev.com/06405c5944436b431f26037fdc93340842c51de5
Cr-Commit-Position: refs/heads/master@{#391382}

[Tom (Use chromium acct), 2016-04-29 00:14:27]

thomasanderson@google.com changed reviewers:
+ erg@chromium.org, sky@chromium.org

[danakj, 2016-04-29 00:22:06]

On Thu, Apr 28, 2016 at 5:14 PM, <thomasanderson@google.com> wrote:

> Reviewers: sky, Elliot Glaysher
> CL: https://codereview.chromium.org/1927203003/
>
> Description:
> Add support for X11 workspaces
>
> The core changes introduced:
> - Add new observers that will get notified when a window's workspace
> changes.
> - Save the browser window workspace on browser close
> - Restore the browser workspace for a restored browser session
> - Switch to the window's workspace when it is Activate()'d
>

This last one should be the window manager's policy?


>
> BUG=18596
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+146, -13 lines):
> M chrome/browser/sessions/session_restore.cc
> M chrome/browser/sessions/session_service.h
> M chrome/browser/sessions/session_service.cc
> M chrome/browser/ui/browser.h
> M chrome/browser/ui/browser.cc
> M chrome/browser/ui/browser_window_state.h
> M chrome/browser/ui/browser_window_state.cc
> M chrome/browser/ui/views/frame/browser_frame.h
> M chrome/browser/ui/views/frame/browser_frame.cc
> M components/sessions/core/session_service_commands.h
> M components/sessions/core/session_service_commands.cc
> M components/sessions/core/session_types.h
> M components/sessions/core/session_types.cc
> M ui/aura/window_tree_host.h
> M ui/aura/window_tree_host.cc
> M ui/aura/window_tree_host_observer.h
> M ui/views/widget/desktop_aura/desktop_native_widget_aura.h
> M ui/views/widget/desktop_aura/desktop_native_widget_aura.cc
> M ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
> M ui/views/widget/desktop_aura/x11_desktop_handler.cc
> M ui/views/widget/native_widget_delegate.h
> M ui/views/widget/widget.h
> M ui/views/widget/widget.cc
>
>
>

-- 
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.

[Tom (Use chromium acct), 2016-04-29 00:27:16]

I was wondering about that also.  On Gnome, when I load a session in another
workspace, I see a popup message in the corner of the screen telling me Chrome
is loaded.

[danakj, 2016-04-29 00:41:39]

On Thu, Apr 28, 2016 at 5:27 PM, <thomasanderson@google.com> wrote:

> I was wondering about that also. On Gnome, when I load a session in another
> workspace, I see a popup message in the corner of the screen telling me
> Chrome
> is loaded.
>

For instance in some window managers, activating may bring the window to
the current desktop, instead of changing desktop. It also may choose to not
activate, in which case you've switched desktop for no reason, it should be
an atomic decision in the WM.

Not sure if "when I load a session" implies that we're activating a window
(or that it should)?

-- 
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.

[sky, 2016-04-29 01:01:20]

On Thu, Apr 28, 2016 at 5:14 PM,  <thomasanderson@google.com> wrote:
> Reviewers: sky, Elliot Glaysher
> CL: https://codereview.chromium.org/1927203003/
>
> Description:
> Add support for X11 workspaces
>
> The core changes introduced:
> - Add new observers that will get notified when a window's workspace
> changes.
> - Save the browser window workspace on browser close

Why do you only save workspace on close? That means if the
machine/chrome crashes you won't restore to the right workspace.

  -Scott

> - Restore the browser workspace for a restored browser session
> - Switch to the window's workspace when it is Activate()'d
>
> BUG=18596
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+146, -13 lines):
> M chrome/browser/sessions/session_restore.cc
> M chrome/browser/sessions/session_service.h
> M chrome/browser/sessions/session_service.cc
> M chrome/browser/ui/browser.h
> M chrome/browser/ui/browser.cc
> M chrome/browser/ui/browser_window_state.h
> M chrome/browser/ui/browser_window_state.cc
> M chrome/browser/ui/views/frame/browser_frame.h
> M chrome/browser/ui/views/frame/browser_frame.cc
> M components/sessions/core/session_service_commands.h
> M components/sessions/core/session_service_commands.cc
> M components/sessions/core/session_types.h
> M components/sessions/core/session_types.cc
> M ui/aura/window_tree_host.h
> M ui/aura/window_tree_host.cc
> M ui/aura/window_tree_host_observer.h
> M ui/views/widget/desktop_aura/desktop_native_widget_aura.h
> M ui/views/widget/desktop_aura/desktop_native_widget_aura.cc
> M ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
> M ui/views/widget/desktop_aura/x11_desktop_handler.cc
> M ui/views/widget/native_widget_delegate.h
> M ui/views/widget/widget.h
> M ui/views/widget/widget.cc
>
>

-- 
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.

[Tom (Use chromium acct), 2016-04-29 01:15:39]

Description was changed from

==========
Add support for X11 workspaces

The core changes introduced:
- Add new observers that will get notified when a window's workspace
changes.
- Save the browser window workspace on browser close
- Restore the browser workspace for a restored browser session
- Switch to the window's workspace when it is Activate()'d

BUG=18596
==========

to

==========
Add support for X11 workspaces

The core changes introduced:
- Add new observers that will get notified when a window's workspace
changes.
- Save the browser window workspace whenever the workspace changes
- Restore the browser workspace for a restored browser session
- Switch to the window's workspace when it is Activate()'d

BUG=18596
==========

[Tom (Use chromium acct), 2016-04-29 01:18:28]

> Why do you only save workspace on close? That means if the
> machine/chrome crashes you won't restore to the right workspace.

Oops, I actually save the workspace whenever the workspace changes.  Updated the
description.

[Tom (Use chromium acct), 2016-04-29 01:19:20]

> For instance in some window managers, activating may bring the window to
> the current desktop, instead of changing desktop. It also may choose to not
> activate, in which case you've switched desktop for no reason, it should be
> an atomic decision in the WM.

Makes sense to me.  Reverted x11_desktop_handler.cc

[Tom (Use chromium acct), 2016-04-29 01:19:48]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-29 01:20:10]

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

[commit-bot: I haz the power, 2016-04-29 02:25:50]

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

[commit-bot: I haz the power, 2016-04-29 02:25:51]

Dry run: This issue passed the CQ dry run.

[Tom (Use chromium acct), 2016-04-29 04:32:53]

Description was changed from

==========
Add support for X11 workspaces

The core changes introduced:
- Add new observers that will get notified when a window's workspace
changes.
- Save the browser window workspace whenever the workspace changes
- Restore the browser workspace for a restored browser session
- Switch to the window's workspace when it is Activate()'d

BUG=18596
==========

to

==========
Add support for X11 workspaces

The core changes introduced:
- Add new observers that will get notified when a window's workspace
changes.
- Save the browser window workspace whenever the workspace changes
- Restore the browser workspace for a restored browser session

BUG=18596
==========

[sky, 2016-04-29 17:05:45]

This looks reasonable to me. My only concern is if we should use a string for
the workspace and not an id. I think we should go with a string as it gives
flexibility with other OSs.

Also, you need to add test coverage.

Lastly, you'll want to update tab restore as well, but you could do that
separately. By tab restore I mean if you close a window and hit restore tab then
the window should be restored to the appropriate workspace.

https://codereview.chromium.org/1927203003/diff/20001/chrome/browser/sessions...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/1927203003/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:826: 
You'll need to save the workspace some where in this function too.

https://codereview.chromium.org/1927203003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1927203003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:245: int override_workspace() const { return
override_workspace_; }
Why is this called override_workspace and not initial_workspace?

https://codereview.chromium.org/1927203003/diff/20001/components/sessions/cor...
File components/sessions/core/session_service_commands.cc (right):

https://codereview.chromium.org/1927203003/diff/20001/components/sessions/cor...
components/sessions/core/session_service_commands.cc:588:
if(!command->GetPayload(&payload, sizeof(payload))) {
nit: space between 'if' and '('.

https://codereview.chromium.org/1927203003/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1927203003/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2000: else if
(changed_atom == atom_cache_.GetAtom("_NET_WM_DESKTOP")) {
when you add {} to one branch, then add {} to all branches.

[Elliot Glaysher, 2016-04-29 17:58:13]

https://codereview.chromium.org/1927203003/diff/20001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1927203003/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.cc:132: workspace(-1),
Should the default value by -1? The docs for _NET_WM_DESKTOP say that the value
0xFFFFFFFF should be used for being placed on all desktops.

[Tom (Use chromium acct), 2016-04-29 19:14:04]

Still need to
- Add tests
- Change workspace ID to use strings

https://codereview.chromium.org/1927203003/diff/20001/chrome/browser/sessions...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/1927203003/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:826: 
On 2016/04/29 17:05:45, sky wrote:
> You'll need to save the workspace some where in this function too.

Done.  Although I had to add a lot of plumbing to get GetRestoredWorkspace
working.  Could you check the deltas on this?  In particular, I'm worried about
NativeWidgetAura::GetRestoredWorkspace and NativeWidgetMus::GetRestoredWorkspace

https://codereview.chromium.org/1927203003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1927203003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:245: int override_workspace() const { return
override_workspace_; }
On 2016/04/29 17:05:45, sky wrote:
> Why is this called override_workspace and not initial_workspace?

Done. (I just copy pasted from override_bounds)

https://codereview.chromium.org/1927203003/diff/20001/components/sessions/cor...
File components/sessions/core/session_service_commands.cc (right):

https://codereview.chromium.org/1927203003/diff/20001/components/sessions/cor...
components/sessions/core/session_service_commands.cc:588:
if(!command->GetPayload(&payload, sizeof(payload))) {
On 2016/04/29 17:05:45, sky wrote:
> nit: space between 'if' and '('.

Done.

https://codereview.chromium.org/1927203003/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1927203003/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2000: else if
(changed_atom == atom_cache_.GetAtom("_NET_WM_DESKTOP")) {
On 2016/04/29 17:05:45, sky wrote:
> when you add {} to one branch, then add {} to all branches.

Done.

https://codereview.chromium.org/1927203003/diff/20001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1927203003/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.cc:132: workspace(-1),
On 2016/04/29 17:58:13, Elliot Glaysher wrote:
> Should the default value by -1? The docs for _NET_WM_DESKTOP say that the
value
> 0xFFFFFFFF should be used for being placed on all desktops.

I use that as a sentinel because it's reserved.  In
desktop_window_tree_host_x11.cc, I only set the workspace if it's not -1.  I
guess this shouldn't matter anymore now that the int is going to chagne to a
string, and I'll just use the empty string as the default value.

[danakj, 2016-04-29 19:59:05]

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

[danakj, 2016-04-29 19:59:06]

https://codereview.chromium.org/1927203003/diff/20001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1927203003/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.cc:132: workspace(-1),
On 2016/04/29 19:14:04, Tom Anderson wrote:
> On 2016/04/29 17:58:13, Elliot Glaysher wrote:
> > Should the default value by -1? The docs for _NET_WM_DESKTOP say that the
> value
> > 0xFFFFFFFF should be used for being placed on all desktops.
> 
> I use that as a sentinel because it's reserved.  In
> desktop_window_tree_host_x11.cc, I only set the workspace if it's not -1.  I
> guess this shouldn't matter anymore now that the int is going to chagne to a
> string, and I'll just use the empty string as the default value.

Have you tested what happens if the chrome window is put on "all desktops" and
then you close/restore etc?

[sky, 2016-05-02 15:11:23]

The plumbing for getting the current workspace looks right.

https://codereview.chromium.org/1927203003/diff/40001/chrome/browser/sessions...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/1927203003/diff/40001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:827: if
(browser->window()->GetRestoredWorkspace() != -1) {
I wouldn't bother checking the return value, always save it.

https://codereview.chromium.org/1927203003/diff/40001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1927203003/diff/40001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:571: else
nit: no else after a return.

https://codereview.chromium.org/1927203003/diff/40001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/1927203003/diff/40001/ui/views/widget/widget....
ui/views/widget/widget.h:431: int GetRestoredWorkspace() const;
This is the workspace the window is on, right? How about GetWorkspace()?

[Tom (Use chromium acct), 2016-05-02 20:32:38]

Changed integer workspace ID to string
Added unit test (may need more test coverage)

On 2016/04/29 19:59:06, danakj wrote:
>
https://codereview.chromium.org/1927203003/diff/20001/ui/views/widget/widget.cc
> File ui/views/widget/widget.cc (right):
> 
>
https://codereview.chromium.org/1927203003/diff/20001/ui/views/widget/widget....
> ui/views/widget/widget.cc:132: workspace(-1),
> On 2016/04/29 19:14:04, Tom Anderson wrote:
> > On 2016/04/29 17:58:13, Elliot Glaysher wrote:
> > > Should the default value by -1? The docs for _NET_WM_DESKTOP say that the
> > value
> > > 0xFFFFFFFF should be used for being placed on all desktops.
> > 
> > I use that as a sentinel because it's reserved.  In
> > desktop_window_tree_host_x11.cc, I only set the workspace if it's not -1.  I
> > guess this shouldn't matter anymore now that the int is going to chagne to a
> > string, and I'll just use the empty string as the default value.
> 
> Have you tested what happens if the chrome window is put on "all desktops" and
> then you close/restore etc?

Nice catch Elliot + Dana
It was broken before with the integer workspace ID, but the string workspace ID
seems to have fixed this.  Now you can save/restore an "all desktops" window.

https://codereview.chromium.org/1927203003/diff/40001/chrome/browser/sessions...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/1927203003/diff/40001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:827: if
(browser->window()->GetRestoredWorkspace() != -1) {
On 2016/05/02 15:11:23, sky wrote:
> I wouldn't bother checking the return value, always save it.

Done.

https://codereview.chromium.org/1927203003/diff/40001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1927203003/diff/40001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:571: else
On 2016/05/02 15:11:23, sky wrote:
> nit: no else after a return.

Done.

https://codereview.chromium.org/1927203003/diff/40001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/1927203003/diff/40001/ui/views/widget/widget....
ui/views/widget/widget.h:431: int GetRestoredWorkspace() const;
On 2016/05/02 15:11:23, sky wrote:
> This is the workspace the window is on, right? How about GetWorkspace()?

yes & done

[Tom (Use chromium acct), 2016-05-02 20:37:44]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-02 20:38:00]

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

[commit-bot: I haz the power, 2016-05-02 20:55:10]

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

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

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

[Tom (Use chromium acct), 2016-05-02 21:19:44]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

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

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

[commit-bot: I haz the power, 2016-05-02 21:37:56]

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

[commit-bot: I haz the power, 2016-05-02 21:37:57]

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

[Elliot Glaysher, 2016-05-02 21:52:36]

lgtm once you fix the few compile errors by adding the empty implementations of
GetWorkspace().

https://codereview.chromium.org/1927203003/diff/80001/chrome/test/base/test_b...
File chrome/test/base/test_browser_window.cc (right):

https://codereview.chromium.org/1927203003/diff/80001/chrome/test/base/test_b...
chrome/test/base/test_browser_window.cc:201: // TestBrowserWindowOwner
Remove the newline so this is one line like before.

[Tom (Use chromium acct), 2016-05-02 22:35:04]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-02 22:37:13]

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

[Tom (Use chromium acct), 2016-05-02 23:34:02]

https://codereview.chromium.org/1927203003/diff/80001/chrome/test/base/test_b...
File chrome/test/base/test_browser_window.cc (right):

https://codereview.chromium.org/1927203003/diff/80001/chrome/test/base/test_b...
chrome/test/base/test_browser_window.cc:201: // TestBrowserWindowOwner
On 2016/05/02 21:52:36, Elliot Glaysher wrote:
> Remove the newline so this is one line like before.

Done.  Idk how that happened lol

[Tom (Use chromium acct), 2016-05-02 23:36:08]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-02 23:36:59]

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

[commit-bot: I haz the power, 2016-05-03 00:39:27]

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

[commit-bot: I haz the power, 2016-05-03 00:39:28]

Dry run: This issue passed the CQ dry run.

[Tom (Use chromium acct), 2016-05-03 02:05:02]

The CQ bit was checked by thomasanderson@google.com

[Tom (Use chromium acct), 2016-05-03 02:05:02]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org
Link to the patchset: https://codereview.chromium.org/1927203003/#ps120001
(title: "Remove newline")

[commit-bot: I haz the power, 2016-05-03 02:05:17]

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

[commit-bot: I haz the power, 2016-05-03 02:13:14]

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

[commit-bot: I haz the power, 2016-05-03 02:13:16]

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

[Tom (Use chromium acct), 2016-05-03 17:28:29]

On 2016/05/03 02:13:16, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Pinging Scott for owner's approval

[sky, 2016-05-03 17:40:53]

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/session...
File chrome/browser/sessions/session_service_unittest.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/session...
chrome/browser/sessions/session_service_unittest.cc:214:
ASSERT_TRUE(window_workspace == windows[0]->workspace);
ASSERT_EQ(window_workspace,

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser.h:245: std::string initial_workspace() const { return
initial_workspace_; }
Make return type const std::string& (just like app_name on line 241).

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser.h:956: std::string initial_workspace_;
const

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_window.h:382: virtual std::string GetWorkspace() const
= 0;
Add description.

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_window_state.cc:178: void
GetSavedWindowWorkspace(const Browser* browser, std::string* workspace) {
Why do we need this function? Can't callers call browser->initial_workspace()
directly?

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:375: return "";
""->std::string()

https://codereview.chromium.org/1927203003/diff/120001/chrome/test/base/test_...
File chrome/test/base/test_browser_window.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/test/base/test_...
chrome/test/base/test_browser_window.cc:198: return "";
""->std::string()

https://codereview.chromium.org/1927203003/diff/120001/components/sessions/co...
File components/sessions/core/session_service_commands.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/components/sessions/co...
components/sessions/core/session_service_commands.cc:765: new
SessionCommand(kCommandSetWindowWorkspace, pickle.size()));
Use the constructor that takes a Pickle (see line 732 for an example).

https://codereview.chromium.org/1927203003/diff/120001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:611: return "";
std::string()

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:681:
desktop_window_tree_host_->GetWorkspace() : "";
std::string()

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:294: return "";
std::string()

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:572: return "";
std::string()

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1237: } else if
(params.workspace.size()) {
!params.workspace.empty()

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2005: if
(changed_atom == atom_cache_.GetAtom("_NET_WM_STATE")) {
nit: no {} here as all branches are a single line.

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/native...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/native...
ui/views/widget/native_widget_aura.cc:400: return "";
std::string()

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/native...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/native...
ui/views/widget/native_widget_mac.mm:310: return "";
std::string()

[Tom (Use chromium acct), 2016-05-03 19:38:15]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

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

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

[Tom (Use chromium acct), 2016-05-03 19:38:39]

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/session...
File chrome/browser/sessions/session_service_unittest.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/session...
chrome/browser/sessions/session_service_unittest.cc:214:
ASSERT_TRUE(window_workspace == windows[0]->workspace);
On 2016/05/03 17:40:52, sky wrote:
> ASSERT_EQ(window_workspace,

Done.

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser.h:245: std::string initial_workspace() const { return
initial_workspace_; }
On 2016/05/03 17:40:52, sky wrote:
> Make return type const std::string& (just like app_name on line 241).

Done.

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser.h:956: std::string initial_workspace_;
On 2016/05/03 17:40:52, sky wrote:
> const

Done.

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_window.h:382: virtual std::string GetWorkspace() const
= 0;
On 2016/05/03 17:40:52, sky wrote:
> Add description.

Done.

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_window_state.cc:178: void
GetSavedWindowWorkspace(const Browser* browser, std::string* workspace) {
On 2016/05/03 17:40:52, sky wrote:
> Why do we need this function? Can't callers call browser->initial_workspace()
> directly?

I completed the TODO in the comment here.  Should we keep this function now that
it actually does something useful? (It still only gets called in one place)

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:375: return "";
On 2016/05/03 17:40:52, sky wrote:
> ""->std::string()

Done.

https://codereview.chromium.org/1927203003/diff/120001/chrome/test/base/test_...
File chrome/test/base/test_browser_window.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/test/base/test_...
chrome/test/base/test_browser_window.cc:198: return "";
On 2016/05/03 17:40:52, sky wrote:
> ""->std::string()

Done.

https://codereview.chromium.org/1927203003/diff/120001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:611: return "";
On 2016/05/03 17:40:53, sky wrote:
> std::string()

Done.

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:681:
desktop_window_tree_host_->GetWorkspace() : "";
On 2016/05/03 17:40:53, sky wrote:
> std::string()

Done.

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:294: return "";
On 2016/05/03 17:40:53, sky wrote:
> std::string()

Done.

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:572: return "";
On 2016/05/03 17:40:53, sky wrote:
> std::string()

Done.

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1237: } else if
(params.workspace.size()) {
On 2016/05/03 17:40:53, sky wrote:
> !params.workspace.empty()

Done.

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2005: if
(changed_atom == atom_cache_.GetAtom("_NET_WM_STATE")) {
On 2016/05/03 17:40:53, sky wrote:
> nit: no {} here as all branches are a single line.

Done.

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/native...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/native...
ui/views/widget/native_widget_mac.mm:310: return "";
On 2016/05/03 17:40:53, sky wrote:
> std::string()

Done.

[sky, 2016-05-03 20:11:59]

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_window_state.cc:178: void
GetSavedWindowWorkspace(const Browser* browser, std::string* workspace) {
On 2016/05/03 19:38:39, Tom Anderson wrote:
> On 2016/05/03 17:40:52, sky wrote:
> > Why do we need this function? Can't callers call
browser->initial_workspace()
> > directly?
> 
> I completed the TODO in the comment here.  Should we keep this function now
that
> it actually does something useful? (It still only gets called in one place)

I don't see the point in this function, so I say nuke it.

[Tom (Use chromium acct), 2016-05-03 20:27:58]

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_window_state.cc:178: void
GetSavedWindowWorkspace(const Browser* browser, std::string* workspace) {
On 2016/05/03 20:11:59, sky wrote:
> On 2016/05/03 19:38:39, Tom Anderson wrote:
> > On 2016/05/03 17:40:52, sky wrote:
> > > Why do we need this function? Can't callers call
> browser->initial_workspace()
> > > directly?
> > 
> > I completed the TODO in the comment here.  Should we keep this function now
> that
> > it actually does something useful? (It still only gets called in one place)
> 
> I don't see the point in this function, so I say nuke it.

Done.

[Tom (Use chromium acct), 2016-05-03 20:28:08]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

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

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

[commit-bot: I haz the power, 2016-05-03 21:33:36]

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

[commit-bot: I haz the power, 2016-05-03 21:33:37]

Dry run: This issue passed the CQ dry run.

[sky, 2016-05-03 22:22:05]

LGTM

[Tom (Use chromium acct), 2016-05-03 22:30:20]

The CQ bit was checked by thomasanderson@google.com

[Tom (Use chromium acct), 2016-05-03 22:30:21]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org
Link to the patchset: https://codereview.chromium.org/1927203003/#ps160001
(title: "Removed GetSavedWindowWorkspace, put command line code into
browser_frame.cc")

[commit-bot: I haz the power, 2016-05-03 22:30:43]

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

[commit-bot: I haz the power, 2016-05-03 22:52:37]

Description was changed from

==========
Add support for X11 workspaces

The core changes introduced:
- Add new observers that will get notified when a window's workspace
changes.
- Save the browser window workspace whenever the workspace changes
- Restore the browser workspace for a restored browser session

BUG=18596
==========

to

==========
Add support for X11 workspaces

The core changes introduced:
- Add new observers that will get notified when a window's workspace
changes.
- Save the browser window workspace whenever the workspace changes
- Restore the browser workspace for a restored browser session

BUG=18596
==========

[commit-bot: I haz the power, 2016-05-03 22:52:39]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-05-03 22:54:12]

Description was changed from

==========
Add support for X11 workspaces

The core changes introduced:
- Add new observers that will get notified when a window's workspace
changes.
- Save the browser window workspace whenever the workspace changes
- Restore the browser workspace for a restored browser session

BUG=18596
==========

to

==========
Add support for X11 workspaces

The core changes introduced:
- Add new observers that will get notified when a window's workspace
changes.
- Save the browser window workspace whenever the workspace changes
- Restore the browser workspace for a restored browser session

BUG=18596

Committed: https://crrev.com/06405c5944436b431f26037fdc93340842c51de5
Cr-Commit-Position: refs/heads/master@{#391382}
==========

[commit-bot: I haz the power, 2016-05-03 22:54:14]

Patchset 9 (id:??) landed as
https://crrev.com/06405c5944436b431f26037fdc93340842c51de5
Cr-Commit-Position: refs/heads/master@{#391382}

[sky, 2016-05-06 17:43:09]

Sorry for not noticing this earlier.

https://codereview.chromium.org/1927203003/diff/160001/components/sessions/co...
File components/sessions/core/session_service_commands.cc (right):

https://codereview.chromium.org/1927203003/diff/160001/components/sessions/co...
components/sessions/core/session_service_commands.cc:589: if
(!it.ReadBytes(reinterpret_cast<const char**>(&window_id),
Why do you reinterpret cast here? You should be able to read the window_id as an
int, right?

https://codereview.chromium.org/1927203003/diff/160001/components/sessions/co...
components/sessions/core/session_service_commands.cc:762:
pickle.WriteBytes(static_cast<const void*>(&window_id), sizeof(window_id));
Same comment here. Use WriteInt.

https://codereview.chromium.org/1927203003/diff/160001/components/sessions/co...
components/sessions/core/session_service_commands.cc:765: new
SessionCommand(kCommandSetWindowWorkspace, pickle.size()));
Use the constructor that takes the pickle.

[Tom (Use chromium acct), 2016-05-06 18:25:41]

New cl: https://codereview.chromium.org/1961563002/

https://codereview.chromium.org/1927203003/diff/160001/components/sessions/co...
File components/sessions/core/session_service_commands.cc (right):

https://codereview.chromium.org/1927203003/diff/160001/components/sessions/co...
components/sessions/core/session_service_commands.cc:589: if
(!it.ReadBytes(reinterpret_cast<const char**>(&window_id),
On 2016/05/06 17:43:09, sky wrote:
> Why do you reinterpret cast here? You should be able to read the window_id as
an
> int, right?

Done.

https://codereview.chromium.org/1927203003/diff/160001/components/sessions/co...
components/sessions/core/session_service_commands.cc:762:
pickle.WriteBytes(static_cast<const void*>(&window_id), sizeof(window_id));
On 2016/05/06 17:43:09, sky wrote:
> Same comment here. Use WriteInt.

Done.

https://codereview.chromium.org/1927203003/diff/160001/components/sessions/co...
components/sessions/core/session_service_commands.cc:765: new
SessionCommand(kCommandSetWindowWorkspace, pickle.size()));
On 2016/05/06 17:43:09, sky wrote:
> Use the constructor that takes the pickle.

Done.