Add support for X11 workspaces (issue 1927203003 by thomasanderson@google.com)

28 views
Skip to first unread message

thomasa...@google.com

unread,
Apr 28, 2016, 8:14:30 PM4/28/16
to s...@chromium.org, e...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
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

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


Dana Jansens

unread,
Apr 28, 2016, 8:22:06 PM4/28/16
to Tom Anderson, Scott Violet, Elliot Glaysher (Chromium), chromium...@chromium.org, Thiago Farina, Sadrul Chowdhury, Kondapally, Kalyan, Lei Zhang, Dana Jansens
On Thu, Apr 28, 2016 at 5:14 PM, <thomasa...@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?

thomasa...@google.com

unread,
Apr 28, 2016, 8:27:17 PM4/28/16
to s...@chromium.org, e...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
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.

https://codereview.chromium.org/1927203003/

Dana Jansens

unread,
Apr 28, 2016, 8:41:39 PM4/28/16
to Tom Anderson, Scott Violet, Elliot Glaysher (Chromium), chromium...@chromium.org, Thiago Farina, Sadrul Chowdhury, Kondapally, Kalyan, Lei Zhang, Dana Jansens
On Thu, Apr 28, 2016 at 5:27 PM, <thomasa...@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)?

Scott Violet

unread,
Apr 28, 2016, 9:01:20 PM4/28/16
to thomasa...@google.com, Scott Violet, Elliot Glaysher (Chromium), chromium...@chromium.org, Thiago Farina, sad...@chromium.org, Kondapally, Kalyan, Lei Zhang, Dana Jansens
On Thu, Apr 28, 2016 at 5:14 PM, <thomasa...@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

thomasa...@google.com

unread,
Apr 28, 2016, 9:18:29 PM4/28/16
to s...@chromium.org, e...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
> 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.



https://codereview.chromium.org/1927203003/

thomasa...@google.com

unread,
Apr 28, 2016, 9:19:22 PM4/28/16
to s...@chromium.org, e...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
> 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

https://codereview.chromium.org/1927203003/

s...@chromium.org

unread,
Apr 29, 2016, 1:05:46 PM4/29/16
to thomasa...@google.com, e...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
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/session_service.cc
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/1927203003/diff/20001/chrome/browser/sessions/session_service.cc#newcode826
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/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1927203003/diff/20001/chrome/browser/ui/browser.h#newcode245
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/core/session_service_commands.cc
File components/sessions/core/session_service_commands.cc (right):

https://codereview.chromium.org/1927203003/diff/20001/components/sessions/core/session_service_commands.cc#newcode588
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_aura/desktop_window_tree_host_x11.cc
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
(right):

https://codereview.chromium.org/1927203003/diff/20001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode2000
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.

https://codereview.chromium.org/1927203003/

e...@chromium.org

unread,
Apr 29, 2016, 1:58:14 PM4/29/16
to thomasa...@google.com, s...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org

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.cc#newcode132
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.

https://codereview.chromium.org/1927203003/

thomasa...@google.com

unread,
Apr 29, 2016, 3:14:05 PM4/29/16
to s...@chromium.org, e...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
Still need to
- Add tests
- Change workspace ID to use strings
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/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1927203003/diff/20001/chrome/browser/ui/browser.h#newcode245
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/core/session_service_commands.cc
File components/sessions/core/session_service_commands.cc (right):

https://codereview.chromium.org/1927203003/diff/20001/components/sessions/core/session_service_commands.cc#newcode588
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_aura/desktop_window_tree_host_x11.cc
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
(right):

https://codereview.chromium.org/1927203003/diff/20001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode2000
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.
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.

https://codereview.chromium.org/1927203003/

dan...@chromium.org

unread,
Apr 29, 2016, 3:59:07 PM4/29/16
to thomasa...@google.com, s...@chromium.org, e...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org
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?

https://codereview.chromium.org/1927203003/

s...@chromium.org

unread,
May 2, 2016, 11:11:25 AM5/2/16
to thomasa...@google.com, e...@chromium.org, dan...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
The plumbing for getting the current workspace looks right.


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

https://codereview.chromium.org/1927203003/diff/40001/chrome/browser/sessions/session_service.cc#newcode827
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_aura/desktop_window_tree_host_x11.cc
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
(right):

https://codereview.chromium.org/1927203003/diff/40001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode571
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.h#newcode431
ui/views/widget/widget.h:431: int GetRestoredWorkspace() const;
This is the workspace the window is on, right? How about GetWorkspace()?

https://codereview.chromium.org/1927203003/

thomasa...@google.com

unread,
May 2, 2016, 4:32:39 PM5/2/16
to s...@chromium.org, e...@chromium.org, dan...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
Changed integer workspace ID to string
Added unit test (may need more test coverage)
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/session_service.cc
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/1927203003/diff/40001/chrome/browser/sessions/session_service.cc#newcode827
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.
On 2016/05/02 15:11:23, sky wrote:
> nit: no else after a return.

Done.
On 2016/05/02 15:11:23, sky wrote:
> This is the workspace the window is on, right? How about
GetWorkspace()?

e...@chromium.org

unread,
May 2, 2016, 5:52:37 PM5/2/16
to thomasa...@google.com, s...@chromium.org, dan...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
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_browser_window.cc
File chrome/test/base/test_browser_window.cc (right):

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

https://codereview.chromium.org/1927203003/

thomasa...@google.com

unread,
May 2, 2016, 7:34:03 PM5/2/16
to s...@chromium.org, e...@chromium.org, dan...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
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

https://codereview.chromium.org/1927203003/

commit-bot@chromium.org via codereview.chromium.org

unread,
May 2, 2016, 10:05:18 PM5/2/16
to thomasa...@google.com, s...@chromium.org, e...@chromium.org, dan...@chromium.org, commi...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
May 2, 2016, 10:13:17 PM5/2/16
to thomasa...@google.com, s...@chromium.org, e...@chromium.org, dan...@chromium.org, commi...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
Try jobs failed on following builders:
chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/176507)

https://codereview.chromium.org/1927203003/

thomasa...@google.com

unread,
May 3, 2016, 1:28:30 PM5/3/16
to s...@chromium.org, e...@chromium.org, dan...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
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_presubmit/builds/176507)

Pinging Scott for owner's approval

https://codereview.chromium.org/1927203003/

s...@chromium.org

unread,
May 3, 2016, 1:42:19 PM5/3/16
to thomasa...@google.com, e...@chromium.org, dan...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org

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

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/sessions/session_service_unittest.cc#newcode214
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/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/browser.h#newcode245
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/browser.h#newcode956
chrome/browser/ui/browser.h:956: std::string initial_workspace_;
const

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

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/browser_window.h#newcode382
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/browser_window_state.cc
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/browser_window_state.cc#newcode178
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/cocoa/browser_window_cocoa.mm
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

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

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

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

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

https://codereview.chromium.org/1927203003/diff/120001/components/sessions/core/session_service_commands.cc#newcode765
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_widget_mus.cc
File ui/views/mus/native_widget_mus.cc (right):

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

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

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/desktop_aura/desktop_native_widget_aura.cc#newcode681
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/desktop_aura/desktop_window_tree_host_win.cc
File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc#newcode294
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/desktop_aura/desktop_window_tree_host_x11.cc
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
(right):

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode572
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/desktop_aura/desktop_window_tree_host_x11.cc#newcode1237
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/desktop_aura/desktop_window_tree_host_x11.cc#newcode2005
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_widget_aura.cc
File ui/views/widget/native_widget_aura.cc (right):

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

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

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

https://codereview.chromium.org/1927203003/

thomasa...@google.com

unread,
May 3, 2016, 3:38:40 PM5/3/16
to s...@chromium.org, e...@chromium.org, dan...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org

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

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/sessions/session_service_unittest.cc#newcode214
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/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/browser.h#newcode245
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/browser.h#newcode956
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/browser_window.h
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/browser_window.h#newcode382
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/browser_window_state.cc
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/browser_window_state.cc#newcode178
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)
On 2016/05/03 17:40:52, sky wrote:
> ""->std::string()

Done.
On 2016/05/03 17:40:52, sky wrote:
> ""->std::string()

Done.
On 2016/05/03 17:40:53, sky wrote:
> std::string()

Done.


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

https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/desktop_aura/desktop_native_widget_aura.cc#newcode681
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.
On 2016/05/03 17:40:53, sky wrote:
> std::string()

Done.
On 2016/05/03 17:40:53, sky wrote:
> std::string()

Done.


https://codereview.chromium.org/1927203003/diff/120001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode1237
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/desktop_aura/desktop_window_tree_host_x11.cc#newcode2005
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.
On 2016/05/03 17:40:53, sky wrote:
> std::string()

Done.

https://codereview.chromium.org/1927203003/

s...@chromium.org

unread,
May 3, 2016, 4:12:00 PM5/3/16
to thomasa...@google.com, e...@chromium.org, dan...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org

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

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/browser_window_state.cc#newcode178
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.

https://codereview.chromium.org/1927203003/

thomasa...@google.com

unread,
May 3, 2016, 4:27:59 PM5/3/16
to s...@chromium.org, e...@chromium.org, dan...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org

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

https://codereview.chromium.org/1927203003/diff/120001/chrome/browser/ui/browser_window_state.cc#newcode178
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.

s...@chromium.org

unread,
May 3, 2016, 6:22:06 PM5/3/16
to thomasa...@google.com, e...@chromium.org, dan...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
May 3, 2016, 6:30:44 PM5/3/16
to thomasa...@google.com, s...@chromium.org, e...@chromium.org, dan...@chromium.org, commi...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
May 3, 2016, 6:52:40 PM5/3/16
to thomasa...@google.com, s...@chromium.org, e...@chromium.org, dan...@chromium.org, commi...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
Committed patchset #9 (id:160001)

https://codereview.chromium.org/1927203003/

commit-bot@chromium.org via codereview.chromium.org

unread,
May 3, 2016, 6:54:15 PM5/3/16
to thomasa...@google.com, s...@chromium.org, e...@chromium.org, dan...@chromium.org, commi...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
Patchset 9 (id:??) landed as
https://crrev.com/06405c5944436b431f26037fdc93340842c51de5
Cr-Commit-Position: refs/heads/master@{#391382}

https://codereview.chromium.org/1927203003/

s...@chromium.org

unread,
May 6, 2016, 1:43:10 PM5/6/16
to thomasa...@google.com, e...@chromium.org, dan...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org
Sorry for not noticing this earlier.


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

https://codereview.chromium.org/1927203003/diff/160001/components/sessions/core/session_service_commands.cc#newcode589
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/core/session_service_commands.cc#newcode762
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/core/session_service_commands.cc#newcode765

components/sessions/core/session_service_commands.cc:765: new
SessionCommand(kCommandSetWindowWorkspace, pickle.size()));
Use the constructor that takes the pickle.

https://codereview.chromium.org/1927203003/

thomasa...@google.com

unread,
May 6, 2016, 2:25:42 PM5/6/16
to s...@chromium.org, e...@chromium.org, dan...@chromium.org, chromium...@chromium.org, tfa...@chromium.org, sad...@chromium.org, kalyan.k...@intel.com, the...@chromium.org, dan...@chromium.org



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

https://codereview.chromium.org/1927203003/diff/160001/components/sessions/core/session_service_commands.cc#newcode589
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/core/session_service_commands.cc#newcode762
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/core/session_service_commands.cc#newcode765
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.

Reply all
Reply to author
Forward
0 new messages