Issue 787193002: Move logic for displaying notifications to the ProfileNotificationServiceImpl.

Issue 787193002: Move logic for displaying notifications to the ProfileNotificationServiceImpl. (Closed)

Created:
6 years ago by Peter Beverloo

Modified:
6 years ago

Reviewers:
dewittj

CC:
chromium-reviews, peter+watch_chromium.org, mlamouri+watch-notifications_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@n-total

Project:
chromium

Visibility:
Public.

More Reviews

Description

Move logic for displaying notifications to the ProfileNotificationServiceImpl. BUG=439950 Committed: https://crrev.com/059ddc27a616ae3ac11db310ab3fd690a123bd8a Cr-Commit-Position: refs/heads/master@{#307794}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : rebase #

Patch Set 4 : tests #

Total comments: 2

Created: 6 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+318 lines, -224 lines)			Patch
M	chrome/browser/notifications/desktop_notification_profile_util.h	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/notifications/desktop_notification_profile_util.cc	View		1 chunk	+0 lines, -8 lines	0 comments	Download
M	chrome/browser/notifications/desktop_notification_service.h	View		4 chunks	+0 lines, -28 lines	0 comments	Download
M	chrome/browser/notifications/desktop_notification_service.cc	View		2 chunks	+0 lines, -68 lines	0 comments	Download
M	chrome/browser/notifications/notification_test_util.h	View	1 2 3	2 chunks	+15 lines, -75 lines	2 comments	Download
M	chrome/browser/notifications/notification_test_util.cc	View	1 2 3	1 chunk	+31 lines, -34 lines	0 comments	Download
M	chrome/browser/notifications/platform_notification_service_impl.h	View	1 2 3	3 chunks	+27 lines, -0 lines	0 comments	Download
M	chrome/browser/notifications/platform_notification_service_impl.cc	View	1 2 3	5 chunks	+86 lines, -8 lines	0 comments	Download
A	chrome/browser/notifications/platform_notification_service_unittest.cc	View	1 2 3	1 chunk	+158 lines, -0 lines	0 comments	Download
M	chrome/chrome_tests_unit.gypi	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	ui/message_center/notifier_settings.h	View	1 2 3	2 chunks	+0 lines, -2 lines	0 comments	Download

Messages

Total messages: 9 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

dewittj

lgtm If convenient, I'd love to see a new unit test since you're moving untested ...

6 years ago (2014-12-09 20:56:16 UTC) #3

Peter Beverloo

Please take another look. With tests this time :-). https://codereview.chromium.org/787193002/diff/60001/chrome/browser/notifications/notification_test_util.h File chrome/browser/notifications/notification_test_util.h (left): https://codereview.chromium.org/787193002/diff/60001/chrome/browser/notifications/notification_test_util.h#oldcode42 chrome/browser/notifications/notification_test_util.h:42: ...

6 years ago (2014-12-10 19:32:28 UTC) #4

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/787193002/60001

6 years ago (2014-12-10 22:09:18 UTC) #7

commit-bot: I haz the power

6 years ago (2014-12-10 23:40:51 UTC) #9

Message was sent while issue was closed.

Patchset 4 (id:??) landed as
https://crrev.com/059ddc27a616ae3ac11db310ab3fd690a123bd8a
Cr-Commit-Position: refs/heads/master@{#307794}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages