Respect Spaces when dragging tabs on OS X

Respect Spaces when dragging tabs on OS X

For 10.6 and above, we can simply filter windows out with isOnActiveSpace. For
10.5, there is another API in CoreGraphics that gives us the same information.
It is slow and causes noticeable CPU usage during a drag, so cache it per-drag.

R=pinkerton
BUG=32796
TEST=open windows on separate spaces, drag a tab over where a window is on the other space

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52960

[David Benjamin, 2010-07-14 23:56:09]

This was annoying me, so I went and gave it a go. :-)

I don't have access to a 10.6 box, so someone will have to check that
isOnActiveSpace does the right thing.

[rohitrao (OOO until 6-22), 2010-07-15 14:45:12]

+pink, because he was just complaining that he didn't have enough reviews.

This is great.  I looked for the isOnActiveSpace call a few months ago but
somehow I missed it.

I do worry about the slow version though, because dropTargetsForController: is
called an awful lot -- basically every time you click on a tab.  Do you have a
sense for how slow it is?

[David Benjamin, 2010-07-15 16:33:20]

On 2010/07/15 14:45:12, rohitrao wrote:
> I do worry about the slow version though, because dropTargetsForController: is
> called an awful lot -- basically every time you click on a tab.  Do you have a
> sense for how slow it is?

So, I've actually only been able to test the slow path since my box is 10.5. I
only did some very rough tests, but: Without the cache and a lot of windows
open, the browser process rises from 1% CPU usage to 20%. With the cache, the
rise is only up to 2%, so I think it's probably fine. Chromium's not likely to
open new windows mid-drag, so really you only have to call the slow code at the
very beginning (modulo uncached dragWindow).

[Mark Mentovai - out til August, 2010-07-15 17:00:26]

http://codereview.chromium.org/2962018/diff/3001/4001
File chrome/browser/cocoa/tab_view.h (right):

http://codereview.chromium.org/2962018/diff/3001/4001#newcode9
chrome/browser/cocoa/tab_view.h:9: #include <CoreGraphics/CGWindow.h>
Overly-specific. Just use <CoreGraphics/CoreGraphics.h>. That’s how we generally
do framework #includes.

Actually, I’m surprised you can get away with this. I’d just include
<ApplicationServices/ApplicationServices.h>, the umbrella framework, to include
CoreGraphics. I guess one of those goofy -F things slipped in again. I’ll have
to look into that.

http://codereview.chromium.org/2962018/diff/3001/4001#newcode88
chrome/browser/cocoa/tab_view.h:88: std::map<CGWindowID, int>
workspace_id_cache_;
workspaceIDCache_ - you’re in Objective-C-land, now, follow the Objective-C
conventions.

http://codereview.chromium.org/2962018/diff/3001/4002
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/2962018/diff/3001/4002#newcode168
chrome/browser/cocoa/tab_view.mm:168: // Skip windows on the wrong space.
I wonder. Are windows “on” a space or “in” one?

http://en.allexperts.com/q/General-Writing-Grammar-680/line-versus-line.htm all
over again.

http://codereview.chromium.org/2962018/diff/3001/4002#newcode989
chrome/browser/cocoa/tab_view.mm:989: CGWindowID window_id = [window
windowNumber];
windowId (or windowID) - when in an Objective-C method, use Objective-C naming
rules.

http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Varia...

http://codereview.chromium.org/2962018/diff/3001/4002#newcode991
chrome/browser/cocoa/tab_view.mm:991: // cache the values.
You should say somewhere how long the cached stuff will be valid for. I know the
answer because I’ve read the rest of the change, but someone looking at this
function in isolation won’t. It might be best to document it in the .h where you
declare workspace_id_cache_.

http://codereview.chromium.org/2962018/diff/3001/4002#newcode993
chrome/browser/cocoa/tab_view.mm:993: std::map<CGWindowID,int>::iterator iter =
nit: space after comma.

Can you get away with a const_iterator here?

http://codereview.chromium.org/2962018/diff/3001/4002#newcode1000
chrome/browser/cocoa/tab_view.mm:1000: // NOTE(davidben): We can query in bulk,
but I don't think we care.
NOTE(name) is non-standard. Why does this note only apply to you?

I don’t like reading “we” in comments. I wonder who “we” are. Most comments are
clearer without “we.”

  // It's possible to query in bulk, but probably not necessary.

http://codereview.chromium.org/2962018/diff/3001/4002#newcode1001
chrome/browser/cocoa/tab_view.mm:1001: scoped_cftyperef<CFArrayRef>
window_ids(CFArrayCreate(
windowIds (or windowIDs)

http://codereview.chromium.org/2962018/diff/3001/4002#newcode1007
chrome/browser/cocoa/tab_view.mm:1007: CFDictionaryRef dict =
static_cast<CFDictionaryRef>(
You might also want to:

    DCHECK(CFTypeID(dict) == CFDictionaryGetTypeID());

http://codereview.chromium.org/2962018/diff/3001/4002#newcode1011
chrome/browser/cocoa/tab_view.mm:1011: CGWindowID other_id;
1. otherID

2. Don’t declare this until you need it - it should come after the
GetValueFromDictionary call.

[David Benjamin, 2010-07-15 17:37:24]

http://codereview.chromium.org/2962018/diff/3001/4001
File chrome/browser/cocoa/tab_view.h (right):

http://codereview.chromium.org/2962018/diff/3001/4001#newcode9
chrome/browser/cocoa/tab_view.h:9: #include <CoreGraphics/CGWindow.h>
On 2010/07/15 17:00:27, Mark Mentovai wrote:
> Overly-specific. Just use <CoreGraphics/CoreGraphics.h>. That’s how we
generally
> do framework #includes.
> 
> Actually, I’m surprised you can get away with this. I’d just include
> <ApplicationServices/ApplicationServices.h>, the umbrella framework, to
include
> CoreGraphics. I guess one of those goofy -F things slipped in again. I’ll have
> to look into that.

Done.

http://codereview.chromium.org/2962018/diff/3001/4001#newcode88
chrome/browser/cocoa/tab_view.h:88: std::map<CGWindowID, int>
workspace_id_cache_;
On 2010/07/15 17:00:27, Mark Mentovai wrote:
> workspaceIDCache_ - you’re in Objective-C-land, now, follow the Objective-C
> conventions.

Done.

http://codereview.chromium.org/2962018/diff/3001/4002
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/2962018/diff/3001/4002#newcode168
chrome/browser/cocoa/tab_view.mm:168: // Skip windows on the wrong space.
On 2010/07/15 17:00:27, Mark Mentovai wrote:
> I wonder. Are windows “on” a space or&nbsp;“in” one?
> 
> http://en.allexperts.com/q/General-Writing-Grammar-680/line-versus-line.htm
all
> over again.

Er. Well, Apple named the API NSWindow:isOnActiveSpace.

http://codereview.chromium.org/2962018/diff/3001/4002#newcode989
chrome/browser/cocoa/tab_view.mm:989: CGWindowID window_id = [window
windowNumber];
On 2010/07/15 17:00:27, Mark Mentovai wrote:
> windowId (or windowID) - when in an Objective-C method, use Objective-C naming
> rules.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Varia...

Done.

http://codereview.chromium.org/2962018/diff/3001/4002#newcode991
chrome/browser/cocoa/tab_view.mm:991: // cache the values.
On 2010/07/15 17:00:27, Mark Mentovai wrote:
> You should say somewhere how long the cached stuff will be valid for. I know
the
> answer because I’ve read the rest of the change, but someone looking at this
> function in isolation won’t. It might be best to document it in the .h where
you
> declare workspace_id_cache_.

Done.

http://codereview.chromium.org/2962018/diff/3001/4002#newcode993
chrome/browser/cocoa/tab_view.mm:993: std::map<CGWindowID,int>::iterator iter =
On 2010/07/15 17:00:27, Mark Mentovai wrote:
> nit: space after comma.
> 
> Can you get away with a const_iterator here?

Comma: Done.

const_iterator: Er, probably? But not without casting the map; I don't think
there's a const_find for non-const objects, and I doubt the standard requires
that const_iterator and iterator be castable. Also not likely to do much good in
the latter case.

http://codereview.chromium.org/2962018/diff/3001/4002#newcode1000
chrome/browser/cocoa/tab_view.mm:1000: // NOTE(davidben): We can query in bulk,
but I don't think we care.
On 2010/07/15 17:00:27, Mark Mentovai wrote:
> NOTE(name) is non-standard. Why does this note only apply to you?
> 
> I don’t like reading “we” in comments. I wonder who “we” are. Most comments
are
> clearer without “we.”
> 
>   // It's possible to query in bulk, but probably not necessary.

Done.

High school math interests to blame there. I have a bit of a habit of using
royal we. :-)

http://codereview.chromium.org/2962018/diff/3001/4002#newcode1001
chrome/browser/cocoa/tab_view.mm:1001: scoped_cftyperef<CFArrayRef>
window_ids(CFArrayCreate(
On 2010/07/15 17:00:27, Mark Mentovai wrote:
> windowIds (or windowIDs)

Done.

http://codereview.chromium.org/2962018/diff/3001/4002#newcode1007
chrome/browser/cocoa/tab_view.mm:1007: CFDictionaryRef dict =
static_cast<CFDictionaryRef>(
On 2010/07/15 17:00:27, Mark Mentovai wrote:
> You might also want to:
> 
>     DCHECK(CFTypeID(dict) == CFDictionaryGetTypeID());

Done.

http://codereview.chromium.org/2962018/diff/3001/4002#newcode1011
chrome/browser/cocoa/tab_view.mm:1011: CGWindowID other_id;
On 2010/07/15 17:00:27, Mark Mentovai wrote:
> 1. otherID
> 
> 2. Don’t declare this until you need it - it should come after the
> GetValueFromDictionary call.

Done.

[pink, 2010-07-19 13:32:46]

almost there!

http://codereview.chromium.org/2962018/diff/11001/12002
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/2962018/diff/11001/12002#newcode176
chrome/browser/cocoa/tab_view.mm:176: // We don't care the workspace of
|dragWindow| because it may
s/care/care about? That's not technically true, we do care what it's workspace
is, but we shouldn't use the cache as it may change. Is that a better
interpretation?

http://codereview.chromium.org/2962018/diff/11001/12002#newcode988
chrome/browser/cocoa/tab_view.mm:988: - (int)getWorkspaceID:(NSWindow*)window
useCache:(BOOL)useCache {
comment for this method?

[David Benjamin, 2010-07-19 17:06:15]

http://codereview.chromium.org/2962018/diff/11001/12002
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/2962018/diff/11001/12002#newcode176
chrome/browser/cocoa/tab_view.mm:176: // We don't care the workspace of
|dragWindow| because it may
On 2010/07/19 13:32:47, pink wrote:
> s/care/care about? That's not technically true, we do care what it's workspace
> is, but we shouldn't use the cache as it may change. Is that a better
> interpretation?

Er... yeah. s/care/cache/ I think was what I meant to type.

http://codereview.chromium.org/2962018/diff/11001/12002#newcode988
chrome/browser/cocoa/tab_view.mm:988: - (int)getWorkspaceID:(NSWindow*)window
useCache:(BOOL)useCache {
On 2010/07/19 13:32:47, pink wrote:
> comment for this method?

Done.

[pink, 2010-07-19 17:14:35]

LGTM with comment fix. Who wants to try to land this?

http://codereview.chromium.org/2962018/diff/14002/16002
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/2962018/diff/14002/16002#newcode988
chrome/browser/cocoa/tab_view.mm:988: // Returns the workspace id of |window|.
If |useCache|, then we lookup
avoid the use of "we", it makes mark grumpy.

[David Benjamin, 2010-07-19 17:25:16]

> Who wants to try to land this?

Er, I should have bits to land it myself, or did you mean "It needs another
LGTM, who wants to okay it?"

http://codereview.chromium.org/2962018/diff/14002/16002
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/2962018/diff/14002/16002#newcode988
chrome/browser/cocoa/tab_view.mm:988: // Returns the workspace id of |window|.
If |useCache|, then we lookup
On 2010/07/19 17:14:35, pink wrote:
> avoid the use of "we", it makes mark grumpy.

...I've really got to do something about that habit. :-)

[pink, 2010-07-19 17:26:49]

On Mon, Jul 19, 2010 at 1:25 PM,  <davidben@chromium.org> wrote:
>> Who wants to try to land this?
>
> Er, I should have bits to land it myself, or did you mean "It needs another
> LGTM, who wants to okay it?"

Sorry, didn't catch you had a chromium account already.

-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com