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

Issue 1155453002: Passing ProfileID instead of Profile* to clarify that profile should not be used for making any cal… (Closed)

Created:
5 years, 7 months ago by Deepak
Modified:
5 years, 6 months ago
CC:
chromium-reviews, asanka, extensions-reviews_chromium.org, benjhayden+dwatch_chromium.org, mlamouri+watch-notifications_chromium.org, chromium-apps-reviews_chromium.org, peter+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Passing ProfileID instead of Profile* to clarify that profile should not be used for making any calls. ProfileID is just a void* cast of Profile* to stop making any call. BUG=490581 Committed: https://crrev.com/909d4debbc0f9b443109ab4eb2f68edd084ac015 Cr-Commit-Position: refs/heads/master@{#332340}

Patch Set 1 #

Total comments: 6

Patch Set 2 : Changes as per review comments. #

Patch Set 3 : Changes as per review comments. #

Total comments: 8

Patch Set 4 : Changes as per review comments. #

Total comments: 4

Patch Set 5 : Changes as per review comments. #

Total comments: 2

Patch Set 6 : Changes as per review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+44 lines, -54 lines) Patch
M chrome/browser/download/notification/download_notification_item_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/api/notifications/notifications_api.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/notifications/message_center_notification_manager.h View 1 2 3 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/notifications/message_center_notification_manager.cc View 1 2 3 4 5 6 chunks +12 lines, -19 lines 0 comments Download
M chrome/browser/notifications/notification_test_util.h View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/notifications/notification_test_util.cc View 1 2 2 chunks +6 lines, -5 lines 0 comments Download
M chrome/browser/notifications/notification_ui_manager.h View 1 chunk +4 lines, -8 lines 0 comments Download
M chrome/browser/notifications/notification_ui_manager_android.h View 1 chunk +4 lines, -4 lines 0 comments Download
M chrome/browser/notifications/notification_ui_manager_android.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/notifications/platform_notification_service_impl.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/notifications/profile_notification.h View 1 2 3 4 5 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/notifications/profile_notification.cc View 1 2 3 4 5 1 chunk +3 lines, -4 lines 0 comments Download

Messages

Total messages: 24 (4 generated)
Deepak
Please review.
5 years, 7 months ago (2015-05-21 10:35:12 UTC) #2
Peter Beverloo
Nice! Thanks :) One comment. https://codereview.chromium.org/1155453002/diff/1/chrome/browser/notifications/message_center_notification_manager.cc File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/1155453002/diff/1/chrome/browser/notifications/message_center_notification_manager.cc#newcode236 chrome/browser/notifications/message_center_notification_manager.cc:236: // The profile pointer ...
5 years, 7 months ago (2015-05-21 11:28:48 UTC) #3
Deepak
@Peter Thanks for review. Changes done as suggested. PTAL https://codereview.chromium.org/1155453002/diff/1/chrome/browser/notifications/message_center_notification_manager.cc File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/1155453002/diff/1/chrome/browser/notifications/message_center_notification_manager.cc#newcode236 chrome/browser/notifications/message_center_notification_manager.cc:236: ...
5 years, 7 months ago (2015-05-21 14:01:07 UTC) #5
Randy Smith (Not in Mondays)
chrome/browser/downloads LGTM (which is why I presume I'm on this review).
5 years, 7 months ago (2015-05-21 14:09:26 UTC) #6
Deepak
On 2015/05/21 14:09:26, rdsmith wrote: > chrome/browser/downloads LGTM (which is why I presume I'm on ...
5 years, 7 months ago (2015-05-21 14:46:25 UTC) #7
dewittj
Thanks for making this change. It seems like a partial fix though, since there are ...
5 years, 7 months ago (2015-05-21 17:37:27 UTC) #8
Deepak
@dewittj Thanks for review. Changes done as suggested. PTAL.
5 years, 7 months ago (2015-05-22 04:17:20 UTC) #9
dewittj
https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifications/message_center_notification_manager.cc File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifications/message_center_notification_manager.cc#newcode372 chrome/browser/notifications/message_center_notification_manager.cc:372: static_cast<Profile*>(profile_notification->profile())); We shouldn't cast ProfileId to Profile. Instead, change ...
5 years, 7 months ago (2015-05-22 16:54:37 UTC) #10
Deepak
@dewittj Thanks for review. Changes done as suggested. PTAL https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifications/message_center_notification_manager.cc File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifications/message_center_notification_manager.cc#newcode372 chrome/browser/notifications/message_center_notification_manager.cc:372: ...
5 years, 7 months ago (2015-05-23 06:19:12 UTC) #11
Deepak
Please review.
5 years, 7 months ago (2015-05-27 11:54:39 UTC) #12
dewittj
https://codereview.chromium.org/1155453002/diff/60001/chrome/browser/notifications/notification_ui_manager.h File chrome/browser/notifications/notification_ui_manager.h (right): https://codereview.chromium.org/1155453002/diff/60001/chrome/browser/notifications/notification_ui_manager.h#newcode76 chrome/browser/notifications/notification_ui_manager.h:76: virtual std::set<std::string> GetAllIdsByProfile(ProfileID profile_id) = 0; I think there ...
5 years, 7 months ago (2015-05-27 18:22:40 UTC) #13
Deepak
@dewittj Thanks for review. Changes done as suggested. PTAL https://codereview.chromium.org/1155453002/diff/60001/chrome/browser/notifications/notification_ui_manager.h File chrome/browser/notifications/notification_ui_manager.h (right): https://codereview.chromium.org/1155453002/diff/60001/chrome/browser/notifications/notification_ui_manager.h#newcode76 chrome/browser/notifications/notification_ui_manager.h:76: ...
5 years, 7 months ago (2015-05-28 05:37:08 UTC) #14
Deepak
Please review
5 years, 6 months ago (2015-05-29 13:55:35 UTC) #15
dewittj
please run try jobs on the next patch https://codereview.chromium.org/1155453002/diff/80001/chrome/browser/notifications/profile_notification.cc File chrome/browser/notifications/profile_notification.cc (right): https://codereview.chromium.org/1155453002/diff/80001/chrome/browser/notifications/profile_notification.cc#newcode32 chrome/browser/notifications/profile_notification.cc:32: multi_user_util::GetUserIDFromProfile(profile_id)); ...
5 years, 6 months ago (2015-05-29 16:44:30 UTC) #16
Deepak
@dewittj * try bot run done. * changes done as suggested. PTAL https://codereview.chromium.org/1155453002/diff/80001/chrome/browser/notifications/profile_notification.cc File chrome/browser/notifications/profile_notification.cc ...
5 years, 6 months ago (2015-05-30 05:38:05 UTC) #17
Deepak
@dewittj * try bot run done. * changes done as suggested. Please review.
5 years, 6 months ago (2015-06-01 13:13:15 UTC) #18
dewittj
lgtm
5 years, 6 months ago (2015-06-01 16:02:56 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1155453002/100001
5 years, 6 months ago (2015-06-02 02:50:23 UTC) #22
commit-bot: I haz the power
Committed patchset #6 (id:100001)
5 years, 6 months ago (2015-06-02 03:38:38 UTC) #23
commit-bot: I haz the power
5 years, 6 months ago (2015-06-02 03:39:30 UTC) #24
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/909d4debbc0f9b443109ab4eb2f68edd084ac015
Cr-Commit-Position: refs/heads/master@{#332340}

Powered by Google App Engine
This is Rietveld 408576698