Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(350)

Issue 10021026: [Mac] Implement a NotificationUIManager that uses Notification Center on 10.8 for text notifications (Closed)

Created:
8 years, 8 months ago by Robert Sesek
Modified:
8 years, 8 months ago
CC:
chromium-reviews, erikwright (departed), brettw-cc_chromium.org
Visibility:
Public.

Description

[Mac] Implement a NotificationUIManager that uses Notification Center on 10.8 for text notifications. BUG=120677 TEST=On 10.8, enable Gmail desktop notifications and chat with somebody. You see notifications and clicking them closes them, brings Chromium to the foreground, and removes them from the tray. TEST=On 10.8, try using a HTML-based notification and it gets displayed in the old style. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132611

Patch Set 1 #

Total comments: 13

Patch Set 2 : Address comments #

Total comments: 14

Patch Set 3 : Change notification_map_ #

Total comments: 10

Patch Set 4 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+447 lines, -21 lines) Patch
M base/mac/cocoa_protocols.h View 1 2 3 2 chunks +12 lines, -5 lines 0 comments Download
M chrome/browser/notifications/notification_ui_manager.h View 1 chunk +3 lines, -0 lines 0 comments Download
A chrome/browser/notifications/notification_ui_manager.cc View 1 2 1 chunk +26 lines, -0 lines 0 comments Download
M chrome/browser/notifications/notification_ui_manager_impl.cc View 1 chunk +0 lines, -16 lines 0 comments Download
A chrome/browser/notifications/notification_ui_manager_mac.h View 1 2 1 chunk +96 lines, -0 lines 0 comments Download
A chrome/browser/notifications/notification_ui_manager_mac.mm View 1 2 3 1 chunk +307 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
Robert Sesek
8 years, 8 months ago (2012-04-09 15:53:05 UTC) #1
Mark Mentovai
https://chromiumcodereview.appspot.com/10021026/diff/1/chrome/browser/notifications/notification_ui_manager_mac.h File chrome/browser/notifications/notification_ui_manager_mac.h (right): https://chromiumcodereview.appspot.com/10021026/diff/1/chrome/browser/notifications/notification_ui_manager_mac.h#newcode33 chrome/browser/notifications/notification_ui_manager_mac.h:33: typedef std::map<id<CrUserNotification>, Notification*> NotificationMap; This typedef doesn’t need to ...
8 years, 8 months ago (2012-04-09 16:39:41 UTC) #2
jianli
Is it possible to add a test for it?
8 years, 8 months ago (2012-04-09 19:02:38 UTC) #3
Robert Sesek
On 2012/04/09 19:02:38, jianli wrote: > Is it possible to add a test for it? ...
8 years, 8 months ago (2012-04-09 19:03:27 UTC) #4
Robert Sesek
https://chromiumcodereview.appspot.com/10021026/diff/1/chrome/browser/notifications/notification_ui_manager_mac.h File chrome/browser/notifications/notification_ui_manager_mac.h (right): https://chromiumcodereview.appspot.com/10021026/diff/1/chrome/browser/notifications/notification_ui_manager_mac.h#newcode33 chrome/browser/notifications/notification_ui_manager_mac.h:33: typedef std::map<id<CrUserNotification>, Notification*> NotificationMap; On 2012/04/09 16:39:41, Mark Mentovai ...
8 years, 8 months ago (2012-04-09 21:36:38 UTC) #5
jianli
http://codereview.chromium.org/10021026/diff/6002/chrome/browser/notifications/notification_ui_manager.cc File chrome/browser/notifications/notification_ui_manager.cc (right): http://codereview.chromium.org/10021026/diff/6002/chrome/browser/notifications/notification_ui_manager.cc#newcode11 chrome/browser/notifications/notification_ui_manager.cc:11: return Create(local_state, BalloonCollection::Create()); nit: seems that we need to ...
8 years, 8 months ago (2012-04-10 00:54:14 UTC) #6
Robert Sesek
PTAL. I've changed NotificationMap. http://codereview.chromium.org/10021026/diff/6002/chrome/browser/notifications/notification_ui_manager.cc File chrome/browser/notifications/notification_ui_manager.cc (right): http://codereview.chromium.org/10021026/diff/6002/chrome/browser/notifications/notification_ui_manager.cc#newcode11 chrome/browser/notifications/notification_ui_manager.cc:11: return Create(local_state, BalloonCollection::Create()); On 2012/04/10 ...
8 years, 8 months ago (2012-04-12 17:48:47 UTC) #7
Mark Mentovai
LGTM
8 years, 8 months ago (2012-04-12 20:10:35 UTC) #8
jianli
https://chromiumcodereview.appspot.com/10021026/diff/11001/chrome/browser/notifications/notification_ui_manager_mac.mm File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://chromiumcodereview.appspot.com/10021026/diff/11001/chrome/browser/notifications/notification_ui_manager_mac.mm#newcode102 chrome/browser/notifications/notification_ui_manager_mac.mm:102: id<CrUserNotification> a_view, Notification* a_model) nit: you don't need to ...
8 years, 8 months ago (2012-04-13 00:03:35 UTC) #9
Robert Sesek
https://chromiumcodereview.appspot.com/10021026/diff/11001/chrome/browser/notifications/notification_ui_manager_mac.mm File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://chromiumcodereview.appspot.com/10021026/diff/11001/chrome/browser/notifications/notification_ui_manager_mac.mm#newcode102 chrome/browser/notifications/notification_ui_manager_mac.mm:102: id<CrUserNotification> a_view, Notification* a_model) On 2012/04/13 00:03:35, jianli wrote: ...
8 years, 8 months ago (2012-04-16 14:20:27 UTC) #10
Avi (use Gerrit)
https://chromiumcodereview.appspot.com/10021026/diff/11001/chrome/browser/notifications/notification_ui_manager_mac.mm File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://chromiumcodereview.appspot.com/10021026/diff/11001/chrome/browser/notifications/notification_ui_manager_mac.mm#newcode102 chrome/browser/notifications/notification_ui_manager_mac.mm:102: id<CrUserNotification> a_view, Notification* a_model) I was about to lay ...
8 years, 8 months ago (2012-04-16 14:33:45 UTC) #11
jianli
lgtm https://chromiumcodereview.appspot.com/10021026/diff/11001/chrome/browser/notifications/notification_ui_manager_mac.mm File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://chromiumcodereview.appspot.com/10021026/diff/11001/chrome/browser/notifications/notification_ui_manager_mac.mm#newcode102 chrome/browser/notifications/notification_ui_manager_mac.mm:102: id<CrUserNotification> a_view, Notification* a_model) On 2012/04/16 14:33:45, Avi ...
8 years, 8 months ago (2012-04-17 17:14:27 UTC) #12
Robert Sesek
Thanks for the review! https://chromiumcodereview.appspot.com/10021026/diff/11001/chrome/browser/notifications/notification_ui_manager_mac.mm File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://chromiumcodereview.appspot.com/10021026/diff/11001/chrome/browser/notifications/notification_ui_manager_mac.mm#newcode102 chrome/browser/notifications/notification_ui_manager_mac.mm:102: id<CrUserNotification> a_view, Notification* a_model) On ...
8 years, 8 months ago (2012-04-17 17:31:12 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/10021026/18001
8 years, 8 months ago (2012-04-17 17:31:20 UTC) #14
commit-bot: I haz the power
Change committed as 132611
8 years, 8 months ago (2012-04-17 19:04:53 UTC) #15
Nico
8 years, 8 months ago (2012-04-17 23:58:10 UTC) #16
https://chromiumcodereview.appspot.com/10021026/diff/11001/chrome/browser/not...
File chrome/browser/notifications/notification_ui_manager_mac.mm (right):

https://chromiumcodereview.appspot.com/10021026/diff/11001/chrome/browser/not...
chrome/browser/notifications/notification_ui_manager_mac.mm:102:
id<CrUserNotification> a_view, Notification* a_model)
On 2012/04/17 17:31:12, rsesek wrote:
> On 2012/04/17 17:14:27, jianli wrote:
> > On 2012/04/16 14:33:45, Avi wrote:
> > > I was about to lay down some standard smackdown, but see [class.base.init]
> in
> > > the C++98 standard, which is §12.6.2. ¶7 explicitly says that jianli is
> > correct;
> > > the first |view| has to be a class member, and the second |view| is the
view
> > in
> > > scope (the parameter). Something weird going on.
> > 
> > The problem might be due to the type of view: id<CrUserNotification>. Can
you
> > try doing only "model(model)" to see if it works or not?
> 
> Also produces the warning.

FWIW, I can't see this warning locally or on the bots (
https://chromiumcodereview.appspot.com/10115015 ). I tried with 10.5 and 10.6
sdks. I checked with last week's clang as well.

I claim you're making this up :-)

Powered by Google App Engine
This is Rietveld 408576698