Implement notifications for the DisplayBalloon method on Linux and Mac.

Implement notifications for the DisplayBalloon method on Linux and Mac.


BUG=74970
TEST=install background app on window, see desktop notification.

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

[Leandro Graciá Gil, 2011-10-24 17:55:53]

To be landed after http://codereview.chromium.org/8351004/ . Will rebase and
check the try-bots after that.

[Andrew T Wilson (Slow), 2011-10-26 01:21:18]

http://codereview.chromium.org/8375027/diff/14/chrome/browser/status_icons/de...
File chrome/browser/status_icons/desktop_notification_balloon_impl.cc (right):

http://codereview.chromium.org/8375027/diff/14/chrome/browser/status_icons/de...
chrome/browser/status_icons/desktop_notification_balloon_impl.cc:23: void
CloseBalloon(const std::string id) {
Is there a reason why this is broken out rather than just inline in the
destructor, or a private routine in DesktopNotificationBalloonImpl?

http://codereview.chromium.org/8375027/diff/14/chrome/browser/status_icons/de...
chrome/browser/status_icons/desktop_notification_balloon_impl.cc:75:
g_browser_process->notification_ui_manager()->Add(
This makes me somewhat nervous - can you have miranda take a look at this to
make sure this is OK? We don't care what profile we're using here - we just want
to get *any* profile, and surely there must be one loaded in memory already so
we don't have to do IO?

http://codereview.chromium.org/8375027/diff/14/chrome/browser/status_icons/de...
File chrome/browser/status_icons/desktop_notification_balloon_impl.h (right):

http://codereview.chromium.org/8375027/diff/14/chrome/browser/status_icons/de...
chrome/browser/status_icons/desktop_notification_balloon_impl.h:19: class
DesktopNotificationBalloonImpl : public StatusIcon {
Can this be a separate helper class? Not sure this should be a derived class
from StatusIcon.

http://codereview.chromium.org/8375027/diff/14/chrome/browser/status_icons/de...
chrome/browser/status_icons/desktop_notification_balloon_impl.h:24: virtual void
DisplayBalloon(const SkBitmap& icon,
Not sure that the current implementation of this routine is correct - the
existing windows implementation closes the balloon after a few seconds, but this
implementation leaves the window on the screen forever?

http://codereview.chromium.org/8375027/diff/14/chrome/browser/ui/cocoa/status...
File chrome/browser/ui/cocoa/status_icons/status_icon_mac.h (right):

http://codereview.chromium.org/8375027/diff/14/chrome/browser/ui/cocoa/status...
chrome/browser/ui/cocoa/status_icons/status_icon_mac.h:20: class StatusIconMac :
public DesktopNotificationBalloonImpl {
See my previous note - I think this should be a helper class, not a base class
we derive from.

[Leandro Graciá Gil, 2011-10-26 13:51:59]

http://codereview.chromium.org/8375027/diff/14/chrome/browser/status_icons/de...
File chrome/browser/status_icons/desktop_notification_balloon_impl.cc (right):

http://codereview.chromium.org/8375027/diff/14/chrome/browser/status_icons/de...
chrome/browser/status_icons/desktop_notification_balloon_impl.cc:23: void
CloseBalloon(const std::string id) {
This comes from a previous testing version that implemented a timeout (it posted
a delayed task to this on startup). This shouldn't be required anymore unless
you think timeout should go back.

On 2011/10/26 01:21:18, Andrew T Wilson wrote:
> Is there a reason why this is broken out rather than just inline in the
> destructor, or a private routine in DesktopNotificationBalloonImpl?

http://codereview.chromium.org/8375027/diff/14/chrome/browser/status_icons/de...
chrome/browser/status_icons/desktop_notification_balloon_impl.cc:75:
g_browser_process->notification_ui_manager()->Add(
On 2011/10/26 01:21:18, Andrew T Wilson wrote:
> This makes me somewhat nervous - can you have miranda take a look at this to
> make sure this is OK? We don't care what profile we're using here - we just
want
> to get *any* profile, and surely there must be one loaded in memory already so
> we don't have to do IO?

Done.

http://codereview.chromium.org/8375027/diff/14/chrome/browser/status_icons/de...
File chrome/browser/status_icons/desktop_notification_balloon_impl.h (right):

http://codereview.chromium.org/8375027/diff/14/chrome/browser/status_icons/de...
chrome/browser/status_icons/desktop_notification_balloon_impl.h:19: class
DesktopNotificationBalloonImpl : public StatusIcon {
On 2011/10/26 01:21:18, Andrew T Wilson wrote:
> Can this be a separate helper class? Not sure this should be a derived class
> from StatusIcon.

Done.

http://codereview.chromium.org/8375027/diff/14/chrome/browser/status_icons/de...
chrome/browser/status_icons/desktop_notification_balloon_impl.h:24: virtual void
DisplayBalloon(const SkBitmap& icon,
It leaves the notification in the screen until the user closes it manually
(there's a close button on it). However, implementing a timeout is very easy for
this case and I it can be done if you think it will be better. I decided to
leave it open to make sure the user was fully aware of it, otherwise it might be
a problem depending on the length of the notification contents.


On 2011/10/26 01:21:18, Andrew T Wilson wrote:
> Not sure that the current implementation of this routine is correct - the
> existing windows implementation closes the balloon after a few seconds, but
this
> implementation leaves the window on the screen forever?

http://codereview.chromium.org/8375027/diff/14/chrome/browser/ui/cocoa/status...
File chrome/browser/ui/cocoa/status_icons/status_icon_mac.h (right):

http://codereview.chromium.org/8375027/diff/14/chrome/browser/ui/cocoa/status...
chrome/browser/ui/cocoa/status_icons/status_icon_mac.h:20: class StatusIconMac :
public DesktopNotificationBalloonImpl {
On 2011/10/26 01:21:18, Andrew T Wilson wrote:
> See my previous note - I think this should be a helper class, not a base class
> we derive from.

Done.

[Andrew T Wilson (Slow), 2011-10-26 22:20:26]

http://codereview.chromium.org/8375027/diff/8003/chrome/browser/status_icons/...
File chrome/browser/status_icons/desktop_notification_balloon.cc (right):

http://codereview.chromium.org/8375027/diff/8003/chrome/browser/status_icons/...
chrome/browser/status_icons/desktop_notification_balloon.cc:32: virtual
std::string id() const OVERRIDE { return kNotificationPrefix + id_; }
Rather than having to regenerate a new string every time id() is called, perhaps
you should change the constructor to be:

: id_(kNotificationPrefix + id_) {}

Then this routine can just return id_

http://codereview.chromium.org/8375027/diff/8003/chrome/browser/ui/cocoa/stat...
File chrome/browser/ui/cocoa/status_icons/status_icon_mac.mm (right):

http://codereview.chromium.org/8375027/diff/8003/chrome/browser/ui/cocoa/stat...
chrome/browser/ui/cocoa/status_icons/status_icon_mac.mm:82:
notification_.DisplayBalloon(icon, title, contents);
I think we need to have identical behavior across platforms - since the windows
notification balloon auto-closes, we should auto-close these balloons as well.

[Leandro Graciá Gil, 2011-10-26 23:10:02]

http://codereview.chromium.org/8375027/diff/8003/chrome/browser/status_icons/...
File chrome/browser/status_icons/desktop_notification_balloon.cc (right):

http://codereview.chromium.org/8375027/diff/8003/chrome/browser/status_icons/...
chrome/browser/status_icons/desktop_notification_balloon.cc:32: virtual
std::string id() const OVERRIDE { return kNotificationPrefix + id_; }
On 2011/10/26 22:20:26, Andrew T Wilson wrote:
> Rather than having to regenerate a new string every time id() is called,
perhaps
> you should change the constructor to be:
> 
> : id_(kNotificationPrefix + id_) {}
> 
> Then this routine can just return id_

Done.

http://codereview.chromium.org/8375027/diff/8003/chrome/browser/ui/cocoa/stat...
File chrome/browser/ui/cocoa/status_icons/status_icon_mac.mm (right):

http://codereview.chromium.org/8375027/diff/8003/chrome/browser/ui/cocoa/stat...
chrome/browser/ui/cocoa/status_icons/status_icon_mac.mm:82:
notification_.DisplayBalloon(icon, title, contents);
On 2011/10/26 22:20:26, Andrew T Wilson wrote:
> I think we need to have identical behavior across platforms - since the
windows
> notification balloon auto-closes, we should auto-close these balloons as well.

Done.

[Andrew T Wilson (Slow), 2011-10-27 01:01:40]

Looks good, except for the bit about allowing IO on the UI thread. What did
Miranda have to say about that?

http://codereview.chromium.org/8375027/diff/15002/chrome/browser/status_icons...
File chrome/browser/status_icons/desktop_notification_balloon.cc (right):

http://codereview.chromium.org/8375027/diff/15002/chrome/browser/status_icons...
chrome/browser/status_icons/desktop_notification_balloon.cc:29: const size_t
kTimeoutMiliseconds = 6000;
nit: kTimeoutMilliseconds (not 'miliseconds')

[Leandro Graciá Gil, 2011-10-27 11:38:03]

Miranda suggested to use ProfileManager::GetLastUsedProfile(), which retrieves
the last used profile from memory (and therefore requires no IO access) instead
of ProfileManager::GetDefaultProfile which indeed requires IO access.

This should avoid accessing IO from the UI thread for all practical uses.
However, there's still the corner case where there is no last profile, in which
case GetLastUsedProfile will load and return the default profile. Basically,
allowing IO here is only a corner case safeguard. I've tested myself to remove
IO access in normal use cases and it still works.

http://codereview.chromium.org/8375027/diff/15002/chrome/browser/status_icons...
File chrome/browser/status_icons/desktop_notification_balloon.cc (right):

http://codereview.chromium.org/8375027/diff/15002/chrome/browser/status_icons...
chrome/browser/status_icons/desktop_notification_balloon.cc:29: const size_t
kTimeoutMiliseconds = 6000;
On 2011/10/27 01:01:41, Andrew T Wilson wrote:
> nit: kTimeoutMilliseconds (not 'miliseconds')

Done.

[Andrew T Wilson (Slow), 2011-10-27 17:19:40]

LGTM, since Miranda approves.