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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 10947046: Eliminate kAshNotifyDisabled and SystemNotification (Closed)

Created:
8 years, 3 months ago by stevenjb
Modified:
8 years, 3 months ago
Reviewers:
oshima, tbarzic
CC:
chromium-reviews, mihaip-chromium-reviews_chromium.org, sadrul, nkostylev+watch_chromium.org, ben+watch_chromium.org, tfarina, Aaron Boodman, rginda+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org
Visibility:
Public.

Description

Eliminate kAshNotifyDisabled and SystemNotification BUG=134561 For OWNERS: TBR=ben Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=158305

Patch Set 1 #

Total comments: 2

Patch Set 2 : Explicitly remove notifications before re-showing them. #

Total comments: 2

Patch Set 3 : Fix nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -760 lines) Patch
M ash/ash_switches.h View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M ash/ash_switches.cc View 1 2 1 chunk +0 lines, -3 lines 0 comments Download
M ash/system/power/tray_power.cc View 1 chunk +4 lines, -7 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 1 chunk +0 lines, -7 lines 0 comments Download
M chrome/browser/chromeos/extensions/file_browser_notifications.h View 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/extensions/file_browser_notifications.cc View 1 2 6 chunks +22 lines, -65 lines 0 comments Download
M chrome/browser/chromeos/extensions/file_browser_notifications_unittest.cc View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/chromeos/locale_change_guard.h View 2 chunks +1 line, -2 lines 0 comments Download
M chrome/browser/chromeos/locale_change_guard.cc View 4 chunks +2 lines, -26 lines 0 comments Download
M chrome/browser/chromeos/login/login_utils.cc View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/chromeos/network_message_observer.cc View 3 chunks +7 lines, -32 lines 0 comments Download
D chrome/browser/chromeos/notifications/system_notification.h View 1 chunk +0 lines, -111 lines 0 comments Download
D chrome/browser/chromeos/notifications/system_notification.cc View 1 chunk +0 lines, -147 lines 0 comments Download
D chrome/browser/chromeos/notifications/system_notification_factory.h View 1 chunk +0 lines, -35 lines 0 comments Download
D chrome/browser/chromeos/notifications/system_notification_factory.cc View 1 chunk +0 lines, -58 lines 0 comments Download
D chrome/browser/chromeos/power/low_battery_observer.h View 1 chunk +0 lines, -38 lines 0 comments Download
D chrome/browser/chromeos/power/low_battery_observer.cc View 1 chunk +0 lines, -85 lines 0 comments Download
M chrome/browser/chromeos/profile_startup.cc View 2 chunks +0 lines, -11 lines 0 comments Download
M chrome/browser/chromeos/sms_observer.cc View 2 chunks +0 lines, -16 lines 0 comments Download
M chrome/browser/feedback/feedback_data.cc View 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/feedback/feedback_util.cc View 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/notifications/desktop_notification_service.cc View 4 chunks +16 lines, -32 lines 0 comments Download
M chrome/browser/ui/views/ash/balloon_collection_impl_ash.h View 3 chunks +2 lines, -13 lines 0 comments Download
M chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc View 5 chunks +8 lines, -52 lines 0 comments Download
M chrome/chrome_browser_chromeos.gypi View 1 2 2 chunks +0 lines, -6 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
stevenjb
SystemNotifications are dead! Long live WebNotificationTray! tbarzic@ for FileBrowserNotifications, ohima@ for the rest.
8 years, 3 months ago (2012-09-19 22:04:42 UTC) #1
tbarzic
lgtm for file_browser_notifications http://codereview.chromium.org/10947046/diff/1/chrome/browser/chromeos/extensions/file_browser_notifications.cc File chrome/browser/chromeos/extensions/file_browser_notifications.cc (right): http://codereview.chromium.org/10947046/diff/1/chrome/browser/chromeos/extensions/file_browser_notifications.cc#newcode354 chrome/browser/chromeos/extensions/file_browser_notifications.cc:354: DCHECK(notification_map_.find(notification_id) == notification_map_.end()); really small nit: ...
8 years, 3 months ago (2012-09-19 22:34:47 UTC) #2
stevenjb
http://codereview.chromium.org/10947046/diff/1/chrome/browser/chromeos/extensions/file_browser_notifications.cc File chrome/browser/chromeos/extensions/file_browser_notifications.cc (right): http://codereview.chromium.org/10947046/diff/1/chrome/browser/chromeos/extensions/file_browser_notifications.cc#newcode354 chrome/browser/chromeos/extensions/file_browser_notifications.cc:354: DCHECK(notification_map_.find(notification_id) == notification_map_.end()); On 2012/09/19 22:34:47, tbarzic wrote: > ...
8 years, 3 months ago (2012-09-19 22:48:32 UTC) #3
tbarzic
ok, thanks for the explanation :) lgtm
8 years, 3 months ago (2012-09-19 22:52:56 UTC) #4
stevenjb
@oshima - ping?
8 years, 3 months ago (2012-09-21 20:39:06 UTC) #5
oshima
lgtm with nit http://codereview.chromium.org/10947046/diff/4001/chrome/browser/chromeos/extensions/file_browser_notifications.cc File chrome/browser/chromeos/extensions/file_browser_notifications.cc (right): http://codereview.chromium.org/10947046/diff/4001/chrome/browser/chromeos/extensions/file_browser_notifications.cc#newcode139 chrome/browser/chromeos/extensions/file_browser_notifications.cc:139: string16 replace_id = UTF8ToUTF16(notification_id_); nit: const ...
8 years, 3 months ago (2012-09-21 21:45:19 UTC) #6
stevenjb
8 years, 3 months ago (2012-09-24 17:27:04 UTC) #7 
http://codereview.chromium.org/10947046/diff/4001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_notifications.cc (right):

http://codereview.chromium.org/10947046/diff/4001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_notifications.cc:139: string16
replace_id = UTF8ToUTF16(notification_id_);
On 2012/09/21 21:45:19, oshima wrote:
> nit: const string16

Done.

Powered by Google App Engine
This is Rietveld 408576698