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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 12052057: Introduces 'context' param to NotifierSettingsView. (Closed)

Created:
7 years, 11 months ago by Jun Mukai
Modified:
7 years, 11 months ago
CC:
chromium-reviews, tfarina, sadrul, ben+watch_chromium.org, dewittj
Visibility:
Public.

Description

Introduces 'context' param to NotifierSettingsView. I don't know why the existing code was once running on my local environment, but 'context' parameter in WidgetParams is mandatory in Aura. To specify the context properly, the best way would be to add another method to show the settings window with the current context. BUG=None Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178815

Patch Set 1 #

Total comments: 4

Patch Set 2 : #

Patch Set 3 : fix a compile error happened by a latest CL #

Total comments: 2

Patch Set 4 : #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+50 lines, -6 lines) Patch
M ash/system/web_notification/web_notification_tray_unittest.cc View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/notifications/message_center_notification_manager.h View 1 2 2 chunks +1 line, -2 lines 0 comments Download
M chrome/browser/notifications/message_center_notification_manager.cc View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/ash/balloon_collection_impl_ash.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc View 2 chunks +10 lines, -0 lines 2 comments Download
M ui/message_center/message_center.h View 1 2 3 chunks +6 lines, -0 lines 0 comments Download
M ui/message_center/message_center.cc View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M ui/message_center/message_center_bubble.cc View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M ui/message_center/notification_list.h View 1 2 2 chunks +5 lines, -1 line 0 comments Download
M ui/message_center/notification_list_unittest.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M ui/message_center/notifier_settings.h View 2 chunks +3 lines, -1 line 0 comments Download
M ui/message_center/notifier_settings.cc View 3 chunks +5 lines, -2 lines 0 comments Download

Messages

Total messages: 15 (0 generated)
Jun Mukai
7 years, 11 months ago (2013-01-24 01:32:44 UTC) #1
Daniel Erat
lgtm https://codereview.chromium.org/12052057/diff/1/ui/message_center/message_center.h File ui/message_center/message_center.h (right): https://codereview.chromium.org/12052057/diff/1/ui/message_center/message_center.h#newcode62 ui/message_center/message_center.h:62: virtual void ShowSettingsDialog(gfx::NativeView context) = 0; nit: add ...
7 years, 11 months ago (2013-01-24 03:16:21 UTC) #2
Jun Mukai
https://codereview.chromium.org/12052057/diff/1/ui/message_center/message_center.h File ui/message_center/message_center.h (right): https://codereview.chromium.org/12052057/diff/1/ui/message_center/message_center.h#newcode62 ui/message_center/message_center.h:62: virtual void ShowSettingsDialog(gfx::NativeView context) = 0; On 2013/01/24 03:16:21, ...
7 years, 11 months ago (2013-01-24 04:04:29 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/12052057/12
7 years, 11 months ago (2013-01-24 18:27:39 UTC) #4
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 11 months ago (2013-01-24 19:02:59 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/12052057/10003
7 years, 11 months ago (2013-01-24 19:58:54 UTC) #6
commit-bot: I haz the power
Presubmit check for 12052057-10003 failed and returned exit status 1. Running presubmit commit checks ...
7 years, 11 months ago (2013-01-24 19:59:05 UTC) #7
Jun Mukai
stevenjb, can you review this? I need your approval for chrome/browser/notifications files especially.
7 years, 11 months ago (2013-01-24 20:34:47 UTC) #8
stevenjb
lgtm https://chromiumcodereview.appspot.com/12052057/diff/10003/ui/message_center/message_center_bubble.cc File ui/message_center/message_center_bubble.cc (right): https://chromiumcodereview.appspot.com/12052057/diff/10003/ui/message_center/message_center_bubble.cc#newcode226 ui/message_center/message_center_bubble.cc:226: // TODO(mukai): Replace ShowNotificationSettings by s/by/with
7 years, 11 months ago (2013-01-25 00:18:05 UTC) #9
Jun Mukai
https://codereview.chromium.org/12052057/diff/10003/ui/message_center/message_center_bubble.cc File ui/message_center/message_center_bubble.cc (right): https://codereview.chromium.org/12052057/diff/10003/ui/message_center/message_center_bubble.cc#newcode226 ui/message_center/message_center_bubble.cc:226: // TODO(mukai): Replace ShowNotificationSettings by On 2013/01/25 00:18:05, stevenjb ...
7 years, 11 months ago (2013-01-25 00:22:35 UTC) #10
miket_OOO
lgtm https://codereview.chromium.org/12052057/diff/10004/chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc File chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc (right): https://codereview.chromium.org/12052057/diff/10004/chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc#newcode92 chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc:92: } else { No braces, don't really care.
7 years, 11 months ago (2013-01-25 00:32:10 UTC) #11
Jun Mukai
https://codereview.chromium.org/12052057/diff/10004/chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc File chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc (right): https://codereview.chromium.org/12052057/diff/10004/chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc#newcode92 chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc:92: } else { On 2013/01/25 00:32:10, miket wrote: > ...
7 years, 11 months ago (2013-01-25 00:37:50 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/12052057/10004
7 years, 11 months ago (2013-01-25 00:41:39 UTC) #13
commit-bot: I haz the power
Change committed as 178815
7 years, 11 months ago (2013-01-25 10:40:12 UTC) #14
miket_OOO
7 years, 11 months ago (2013-01-25 14:08:16 UTC) #15
Message was sent while issue was closed. 
> hmm, else-clause has two lines (it doesn't fit to one line) so brace is
> necessary here.

Interesting, I've interpreted the rule as being a single statement vs. a single
line of text. But you might very well be right about that; now that I read it,
the style guide does consistently use "statement" and "line" to refer to
different things.

Powered by Google App Engine
This is Rietveld 408576698