Initial implementation of status tray functionality (mac-only, currently).

Initial implementation of status tray functionality (mac-only, currently).

Added Mac implementation of StatusIcon, and added a simple click callback that
displays the "extensions" tab when clicked on, to allow us to dogfood long-lived
extensions.

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

[tfarina (gmail-do not use), 2010-03-03 13:08:11]

Is there a bug filed for this (for the other platforms)? I couldn't find one at
crbug.com

-Thiago

[John Gregg, 2010-03-03 18:11:09]

preliminary comments...

http://codereview.chromium.org/661454/diff/1/10
File chrome/browser/cocoa/status_icons/status_icon_mac.mm (right):

http://codereview.chromium.org/661454/diff/1/10#newcode16
chrome/browser/cocoa/status_icons/status_icon_mac.mm:16: @end // @interface
StatusItemController
StatusItemController is simple, but it might still be worth its own file.

http://codereview.chromium.org/661454/diff/1/10#newcode27
chrome/browser/cocoa/status_icons/status_icon_mac.mm:27:
statusIcon_->HandleClick();
if() or DCHECK for statusIcon_ ?

http://codereview.chromium.org/661454/diff/1/10#newcode33
chrome/browser/cocoa/status_icons/status_icon_mac.mm:33: : item_(NULL) {
indent 4 for :

http://codereview.chromium.org/661454/diff/1/10#newcode48
chrome/browser/cocoa/status_icons/status_icon_mac.mm:48: [item_
setEnabled:true];
i think style wise it should be YES here

http://codereview.chromium.org/661454/diff/1/10#newcode83
chrome/browser/cocoa/status_icons/status_icon_mac.mm:83: for
(std::vector<StatusIcon::StatusIconListener*>::const_iterator iter =
Just for verbosity, do you need the StatusIcon:: qualifier every time given that
you're in StatusIconMac, which is a StatusIcon?

http://codereview.chromium.org/661454/diff/1/12
File chrome/browser/status_icons/status_tray.cc (right):

http://codereview.chromium.org/661454/diff/1/12#newcode20
chrome/browser/status_icons/status_tray.cc:20: ++iter)
there is an stl-util method you want here.

http://codereview.chromium.org/661454/diff/1/12#newcode36
chrome/browser/status_icons/status_tray.cc:36: return 0;
NOTIMPLEMENTED() would be safe, right, because it's behind a command line flag?

http://codereview.chromium.org/661454/diff/1/12#newcode44
chrome/browser/status_icons/status_tray.cc:44: delete iter->second;
i've been advised in the past to put the pointer in a scoped_ptr local to the
current function, rather than calling delete.

http://codereview.chromium.org/661454/diff/1/13
File chrome/browser/status_icons/status_tray.h (right):

http://codereview.chromium.org/661454/diff/1/13#newcode23
chrome/browser/status_icons/status_tray.h:23: // a new one if none exists.
Should add to the comment that the StatusTray class retains ownership of the
created object.

http://codereview.chromium.org/661454/diff/1/13#newcode31
chrome/browser/status_icons/status_tray.h:31: typedef base::hash_map<string16,
StatusIcon*> StatusIconMap;
comment needed - what do the identifiers / keys of this map represent?  just
anything that can show a status icon?

[John Gregg, 2010-03-03 19:04:17]

http://codereview.chromium.org/661454/diff/1/10
File chrome/browser/cocoa/status_icons/status_icon_mac.mm (right):

http://codereview.chromium.org/661454/diff/1/10#newcode16
chrome/browser/cocoa/status_icons/status_icon_mac.mm:16: @end // @interface
StatusItemController
On 2010/03/03 18:11:09, John Gregg wrote:
> StatusItemController is simple, but it might still be worth its own file.

I take this one back, there's no need to expose it outside this scope.

http://codereview.chromium.org/661454/diff/1/10#newcode20
chrome/browser/cocoa/status_icons/status_icon_mac.mm:20: -
(id)init:(StatusIconMac*)icon {
also, this should be called initWithIcon

[Andrew T Wilson (Slow), 2010-03-03 22:32:02]

Not yet.

On Wed, Mar 3, 2010 at 5:08 AM, <thiago.farina@gmail.com> wrote:

> Is there a bug filed for this (for the other platforms)? I couldn't find
> one at
> crbug.com
>
> -Thiago
>
> http://codereview.chromium.org/661454
>

[Andrew T Wilson (Slow), 2010-03-04 02:06:23]

http://codereview.chromium.org/661454/diff/1/12
File chrome/browser/status_icons/status_tray.cc (right):

http://codereview.chromium.org/661454/diff/1/12#newcode36
chrome/browser/status_icons/status_tray.cc:36: return 0;
On 2010/03/03 18:11:09, John Gregg wrote:
> NOTIMPLEMENTED() would be safe, right, because it's behind a command line
flag?
I was trying to have non-implemented platforms continue working (that way people
could test long-lived extensions on Linux even though the status bar icons might
not display). I've updated the docs to reflect that GetStatusIcon can return
NULL.

http://codereview.chromium.org/661454/diff/1/12#newcode44
chrome/browser/status_icons/status_tray.cc:44: delete iter->second;
On 2010/03/03 18:11:09, John Gregg wrote:
> i've been advised in the past to put the pointer in a scoped_ptr local to the
> current function, rather than calling delete.
OK, I've gritted my teeth and made that change :)

[Nico, 2010-03-04 02:10:27]

On 2010/03/03 22:32:02, Andrew T Wilson wrote:
> Not yet.

Can you file a bug and explain in some detail what this is supposed to do?
("some detail" being like one 6-line paragraph).

Can you upload a screenshot to imgur.com and post a link to that in a reply in
this review?

[Nico, 2010-03-04 02:22:07]

Battery empty, I'll finish this when I get home.

Generally, this looks pretty good so far. First round of minor comments below.

http://codereview.chromium.org/661454/diff/4001/4009
File chrome/browser/cocoa/status_icons/status_icon_mac.h (right):

http://codereview.chromium.org/661454/diff/4001/4009#newcode10
chrome/browser/cocoa/status_icons/status_icon_mac.h:10: #include
"chrome/browser/status_icons/status_icon.h"
put this into the common include block

http://codereview.chromium.org/661454/diff/4001/4009#newcode13
chrome/browser/cocoa/status_icons/status_icon_mac.h:13: #include
"base/ref_counted.h"
you don't need this include

http://codereview.chromium.org/661454/diff/4001/4009#newcode15
chrome/browser/cocoa/status_icons/status_icon_mac.h:15: #include
"base/scoped_vector.h"
you don't need this include

http://codereview.chromium.org/661454/diff/4001/4009#newcode17
chrome/browser/cocoa/status_icons/status_icon_mac.h:17: #include
"third_party/skia/include/core/SkBitmap.h"
"class SkBitmap;" instead

http://codereview.chromium.org/661454/diff/4001/4009#newcode19
chrome/browser/cocoa/status_icons/status_icon_mac.h:19: @class NSStatusItem,
StatusItemController;
one line per @class

http://codereview.chromium.org/661454/diff/4001/4010
File chrome/browser/cocoa/status_icons/status_icon_mac.mm (right):

http://codereview.chromium.org/661454/diff/4001/4010#newcode35
chrome/browser/cocoa/status_icons/status_icon_mac.mm:35:
controller_.reset([[[StatusItemController alloc] initWithIcon:this] retain]);
alloc returns a retained object, having another retain makes this elak

[Andrew T Wilson (Slow), 2010-03-04 02:38:35]

Logged bug: http://crbug.com/37375

[Nico, 2010-03-04 04:05:38]

Is the new status code actually called anywhere in this CL? No, right?

The new files are missing tests.

http://codereview.chromium.org/661454/diff/4001/4010
File chrome/browser/cocoa/status_icons/status_icon_mac.mm (right):

http://codereview.chromium.org/661454/diff/4001/4010#newcode48
chrome/browser/cocoa/status_icons/status_icon_mac.mm:48:
statusItemWithLength:NSSquareStatusItemLength] retain]);
this retain you do need.

The rule is: all method names that contain "alloc" or "copy" return retained
objects. Others don't.

http://codereview.chromium.org/661454/diff/4001/4010#newcode51
chrome/browser/cocoa/status_icons/status_icon_mac.mm:51: [item_
setAction:@selector(handleClick)];
action methods have to get a single argument of type id (the sender).
|handleClick| is not a valid action method as is.

http://codereview.chromium.org/661454/diff/4001/4010#newcode56
chrome/browser/cocoa/status_icons/status_icon_mac.mm:56: void
StatusIconMac::SetImage(const SkBitmap& bitmap) {
Do you need to check if bitmap.isNull()?

http://codereview.chromium.org/661454/diff/4001/4011
File chrome/browser/status_icons/status_icon.h (right):

http://codereview.chromium.org/661454/diff/4001/4011#newcode8
chrome/browser/status_icons/status_icon.h:8: #include "base/basictypes.h"
not needed

http://codereview.chromium.org/661454/diff/4001/4011#newcode9
chrome/browser/status_icons/status_icon.h:9: #include "base/ref_counted.h"
not needed

http://codereview.chromium.org/661454/diff/4001/4011#newcode11
chrome/browser/status_icons/status_icon.h:11: #include
"third_party/skia/include/core/SkBitmap.h"
class SkBitmap; instead

http://codereview.chromium.org/661454/diff/4001/4012
File chrome/browser/status_icons/status_tray.cc (right):

http://codereview.chromium.org/661454/diff/4001/4012#newcode44
chrome/browser/status_icons/status_tray.cc:44: scoped_ptr<StatusIcon>
ptr(iter->second);
isn't |delete iter->second;| a lot more readable here?

http://codereview.chromium.org/661454/diff/4001/4013
File chrome/browser/status_icons/status_tray.h (right):

http://codereview.chromium.org/661454/diff/4001/4013#newcode10
chrome/browser/status_icons/status_tray.h:10: #include "base/scoped_ptr.h"
include only what you actually need

http://codereview.chromium.org/661454/diff/4001/4013#newcode35
chrome/browser/status_icons/status_tray.h:35: // Value: The StatusIcon
associated with that identifier.
Say if these pointers are weak or strong

http://codereview.chromium.org/661454/diff/4001/4015
File chrome/browser/status_icons/status_tray_manager.h (right):

http://codereview.chromium.org/661454/diff/4001/4015#newcode11
chrome/browser/status_icons/status_tray_manager.h:11: #include
"chrome/browser/status_icons/status_icon.h"
too many includes

[Andrew T Wilson (Slow), 2010-03-04 20:02:41]

This code is called as part of browser_init (we create a StatusTrayManager,
which results in a StatusIcon being created if the kLongLivedExtensions command
line flag is present).

I've talked to johnnyg about how to write tests for the cocoa code, and I'll
upload tests shortly.

http://codereview.chromium.org/661454/diff/4001/4010
File chrome/browser/cocoa/status_icons/status_icon_mac.mm (right):

http://codereview.chromium.org/661454/diff/4001/4010#newcode48
chrome/browser/cocoa/status_icons/status_icon_mac.mm:48:
statusItemWithLength:NSSquareStatusItemLength] retain]);
On 2010/03/04 04:05:38, Nico wrote:
> this retain you do need.
> 
> The rule is: all method names that contain "alloc" or "copy" return retained
> objects. Others don't.

Thanks - that's a great rule of thumb I wasn't aware of.

http://codereview.chromium.org/661454/diff/4001/4012
File chrome/browser/status_icons/status_tray.cc (right):

http://codereview.chromium.org/661454/diff/4001/4012#newcode44
chrome/browser/status_icons/status_tray.cc:44: scoped_ptr<StatusIcon>
ptr(iter->second);
On 2010/03/04 04:05:38, Nico wrote:
> isn't |delete iter->second;| a lot more readable here?

I thought so, but my previous reviewer had me change it :-P

http://codereview.chromium.org/661454/diff/4001/4013
File chrome/browser/status_icons/status_tray.h (right):

http://codereview.chromium.org/661454/diff/4001/4013#newcode10
chrome/browser/status_icons/status_tray.h:10: #include "base/scoped_ptr.h"
On 2010/03/04 04:05:38, Nico wrote:
> include only what you actually need

Yeah, I refactored the code and neglected to update the #includes in several
places. thanks for catching this.

http://codereview.chromium.org/661454/diff/4001/4015
File chrome/browser/status_icons/status_tray_manager.h (right):

http://codereview.chromium.org/661454/diff/4001/4015#newcode11
chrome/browser/status_icons/status_tray_manager.h:11: #include
"chrome/browser/status_icons/status_icon.h"
On 2010/03/04 04:05:38, Nico wrote:
> too many includes
Gah, this is embarrassing :(

[Andrew T Wilson (Slow), 2010-03-05 18:06:31]

OK, unittests added, and it passes mac valgrind. Please take another look.

[Nico, 2010-03-05 18:22:35]

Nice tests! Almost there.

http://codereview.chromium.org/661454/diff/4040/5012
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/661454/diff/4040/5012#newcode394
chrome/browser/browser_init.cc:394: // TODO(atwilson): Status tray UI is
currently only supported on the mac.
Add

// http://crbug.com/# of bug you opened for this

http://codereview.chromium.org/661454/diff/4040/5017
File chrome/browser/cocoa/status_icons/status_icon_mac.mm (right):

http://codereview.chromium.org/661454/diff/4040/5017#newcode83
chrome/browser/cocoa/status_icons/status_icon_mac.mm:83: }
Use std...::iterator iter = std::find(listeners_.begin(), listeners_.end(),
listener); to make this shorter

http://codereview.chromium.org/661454/diff/4040/5018
File chrome/browser/cocoa/status_icons/status_icon_mac_unittest.mm (right):

http://codereview.chromium.org/661454/diff/4040/5018#newcode21
chrome/browser/cocoa/status_icons/status_icon_mac_unittest.mm:21: int
num_clicks_;
Have you used googlemock before? If not, I'd encourage you to read up on it and
consider using it here.

http://codereview.chromium.org/661454/diff/4040/5018#newcode38
chrome/browser/cocoa/status_icons/status_icon_mac_unittest.mm:38:
scoped_ptr<StatusIconMac> icon(new StatusIconMac());
Any reason this needs to be allocated on the heap?

http://codereview.chromium.org/661454/diff/4040/5024
File chrome/browser/status_icons/status_tray_unittest.cc (right):

http://codereview.chromium.org/661454/diff/4040/5024#newcode32
chrome/browser/status_icons/status_tray_unittest.cc:32: scoped_ptr<StatusTray>
tray(new StatusTray(factory));
again, could this live on the stack

[Andrew T Wilson (Slow), 2010-03-06 01:55:59]

OK, made those tweaks, and changed status_icon_mac_unittest.mm to use
googlemock.

[Nico, 2010-03-06 02:52:09]

LG. Some final nits below, but they don't need rereview.

http://codereview.chromium.org/661454/diff/4045/4058
File chrome/browser/status_icons/status_tray.h (right):

http://codereview.chromium.org/661454/diff/4045/4058#newcode23
chrome/browser/status_icons/status_tray.h:23: StatusTray(StatusIconFactory*
factory);
Add comment that says "takes ownership of |factory|"

http://codereview.chromium.org/661454/diff/4045/4059
File chrome/browser/status_icons/status_tray_manager.cc (right):

http://codereview.chromium.org/661454/diff/4045/4059#newcode42
chrome/browser/status_icons/status_tray_manager.cc:42: if (status_tray_.get()) {
new throws on oom (and our allocators exit on oom i think), so this if will
always be true

http://codereview.chromium.org/661454/diff/4045/4061
File chrome/browser/status_icons/status_tray_unittest.cc (right):

http://codereview.chromium.org/661454/diff/4045/4061#newcode22
chrome/browser/status_icons/status_tray_unittest.cc:22: return new
MockStatusIcon();
If you want, you can use gmock for this as well (.AndReturn()...)

http://codereview.chromium.org/661454/diff/4045/4061#newcode32
chrome/browser/status_icons/status_tray_unittest.cc:32: scoped_ptr<StatusTray>
tray(new StatusTray(factory));
can be on the stack

[tfarina (gmail-do not use), 2010-03-06 05:09:48]

http://codereview.chromium.org/661454/diff/4045/4053
File chrome/browser/cocoa/status_icons/status_icon_mac.h (right):

http://codereview.chromium.org/661454/diff/4045/4053#newcode44
chrome/browser/cocoa/status_icons/status_icon_mac.h:44:
std::vector<StatusIcon::StatusIconListener*> listeners_;
why not ObserverList<StatusIconObserver>?

http://codereview.chromium.org/661454/diff/4045/4056
File chrome/browser/status_icons/status_icon.h (right):

http://codereview.chromium.org/661454/diff/4045/4056#newcode25
chrome/browser/status_icons/status_icon.h:25: class StatusIconListener {
I would call this as StatusIconObserver.

http://codereview.chromium.org/661454/diff/4045/4056#newcode30
chrome/browser/status_icons/status_icon.h:30: virtual void Click() = 0;
And this as OnClicked

http://codereview.chromium.org/661454/diff/4045/4056#newcode34
chrome/browser/status_icons/status_icon.h:34: virtual void
AddListener(StatusIcon::StatusIconListener* listener) = 0;
Then this would be called as AddObserver(StatusIconObserver* observer)