Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11819048:
Implement message center on Windows (Closed)
|
Created:
7 years, 11 months ago by dewittj Modified:
7 years, 10 months ago CC:
chromium-reviews, tfarina, alicet1, sadrul, msw+watch_chromium.org, ben+watch_chromium.org, Dmitry Titov, Michael Courage Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImplement message center on Windows.
This takes the existing message center code for Ash, and refactors it
into two layers - a system-specific layer (WebNotificationTray) and a
platform-independent (modulo Views) layer (MessageCenterTray). The
WebNotificationTray is responsible for rendering the tray icon and
noticing system changes that cause differences in rendering. The
MessageCenterTray delegates responsibility for rendering the message
center and notification bubbles to the WebNotificationTray.
This patch also adds a Windows port of MessageCenterTray -
MessageCenterTrayWin.
BUG=168605
TEST:
message_center_unittests
browser_tests (WebNotificationTrayWinTest.*)
ash_unittests (WebNotificationTrayTest.*)
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179807
Patch Set 1 #Patch Set 2 : Rebase on master @fa1d2262 and rearrange dependencies. #
Total comments: 16
Patch Set 3 : Redraw the API to allow less code change in Ash. #Patch Set 4 : Redraw the API to allow less code change in Ash #Patch Set 5 : Rebase. #
Total comments: 20
Patch Set 6 : Address Pete's comments, enable status icon whenever Chrome is running. #Patch Set 7 : Now with more tests, and corrected copyright notices. #
Total comments: 70
Patch Set 8 : Use notification manager instead of balloon view, remove singleton-ness from MessageCenterTray. #
Total comments: 14
Patch Set 9 : remove Singleton references, merge Delegate/Observer, style cleanup. #
Total comments: 25
Patch Set 10 : Merge with master, address comments. #
Total comments: 32
Patch Set 11 : Address petewil & stevenjb comments. Move MessageCenterTrayDelegate to its own class. #
Total comments: 42
Patch Set 12 : Address miket comments. #
Total comments: 30
Patch Set 13 : Address sky/msw comments + rebase. #Messages
Total messages: 38 (0 generated)
Hi guys, I'm trying to get a handle on this change, and would love if you could give it a look. Since it's a large change, I'd be happy to go along with you to explain the change, but I've tried to put the most basic comments in the description. Any suggestions on how to break this up into smaller changes, or interface changes, would be appreciated. Thanks!
On Thu, Jan 10, 2013 at 5:17 PM, <dewittj@chromium.org> wrote: > Reviewers: Pete Williamson, Dmitry Titov, Michael Courage, > > Message: > Hi guys, I'm trying to get a handle on this change, and would love if you > could > give it a look. Since it's a large change, I'd be happy to go along with > you to > explain the change, but I've tried to put the most basic comments in the > description. Any suggestions on how to break this up into smaller changes, > or > interface changes, would be appreciated. Thanks! > > Description: > Implement message center on Windows. > > This is the massive version, which has most functionality but > several TODOs and known issues. > > BalloonViewWin and BalloonCollectionImplWin are temporary, to allow > notifications to be shown in the same way as Ash. They'll be replaced by > MessageCenterNotificationManager. > > Before, the Ash WebNotificationTray directly owned the message center, and > that > hung off a child of the ash::Shell object. In fact, it was a view itself, > the > button that the user clicks to toggle the message center View. > > Now, there is a layer in between message center and WebNotificationTray > called > MessageCenterTray. This is a View that can display the message center View > and > the individual notification popups. WebNotificationTray implements > MessageCenterTray::Host, and owns the MessageCenterTray. > MessageCenterTray::Host could possibly be made smaller, but it has both > event > listeners and system configuration query functions in the interface. It is > possibly better named MessageCenterTray::Impl. > > MessageCenterTray::Host::GetInstance() is a static function that implements > the > system-specific way to get the single Host object. > > There are a bunch of #ifdef USE_AURA statements which were required to get > the > message center bubble to render on Windows. This might be better done > differently, since #ifs suck. > There is no BUG filed for this? If there is one in crbug.com/, could you fill it here? > Please review this at https://codereview.chromium.org/11819048/ > -- Thiago
Bug number added.
I understand you have a new patch, and you may have already addressed many of my questions, but I'll pass along what I have so far, then continue reviewing from the new patch. https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... ash/system/web_notification/web_notification_tray.cc:20: #if defined(ENABLE_MESSAGE_CENTER) && !defined(OS_WIN) Should we check for not windows or that we do have chrome_os here? I think ash might not exist for mac, so this might not compile well on mac. Also, is there a Windows version of this function somewhere? https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... ash/system/web_notification/web_notification_tray.cc:33: Where did the WebNotificationBubbleWrapper functionality go to? Is it no longer needed? https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... ash/system/web_notification/web_notification_tray.cc:191: //XXX Put these back in? This should be resolved before checkin. Also, the comment is confusing, it says put them *back* in, but they weren't in for the previous version, so I don't understand what "back" means here. https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... ash/system/web_notification/web_notification_tray.cc:201: Where did BubbleViewDestroyed go? https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... ash/system/web_notification/web_notification_tray.cc:287: message_center_tray_->ShowMessageCenterBubble(); We removed some testing methods, does that mean we lost some test coverage, or were these just obsolete? I don't see any missing tests in the associated _unittest.cc file, so I have to assume these are obsolete. https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... File ash/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... ash/system/web_notification/web_notification_tray.h:62: // Displays the quiet mode bubble and adds |this| as an observer. The comment makes it sound like the function does two things. It should only do one thing per function. Let's add a second function to add \this\ as an observer. https://codereview.chromium.org/11819048/diff/4005/chrome/browser/ui/message_... File chrome/browser/ui/message_center/message_center_util.h (right): https://codereview.chromium.org/11819048/diff/4005/chrome/browser/ui/message_... chrome/browser/ui/message_center/message_center_util.h:7: General guidance - whenever something has "util" or "manager" in its name, that is a sign that either the name is weak or the responsibilities of the class are not well thought out. It also invites becoming a dumping ground for other functionality which is only loosely related (low cohesion). Consider whether this can be named better. MessageCenter? MessageCenterSingleton? MessageCenterFactory? MessageCenterAccessor? https://codereview.chromium.org/11819048/diff/4005/chrome/browser/ui/startup/... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/11819048/diff/4005/chrome/browser/ui/startup/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:89: #include "grit/theme_resources.h" This seems a bit odd, why add a new .h file if there is no corresponding code that might need it? If it is required by a header, shouldn't that header include it?
A new patch has been uploaded that should address the problems you raised. The primary new interface, MessageCenterTrayHost, has been broken into two: MessageCenterTrayDelegate, which does the actual rendering of bubbled, and MessageCenterTrayObserver, which is told when things change about the message_center. WebNotificationTray and MessageCenterTrayHostWin (to be renamed) both implement this interface. https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... ash/system/web_notification/web_notification_tray.cc:20: #if defined(ENABLE_MESSAGE_CENTER) && !defined(OS_WIN) On 2013/01/16 19:43:20, Pete Williamson wrote: > Should we check for not windows or that we do have chrome_os here? I think ash > might not exist for mac, so this might not compile well on mac. > > Also, is there a Windows version of this function somewhere? There is a windows version of this function, in chrome/browser/ui/message_center/message_center_util.h and implemented in chrome/browser/ui/views/message_center/message_center_tray_host_win.cc The version here is only for Ash builds that have the message center and don't run on Windows. Ash is currently built on ChromeOS and on Windows when USE_AURA is set, hence the !OS_WIN. https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... ash/system/web_notification/web_notification_tray.cc:33: On 2013/01/16 19:43:20, Pete Williamson wrote: > Where did the WebNotificationBubbleWrapper functionality go to? Is it no longer > needed? It was squashed partly into the MessageCenterTray and left in pieces in this file. New code puts it back here. https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... ash/system/web_notification/web_notification_tray.cc:191: //XXX Put these back in? On 2013/01/16 19:43:20, Pete Williamson wrote: > This should be resolved before checkin. Also, the comment is confusing, it says > put them *back* in, but they weren't in for the previous version, so I don't > understand what "back" means here. The XXX means I'm definitely not checking it in. I was merging a patch and trying to get parity ASAP. I have since figured this out and removed the comment. https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... ash/system/web_notification/web_notification_tray.cc:201: On 2013/01/16 19:43:20, Pete Williamson wrote: > Where did BubbleViewDestroyed go? It's back. https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... ash/system/web_notification/web_notification_tray.cc:287: message_center_tray_->ShowMessageCenterBubble(); On 2013/01/16 19:43:20, Pete Williamson wrote: > We removed some testing methods, does that mean we lost some test coverage, or > were these just obsolete? I don't see any missing tests in the associated > _unittest.cc file, so I have to assume these are obsolete. They were a work in progress. The patch you read made it clumsy to provide that method, so I commented out the test that used it. I have since reinstated the methods and the test. https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... File ash/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/11819048/diff/4005/ash/system/web_notificatio... ash/system/web_notification/web_notification_tray.h:62: // Displays the quiet mode bubble and adds |this| as an observer. On 2013/01/16 19:43:20, Pete Williamson wrote: > The comment makes it sound like the function does two things. It should only do > one thing per function. Let's add a second function to add \this\ as an > observer. This isn't a functionality change, just updated comment. https://codereview.chromium.org/11819048/diff/4005/chrome/browser/ui/message_... File chrome/browser/ui/message_center/message_center_util.h (right): https://codereview.chromium.org/11819048/diff/4005/chrome/browser/ui/message_... chrome/browser/ui/message_center/message_center_util.h:7: On 2013/01/16 19:43:20, Pete Williamson wrote: > General guidance - whenever something has "util" or "manager" in its name, that > is a sign that either the name is weak or the responsibilities of the class are > not well thought out. It also invites becoming a dumping ground for other > functionality which is only loosely related (low cohesion). Consider whether > this can be named better. MessageCenter? MessageCenterSingleton? > MessageCenterFactory? MessageCenterAccessor? I agree the name is weak. It is modeled after the app list code which has similar platform dependencies, and which has a file much like this one. The issue here was getting a static function that would link in a component build. https://codereview.chromium.org/11819048/diff/4005/chrome/browser/ui/startup/... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/11819048/diff/4005/chrome/browser/ui/startup/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:89: #include "grit/theme_resources.h" On 2013/01/16 19:43:20, Pete Williamson wrote: > This seems a bit odd, why add a new .h file if there is no corresponding code > that might need it? If it is required by a header, shouldn't that header > include it? This was a bug.
By the way, this code now depends on https://codereview.chromium.org/11836003/ to render on Windows.
Here are a few comments based on our review together, more later. 1.) Make a doc with diagrams 2.) Rename MessageCenterTrayHostWin to WebNotificationTrayWin, and someday rename WebNotificationTray to WebNotificationTrayAsh. 3.) Someday move delegate functionality for Ash's TrayBubbleView from WebNotificationTray to WebMotificationBubbleWrapper. 4.) Move WebNotificationBubbleWrapper into its own file (WebNotificationBubbleWrapperWin/Ash) 5.) There is lots of similarity between WNBW-win and WNBW-ash - consider refactoring to a common file (after this first checkin) https://codereview.chromium.org/11819048/diff/23022/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/23022/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:234: if (!popup_bubble_view) While not harmful, this might not still be required, remove if not needed. https://codereview.chromium.org/11819048/diff/23022/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:235: return false; While not necessary, this might not still be required, remove if not needed. https://codereview.chromium.org/11819048/diff/23022/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:331: gfx::Rect WebNotificationTray::GetAnchorRect( comment somewhere explaining what the anchor rect is. https://codereview.chromium.org/11819048/diff/23022/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:348: remove extra blank line https://codereview.chromium.org/11819048/diff/23022/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/11819048/diff/23022/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.h:123: virtual void HidePopups() OVERRIDE; It would be less confusing if these were public https://codereview.chromium.org/11819048/diff/23022/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/message_center_tray_host_win.cc (right): https://codereview.chromium.org/11819048/diff/23022/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/message_center_tray_host_win.cc:90: WebNotificationBubbleWrapper(MessageCenterTrayHostWin* tray, Make this a platform specific class in its own file. https://codereview.chromium.org/11819048/diff/23022/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/message_center_tray_host_win.cc:307: bool MessageCenterTrayHostWin::CanShowPopups() { Check to see if we still need this method. https://codereview.chromium.org/11819048/diff/23022/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/message_center_tray_host_win.h (right): https://codereview.chromium.org/11819048/diff/23022/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/message_center_tray_host_win.h:73: scoped_ptr<internal::WebNotificationBubbleWrapper> popup_bubble_; Change name to something win specific. https://codereview.chromium.org/11819048/diff/23022/ui/message_center/message... File ui/message_center/message_center_tray.cc (right): https://codereview.chromium.org/11819048/diff/23022/ui/message_center/message... ui/message_center/message_center_tray.cc:88: if (!popups_visible_) Remove these two lines https://codereview.chromium.org/11819048/diff/23022/ui/message_center/message... File ui/message_center/message_center_tray.h (right): https://codereview.chromium.org/11819048/diff/23022/ui/message_center/message... ui/message_center/message_center_tray.h:32: virtual void OnShowMessageCenterBubble() {}; Remove these events if they end up not getting used https://codereview.chromium.org/11819048/diff/23022/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/11819048/diff/23022/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:155: #ifdef USE_AURA Remove ifdefs
Take another look, pulled the windows WebNotificationBubbleWrapper out to its own file. https://codereview.chromium.org/11819048/diff/23022/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/23022/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:234: if (!popup_bubble_view) On 2013/01/17 19:07:45, Pete Williamson wrote: > While not harmful, this might not still be required, remove if not needed. Done. https://codereview.chromium.org/11819048/diff/23022/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:235: return false; On 2013/01/17 19:07:45, Pete Williamson wrote: > While not necessary, this might not still be required, remove if not needed. Done. https://codereview.chromium.org/11819048/diff/23022/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:331: gfx::Rect WebNotificationTray::GetAnchorRect( On 2013/01/17 19:07:45, Pete Williamson wrote: > comment somewhere explaining what the anchor rect is. BubbleDelegateView is where this originates, and it is commented there. https://codereview.chromium.org/11819048/diff/23022/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/11819048/diff/23022/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.h:123: virtual void HidePopups() OVERRIDE; On 2013/01/17 19:07:45, Pete Williamson wrote: > It would be less confusing if these were public Done. https://codereview.chromium.org/11819048/diff/23022/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/message_center_tray_host_win.cc (right): https://codereview.chromium.org/11819048/diff/23022/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/message_center_tray_host_win.cc:90: WebNotificationBubbleWrapper(MessageCenterTrayHostWin* tray, On 2013/01/17 19:07:45, Pete Williamson wrote: > Make this a platform specific class in its own file. Done. https://codereview.chromium.org/11819048/diff/23022/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/message_center_tray_host_win.h (right): https://codereview.chromium.org/11819048/diff/23022/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/message_center_tray_host_win.h:73: scoped_ptr<internal::WebNotificationBubbleWrapper> popup_bubble_; On 2013/01/17 19:07:45, Pete Williamson wrote: > Change name to something win specific. Done. https://codereview.chromium.org/11819048/diff/23022/ui/message_center/message... File ui/message_center/message_center_tray.cc (right): https://codereview.chromium.org/11819048/diff/23022/ui/message_center/message... ui/message_center/message_center_tray.cc:88: if (!popups_visible_) On 2013/01/17 19:07:45, Pete Williamson wrote: > Remove these two lines Done. https://codereview.chromium.org/11819048/diff/23022/ui/message_center/message... File ui/message_center/message_center_tray.h (right): https://codereview.chromium.org/11819048/diff/23022/ui/message_center/message... ui/message_center/message_center_tray.h:32: virtual void OnShowMessageCenterBubble() {}; On 2013/01/17 19:07:45, Pete Williamson wrote: > Remove these events if they end up not getting used Done. https://codereview.chromium.org/11819048/diff/23022/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/11819048/diff/23022/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:155: #ifdef USE_AURA On 2013/01/17 19:07:45, Pete Williamson wrote: > Remove ifdefs Done.
stevenjb: all miket: all sky: gyp, chrome/browser/ui/views This is a pretty big patch, I appreciate your time. Please take a look at the bug which currently has the PRD and will soon have another doc outlining the structure of the message center classes. Shooting for M25, so I'd love to have this in by 1/25 for the dev branch if possible. I believe the balloon code will be taken out before I commit this so review of that code is less necessary - but I left them in so it's clear how message_center gets it's messages currently.
This looks like it includes changes from https://codereview.chromium.org/11958025/? It would be easier to review if 'git branch --set-upstream dependent-branch start-point' were used to isolate the new changes. I'll try to review it this afternoon anyway if I finish some urgent LTE fixes.
On 2013/01/18 19:58:51, stevenjb (chromium) wrote: > This looks like it includes changes from > https://codereview.chromium.org/11958025/? It would be easier to review if 'git > branch --set-upstream dependent-branch start-point' were used to isolate the new > changes. I'll try to review it this afternoon anyway if I finish some urgent LTE > fixes. I was waiting until http://crrev.com/11836003 landed, and am in the process of rebasing and patching based on 11958025.
I started to look through this, then fitured Steven should be happy first. I'll take another look after hime. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/message... File chrome/browser/ui/message_center/message_center_util.h (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/message... chrome/browser/ui/message_center/message_center_util.h:14: // Show the message center tray widget. Comment is wrong. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/message... chrome/browser/ui/message_center/message_center_util.h:15: ui::MessageCenterTrayDelegate* GetMessageCenterTray(); Don't indent this. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... File chrome/browser/ui/views/balloon_collection_impl_win.cc (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.cc:25: host->message_center()->SetDelegate(this); You never unset this. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.cc:32: return true; // Overflow is handled by messagecentertray end with period. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... File chrome/browser/ui/views/balloon_collection_impl_win.h (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.h:8: #include <set> You don't use set in this file.
On 2013/01/18 21:30:56, sky wrote: > I started to look through this, then fitured Steven should be happy first. I'll > take another look after hime. > > https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/message... > File chrome/browser/ui/message_center/message_center_util.h (right): > > https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/message... > chrome/browser/ui/message_center/message_center_util.h:14: // Show the message > center tray widget. > Comment is wrong. > > https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/message... > chrome/browser/ui/message_center/message_center_util.h:15: > ui::MessageCenterTrayDelegate* GetMessageCenterTray(); > Don't indent this. > > https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... > File chrome/browser/ui/views/balloon_collection_impl_win.cc (right): > > https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... > chrome/browser/ui/views/balloon_collection_impl_win.cc:25: > host->message_center()->SetDelegate(this); > You never unset this. > > https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... > chrome/browser/ui/views/balloon_collection_impl_win.cc:32: return true; // > Overflow is handled by messagecentertray > end with period. > > https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... > File chrome/browser/ui/views/balloon_collection_impl_win.h (right): > > https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... > chrome/browser/ui/views/balloon_collection_impl_win.h:8: #include <set> > You don't use set in this file. Thanks sky for starting, I agree that you should come in later, especially since most of the bubble* files are going away once I rebase off https://codereview.chromium.org/11958025/?
+mukai@ The TrayBubbleView changes need to be tested very carefully in Ash/Chromeos with both the web notification and system tray. Testing should include showing/hiding the launcher and launcher on left/right (which may be broken but at least shouldn't regress). We've seen regressions there before, and test coverage is challenging and not as thorough as it ought to be. https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:32: } This does not belong here, it needs to be in src/chrome https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:92: message_center_tray_ = make_scoped_ptr(new ui::MessageCenterTray(this)); message_center_tray_.reset(new ...) https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:163: popup_bubble)); nit: this can be on the same line as 'this,' https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... File ui/message_center/message_center_tray.cc (right): https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray.cc:20: message_center_ = new message_center::MessageCenter(); This should be scoped if it owns MessageCenter, but I assume this will change once dimich@'s change is in. https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... File ui/message_center/message_center_tray.h (right): https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray.h:29: virtual ~MessageCenterTrayObserver() {} destructor should be protected https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray.h:31: virtual void OnMessageCenterTrayChanged() {} With just a single method, it should be a pure virtual https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray.h:40: virtual void UpdatePopups() {} Default implementations, even trivial ones, should not be inlined. Only exception is the destructor in a pure interface class. https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray.h:46: one blank line https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray.h:75: bool IsMessageCenterVisible() { return message_center_visible_; } Should just be message_center_visible() https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray.h:77: bool IsPopupVisible() { return popups_visible_; } popups_visible() https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... File ui/message_center/message_center_tray_unittest.cc (right): https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray_unittest.cc:21: virtual ~MockDelegate() {} virtual void OnMessageCenterTrayChanged() {} two lines https://codereview.chromium.org/11819048/diff/43001/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/11819048/diff/43001/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:83: } No {} https://codereview.chromium.org/11819048/diff/43001/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:95: pt.set_y(tray_arrow_offset_); indent
Thanks for the comments, miket, stevenjb, mukai PTAL. The style nits you pointed out and others have been fixed. I incorporated http://crrev.com/11958025 so BalloonViewWin and friends are gone. Refactored MessageCenterTray so that it's no longer a singleton, it's owned by the Shell or the NotificationUIManager, depending on the platform. https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:32: } On 2013/01/18 23:11:46, stevenjb (chromium) wrote: > This does not belong here, it needs to be in src/chrome This is now a static method on MessageCenterTrayDelegate. https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:92: message_center_tray_ = make_scoped_ptr(new ui::MessageCenterTray(this)); On 2013/01/18 23:11:46, stevenjb (chromium) wrote: > message_center_tray_.reset(new ...) Done. https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:163: popup_bubble)); On 2013/01/18 23:11:46, stevenjb (chromium) wrote: > nit: this can be on the same line as 'this,' Done. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/message... File chrome/browser/ui/message_center/message_center_util.h (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/message... chrome/browser/ui/message_center/message_center_util.h:14: // Show the message center tray widget. On 2013/01/18 21:30:56, sky wrote: > Comment is wrong. File gone. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/message... chrome/browser/ui/message_center/message_center_util.h:15: ui::MessageCenterTrayDelegate* GetMessageCenterTray(); On 2013/01/18 21:30:56, sky wrote: > Don't indent this. File gone. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... File chrome/browser/ui/views/balloon_collection_impl_win.cc (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.cc:25: host->message_center()->SetDelegate(this); On 2013/01/18 21:30:56, sky wrote: > You never unset this. File gone. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.cc:32: return true; // Overflow is handled by messagecentertray On 2013/01/18 21:30:56, sky wrote: > end with period. File gone. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... File chrome/browser/ui/views/balloon_collection_impl_win.h (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.h:8: #include <set> On 2013/01/18 21:30:56, sky wrote: > You don't use set in this file. File gone. https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... File ui/message_center/message_center_tray.cc (right): https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray.cc:20: message_center_ = new message_center::MessageCenter(); On 2013/01/18 23:11:46, stevenjb (chromium) wrote: > This should be scoped if it owns MessageCenter, but I assume this will change > once dimich@'s change is in. Done. https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... File ui/message_center/message_center_tray.h (right): https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray.h:29: virtual ~MessageCenterTrayObserver() {} On 2013/01/18 23:11:46, stevenjb (chromium) wrote: > destructor should be protected Done. https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray.h:31: virtual void OnMessageCenterTrayChanged() {} On 2013/01/18 23:11:46, stevenjb (chromium) wrote: > With just a single method, it should be a pure virtual Done. https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray.h:40: virtual void UpdatePopups() {} On 2013/01/18 23:11:46, stevenjb (chromium) wrote: > Default implementations, even trivial ones, should not be inlined. Only > exception is the destructor in a pure interface class. Done. https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray.h:46: On 2013/01/18 23:11:46, stevenjb (chromium) wrote: > one blank line Done. https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray.h:75: bool IsMessageCenterVisible() { return message_center_visible_; } On 2013/01/18 23:11:46, stevenjb (chromium) wrote: > Should just be message_center_visible() Done. https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray.h:77: bool IsPopupVisible() { return popups_visible_; } On 2013/01/18 23:11:46, stevenjb (chromium) wrote: > popups_visible() Done. https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... File ui/message_center/message_center_tray_unittest.cc (right): https://codereview.chromium.org/11819048/diff/43001/ui/message_center/message... ui/message_center/message_center_tray_unittest.cc:21: virtual ~MockDelegate() {} virtual void OnMessageCenterTrayChanged() {} On 2013/01/18 23:11:46, stevenjb (chromium) wrote: > two lines Done. https://codereview.chromium.org/11819048/diff/43001/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/11819048/diff/43001/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:83: } On 2013/01/18 23:11:46, stevenjb (chromium) wrote: > No {} Gone. This method is now unused by the Win code, so no changes necessary. https://codereview.chromium.org/11819048/diff/43001/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:95: pt.set_y(tray_arrow_offset_); On 2013/01/18 23:11:46, stevenjb (chromium) wrote: > indent Gone. This method is now unused by the Win code, so no changes necessary.
https://codereview.chromium.org/11819048/diff/47001/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/47001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:39: IIRC it should be single-line blank between namespaces https://codereview.chromium.org/11819048/diff/47001/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/11819048/diff/47001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.h:43: public ui::MessageCenterTrayObserver, A class being delegate and observer at the same time sounds a bit weird. Why not merging these two classes into two? Based on my feeling at looking on ui/views/widget/widget_delegate.h, it's good to have 'On...'-style method (event handler method) in a delegate class. https://codereview.chromium.org/11819048/diff/47001/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.cc (right): https://codereview.chromium.org/11819048/diff/47001/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:98: this, g_browser_process->message_center()); BrowserProcess doesn't have 'message_center' field right now. Missing an edit in this CL? https://codereview.chromium.org/11819048/diff/47001/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.h (right): https://codereview.chromium.org/11819048/diff/47001/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.h:87: MessageCenterTray* message_center_tray_; message_center_tray_ is created in constructor but not deleted at all. Who owns? Should this be scoped_ptr? https://codereview.chromium.org/11819048/diff/47001/ui/message_center/message... File ui/message_center/message_center_tray.cc (right): https://codereview.chromium.org/11819048/diff/47001/ui/message_center/message... ui/message_center/message_center_tray.cc:7: #include "base/memory/singleton.h" same -- singleton is not used anymore? https://codereview.chromium.org/11819048/diff/47001/ui/message_center/message... File ui/message_center/message_center_tray.h (right): https://codereview.chromium.org/11819048/diff/47001/ui/message_center/message... ui/message_center/message_center_tray.h:13: template <typename T> struct DefaultSingletonTraits; No one looks singleton in this file. Can we remove this? https://codereview.chromium.org/11819048/diff/47001/ui/message_center/message... ui/message_center/message_center_tray.h:48: // Class that owns a MessageCenter and hosts it. Manages the popup and message This class doesn't own MessageCenter but only has the pointer to the instance.
Thanks mukai. Fixed your comments. https://codereview.chromium.org/11819048/diff/47001/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/47001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:39: On 2013/01/22 18:55:34, Jun Mukai wrote: > IIRC it should be single-line blank between namespaces Done. https://codereview.chromium.org/11819048/diff/47001/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/11819048/diff/47001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.h:43: public ui::MessageCenterTrayObserver, On 2013/01/22 18:55:34, Jun Mukai wrote: > A class being delegate and observer at the same time sounds a bit weird. Why not > merging these two classes into two? > Based on my feeling at looking on ui/views/widget/widget_delegate.h, it's good > to have 'On...'-style method (event handler method) in a delegate class. I thought it would be better to separate the roles, but since there is only one observer and precedent for On* methods in delegate interfaces, I see no problem combining them. Done. https://codereview.chromium.org/11819048/diff/47001/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.cc (right): https://codereview.chromium.org/11819048/diff/47001/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:98: this, g_browser_process->message_center()); On 2013/01/22 18:55:34, Jun Mukai wrote: > BrowserProcess doesn't have 'message_center' field right now. Missing an edit in > this CL? This CL depends on crrev.com/11958025 which makes this change. https://codereview.chromium.org/11819048/diff/47001/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.h (right): https://codereview.chromium.org/11819048/diff/47001/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.h:87: MessageCenterTray* message_center_tray_; On 2013/01/22 18:55:34, Jun Mukai wrote: > message_center_tray_ is created in constructor but not deleted at all. Who owns? > Should this be scoped_ptr? Yes. Done. https://codereview.chromium.org/11819048/diff/47001/ui/message_center/message... File ui/message_center/message_center_tray.cc (right): https://codereview.chromium.org/11819048/diff/47001/ui/message_center/message... ui/message_center/message_center_tray.cc:7: #include "base/memory/singleton.h" On 2013/01/22 18:55:34, Jun Mukai wrote: > same -- singleton is not used anymore? Done. https://codereview.chromium.org/11819048/diff/47001/ui/message_center/message... File ui/message_center/message_center_tray.h (right): https://codereview.chromium.org/11819048/diff/47001/ui/message_center/message... ui/message_center/message_center_tray.h:13: template <typename T> struct DefaultSingletonTraits; On 2013/01/22 18:55:34, Jun Mukai wrote: > No one looks singleton in this file. Can we remove this? Done. https://codereview.chromium.org/11819048/diff/47001/ui/message_center/message... ui/message_center/message_center_tray.h:48: // Class that owns a MessageCenter and hosts it. Manages the popup and message On 2013/01/22 18:55:34, Jun Mukai wrote: > This class doesn't own MessageCenter but only has the pointer to the instance. Fixed comment.
lgtm from nits https://codereview.chromium.org/11819048/diff/52004/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/11819048/diff/52004/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.h:109: // TODO(dewittj): move these up above the private area after merging trunk. Why is this a TODO? You can just move FRIEND_TEST_ALL_ to the private: below, am I wrong? https://codereview.chromium.org/11819048/diff/52004/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.h:110: // MessageCenterTrayDelegate implementation. "Overridden from MessageCenterTrayDelegate." probably better to follow the format
I still need to review web_notification_tray_win_browsertest and the message center tray class, but here are the comments I have so far. Many are just questions about how the code works, depending on the answers I may have more comments. https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:92: message_center_tray_ = make_scoped_ptr(new ui::MessageCenterTray(this)); Since WNT creates MCT, how does the outside world get to know about MCT to send notifications through it? Furthermore, how does MessageCenter get hooked up to MCT? https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:244: SetShelfAlignment(alignment); Hmm, I don't see this function in the file, where is it, is this recursive? https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:319: gfx::Rect WebNotificationTray::GetAnchorRect( This function could use a better name, and possibly a better idiom. We don't really care about the rect, we really care about where to put the point of our speech bubble, and the orientation. Perhaps GetBubbleAnchorPoint? It could return LEFT, RIGHT, TOP, or BOTTOM in an outparam https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:329: Note to self - this moved, compare it with the new version in message center tray https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray_unittest.cc (right): https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray_unittest.cc:210: tray->SetHidePopupBubble(false); Although one function can do both, it was probably clearer the way it was before. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/notificati... File chrome/browser/notifications/notification_ui_manager.cc (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/notificati... chrome/browser/notifications/notification_ui_manager.cc:35: // return new MessageCenterNotificationManager(); Generally we don't check in commented out code, even if we plan to re-enable it later, we just have a TODO comment that says what we need to do someday. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/message... File chrome/browser/ui/message_center/message_center_util.h (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/message... chrome/browser/ui/message_center/message_center_util.h:12: namespace chrome { Is this the right namespace? That seems like a pretty crowded namespace already. Perhaps a namespace message_center or message_center_tray would be good. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... File chrome/browser/ui/views/balloon_collection_impl_win.cc (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.cc:32: return true; // Overflow is handled by messagecentertray On 2013/01/18 21:30:56, sky wrote: > end with period. nit: also camel case it: MessageCenterTray. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.cc:38: return; // HTML notifications are not supported in Win. nit - they aren't supported anywhere afaik, so we can change this to ..."not supported." https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.cc:63: Overall comment - there is a lot of similarity between the ash and windows versions - can we push the common code somewhere and shrink the OS specific parts down? https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.cc:130: #if defined(OS_WIN) Why do we need this #define? I would have expected this file to only get compiled on the windows platform. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... File chrome/browser/ui/views/balloon_collection_impl_win.h (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.h:41: I see that balloon_collection_impl_ash.h has a function here called AddWebUIMessageCallback. How come windows doesn't have a similar function? https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... File chrome/browser/ui/views/balloon_view_win.cc (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_view_win.cc:33: Once again, this file is very similar to balloon_view_ash.cc. Can we move the common code somewhere and encapsulate only the true platform differences here? https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:14: namespace internal { instead of "internal", should we use a more specific namespace everywhere? messagbe_center or message_center_tray, maybe? https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:43: // Convenience accessors. Do we need to expose these as public? It seems a bit strange to ask the NBWW to give you a bubble_view to do something on it instead of asking the NBWW to do something on your behalf. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:50: // |bubble_view_| is unowned. If bubble view is unowned, how do we know it is always valid (hasn't been freed) when we go to use it? A comment would help. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc (right): https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:40: tray->GetBubbleWindowContainer(), NULL, this, &init_params); So who owns the bubble_view_? The .h file says it is unowned, but this sure looks like it is being created and ownership transferred to NBWW here. Maybe the bubble_widget_ takes ownership? https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:46: bubble_widget_->Activate(); If we always do these 4 steps, we should consider moving them into CreateBubble. The first might create a bad dependency graph, but the others don't have any dependence on NBWW https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:82: return ASCIIToUTF16("Windows Notification Center"); This string should be a constant at the top of the file. const char kTitle[] = "Windows Notification Center"; https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:84: Again, I wonder why we need an anchor rect. It seems that we really only need a point, and the platform specific code should calculate the point. We might even be able to get away with an X coordinate, since the Y coordinate is the top of the toolbar and can be calculated from the system. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.cc (right): https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:68: gfx::ImageSkia GetIcon(bool has_unread_notifications) { Is the caller responsible for freeing the icon resource? If so, let's add a comment to that effect. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:83: #if !defined(USE_ASH) Why do we have this #define? This is a windows specific file, I thought ASH was linux only? https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:107: // problems. Great comment! https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:111: status_tray->RemoveStatusIcon(status_icon_); Does this free the icon? If not, we might be leaking it. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:189: return views::TrayBubbleView::ANCHOR_ALIGNMENT_BOTTOM; Could the task bar be on the top? I don't see any case for ANCHOR_ALIGNMENT_TOP. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:196: return NULL; This looks unfinished, might we be missing a TODO here? Alternatively, this might be a case where ASH/AURA does something that WIN does not. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.h (right): https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.h:30: // A Message Center Tray Delegate class that manages a system tray icon. The Perhaps "A class derived from the Message Center Tray Delegate, that"...
PTAL - now there are no dependencies on open bugs. https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:92: message_center_tray_ = make_scoped_ptr(new ui::MessageCenterTray(this)); On 2013/01/23 19:52:16, Pete Williamson wrote: > Since WNT creates MCT, how does the outside world get to know about MCT to send > notifications through it? Furthermore, how does MessageCenter get hooked up to > MCT? The outside world adds notifications to the global MessageCenter object. MessageCenterTray looks up the global MessageCenter object. https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:244: SetShelfAlignment(alignment); On 2013/01/23 19:52:16, Pete Williamson wrote: > Hmm, I don't see this function in the file, where is it, is this recursive? Good catch, that must have been a merge issue. Reverted that part of the change. https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:319: gfx::Rect WebNotificationTray::GetAnchorRect( On 2013/01/23 19:52:16, Pete Williamson wrote: > This function could use a better name, and possibly a better idiom. We don't > really care about the rect, we really care about where to put the point of our > speech bubble, and the orientation. Perhaps GetBubbleAnchorPoint? It could > return LEFT, RIGHT, TOP, or BOTTOM in an outparam As described elsewhere, sometimes the edge of the rectangle is used for alignment purposes. Its name is from TrayBubbleView::Delegate interface, which I'm not changing here. https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:329: On 2013/01/23 19:52:16, Pete Williamson wrote: > Note to self - this moved, compare it with the new version in message center > tray Noted. https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray_unittest.cc (right): https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray_unittest.cc:210: tray->SetHidePopupBubble(false); On 2013/01/23 19:52:16, Pete Williamson wrote: > Although one function can do both, it was probably clearer the way it was > before. The Ash infrastructure uses SetHidePopupBubble and not HidePopupBubble/ShowPopupBubble, so it seems better to use what is actually used in the code than to have functions which solely exist for testing. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/notificati... File chrome/browser/notifications/notification_ui_manager.cc (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/notificati... chrome/browser/notifications/notification_ui_manager.cc:35: // return new MessageCenterNotificationManager(); On 2013/01/23 19:52:16, Pete Williamson wrote: > Generally we don't check in commented out code, even if we plan to re-enable it > later, we just have a TODO comment that says what we need to do someday. This has been fixed in later patchsets. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/message... File chrome/browser/ui/message_center/message_center_util.h (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/message... chrome/browser/ui/message_center/message_center_util.h:12: namespace chrome { On 2013/01/23 19:52:16, Pete Williamson wrote: > Is this the right namespace? That seems like a pretty crowded namespace > already. Perhaps a namespace message_center or message_center_tray would be > good. This was modeled on app_list_util.h, but the file has been removed anyway. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... File chrome/browser/ui/views/balloon_collection_impl_win.cc (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.cc:32: return true; // Overflow is handled by messagecentertray On 2013/01/23 19:52:16, Pete Williamson wrote: > On 2013/01/18 21:30:56, sky wrote: > > end with period. > > nit: also camel case it: MessageCenterTray. File gone. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.cc:38: return; // HTML notifications are not supported in Win. On 2013/01/23 19:52:16, Pete Williamson wrote: > nit - they aren't supported anywhere afaik, so we can change this to ..."not > supported." File gone. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.cc:63: On 2013/01/23 19:52:16, Pete Williamson wrote: > Overall comment - there is a lot of similarity between the ash and windows > versions - can we push the common code somewhere and shrink the OS specific > parts down? File gone. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.cc:130: #if defined(OS_WIN) On 2013/01/23 19:52:16, Pete Williamson wrote: > Why do we need this #define? I would have expected this file to only get > compiled on the windows platform. File gone. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... File chrome/browser/ui/views/balloon_collection_impl_win.h (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_collection_impl_win.h:41: On 2013/01/23 19:52:16, Pete Williamson wrote: > I see that balloon_collection_impl_ash.h has a function here called > AddWebUIMessageCallback. How come windows doesn't have a similar function? File gone. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... File chrome/browser/ui/views/balloon_view_win.cc (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/b... chrome/browser/ui/views/balloon_view_win.cc:33: On 2013/01/23 19:52:16, Pete Williamson wrote: > Once again, this file is very similar to balloon_view_ash.cc. Can we move the > common code somewhere and encapsulate only the true platform differences here? File gone. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:14: namespace internal { On 2013/01/23 19:52:16, Pete Williamson wrote: > instead of "internal", should we use a more specific namespace everywhere? > messagbe_center or message_center_tray, maybe? Classes in ui::internal should not be used by classes outside of ui. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:43: // Convenience accessors. On 2013/01/23 19:52:16, Pete Williamson wrote: > Do we need to expose these as public? It seems a bit strange to ask the NBWW to > give you a bubble_view to do something on it instead of asking the NBWW to do > something on your behalf. This is only used for equality testing, not for direct manipulation. https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:50: // |bubble_view_| is unowned. On 2013/01/23 19:52:16, Pete Williamson wrote: > If bubble view is unowned, how do we know it is always valid (hasn't been freed) > when we go to use it? A comment would help. The widget owns bubble_view_. https://codereview.chromium.org/11819048/diff/52004/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/11819048/diff/52004/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.h:109: // TODO(dewittj): move these up above the private area after merging trunk. On 2013/01/23 19:40:13, Jun Mukai wrote: > Why is this a TODO? You can just move FRIEND_TEST_ALL_ to the private: below, > am I wrong? Done. https://codereview.chromium.org/11819048/diff/52004/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.h:110: // MessageCenterTrayDelegate implementation. On 2013/01/23 19:40:13, Jun Mukai wrote: > "Overridden from MessageCenterTrayDelegate." > probably better to follow the format Done. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc (right): https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:40: tray->GetBubbleWindowContainer(), NULL, this, &init_params); On 2013/01/23 19:52:16, Pete Williamson wrote: > So who owns the bubble_view_? The .h file says it is unowned, but this sure > looks like it is being created and ownership transferred to NBWW here. Maybe > the bubble_widget_ takes ownership? The Widget is indeed responsible for destruction of bubble_view_. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:46: bubble_widget_->Activate(); On 2013/01/23 19:52:16, Pete Williamson wrote: > If we always do these 4 steps, we should consider moving them into CreateBubble. > The first might create a bad dependency graph, but the others don't have any > dependence on NBWW These are always done on Windows only. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:82: return ASCIIToUTF16("Windows Notification Center"); On 2013/01/23 19:52:16, Pete Williamson wrote: > This string should be a constant at the top of the file. > const char kTitle[] = "Windows Notification Center"; Done. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:84: On 2013/01/23 19:52:16, Pete Williamson wrote: > Again, I wonder why we need an anchor rect. It seems that we really only need a > point, and the platform specific code should calculate the point. We might even > be able to get away with an X coordinate, since the Y coordinate is the top of > the toolbar and can be calculated from the system. At the moment on Windows, only a point is required. However, if we ever decide to align the edge of the bubble with the edge of the anchor (in this case the tray icon), we will need to support a Rect. On other platforms such as ChromeOS we already have this behavior for UI design reasons. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.cc (right): https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:68: gfx::ImageSkia GetIcon(bool has_unread_notifications) { On 2013/01/23 19:52:16, Pete Williamson wrote: > Is the caller responsible for freeing the icon resource? If so, let's add a > comment to that effect. No, the ResourceBundle owns the icon resource. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:83: #if !defined(USE_ASH) On 2013/01/23 19:52:16, Pete Williamson wrote: > Why do we have this #define? This is a windows specific file, I thought ASH was > linux only? Ash is run on Windows by default when Aura is enabled. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:111: status_tray->RemoveStatusIcon(status_icon_); On 2013/01/23 19:52:16, Pete Williamson wrote: > Does this free the icon? If not, we might be leaking it. The status tray owns the status icon, so no leaks. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:189: return views::TrayBubbleView::ANCHOR_ALIGNMENT_BOTTOM; On 2013/01/23 19:52:16, Pete Williamson wrote: > Could the task bar be on the top? I don't see any case for > ANCHOR_ALIGNMENT_TOP. The task bar can in fact be on any edge of the screen, but the automatic bounds-checking code within the views will flip it to ANCHOR_ALIGNMENT_TOP if the bubble won't fit in the screen. I've tested the bar on the top. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:196: return NULL; On 2013/01/23 19:52:16, Pete Williamson wrote: > This looks unfinished, might we be missing a TODO here? Alternatively, this > might be a case where ASH/AURA does something that WIN does not. Indeed, this is finished - there is a parent NativeView on Ash but not on Win. https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.h (right): https://codereview.chromium.org/11819048/diff/52004/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.h:30: // A Message Center Tray Delegate class that manages a system tray icon. The On 2013/01/23 19:52:16, Pete Williamson wrote: > Perhaps "A class derived from the Message Center Tray Delegate, that"... Done.
https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:28: namespace ui { There shouldn't be any code in the ui:: namespace here. https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:160: return false; {} https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.h:77: void ShowMessageCenter(); This is the same name as ShowMessageCenter below but with a different signature; it needs to be renamed (e.g. ShowMessageCenterBubble or ShowMessageCenterForTray). https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray_unittest.cc (right): https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notificati... ash/system/web_notification/web_notification_tray_unittest.cc:236: // tray->GetPopupBubbleForTest()->NumMessageViewsForTest()); ?? https://codereview.chromium.org/11819048/diff/47005/chrome/browser/notificati... File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/11819048/diff/47005/chrome/browser/notificati... chrome/browser/notifications/message_center_notification_manager.cc:24: tray_.reset(ui::MessageCenterTrayDelegate::CreateForPlatform()); I think this should use #if USE_ASH... relying on correct linkage for code that's in the ui:: namespace but defined in src/ash is ugly at best. I would do something like: #if defined(USE_ASH) tray_.reset(ash::Shell::GetInstance()->message_center_tray_delegate()) #elif defined(OS_WIN) code for windows #else NOTIMPLEMENTED() #endif https://codereview.chromium.org/11819048/diff/47005/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.cc (right): https://codereview.chromium.org/11819048/diff/47005/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:89: } Same comment about using the ui:: namespace here. Also, where is this defined for non-ash, non-windows platforms?
https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:50: // |bubble_view_| is unowned. On 2013/01/23 22:07:51, dewittj wrote: > On 2013/01/23 19:52:16, Pete Williamson wrote: > > If bubble view is unowned, how do we know it is always valid (hasn't been > freed) > > when we go to use it? A comment would help. > > The widget owns bubble_view_. In that case, let's change the comment to note that the widget owns the bubble_view (as opposed to the NBWW)
Finished review pass. https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:329: On 2013/01/23 22:07:51, dewittj wrote: > On 2013/01/23 19:52:16, Pete Williamson wrote: > > Note to self - this moved, compare it with the new version in message center > > tray > > Noted. OK, I compared the two implementations as part of my review, no problems found. https://codereview.chromium.org/11819048/diff/47005/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc (right): https://codereview.chromium.org/11819048/diff/47005/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:5: I'm happy to see this unit test. It seems like there is some new code not covered by unit tests in other files - let's add a TODO to write some tests for them. https://codereview.chromium.org/11819048/diff/47005/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:94: message_center::MessageCenter* message_center_; Consider adding a comment: // The message_center pointer is owned by the caller, we do not free it here. https://codereview.chromium.org/11819048/diff/47005/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:105: message_center::MessageCenter* message_center = tray->message_center(); scoped_ptr would be better here, then you can remove the delete call below. (same comment for the other tests) https://codereview.chromium.org/11819048/diff/47005/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:111: EXPECT_TRUE(message_center->notification_list()->HasNotification("test_id1")); Extra credit - you could add EXPECT_FALSE(message_center->notification_list()->HasNotification("test_id2")); https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... File ui/message_center/message_center_tray.cc (right): https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... ui/message_center/message_center_tray.cc:38: message_center_visible_ = delegate_->ShowMessageCenter(bubble); Do we need some error handling here if ShowMessageCenter returns false, or is a silent failure the right thing to do? https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... ui/message_center/message_center_tray.cc:51: void MessageCenterTray::ToggleMessageCenterBubble() { Do we really use this function? If not, this may be a good candidate for removing dead code. https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... ui/message_center/message_center_tray.cc:59: if (message_center_visible_) This seems a bit strange - if we ask for a popup bubble, but the message center is visible instead, we don't give you a popup, we just return. Is that the right behavior? https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... File ui/message_center/message_center_tray.h (right): https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... ui/message_center/message_center_tray.h:24: Put MessageCenterTrayDelegate into its own .h file, since it is used in more places than just this file. Also, let's add any useful comments that we can on each method. Since this is an interface, implementors will come here to see how to implement the interface. https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... ui/message_center/message_center_tray.h:73: Let's add comments about ownership of the message center and delegate pointers. It seems that this class expects them to be valid for the entire lifetime of the class, and for the code that creates this class to enforce that constraint and manage the lifetimes of the delegate and message center, is that right? https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... File ui/message_center/message_center_tray_unittest.cc (right): https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... ui/message_center/message_center_tray_unittest.cc:174: Why does show popup bubble fail here? It looks like we fed it a good notification.
Did a patchset just get deleted? That also deletes any in-progress comments on the patchset, generally better to just upload a new one...
On 2013/01/25 00:11:03, stevenjb (chromium) wrote: > Did a patchset just get deleted? That also deletes any in-progress comments on > the patchset, generally better to just upload a new one... Sorry, I was assuming that you would wait until I hit publish + mail comments. Will upload next patch soon.
Thanks for all the comments, everyone. https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:28: namespace ui { On 2013/01/23 23:14:18, stevenjb (chromium) wrote: > There shouldn't be any code in the ui:: namespace here. Removed. https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:160: return false; On 2013/01/23 23:14:18, stevenjb (chromium) wrote: > {} Done. https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.h:77: void ShowMessageCenter(); On 2013/01/23 23:14:18, stevenjb (chromium) wrote: > This is the same name as ShowMessageCenter below but with a different signature; > it needs to be renamed (e.g. ShowMessageCenterBubble or > ShowMessageCenterForTray). Removed. https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray_unittest.cc (right): https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notificati... ash/system/web_notification/web_notification_tray_unittest.cc:236: // tray->GetPopupBubbleForTest()->NumMessageViewsForTest()); On 2013/01/23 23:14:18, stevenjb (chromium) wrote: > ?? Done. https://codereview.chromium.org/11819048/diff/47005/chrome/browser/notificati... File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/11819048/diff/47005/chrome/browser/notificati... chrome/browser/notifications/message_center_notification_manager.cc:24: tray_.reset(ui::MessageCenterTrayDelegate::CreateForPlatform()); On 2013/01/23 23:14:18, stevenjb (chromium) wrote: > I think this should use #if USE_ASH... relying on correct linkage for code > that's in the ui:: namespace but defined in src/ash is ugly at best. I would do > something like: > > #if defined(USE_ASH) > tray_.reset(ash::Shell::GetInstance()->message_center_tray_delegate()) > #elif defined(OS_WIN) > code for windows > #else > NOTIMPLEMENTED() > #endif Since on ChromeOS the Shell has the scoped pointer, I will just do nothing there #if OS_CHROMEOS. https://codereview.chromium.org/11819048/diff/47005/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.cc (right): https://codereview.chromium.org/11819048/diff/47005/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:89: } On 2013/01/23 23:14:18, stevenjb (chromium) wrote: > Same comment about using the ui:: namespace here. > Also, where is this defined for non-ash, non-windows platforms? Non-ash non-windows platforms don't get message center code, at the moment. I could wrap that with an #ifdef ENABLE_MESSAGE_CENTER but it'd be redundant with the _win in the filename. Namespace ui removed. https://codereview.chromium.org/11819048/diff/47005/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc (right): https://codereview.chromium.org/11819048/diff/47005/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:5: On 2013/01/24 19:41:59, Pete Williamson wrote: > I'm happy to see this unit test. It seems like there is some new code not > covered by unit tests in other files - let's add a TODO to write some tests for > them. Done. https://codereview.chromium.org/11819048/diff/47005/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:94: message_center::MessageCenter* message_center_; On 2013/01/24 19:41:59, Pete Williamson wrote: > Consider adding a comment: > // The message_center pointer is owned by the caller, we do not free it here. Done. https://codereview.chromium.org/11819048/diff/47005/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:105: message_center::MessageCenter* message_center = tray->message_center(); On 2013/01/24 19:41:59, Pete Williamson wrote: > scoped_ptr would be better here, then you can remove the delete call below. > (same comment for the other tests) Done. https://codereview.chromium.org/11819048/diff/47005/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:111: EXPECT_TRUE(message_center->notification_list()->HasNotification("test_id1")); On 2013/01/24 19:41:59, Pete Williamson wrote: > Extra credit - you could add > EXPECT_FALSE(message_center->notification_list()->HasNotification("test_id2")); Done. https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... File ui/message_center/message_center_tray.cc (right): https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... ui/message_center/message_center_tray.cc:38: message_center_visible_ = delegate_->ShowMessageCenter(bubble); On 2013/01/24 19:41:59, Pete Williamson wrote: > Do we need some error handling here if ShowMessageCenter returns false, or is a > silent failure the right thing to do? At this point I'm leaving error handling up to the delegate class. There is no error here, just the internal state is not updated. https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... ui/message_center/message_center_tray.cc:51: void MessageCenterTray::ToggleMessageCenterBubble() { On 2013/01/24 19:41:59, Pete Williamson wrote: > Do we really use this function? If not, this may be a good candidate for > removing dead code. It is used in WebNotificationTrayWin. https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... ui/message_center/message_center_tray.cc:59: if (message_center_visible_) On 2013/01/24 19:41:59, Pete Williamson wrote: > This seems a bit strange - if we ask for a popup bubble, but the message center > is visible instead, we don't give you a popup, we just return. Is that the > right behavior? Yes, though I should probably throw in an UpdateMessageCenter and a comment. Done. https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... File ui/message_center/message_center_tray.h (right): https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... ui/message_center/message_center_tray.h:24: On 2013/01/24 19:41:59, Pete Williamson wrote: > Put MessageCenterTrayDelegate into its own .h file, since it is used in more > places than just this file. > Also, let's add any useful comments that we can on each method. Since this is > an interface, implementors will come here to see how to implement the interface. Done. https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... ui/message_center/message_center_tray.h:73: On 2013/01/24 19:41:59, Pete Williamson wrote: > Let's add comments about ownership of the message center and delegate pointers. > It seems that this class expects them to be valid for the entire lifetime of the > class, and for the code that creates this class to enforce that constraint and > manage the lifetimes of the delegate and message center, is that right? Done. https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... File ui/message_center/message_center_tray_unittest.cc (right): https://codereview.chromium.org/11819048/diff/47005/ui/message_center/message... ui/message_center/message_center_tray_unittest.cc:174: On 2013/01/24 19:41:59, Pete Williamson wrote: > Why does show popup bubble fail here? It looks like we fed it a good > notification. The delegate was configured to return false for ShowPopups on line 158, so the MessageCenterTray should register them as not visible even after a ShowPopupBubble call.
https://codereview.chromium.org/11819048/diff/55013/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/55013/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:142: message_center_bubble_->bubble()->ScheduleUpdate(); Unless there's a good reason not to, you might want to be consistent about the use of the getter rather than also accessing the member variable directly. https://codereview.chromium.org/11819048/diff/55013/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:229: OnMessageCenterTrayChanged(); Would it make sense to create a message_center_tray setter that automatically called this notifier? Might prevent a bug in the future. https://codereview.chromium.org/11819048/diff/55013/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray_unittest.cc (right): https://codereview.chromium.org/11819048/diff/55013/ash/system/web_notificati... ash/system/web_notification/web_notification_tray_unittest.cc:33: // Must be called only when expecting a clean message center. I'm not sure what this means. Is there a way to express it programmatically through DCHECKs? https://codereview.chromium.org/11819048/diff/55013/chrome/browser/notificati... File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/11819048/diff/55013/chrome/browser/notificati... chrome/browser/notifications/message_center_notification_manager.cc:25: tray_.reset(message_center::CreateMessageCenterTray()); I don't have all the context I need to know for sure, but you might be able to put a NOT_REACHED in the CrOS implementation of CreateMessageCenterTray(), which will explode more informatively if someone does the wrong thing. It's possible that someone else calls this method, though, in which case ignore this suggestion. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc (right): https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:64: bubble_widget_ = NULL; If you feel strongly about this NULL, go ahead and leave it, but given that it's in the destructor and no dependent code could run after it, it is probably just a wasted couple of bytes. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:69: bubble_widget_->RemoveObserver(this); Did you want to call Close() after this? https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h (right): https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:31: void OnWidgetClosing(views::Widget* widget); Add OVERRIDE keyword to get the compiler to help check this. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:34: virtual void BubbleViewDestroyed(); Same down here. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.cc (right): https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:41: // whose behavior depends on where the Windows taskbar is. If it is on the One space after period. http://www.slate.com/articles/technology/technology/2011/01/space_invaders.si... https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:73: rb.GetImageSkiaNamed(IDR_ALLOWED_NOTIFICATION); Consider a DCHECK(icon) here to isolate the failure to a single line. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:104: StatusTray * status_tray = g_browser_process->status_tray(); oops https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:119: } vertical whitespace https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:172: return message_center_anchor_rect_; no braces needed https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.h (right): https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.h:54: virtual gfx::Rect GetAnchorRect( It's unclear from the comments whether these should have OVERRIDE keywords. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc (right): https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:4: remove extra vertical space https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:36: } vertical whitespace https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:43: virtual void NotificationRemoved(const std::string& notifcation_id) { spelling (and elsewhere) https://codereview.chromium.org/11819048/diff/55013/ui/message_center/message... File ui/message_center/message_center_tray.cc (right): https://codereview.chromium.org/11819048/diff/55013/ui/message_center/message... ui/message_center/message_center_tray.cc:27: void MessageCenterTray::ShowMessageCenterBubble() { This isn't a big deal, but I wonder why only hide returns the boolean indicating whether the state changed. I'm sure the answer is because nobody cares about show, but the asymmetry is notable. https://codereview.chromium.org/11819048/diff/55013/ui/message_center/message... ui/message_center/message_center_tray.cc:34: message_center_visible_ = delegate_->ShowMessageCenter(); Is there any chance this could fail? Could message_center_'s idea of visibility and message_center_visible_ get out of sync? https://codereview.chromium.org/11819048/diff/55013/ui/message_center/message... File ui/message_center/message_center_tray.h (right): https://codereview.chromium.org/11819048/diff/55013/ui/message_center/message... ui/message_center/message_center_tray.h:23: // Class that observes a MessageCenter. Manages the popup and message center That Slate article again https://codereview.chromium.org/11819048/diff/55013/ui/message_center/message... File ui/message_center/message_center_tray_delegate.h (right): https://codereview.chromium.org/11819048/diff/55013/ui/message_center/message... ui/message_center/message_center_tray_delegate.h:14: virtual ~MessageCenterTrayDelegate() {}; Good! http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts
Thanks miket for the helpful review. https://codereview.chromium.org/11819048/diff/55013/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/55013/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:142: message_center_bubble_->bubble()->ScheduleUpdate(); On 2013/01/25 17:14:48, miket wrote: > Unless there's a good reason not to, you might want to be consistent about the > use of the getter rather than also accessing the member variable directly. Done. https://codereview.chromium.org/11819048/diff/55013/ash/system/web_notificati... ash/system/web_notification/web_notification_tray.cc:229: OnMessageCenterTrayChanged(); On 2013/01/25 17:14:48, miket wrote: > Would it make sense to create a message_center_tray setter that automatically > called this notifier? Might prevent a bug in the future. In this case, OnMessageCenterTrayChanged is not acting as a notifier. However, with a reordering of the code we don't need to manually call this method anymore. https://codereview.chromium.org/11819048/diff/55013/ash/system/web_notificati... File ash/system/web_notification/web_notification_tray_unittest.cc (right): https://codereview.chromium.org/11819048/diff/55013/ash/system/web_notificati... ash/system/web_notification/web_notification_tray_unittest.cc:33: // Must be called only when expecting a clean message center. On 2013/01/25 17:14:48, miket wrote: > I'm not sure what this means. Is there a way to express it programmatically > through DCHECKs? This is a vestigal comment from when the message center was a singleton that could outlive individual tests. Removed. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/notificati... File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/11819048/diff/55013/chrome/browser/notificati... chrome/browser/notifications/message_center_notification_manager.cc:25: tray_.reset(message_center::CreateMessageCenterTray()); On 2013/01/25 17:14:48, miket wrote: > I don't have all the context I need to know for sure, but you might be able to > put a NOT_REACHED in the CrOS implementation of CreateMessageCenterTray(), which > will explode more informatively if someone does the wrong thing. It's possible > that someone else calls this method, though, in which case ignore this > suggestion. Done. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc (right): https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:64: bubble_widget_ = NULL; On 2013/01/25 17:14:48, miket wrote: > If you feel strongly about this NULL, go ahead and leave it, but given that it's > in the destructor and no dependent code could run after it, it is probably just > a wasted couple of bytes. Done. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:69: bubble_widget_->RemoveObserver(this); On 2013/01/25 17:14:48, miket wrote: > Did you want to call Close() after this? I was under the impression that the widget is already in the process of being closed. I'll take another look. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h (right): https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:31: void OnWidgetClosing(views::Widget* widget); On 2013/01/25 17:14:48, miket wrote: > Add OVERRIDE keyword to get the compiler to help check this. Done. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:34: virtual void BubbleViewDestroyed(); On 2013/01/25 17:14:48, miket wrote: > Same down here. Done. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.cc (right): https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:41: // whose behavior depends on where the Windows taskbar is. If it is on the On 2013/01/25 17:14:48, miket wrote: > One space after period. > http://www.slate.com/articles/technology/technology/2011/01/space_invaders.si... I'll change it but note the paragraph in the middle of the article: 'Monospaced type gives you text that looks "loose" and uneven; there's a lot of white space between characters and words, so it's more difficult to spot the spaces between sentences immediately. Hence the adoption of the two-space rule—on a typewriter, an extra space after a sentence makes text easier to read.' Interestingly, we use monospaced fonts in code. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:73: rb.GetImageSkiaNamed(IDR_ALLOWED_NOTIFICATION); On 2013/01/25 17:14:48, miket wrote: > Consider a DCHECK(icon) here to isolate the failure to a single line. Done. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:104: StatusTray * status_tray = g_browser_process->status_tray(); On 2013/01/25 17:14:48, miket wrote: > oops Done. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:119: } On 2013/01/25 17:14:48, miket wrote: > vertical whitespace Done. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:172: return message_center_anchor_rect_; On 2013/01/25 17:14:48, miket wrote: > no braces needed Done. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.h (right): https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.h:54: virtual gfx::Rect GetAnchorRect( On 2013/01/25 17:14:48, miket wrote: > It's unclear from the comments whether these should have OVERRIDE keywords. Comments updated - they are not actually overrides. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc (right): https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:4: On 2013/01/25 17:14:48, miket wrote: > remove extra vertical space Done. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:36: } On 2013/01/25 17:14:48, miket wrote: > vertical whitespace Done. https://codereview.chromium.org/11819048/diff/55013/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:43: virtual void NotificationRemoved(const std::string& notifcation_id) { On 2013/01/25 17:14:48, miket wrote: > spelling (and elsewhere) Notification is one of the hardest words to spell on a qwerty keyboard. Done https://codereview.chromium.org/11819048/diff/55013/ui/message_center/message... File ui/message_center/message_center_tray.cc (right): https://codereview.chromium.org/11819048/diff/55013/ui/message_center/message... ui/message_center/message_center_tray.cc:27: void MessageCenterTray::ShowMessageCenterBubble() { On 2013/01/25 17:14:48, miket wrote: > This isn't a big deal, but I wonder why only hide returns the boolean indicating > whether the state changed. I'm sure the answer is because nobody cares about > show, but the asymmetry is notable. Hm, maybe it would be useful to return the answer in tests. Good comment. https://codereview.chromium.org/11819048/diff/55013/ui/message_center/message... ui/message_center/message_center_tray.cc:34: message_center_visible_ = delegate_->ShowMessageCenter(); On 2013/01/25 17:14:48, miket wrote: > Is there any chance this could fail? Could message_center_'s idea of visibility > and message_center_visible_ get out of sync? I suppose there is a chance. It can fail when the screen is locked on ChromeOS, for example. Fixed. https://codereview.chromium.org/11819048/diff/55013/ui/message_center/message... File ui/message_center/message_center_tray.h (right): https://codereview.chromium.org/11819048/diff/55013/ui/message_center/message... ui/message_center/message_center_tray.h:23: // Class that observes a MessageCenter. Manages the popup and message center On 2013/01/25 17:14:48, miket wrote: > That Slate article again Done. https://codereview.chromium.org/11819048/diff/55013/ui/message_center/message... File ui/message_center/message_center_tray_delegate.h (right): https://codereview.chromium.org/11819048/diff/55013/ui/message_center/message... ui/message_center/message_center_tray_delegate.h:14: virtual ~MessageCenterTrayDelegate() {}; On 2013/01/25 17:14:48, miket wrote: > Good! http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts Thanks.
> > Did you want to call Close() after this? > > I was under the impression that the widget is already in the process of being > closed. I'll take another look. Don't look too hard -- I was just noticing the difference between this code and the earlier, similar code. You're probably right (though maybe a comment would help relatively uninformed reviewers like me). > I'll change it but note the paragraph in the middle of the article: > 'Monospaced type gives you text that looks "loose" and uneven; there's a lot of > white space between characters and words, so it's more difficult to spot the > spaces between sentences immediately. Hence the adoption of the two-space > rule—on a typewriter, an extra space after a sentence makes text easier to > read.' > > Interestingly, we use monospaced fonts in code. I once followed the same line of reasoning, and it caused me an almost HAL 9000-level of internal conflict, until I found out the monospace justification wasn't entirely true: http://www.creativepro.com/article/double-space-or-not-double-space. The idea of giant spaces between sentences far predates monospaced typewriters; the idea of two typewritten spaces was introduced to mimic the prevailing visual style at the time. Today it seems that one space after a period is a universal rule (universal in the sense that you can safely follow it in code, prose, layout, etc., not necessarily that everyone agrees with it), which is a practical argument in favor of following it -- you don't have to switch styles depending on the intra-letter spacing of your current typeface. As far as style goes in terms of amount of space after a period, the prevailing style appears to be a small amount. Which I'd argue is a tiebreaker. YOUR MOVE. LGTM.
lgtm
lgtm
added msw: ui/views/bubble/tray_bubble_view.* sky: chrome/browser/ui/views/* Thanks
Steven (and Jenny?) are better reviewers for ui/views/bubble/tray_bubble_view.*, but those files lgtm with nits. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.cc (right): https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:185: if (work_area.height() < screen_bounds.height()) nit: Offhand, this is confusing, why are you comparing the primary screen's bounds and work area? Can you add a simple comment explaining what this is doing, like: "If <x> then align bubbles like <y>". https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.h (right): https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.h:57: virtual gfx::Rect GetAnchorRect( Why are these three functions virtual? They don't appear to be overridden or overriding. https://codereview.chromium.org/11819048/diff/48049/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.h (right): https://codereview.chromium.org/11819048/diff/48049/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.h:40: enum AnchorAlignment { nit: Add a comment explaining the meaning of the enum/values.
https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc (right): https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:30: : tray_(tray) { ember initialize other methods to NULL (in case some one early returns). https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:50: bubble_widget_->StackAtTop(); You sure this is needed? Won't SetAlwaysOnTop force the window to the top of the stacking order? https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:68: DCHECK(widget == bubble_widget_); DCHECK_EQ https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:80: }; remove ; https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h (right): https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:26: message_center::MessageBubbleBase* bubble, Pass in as scoped_ptr so ownership if clear. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:28: ~NotificationBubbleWrapperWin(); virtual https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:46: views::TrayBubbleView* bubble_view() const { return bubble_view_; } in general const methods shouldn't return non-const objects. Doing so can effectively allow for treating this object as non-const. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:56: }; DISALLOW_... https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.h (right): https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.h:43: virtual message_center::MessageCenter* message_center(); unix_hacker_style methods shouldn't be virtual. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc (right): https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:33: : message_center_(message_center) { spacing is off here. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:42: // WebNotificationTray::Delegate overrides. Use OVERRIDE. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:105: IN_PROC_BROWSER_TEST_F(WebNotificationTrayWinTest, WebNotifications) { What prevents these tests from being unit tests?
Thanks for the great comments. PTAL, especially discussion about SetAlwaysOnTop and BrowserTest. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc (right): https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:30: : tray_(tray) { On 2013/01/30 14:38:17, sky wrote: > ember initialize other methods to NULL (in case some one early returns). Done. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:50: bubble_widget_->StackAtTop(); On 2013/01/30 14:38:17, sky wrote: > You sure this is needed? Won't SetAlwaysOnTop force the window to the top of the > stacking order? Perhaps this is my relative newness to Windows, but it stems from wanting to display the bubble on top of the taskbar. In the case of the taskbar appearing on the left or right side of the screen, we will need to render it directly above the status icon. The bubble appears beneath the taskbar unless both of those methods have been called. If this is a hack, I'd love to hear a more standard way to do this. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:68: DCHECK(widget == bubble_widget_); On 2013/01/30 14:38:17, sky wrote: > DCHECK_EQ Done. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:80: }; On 2013/01/30 14:38:17, sky wrote: > remove ; Done. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h (right): https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:26: message_center::MessageBubbleBase* bubble, On 2013/01/30 14:38:17, sky wrote: > Pass in as scoped_ptr so ownership if clear. Done. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:28: ~NotificationBubbleWrapperWin(); On 2013/01/30 14:38:17, sky wrote: > virtual Done. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:46: views::TrayBubbleView* bubble_view() const { return bubble_view_; } On 2013/01/30 14:38:17, sky wrote: > in general const methods shouldn't return non-const objects. Doing so can > effectively allow for treating this object as non-const. Done. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h:56: }; On 2013/01/30 14:38:17, sky wrote: > DISALLOW_... Done. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.cc (right): https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.cc:185: if (work_area.height() < screen_bounds.height()) On 2013/01/29 23:23:16, msw wrote: > nit: Offhand, this is confusing, why are you comparing the primary screen's > bounds and work area? Can you add a simple comment explaining what this is > doing, like: "If <x> then align bubbles like <y>". Done. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win.h (right): https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.h:43: virtual message_center::MessageCenter* message_center(); On 2013/01/30 14:38:17, sky wrote: > unix_hacker_style methods shouldn't be virtual. Done. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win.h:57: virtual gfx::Rect GetAnchorRect( On 2013/01/29 23:23:16, msw wrote: > Why are these three functions virtual? > They don't appear to be overridden or overriding. Relic. Removed. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... File chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc (right): https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:33: : message_center_(message_center) { On 2013/01/30 14:38:17, sky wrote: > spacing is off here. Done. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:42: // WebNotificationTray::Delegate overrides. On 2013/01/30 14:38:17, sky wrote: > Use OVERRIDE. Done. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/m... chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc:105: IN_PROC_BROWSER_TEST_F(WebNotificationTrayWinTest, WebNotifications) { On 2013/01/30 14:38:17, sky wrote: > What prevents these tests from being unit tests? Two main reasons: First, they actually create UI surfaces - the status tray icon and the views for the message center. Second, they use the MessageCenter object that is hosted on g_browser_process, which is not recreated between unit tests as far as I know (unless the behavior is different from the ash_unittests target which I'm slightly more familiar with). https://codereview.chromium.org/11819048/diff/48049/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.h (right): https://codereview.chromium.org/11819048/diff/48049/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.h:40: enum AnchorAlignment { On 2013/01/29 23:23:16, msw wrote: > nit: Add a comment explaining the meaning of the enum/values. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dewittj@chromium.org/11819048/81006
Message was sent while issue was closed.
Change committed as 179807 |
