Adds a first-run balloon to the Windows notification center.

Adds a first-run balloon to the Windows notification center.

This is designed to attract attention to the system tray icon which
can be hidden in the overflow area.

BUG=246322
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208031

[Andrew T Wilson (Slow), 2013-06-19 06:28:56]

https://codereview.chromium.org/17286015/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/17286015/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/message_center_notification_manager.cc:29: 
Remove blank line, and add a comment for what this is.

https://codereview.chromium.org/17286015/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/message_center_notification_manager.cc:59: return;
I think it's better to make the entire method #if defined(OS_WIN) here and below
- no reason to include code that's never going to get executed.

https://codereview.chromium.org/17286015/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/message_center_notification_manager.cc:540: if
(!tray_.get() || first_run_pref_.GetValue())
What's the case where tray_ is null? Can we ever *not* have a tray on windows?

https://codereview.chromium.org/17286015/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/message_center_notification_manager.h (right):

https://codereview.chromium.org/17286015/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/message_center_notification_manager.h:34: public
base::SupportsWeakPtr<MessageCenterNotificationManager> {
Unless external classes need to be able to create WeakPtrs for your class, I
prefer using an internal WeakPtrFactory instead of inheriting from
SupportsWeakPtr<>.

https://codereview.chromium.org/17286015/diff/1/ui/base/strings/ui_strings.grd
File ui/base/strings/ui_strings.grd (right):

https://codereview.chromium.org/17286015/diff/1/ui/base/strings/ui_strings.gr...
ui/base/strings/ui_strings.grd:492: <ph name="product">$1<ex>Chrome</ex></ph>
Notification Center
Not certain but perhaps these should be in chromium_strings.grd and
google_chrome_strings.grd insead of having the product name embedded as a
parameter?

[dewittj, 2013-06-20 02:20:54]

Thanks atwilson for the pre-review.  PTAL

atwilson: status_icons, or more if you like
sky: chrome/browser/ui/views/, gyp
dimich: all

https://codereview.chromium.org/17286015/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/17286015/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/message_center_notification_manager.cc:29: 
On 2013/06/19 06:28:57, Andrew T Wilson wrote:
> Remove blank line, and add a comment for what this is.

Done.

https://codereview.chromium.org/17286015/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/message_center_notification_manager.cc:59: return;
On 2013/06/19 06:28:57, Andrew T Wilson wrote:
> I think it's better to make the entire method #if defined(OS_WIN) here and
below
> - no reason to include code that's never going to get executed.

Done.

https://codereview.chromium.org/17286015/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/message_center_notification_manager.cc:540: if
(!tray_.get() || first_run_pref_.GetValue())
On 2013/06/19 06:28:57, Andrew T Wilson wrote:
> What's the case where tray_ is null? Can we ever *not* have a tray on windows?

There is no case where tray_ is null.  I was just being extra cautious in case
that ever happens.

https://codereview.chromium.org/17286015/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/message_center_notification_manager.h (right):

https://codereview.chromium.org/17286015/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/message_center_notification_manager.h:34: public
base::SupportsWeakPtr<MessageCenterNotificationManager> {
On 2013/06/19 06:28:57, Andrew T Wilson wrote:
> Unless external classes need to be able to create WeakPtrs for your class, I
> prefer using an internal WeakPtrFactory instead of inheriting from
> SupportsWeakPtr<>.

Done.

https://codereview.chromium.org/17286015/diff/1/ui/base/strings/ui_strings.grd
File ui/base/strings/ui_strings.grd (right):

https://codereview.chromium.org/17286015/diff/1/ui/base/strings/ui_strings.gr...
ui/base/strings/ui_strings.grd:492: <ph name="product">$1<ex>Chrome</ex></ph>
Notification Center
On 2013/06/19 06:28:57, Andrew T Wilson wrote:
> Not certain but perhaps these should be in chromium_strings.grd and
> google_chrome_strings.grd insead of having the product name embedded as a
> parameter?

Done.

[sky, 2013-06-20 17:22:01]

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/web_notification_tray.cc (right):

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.cc:282: Browser*
browser = chrome::FindOrCreateTabbedBrowser(
If this creates a new browser who shows it?

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/web_notification_tray.h (right):

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.h:91: bool
shown_status_balloon_;
I don't see you using this any where.

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/status_icons/status_icon_win.cc (right):

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_icons/status_icon_win.cc:66: void
StatusIconWin::HandleBalloonClickEvent(const gfx::Point& cursor_pos) {
You don't use cursor_pos, can it be removed?

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc (right):

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc:25:
MOCK_METHOD0(OnBalloonClicked, void());
gmock is generally frowned upon in chrome.

[dewittj, 2013-06-20 17:51:51]

Thanks, removed GMock from both test files I touched.

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/web_notification_tray.cc (right):

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.cc:282: Browser*
browser = chrome::FindOrCreateTabbedBrowser(
On 2013/06/20 17:22:01, sky wrote:
> If this creates a new browser who shows it?

line 285 shows a tab in the browser.  But it's possible I don't understand your
question.

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/web_notification_tray.h (right):

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.h:91: bool
shown_status_balloon_;
On 2013/06/20 17:22:01, sky wrote:
> I don't see you using this any where.

Done.

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/status_icons/status_icon_win.cc (right):

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_icons/status_icon_win.cc:66: void
StatusIconWin::HandleBalloonClickEvent(const gfx::Point& cursor_pos) {
On 2013/06/20 17:22:01, sky wrote:
> You don't use cursor_pos, can it be removed?

I suppose I was going for symmetry.  Done.

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc (right):

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc:25:
MOCK_METHOD0(OnBalloonClicked, void());
On 2013/06/20 17:22:01, sky wrote:
> gmock is generally frowned upon in chrome.

Okay, I was following the existing style but can easily not mock this test.

[sky, 2013-06-20 17:56:27]

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/web_notification_tray.cc (right):

https://codereview.chromium.org/17286015/diff/12001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.cc:282: Browser*
browser = chrome::FindOrCreateTabbedBrowser(
On 2013/06/20 17:51:51, dewittj wrote:
> On 2013/06/20 17:22:01, sky wrote:
> > If this creates a new browser who shows it?
> 
> line 285 shows a tab in the browser.  But it's possible I don't understand
your
> question.

That shows the tab, but if FindOrCreateTabbedBrowser creates a new browser it is
*not* visible.

[dewittj, 2013-06-20 18:20:35]

ShowSingletonTab internally creates a NavigateParams with window_action =
NavigateParams::SHOW_WINDOW.  Then in BrowserNavigator the
ScopedBrowserDisplayer will call browser->window()->Show().

[sky, 2013-06-20 20:07:59]

Excellent, LGTM

[Andrew T Wilson (Slow), 2013-06-20 21:08:58]

LGTM with a couple suggestions which you can act on or ignore as you see fit.

https://chromiumcodereview.appspot.com/17286015/diff/40001/chrome/browser/not...
File chrome/browser/notifications/message_center_notification_manager.h (right):

https://chromiumcodereview.appspot.com/17286015/diff/40001/chrome/browser/not...
chrome/browser/notifications/message_center_notification_manager.h:207:
base::OneShotTimer<MessageCenterNotificationManager> first_run_balloon_timer_;
So all this stuff is windows-only -- can we #if this stuff out also?

https://chromiumcodereview.appspot.com/17286015/diff/40001/chrome/browser/ui/...
File chrome/browser/ui/views/status_icons/status_tray_win.cc (right):

https://chromiumcodereview.appspot.com/17286015/diff/40001/chrome/browser/ui/...
chrome/browser/ui/views/status_icons/status_tray_win.cc:75:
gfx::Screen::GetNativeScreen()->GetCursorScreenPoint());
Do we really need to move the cursor_pos stuff up here, since it's not used for
HandleBalloonClickEvent?

https://chromiumcodereview.appspot.com/17286015/diff/40001/chrome/browser/ui/...
File chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc (right):

https://chromiumcodereview.appspot.com/17286015/diff/40001/chrome/browser/ui/...
chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc:77:
tray.WndProc(NULL, icon->message_id(), icon->icon_id(), WM_USER + 5);
The WM_USER + 5 seems a bit kludgey here. Is there any way to make this any
cleaner/symbolic?

[dewittj, 2013-06-21 20:35:15]

https://chromiumcodereview.appspot.com/17286015/diff/40001/chrome/browser/ui/...
File chrome/browser/ui/views/status_icons/status_tray_win.cc (right):

https://chromiumcodereview.appspot.com/17286015/diff/40001/chrome/browser/ui/...
chrome/browser/ui/views/status_icons/status_tray_win.cc:75:
gfx::Screen::GetNativeScreen()->GetCursorScreenPoint());
On 2013/06/20 21:08:59, Andrew T Wilson wrote:
> Do we really need to move the cursor_pos stuff up here, since it's not used
for
> HandleBalloonClickEvent?

Done.

https://chromiumcodereview.appspot.com/17286015/diff/40001/chrome/browser/ui/...
File chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc (right):

https://chromiumcodereview.appspot.com/17286015/diff/40001/chrome/browser/ui/...
chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc:77:
tray.WndProc(NULL, icon->message_id(), icon->icon_id(), WM_USER + 5);
On 2013/06/20 21:08:59, Andrew T Wilson wrote:
> The WM_USER + 5 seems a bit kludgey here. Is there any way to make this any
> cleaner/symbolic?

Done.  Unfortunately TB_INDETERMINATE doesn't seem to be defined on win-aura
builds, so I can't use the name of the signal.  I replaced it here with a const
UINT.

[Dmitry Titov, 2013-06-21 20:53:40]

lgtm with notes.

Please make sure the help page about notifications will have actual content.

https://codereview.chromium.org/17286015/diff/76001/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notifications_unittest.cc
(right):

https://codereview.chromium.org/17286015/diff/76001/chrome/browser/notificati...
chrome/browser/notifications/message_center_notifications_unittest.cc:96: const
::Notification GetANotification(const std::string& id) {
Get->Create?

can also return a const ref, for the sake of consistency with other code...

https://codereview.chromium.org/17286015/diff/76001/chrome/browser/notificati...
chrome/browser/notifications/message_center_notifications_unittest.cc:151:
TestTimeouts::tiny_timeout() + base::TimeDelta::FromMilliseconds(10));
tests based on timeouts are not welcome in Chrome. Especially if the ode in
question also is implemented using timeout. Is it possible to make it more
deterministic? Can be a separate CL.

https://codereview.chromium.org/17286015/diff/76001/chrome/browser/status_ico...
File chrome/browser/status_icons/status_icon_observer.h (right):

https://codereview.chromium.org/17286015/diff/76001/chrome/browser/status_ico...
chrome/browser/status_icons/status_icon_observer.h:19: // Called when the user
clicks on a balloon generated for a system tray icon.
surround by #if (OS_WIN)?

https://codereview.chromium.org/17286015/diff/76001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/status_icons/status_tray_win.cc (right):

https://codereview.chromium.org/17286015/diff/76001/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_icons/status_tray_win.cc:15: // This message is
TB_INDETERMINATE, fired when the balloon is clicked.
Is there a link to msdn or some other source that can be used to verify/update?
Where this knowledge came from?

[dewittj, 2013-06-21 21:56:51]

https://codereview.chromium.org/17286015/diff/40001/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.h (right):

https://codereview.chromium.org/17286015/diff/40001/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.h:207:
base::OneShotTimer<MessageCenterNotificationManager> first_run_balloon_timer_;
On 2013/06/20 21:08:59, Andrew T Wilson wrote:
> So all this stuff is windows-only -- can we #if this stuff out also?

Done.

https://codereview.chromium.org/17286015/diff/76001/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notifications_unittest.cc
(right):

https://codereview.chromium.org/17286015/diff/76001/chrome/browser/notificati...
chrome/browser/notifications/message_center_notifications_unittest.cc:96: const
::Notification GetANotification(const std::string& id) {
On 2013/06/21 20:53:40, Dmitry Titov wrote:
> Get->Create?
> 
> can also return a const ref, for the sake of consistency with other code...

Actually it can't since that would return a reference to a stack frame that no
longer exists.

https://codereview.chromium.org/17286015/diff/76001/chrome/browser/notificati...
chrome/browser/notifications/message_center_notifications_unittest.cc:151:
TestTimeouts::tiny_timeout() + base::TimeDelta::FromMilliseconds(10));
On 2013/06/21 20:53:40, Dmitry Titov wrote:
> tests based on timeouts are not welcome in Chrome. Especially if the ode in
> question also is implemented using timeout. Is it possible to make it more
> deterministic? Can be a separate CL.

Done.

https://codereview.chromium.org/17286015/diff/76001/chrome/browser/status_ico...
File chrome/browser/status_icons/status_icon_observer.h (right):

https://codereview.chromium.org/17286015/diff/76001/chrome/browser/status_ico...
chrome/browser/status_icons/status_icon_observer.h:19: // Called when the user
clicks on a balloon generated for a system tray icon.
On 2013/06/21 20:53:40, Dmitry Titov wrote:
> surround by #if (OS_WIN)?

Done.

https://codereview.chromium.org/17286015/diff/76001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/status_icons/status_tray_win.cc (right):

https://codereview.chromium.org/17286015/diff/76001/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_icons/status_tray_win.cc:15: // This message is
TB_INDETERMINATE, fired when the balloon is clicked.
On 2013/06/21 20:53:40, Dmitry Titov wrote:
> Is there a link to msdn or some other source that can be used to
verify/update?
> Where this knowledge came from? 

Instead, I found the header file and included that properly.

[commit-bot: I haz the power, 2013-06-21 21:57:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/17286015/77028

[commit-bot: I haz the power, 2013-06-22 01:29:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/17286015/77028

[commit-bot: I haz the power, 2013-06-22 02:38:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/17286015/77028

[commit-bot: I haz the power, 2013-06-22 14:10:52]

Change committed as 208031