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

Issue 11819048: Implement message center on Windows (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 3 months ago by dewittj
Modified:
1 year, 2 months ago
CC:
chromium-reviews_chromium.org, 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) Lint 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 1 errors 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 0 errors 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 ? errors 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 ? errors 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 ? errors 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 4 errors 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 2 errors 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 1 errors 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 1 errors 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 1 errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 1 errors 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 1 errors 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 2 errors 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 1 errors 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 ? errors 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 0 errors Download
Commit:

Messages

Total messages: 38
dewittj
Hi guys, I'm trying to get a handle on this change, and would love if ...
1 year, 3 months ago #1
tfarina
On Thu, Jan 10, 2013 at 5:17 PM, <dewittj@chromium.org> wrote: > Reviewers: Pete Williamson, Dmitry ...
1 year, 3 months ago #2
dewittj
Bug number added.
1 year, 3 months ago #3
Pete Williamson
I understand you have a new patch, and you may have already addressed many of ...
1 year, 3 months ago #4
dewittj
A new patch has been uploaded that should address the problems you raised. The primary ...
1 year, 3 months ago #5
dewittj
By the way, this code now depends on https://codereview.chromium.org/11836003/ to render on Windows.
1 year, 3 months ago #6
Pete Williamson
Here are a few comments based on our review together, more later. 1.) Make a ...
1 year, 3 months ago #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 ...
1 year, 3 months ago #8
dewittj
stevenjb: all miket: all sky: gyp, chrome/browser/ui/views This is a pretty big patch, I appreciate ...
1 year, 3 months ago #9
stevenjb
This looks like it includes changes from https://codereview.chromium.org/11958025/? It would be easier to review if ...
1 year, 3 months ago #10
dewittj
On 2013/01/18 19:58:51, stevenjb (chromium) wrote: > This looks like it includes changes from > ...
1 year, 3 months ago #11
sky
I started to look through this, then fitured Steven should be happy first. I'll take ...
1 year, 3 months ago #12
dewittj
On 2013/01/18 21:30:56, sky wrote: > I started to look through this, then fitured Steven ...
1 year, 3 months ago #13
stevenjb
+mukai@ The TrayBubbleView changes need to be tested very carefully in Ash/Chromeos with both the ...
1 year, 3 months ago #14
dewittj
Thanks for the comments, miket, stevenjb, mukai PTAL. The style nits you pointed out and ...
1 year, 3 months ago #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 ...
1 year, 3 months ago #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 ...
1 year, 3 months ago #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 ...
1 year, 3 months ago #18
Pete Williamson
I still need to review web_notification_tray_win_browsertest and the message center tray class, but here are ...
1 year, 3 months ago #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 ...
1 year, 3 months ago #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 ...
1 year, 3 months ago #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: ...
1 year, 3 months ago #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: > ...
1 year, 3 months ago #23
stevenjb
Did a patchset just get deleted? That also deletes any in-progress comments on the patchset, ...
1 year, 2 months ago #24
dewittj
On 2013/01/25 00:11:03, stevenjb (chromium) wrote: > Did a patchset just get deleted? That also ...
1 year, 2 months ago #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 { ...
1 year, 2 months ago #26
miket
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 ...
1 year, 2 months ago #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 ...
1 year, 2 months ago #28
miket
> > Did you want to call Close() after this? > > I was under ...
1 year, 2 months ago #29
stevenjb
lgtm
1 year, 2 months ago #30
Pete Williamson
lgtm
1 year, 2 months ago #31
dewittj
added msw: ui/views/bubble/tray_bubble_view.* sky: chrome/browser/ui/views/* Thanks
1 year, 2 months ago #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 ...
1 year, 2 months ago #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 ...
1 year, 2 months ago #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 ...
1 year, 2 months ago #35
sky
LGTM
1 year, 2 months ago #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
1 year, 2 months ago #37
I haz the power (commit-bot)
1 year, 2 months ago #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 1280:2d3e6564b7b6