Chromium Code Reviews
Help | Chromium Project | Sign in
(324)

Issue 11819048: Implement message center on Windows (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 1 month ago by dewittj
Modified:
2 years 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.

Description

Implement 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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1373 lines, -204 lines) Patch
M ash/system/web_notification/web_notification_tray.h View 1 2 3 4 5 6 7 8 9 10 8 chunks +27 lines, -44 lines 0 comments Download
M ash/system/web_notification/web_notification_tray.cc View 1 2 3 4 5 6 7 8 9 10 11 14 chunks +126 lines, -129 lines 0 comments Download
M ash/system/web_notification/web_notification_tray_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 10 chunks +32 lines, -24 lines 0 comments Download
M chrome/browser/notifications/message_center_notification_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/notifications/message_center_notification_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +7 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +65 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +109 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/message_center/web_notification_tray_win.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +95 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/message_center/web_notification_tray_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +237 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/message_center/web_notification_tray_win_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +214 lines, -0 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +3 lines, -0 lines 0 comments Download
M ui/message_center/message_center.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +0 lines, -2 lines 0 comments Download
M ui/message_center/message_center.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +4 lines, -0 lines 0 comments Download
A ui/message_center/message_center_tray.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +73 lines, -0 lines 0 comments Download
A ui/message_center/message_center_tray.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +111 lines, -0 lines 0 comments Download
A ui/message_center/message_center_tray_delegate.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +44 lines, -0 lines 0 comments Download
A ui/message_center/message_center_tray_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +208 lines, -0 lines 0 comments Download
M ui/views/bubble/tray_bubble_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +7 lines, -1 line 0 comments Download
M ui/views/bubble/tray_bubble_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +5 lines, -4 lines 0 comments Download
Commit:

Messages

Total messages: 38 (0 generated)
dewittj
Hi guys, I'm trying to get a handle on this change, and would love if ...
2 years, 1 month ago (2013-01-10 19:17:17 UTC) #1
tfarina
On Thu, Jan 10, 2013 at 5:17 PM, <dewittj@chromium.org> wrote: > Reviewers: Pete Williamson, Dmitry ...
2 years, 1 month ago (2013-01-10 20:05:33 UTC) #2
dewittj
Bug number added.
2 years, 1 month ago (2013-01-10 22:04:13 UTC) #3
Pete Williamson
I understand you have a new patch, and you may have already addressed many of ...
2 years, 1 month ago (2013-01-16 19:43:20 UTC) #4
dewittj
A new patch has been uploaded that should address the problems you raised. The primary ...
2 years, 1 month ago (2013-01-16 22:30:40 UTC) #5
dewittj
By the way, this code now depends on https://codereview.chromium.org/11836003/ to render on Windows.
2 years, 1 month ago (2013-01-16 23:17:37 UTC) #6
Pete Williamson
Here are a few comments based on our review together, more later. 1.) Make a ...
2 years, 1 month ago (2013-01-17 19:07:45 UTC) #7
dewittj
Take another look, pulled the windows WebNotificationBubbleWrapper out to its own file. https://codereview.chromium.org/11819048/diff/23022/ash/system/web_notification/web_notification_tray.cc File ash/system/web_notification/web_notification_tray.cc ...
2 years, 1 month ago (2013-01-18 00:57:45 UTC) #8
dewittj
stevenjb: all miket: all sky: gyp, chrome/browser/ui/views This is a pretty big patch, I appreciate ...
2 years, 1 month ago (2013-01-18 19:29:02 UTC) #9
stevenjb
This looks like it includes changes from https://codereview.chromium.org/11958025/? It would be easier to review if ...
2 years, 1 month ago (2013-01-18 19:58:51 UTC) #10
dewittj
On 2013/01/18 19:58:51, stevenjb (chromium) wrote: > This looks like it includes changes from > ...
2 years, 1 month ago (2013-01-18 21:10:21 UTC) #11
sky
I started to look through this, then fitured Steven should be happy first. I'll take ...
2 years, 1 month ago (2013-01-18 21:30:56 UTC) #12
dewittj
On 2013/01/18 21:30:56, sky wrote: > I started to look through this, then fitured Steven ...
2 years, 1 month ago (2013-01-18 21:47:07 UTC) #13
stevenjb
+mukai@ The TrayBubbleView changes need to be tested very carefully in Ash/Chromeos with both the ...
2 years, 1 month ago (2013-01-18 23:11:45 UTC) #14
dewittj
Thanks for the comments, miket, stevenjb, mukai PTAL. The style nits you pointed out and ...
2 years, 1 month ago (2013-01-20 19:02:06 UTC) #15
Jun Mukai
https://codereview.chromium.org/11819048/diff/47001/ash/system/web_notification/web_notification_tray.cc File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/47001/ash/system/web_notification/web_notification_tray.cc#newcode39 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_notification/web_notification_tray.h ...
2 years, 1 month ago (2013-01-22 18:55:34 UTC) #16
dewittj
Thanks mukai. Fixed your comments. https://codereview.chromium.org/11819048/diff/47001/ash/system/web_notification/web_notification_tray.cc File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/47001/ash/system/web_notification/web_notification_tray.cc#newcode39 ash/system/web_notification/web_notification_tray.cc:39: On 2013/01/22 18:55:34, Jun ...
2 years, 1 month ago (2013-01-22 20:49:21 UTC) #17
Jun Mukai
lgtm from nits https://codereview.chromium.org/11819048/diff/52004/ash/system/web_notification/web_notification_tray.h File ash/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/11819048/diff/52004/ash/system/web_notification/web_notification_tray.h#newcode109 ash/system/web_notification/web_notification_tray.h:109: // TODO(dewittj): move these up above ...
2 years, 1 month ago (2013-01-23 19:40:12 UTC) #18
Pete Williamson
I still need to review web_notification_tray_win_browsertest and the message center tray class, but here are ...
2 years, 1 month ago (2013-01-23 19:52:16 UTC) #19
dewittj
PTAL - now there are no dependencies on open bugs. https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notification/web_notification_tray.cc File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notification/web_notification_tray.cc#newcode92 ...
2 years, 1 month ago (2013-01-23 22:07:51 UTC) #20
stevenjb
https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notification/web_notification_tray.cc File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notification/web_notification_tray.cc#newcode28 ash/system/web_notification/web_notification_tray.cc:28: namespace ui { There shouldn't be any code in ...
2 years, 1 month ago (2013-01-23 23:14:18 UTC) #21
Pete Williamson
https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h (right): https://codereview.chromium.org/11819048/diff/43001/chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.h#newcode50 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: ...
2 years, 1 month ago (2013-01-23 23:47:53 UTC) #22
Pete Williamson
Finished review pass. https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notification/web_notification_tray.cc File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/43001/ash/system/web_notification/web_notification_tray.cc#newcode329 ash/system/web_notification/web_notification_tray.cc:329: On 2013/01/23 22:07:51, dewittj wrote: > ...
2 years, 1 month ago (2013-01-24 19:41:59 UTC) #23
stevenjb
Did a patchset just get deleted? That also deletes any in-progress comments on the patchset, ...
2 years, 1 month ago (2013-01-25 00:11:03 UTC) #24
dewittj
On 2013/01/25 00:11:03, stevenjb (chromium) wrote: > Did a patchset just get deleted? That also ...
2 years, 1 month ago (2013-01-25 00:19:33 UTC) #25
dewittj
Thanks for all the comments, everyone. https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notification/web_notification_tray.cc File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/47005/ash/system/web_notification/web_notification_tray.cc#newcode28 ash/system/web_notification/web_notification_tray.cc:28: namespace ui { ...
2 years, 1 month ago (2013-01-25 00:49:04 UTC) #26
miket_OOO
https://codereview.chromium.org/11819048/diff/55013/ash/system/web_notification/web_notification_tray.cc File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/55013/ash/system/web_notification/web_notification_tray.cc#newcode142 ash/system/web_notification/web_notification_tray.cc:142: message_center_bubble_->bubble()->ScheduleUpdate(); Unless there's a good reason not to, you ...
2 years, 1 month ago (2013-01-25 17:14:47 UTC) #27
dewittj
Thanks miket for the helpful review. https://codereview.chromium.org/11819048/diff/55013/ash/system/web_notification/web_notification_tray.cc File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/11819048/diff/55013/ash/system/web_notification/web_notification_tray.cc#newcode142 ash/system/web_notification/web_notification_tray.cc:142: message_center_bubble_->bubble()->ScheduleUpdate(); On 2013/01/25 ...
2 years, 1 month ago (2013-01-25 19:38:46 UTC) #28
miket_OOO
> > Did you want to call Close() after this? > > I was under ...
2 years, 1 month ago (2013-01-25 20:00:22 UTC) #29
stevenjb
lgtm
2 years, 1 month ago (2013-01-28 20:28:41 UTC) #30
Pete Williamson
lgtm
2 years, 1 month ago (2013-01-28 21:43:48 UTC) #31
dewittj
added msw: ui/views/bubble/tray_bubble_view.* sky: chrome/browser/ui/views/* Thanks
2 years ago (2013-01-29 21:25:08 UTC) #32
msw
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/message_center/web_notification_tray_win.cc ...
2 years ago (2013-01-29 23:23:16 UTC) #33
sky
https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc (right): https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc#newcode30 chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc:30: : tray_(tray) { ember initialize other methods to NULL ...
2 years ago (2013-01-30 14:38:17 UTC) #34
dewittj
Thanks for the great comments. PTAL, especially discussion about SetAlwaysOnTop and BrowserTest. https://codereview.chromium.org/11819048/diff/48049/chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc File chrome/browser/ui/views/message_center/notification_bubble_wrapper_win.cc ...
2 years ago (2013-01-30 23:08:32 UTC) #35
sky
LGTM
2 years ago (2013-01-31 01:20:53 UTC) #36
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dewittj@chromium.org/11819048/81006
2 years ago (2013-01-31 01:55:00 UTC) #37
I haz the power (commit-bot)
2 years ago (2013-01-31 06:07:38 UTC) #38
Message was sent while issue was closed.
Change committed as 179807
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87e6a26