Passing ProfileID instead of Profile* to clarify that profile should not be used for making any cal…

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}

[Deepak, 2015-05-21 10:31:20]

deepak.m1@samsung.com changed reviewers:
+ peter@chromium.org

[Deepak, 2015-05-21 10:35:12]

Please review.

[Peter Beverloo, 2015-05-21 11:28:48]

Nice! Thanks :) One comment.

https://codereview.chromium.org/1155453002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/1155453002/diff/1/chrome/browser/notification...
chrome/browser/notifications/message_center_notification_manager.cc:236: // The
profile pointer can be weak, the instance may have been destroyed, so
nit: You can now remove this comment.

https://codereview.chromium.org/1155453002/diff/1/chrome/browser/notification...
chrome/browser/notifications/message_center_notification_manager.cc:253: // The
profile pointer can be weak, the instance may have been destroyed, so
nit: You can now remove this comment.

https://codereview.chromium.org/1155453002/diff/1/chrome/browser/notification...
chrome/browser/notifications/message_center_notification_manager.cc:258: if
(pair.second->profile() == profile_id)
This is now a [Profile* == void*] comparison. Even if the bots were fine with
that, comparing a [profile*] to a [profile id] doesn't read well.

What about the following? It's more verbose, but more correct to read.

=========================
for (const auto& pair : profile_notifications_) {
  if (NotificationUIManager::GetProfileID(pair.second->profile()) == profile_id)
    delegate_ids.insert(pair.second->notification().delegate_id());
}
=========================

Dito on line 244.

[Deepak, 2015-05-21 14:00:38]

deepak.m1@samsung.com changed reviewers:
+ dewittj@chromium.org, rdsmith@chromium.org

[Deepak, 2015-05-21 14:01:07]

@Peter
Thanks for review.
Changes done as suggested.
PTAL

https://codereview.chromium.org/1155453002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/1155453002/diff/1/chrome/browser/notification...
chrome/browser/notifications/message_center_notification_manager.cc:236: // The
profile pointer can be weak, the instance may have been destroyed, so
On 2015/05/21 11:28:48, Peter Beverloo wrote:
> nit: You can now remove this comment.

Done.

https://codereview.chromium.org/1155453002/diff/1/chrome/browser/notification...
chrome/browser/notifications/message_center_notification_manager.cc:253: // The
profile pointer can be weak, the instance may have been destroyed, so
On 2015/05/21 11:28:48, Peter Beverloo wrote:
> nit: You can now remove this comment.

Done.

https://codereview.chromium.org/1155453002/diff/1/chrome/browser/notification...
chrome/browser/notifications/message_center_notification_manager.cc:258: if
(pair.second->profile() == profile_id)
On 2015/05/21 11:28:48, Peter Beverloo wrote:
> This is now a [Profile* == void*] comparison. Even if the bots were fine with
> that, comparing a [profile*] to a [profile id] doesn't read well.
> 
> What about the following? It's more verbose, but more correct to read.
> 
> =========================
> for (const auto& pair : profile_notifications_) {
>   if (NotificationUIManager::GetProfileID(pair.second->profile()) ==
profile_id)
>     delegate_ids.insert(pair.second->notification().delegate_id());
> }
> =========================
> 
> Dito on line 244.

Done.

[Randy Smith (Not in Mondays), 2015-05-21 14:09:26]

chrome/browser/downloads LGTM (which is why I presume I'm on this review).

[Deepak, 2015-05-21 14:46:25]

On 2015/05/21 14:09:26, rdsmith wrote:
> chrome/browser/downloads LGTM (which is why I presume I'm on this review).

Thanks @rdsmith.
Looking forward to get clearance from @peter and @dewittj :)

[dewittj, 2015-05-21 17:37:27]

Thanks for making this change.  It seems like a partial fix though, since there
are still Profile pointers in some places such as ProfileNotification - can we
change that to ProfileID as well?  Then there's also no need to do GetProfileID
all over the place.

[Deepak, 2015-05-22 04:17:20]

@dewittj
Thanks for review.
Changes done as suggested.
PTAL.

[dewittj, 2015-05-22 16:54:37]

https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:372:
static_cast<Profile*>(profile_notification->profile()));
We shouldn't cast ProfileId to Profile.  Instead, change the signature of this
method to accept a const Notification&, a Profile* and a const std::string&

https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/profile_notification.h (right):

https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/profile_notification.h:27: ProfileID profile()
const { return profile_; }
nit: change to profile_id()

https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/profile_notification.h:31: // Weak, guaranteed not
to be used after profile removal by parent class.
please update this comment to reflect that it's just used as an equality
comparison.

https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/profile_notification.h:32: ProfileID profile_;
nit: change to profile_id_

[Deepak, 2015-05-23 06:19:12]

@dewittj
Thanks for review.
Changes done as suggested.
PTAL

https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:372:
static_cast<Profile*>(profile_notification->profile()));
On 2015/05/22 16:54:37, dewittj wrote:
> We shouldn't cast ProfileId to Profile.  Instead, change the signature of this
> method to accept a const Notification&, a Profile* and a const std::string&

Done.

https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/profile_notification.h (right):

https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/profile_notification.h:27: ProfileID profile()
const { return profile_; }
On 2015/05/22 16:54:37, dewittj wrote:
> nit: change to profile_id()

Done.

https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/profile_notification.h:31: // Weak, guaranteed not
to be used after profile removal by parent class.
On 2015/05/22 16:54:37, dewittj wrote:
> please update this comment to reflect that it's just used as an equality
> comparison.

Done.

https://codereview.chromium.org/1155453002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/profile_notification.h:32: ProfileID profile_;
On 2015/05/22 16:54:37, dewittj wrote:
> nit: change to profile_id_

Done.

[Deepak, 2015-05-27 11:54:39]

Please review.

[dewittj, 2015-05-27 18:22:40]

https://codereview.chromium.org/1155453002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_ui_manager.h (right):

https://codereview.chromium.org/1155453002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_ui_manager.h:76: virtual
std::set<std::string> GetAllIdsByProfile(ProfileID profile_id) = 0;
I think there is at least one callsite of this function
(platform_notification_service_impl.cc) that needs to use GetProfileId now.

https://codereview.chromium.org/1155453002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/profile_notification.cc (right):

https://codereview.chromium.org/1155453002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/profile_notification.cc:21:
ProfileNotification::ProfileNotification(ProfileID profile,
nit: s/profile/profile_id/

[Deepak, 2015-05-28 05:37:08]

@dewittj
Thanks for review.
Changes done as suggested.
PTAL

https://codereview.chromium.org/1155453002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_ui_manager.h (right):

https://codereview.chromium.org/1155453002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_ui_manager.h:76: virtual
std::set<std::string> GetAllIdsByProfile(ProfileID profile_id) = 0;
On 2015/05/27 18:22:39, dewittj wrote:
> I think there is at least one callsite of this function
> (platform_notification_service_impl.cc) that needs to use GetProfileId now.

Done.

https://codereview.chromium.org/1155453002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/profile_notification.cc (right):

https://codereview.chromium.org/1155453002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/profile_notification.cc:21:
ProfileNotification::ProfileNotification(ProfileID profile,
On 2015/05/27 18:22:39, dewittj wrote:
> nit: s/profile/profile_id/

Done.

[Deepak, 2015-05-29 13:55:35]

Please review

[dewittj, 2015-05-29 16:44:30]

please run try jobs on the next patch

https://codereview.chromium.org/1155453002/diff/80001/chrome/browser/notifica...
File chrome/browser/notifications/profile_notification.cc (right):

https://codereview.chromium.org/1155453002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/profile_notification.cc:32:
multi_user_util::GetUserIDFromProfile(profile_id));
I'm surprised that this compiles; it takes a Profile* not a void*. 

For now, we can return the constructor to take a Profile*, but transform it into
a ProfileID for storage in the member var.

[Deepak, 2015-05-30 05:38:05]

@dewittj
* try bot run done.
* changes done as suggested.
PTAL

https://codereview.chromium.org/1155453002/diff/80001/chrome/browser/notifica...
File chrome/browser/notifications/profile_notification.cc (right):

https://codereview.chromium.org/1155453002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/profile_notification.cc:32:
multi_user_util::GetUserIDFromProfile(profile_id));
On 2015/05/29 16:44:30, dewittj wrote:
> I'm surprised that this compiles; it takes a Profile* not a void*. 
> 
> For now, we can return the constructor to take a Profile*, but transform it
into
> a ProfileID for storage in the member var.

Done.

[Deepak, 2015-06-01 13:13:15]

@dewittj
 * try bot run done.
 * changes done as suggested.
Please review.

[dewittj, 2015-06-01 16:02:56]

lgtm

[Deepak, 2015-06-02 02:50:05]

The CQ bit was checked by deepak.m1@samsung.com

[Deepak, 2015-06-02 02:50:05]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1155453002/#ps100001
(title: "Changes as per review comments.")

[commit-bot: I haz the power, 2015-06-02 02:50:23]

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

[commit-bot: I haz the power, 2015-06-02 03:38:38]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-02 03:39:30]

Patchset 6 (id:??) landed as
https://crrev.com/909d4debbc0f9b443109ab4eb2f68edd084ac015
Cr-Commit-Position: refs/heads/master@{#332340}