Implement the status tray/icon API for ChromeOS.

Implement the status tray/icon API for ChromeOS.


BUG=74970
TEST=status_tray_chromeos_unittest

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

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

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

[stevenjb, 2011-11-14 19:03:22]

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
File chrome/browser/ui/views/status_icons/status_icon_chromeos.cc (right):

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.cc:23:
BrowserView::GetBrowserViewForBrowser(browser));
We should really put this in chromeos/frame/browser_view.cc to make it more
clear that this is a safe cast, i.e. add
chromeos::BrowserView::GetBrowserViewForBrowser()

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.cc:36:
StatusAreaButton::Delegate* delegate)
nit: align parameters

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.cc:77: return;
What is the purpose of the if statement here?

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.cc:87: // Since the
target of this API is to provide notification as much
This sentence isn't clear to me.

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.cc:133:
DCHECK(tray_button_map_.find(browser_view) == tray_button_map_.end());
We should probably do more than a DCHECK here, since a spurious notification
could cause a leak,  e.g. if (...) { NOTREACHED() ; return; }

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
File chrome/browser/ui/views/status_icons/status_icon_chromeos.h (right):

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.h:50: void Clicked();
nit: name something like TestClicked() so that the testing nature is in the
function name (or protect and make the testing class a friend)

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.h:64: TrayButtonMap;
nit: typedef at top of section

[Andrew T Wilson (Slow), 2011-11-14 23:46:03]

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/chromeos/fram...
File chrome/browser/chromeos/frame/browser_view.cc (right):

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/browser_view.cc:338: button->SchedulePaint();
This seems like an odd place to have to force the button non-visible and calling
SchedulePaint(). Perhaps status_area_->RemoveButton() ought to be doing this?

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
File chrome/browser/ui/views/status_icons/status_icon_chromeos.cc (right):

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.cc:90: for
(BrowserList::BrowserVector::const_iterator it = BrowserList::begin();
Just to verify - do we want this on all browser windows (including incognito?)

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
File chrome/browser/ui/views/status_icons/status_tray_chromeos_unittest.cc
(right):

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_tray_chromeos_unittest.cc:40: }
Is there any way for us to test that the appropriate button gets added to all
browser windows? I think all of the work to actually create browser window icons
is skipped in these tests since BrowserList is empty.

[Leandro Graciá Gil, 2011-11-15 22:23:31]

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/chromeos/fram...
File chrome/browser/chromeos/frame/browser_view.cc (right):

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/browser_view.cc:338: button->SchedulePaint();
It looks like this is not required anymore. Removed.

On 2011/11/14 23:46:03, Andrew T Wilson wrote:
> This seems like an odd place to have to force the button non-visible and
calling
> SchedulePaint(). Perhaps status_area_->RemoveButton() ought to be doing this?

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
File chrome/browser/ui/views/status_icons/status_icon_chromeos.cc (right):

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.cc:23:
BrowserView::GetBrowserViewForBrowser(browser));
On 2011/11/14 19:03:22, Steven Bennetts wrote:
> We should really put this in chromeos/frame/browser_view.cc to make it more
> clear that this is a safe cast, i.e. add
> chromeos::BrowserView::GetBrowserViewForBrowser()

Done.

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.cc:36:
StatusAreaButton::Delegate* delegate)
On 2011/11/14 19:03:22, Steven Bennetts wrote:
> nit: align parameters

Done.

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.cc:77: return;
On 2011/11/14 19:03:22, Steven Bennetts wrote:
> What is the purpose of the if statement here?

RunMenuAt is defined using WARN_UNUSED_RESULT. So, this if is redundant but
preventing a warning/error to be generated. This is following the example of
other codes like BalloonViewImpl::RunMenu.

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.cc:87: // Since the
target of this API is to provide notification as much
On 2011/11/14 19:03:22, Steven Bennetts wrote:
> This sentence isn't clear to me.

Done.

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.cc:90: for
(BrowserList::BrowserVector::const_iterator it = BrowserList::begin();
On 2011/11/14 23:46:03, Andrew T Wilson wrote:
> Just to verify - do we want this on all browser windows (including incognito?)

Yes, I have discussed this with the Chrome OS UI designers and they are fine
with it. Since there is no system tray and opening a new window occupies the
full screen this is the best way to ensure it's visible.

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.cc:133:
DCHECK(tray_button_map_.find(browser_view) == tray_button_map_.end());
On 2011/11/14 19:03:22, Steven Bennetts wrote:
> We should probably do more than a DCHECK here, since a spurious notification
> could cause a leak,  e.g. if (...) { NOTREACHED() ; return; }

Done.

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
File chrome/browser/ui/views/status_icons/status_icon_chromeos.h (right):

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.h:50: void Clicked();
On 2011/11/14 19:03:22, Steven Bennetts wrote:
> nit: name something like TestClicked() so that the testing nature is in the
> function name (or protect and make the testing class a friend)

Done.

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_icon_chromeos.h:64: TrayButtonMap;
On 2011/11/14 19:03:22, Steven Bennetts wrote:
> nit: typedef at top of section

Done.

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
File chrome/browser/ui/views/status_icons/status_tray_chromeos_unittest.cc
(right):

http://codereview.chromium.org/8476003/diff/4001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_tray_chromeos_unittest.cc:40: }
On 2011/11/14 23:46:03, Andrew T Wilson wrote:
> Is there any way for us to test that the appropriate button gets added to all
> browser windows? I think all of the work to actually create browser window
icons
> is skipped in these tests since BrowserList is empty.

Good point. I guess we need to make this a browser test instead of a unit test.

[Andrew T Wilson (Slow), 2011-11-16 00:28:54]

This is pretty close - I just had a few more questions. Thanks for putting this
together!

http://codereview.chromium.org/8476003/diff/8001/chrome/browser/chromeos/fram...
File chrome/browser/chromeos/frame/browser_view.cc (right):

http://codereview.chromium.org/8476003/diff/8001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/browser_view.cc:341: return
static_cast<BrowserView*>(::BrowserView::GetBrowserViewForBrowser(
I'm sure I'm being stupid here - does this call itself? I'm confused about what
this is doing?

http://codereview.chromium.org/8476003/diff/8001/chrome/browser/ui/views/stat...
File chrome/browser/ui/views/status_icons/status_tray_chromeos_browsertest.cc
(right):

http://codereview.chromium.org/8476003/diff/8001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_tray_chromeos_browsertest.cc:44:
icon->SetContextMenu(menu);
This is just checking to make sure we don't crash - is there any way to actually
make sure an icon is displayed in the BrowserView?

http://codereview.chromium.org/8476003/diff/8001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_tray_chromeos_browsertest.cc:56:
icon->TestClicked();
Instead of exposing TestClicked(), is it possible to find the button that was
added to the BrowserView, and force a click event on it? I think that'd be a
better test.

http://codereview.chromium.org/8476003/diff/11001/chrome/browser/ui/views/sta...
File chrome/browser/ui/views/status_icons/status_tray_linux.cc (right):

http://codereview.chromium.org/8476003/diff/11001/chrome/browser/ui/views/sta...
chrome/browser/ui/views/status_icons/status_tray_linux.cc:7: #if
!defined(OS_CHROMEOS)
Is this required? I thought that gyp would include the chromeos.cc file if we
are compiling the chromeos version, so there's no need to #if here?

http://codereview.chromium.org/8476003/diff/11001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8476003/diff/11001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:2765:
'browser/ui/browser_navigator_chromeos_browsertest.cc',
Why is this added here?

[Leandro Graciá Gil, 2011-11-17 16:18:48]

http://codereview.chromium.org/8476003/diff/8001/chrome/browser/chromeos/fram...
File chrome/browser/chromeos/frame/browser_view.cc (right):

http://codereview.chromium.org/8476003/diff/8001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/browser_view.cc:341: return
static_cast<BrowserView*>(::BrowserView::GetBrowserViewForBrowser(
On 2011/11/16 00:28:54, Andrew T Wilson wrote:
> I'm sure I'm being stupid here - does this call itself? I'm confused about
what
> this is doing?

Looks tricky, but it's just calling the BrowserView::GetBrowserViewForBrowser
method in the global namespace. Check
chrome/browser/ui/views/frame/browser_view.h.

http://codereview.chromium.org/8476003/diff/8001/chrome/browser/ui/views/stat...
File chrome/browser/ui/views/status_icons/status_tray_chromeos_browsertest.cc
(right):

http://codereview.chromium.org/8476003/diff/8001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_tray_chromeos_browsertest.cc:44:
icon->SetContextMenu(menu);
On 2011/11/16 00:28:54, Andrew T Wilson wrote:
> This is just checking to make sure we don't crash - is there any way to
actually
> make sure an icon is displayed in the BrowserView?

I'm not sure about displayed, but we should be able to check if the button is
contained as a child view of the status area. I'm implementing it.

http://codereview.chromium.org/8476003/diff/8001/chrome/browser/ui/views/stat...
chrome/browser/ui/views/status_icons/status_tray_chromeos_browsertest.cc:56:
icon->TestClicked();
On 2011/11/16 00:28:54, Andrew T Wilson wrote:
> Instead of exposing TestClicked(), is it possible to find the button that was
> added to the BrowserView, and force a click event on it? I think that'd be a
> better test.

I can reverse the way it's done. The button's Activate method is called when
clicked. Instead of calling TestClicked from it, I can add a testing method to
invoke Activate. It's still not a real click but at least it will follow the
same callback flow.

http://codereview.chromium.org/8476003/diff/11001/chrome/browser/ui/views/sta...
File chrome/browser/ui/views/status_icons/status_tray_linux.cc (right):

http://codereview.chromium.org/8476003/diff/11001/chrome/browser/ui/views/sta...
chrome/browser/ui/views/status_icons/status_tray_linux.cc:7: #if
!defined(OS_CHROMEOS)
On 2011/11/16 00:28:54, Andrew T Wilson wrote:
> Is this required? I thought that gyp would include the chromeos.cc file if we
> are compiling the chromeos version, so there's no need to #if here?

This is to avoid problems with the linux aura bot. It was actually a suggestion
from Steven.

http://codereview.chromium.org/8476003/diff/11001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8476003/diff/11001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:2765:
'browser/ui/browser_navigator_chromeos_browsertest.cc',
On 2011/11/16 00:28:54, Andrew T Wilson wrote:
> Why is this added here?

Oops, that's a wrong paste. I meant to put status_tray_chromeos_browsertest.cc
here.

[stevenjb, 2011-11-17 20:49:53]

http://codereview.chromium.org/8476003/diff/14002/chrome/browser/chromeos/fra...
File chrome/browser/chromeos/frame/browser_view.cc (right):

http://codereview.chromium.org/8476003/diff/14002/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/browser_view.cc:346: browser));
Even though this is already in the chromeos:: namespace, let's add chromeos::
before BrowserView here to make it clear what is going on and why this is
necessary, i.e.:

chromeos::BrowserView* BrowserView::GetBrowserViewForBrowser(Browser* browser) {
   return static_cast<chromeos::BrowserView*>(
      ::BrowserView::GetBrowserViewForBrowser(browser));
}

http://codereview.chromium.org/8476003/diff/14002/chrome/browser/ui/views/sta...
File chrome/browser/ui/views/status_icons/status_icon_chromeos.h (right):

http://codereview.chromium.org/8476003/diff/14002/chrome/browser/ui/views/sta...
chrome/browser/ui/views/status_icons/status_icon_chromeos.h:58: class
NotificationTrayButton;
nit: class forward declarations should be at top of the file, not inside the
class.

[Leandro Graciá Gil, 2011-11-17 20:56:20]

http://codereview.chromium.org/8476003/diff/14002/chrome/browser/chromeos/fra...
File chrome/browser/chromeos/frame/browser_view.cc (right):

http://codereview.chromium.org/8476003/diff/14002/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/browser_view.cc:346: browser));
On 2011/11/17 20:49:53, Steven Bennetts wrote:
> Even though this is already in the chromeos:: namespace, let's add chromeos::
> before BrowserView here to make it clear what is going on and why this is
> necessary, i.e.:
> 
> chromeos::BrowserView* BrowserView::GetBrowserViewForBrowser(Browser* browser)
{
>    return static_cast<chromeos::BrowserView*>(
>       ::BrowserView::GetBrowserViewForBrowser(browser));
> }

Done.

http://codereview.chromium.org/8476003/diff/14002/chrome/browser/ui/views/sta...
File chrome/browser/ui/views/status_icons/status_icon_chromeos.h (right):

http://codereview.chromium.org/8476003/diff/14002/chrome/browser/ui/views/sta...
chrome/browser/ui/views/status_icons/status_icon_chromeos.h:58: class
NotificationTrayButton;
On 2011/11/17 20:49:53, Steven Bennetts wrote:
> nit: class forward declarations should be at top of the file, not inside the
> class.

In this case it's an internal class that needs to be forward declared for the
map below.

[stevenjb, 2011-11-17 20:57:23]

lgtm

[Andrew T Wilson (Slow), 2011-11-17 21:32:31]

3 more comments - LGTM once you've addressed them.

http://codereview.chromium.org/8476003/diff/21004/chrome/browser/chromeos/fra...
File chrome/browser/chromeos/frame/browser_view.cc (right):

http://codereview.chromium.org/8476003/diff/21004/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/browser_view.cc:346:
::BrowserView::GetBrowserViewForBrowser(browser));
Can you add a comment here about what exactly this calls? My preference would be
to call this method a different name so it's not shadowing the version in the
global namespace, but if this is the pattern the chromeos folks want, then at
least comment it so readers know where the call is going.

http://codereview.chromium.org/8476003/diff/21004/chrome/browser/ui/views/sta...
File chrome/browser/ui/views/status_icons/status_icon_chromeos.h (right):

http://codereview.chromium.org/8476003/diff/21004/chrome/browser/ui/views/sta...
chrome/browser/ui/views/status_icons/status_icon_chromeos.h:50: bool
TestIsIconContainedInView();
One nit: I don't typically like having test logic in the class being tested.
Instead of exposing these, can you instead expose just a single function:

NotificationTrayButton* GetNotificationTrayButtonForCurrentBrowser()

then move the other logic (Activate(), ContainsButton()) into the tests itself?

http://codereview.chromium.org/8476003/diff/21004/chrome/browser/ui/views/sta...
File chrome/browser/ui/views/status_icons/status_tray_linux.cc (right):

http://codereview.chromium.org/8476003/diff/21004/chrome/browser/ui/views/sta...
chrome/browser/ui/views/status_icons/status_tray_linux.cc:7: #if
!defined(OS_CHROMEOS)
I have no strong objections to this, but it's somewhat troubling to me that
we're adding this here when it shouldn't (in theory) be needed, as that may
indicate we're not properly setting up our .gyp file. Can you please confirm
it's necessary?

[Leandro Graciá Gil, 2011-11-17 22:42:45]

http://codereview.chromium.org/8476003/diff/21004/chrome/browser/chromeos/fra...
File chrome/browser/chromeos/frame/browser_view.cc (right):

http://codereview.chromium.org/8476003/diff/21004/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/browser_view.cc:346:
::BrowserView::GetBrowserViewForBrowser(browser));
On 2011/11/17 21:32:31, Andrew T Wilson wrote:
> Can you add a comment here about what exactly this calls? My preference would
be
> to call this method a different name so it's not shadowing the version in the
> global namespace, but if this is the pattern the chromeos folks want, then at
> least comment it so readers know where the call is going.

Done.

http://codereview.chromium.org/8476003/diff/21004/chrome/browser/ui/views/sta...
File chrome/browser/ui/views/status_icons/status_icon_chromeos.h (right):

http://codereview.chromium.org/8476003/diff/21004/chrome/browser/ui/views/sta...
chrome/browser/ui/views/status_icons/status_icon_chromeos.h:50: bool
TestIsIconContainedInView();
On 2011/11/17 21:32:31, Andrew T Wilson wrote:
> One nit: I don't typically like having test logic in the class being tested.
> Instead of exposing these, can you instead expose just a single function:
> 
> NotificationTrayButton* GetNotificationTrayButtonForCurrentBrowser()
> 
> then move the other logic (Activate(), ContainsButton()) into the tests
itself?

Done.

http://codereview.chromium.org/8476003/diff/21004/chrome/browser/ui/views/sta...
File chrome/browser/ui/views/status_icons/status_tray_linux.cc (right):

http://codereview.chromium.org/8476003/diff/21004/chrome/browser/ui/views/sta...
chrome/browser/ui/views/status_icons/status_tray_linux.cc:7: #if
!defined(OS_CHROMEOS)
On 2011/11/17 21:32:31, Andrew T Wilson wrote:
> I have no strong objections to this, but it's somewhat troubling to me that
> we're adding this here when it shouldn't (in theory) be needed, as that may
> indicate we're not properly setting up our .gyp file. Can you please confirm
> it's necessary?

This was checked this with Steven because of link errors with the linux aura
bot. This seems to be the simplest way to solve the problem.

[Andrew T Wilson (Slow), 2011-11-17 22:50:35]

LGTM

http://codereview.chromium.org/8476003/diff/23001/chrome/browser/ui/views/sta...
File chrome/browser/ui/views/status_icons/status_tray_chromeos_browsertest.cc
(right):

http://codereview.chromium.org/8476003/diff/23001/chrome/browser/ui/views/sta...
chrome/browser/ui/views/status_icons/status_tray_chromeos_browsertest.cc:50:
ASSERT_TRUE(browser);
Small nit: change these to ASSERT_TRUE(blah != NULL) since that seems to be the
chrome style.

[Leandro Graciá Gil, 2011-11-17 23:19:46]

Landing shortly with the nits fixed.

http://codereview.chromium.org/8476003/diff/23001/chrome/browser/ui/views/sta...
File chrome/browser/ui/views/status_icons/status_tray_chromeos_browsertest.cc
(right):

http://codereview.chromium.org/8476003/diff/23001/chrome/browser/ui/views/sta...
chrome/browser/ui/views/status_icons/status_tray_chromeos_browsertest.cc:50:
ASSERT_TRUE(browser);
On 2011/11/17 22:50:35, Andrew T Wilson wrote:
> Small nit: change these to ASSERT_TRUE(blah != NULL) since that seems to be
the
> chrome style.

Done.

[Nikita (slow), 2011-11-21 13:34:05]

http://codereview.chromium.org/8476003/diff/23001/chrome/browser/ui/views/sta...
File chrome/browser/ui/views/status_icons/status_icon_chromeos.cc (right):

http://codereview.chromium.org/8476003/diff/23001/chrome/browser/ui/views/sta...
chrome/browser/ui/views/status_icons/status_icon_chromeos.cc:19: class
StatusIconChromeOS::NotificationTrayButton
nit: Please add missing class level comment in next CLs.
There're now StatusIcon / StatusAreaButton entities so it would be good to
understand quickly where each one is used.