Close privet printer notifications when clicked.

Close privet printer notifications when clicked.

BUG=643268
Committed: https://crrev.com/84a0c256d7371bf0bd495f6d3ac3d0157e61fa44
Cr-Commit-Position: refs/heads/master@{#427720}

[Lei Zhang, 2016-10-25 02:54:30]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 02:54:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 03:36:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 03:36:18]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-10-25 04:52:27]

thestig@chromium.org changed reviewers:
+ yoshiki@chromium.org

[Lei Zhang, 2016-10-25 04:52:28]

yoshiki: Since you are in chrome/browser/notifications/OWNERS and
ui/message_center/OWNERS, I believe you are familiar with the functions I'm
calling here. Can you review?

[Lei Zhang, 2016-10-25 18:34:27]

Description was changed from

==========
Close privet printer notifications when clicked.

BUG=643268
==========

to

==========
Close privet printer notifications when clicked.

BUG=643268
==========

[Lei Zhang, 2016-10-25 18:34:27]

thestig@chromium.org changed reviewers:
+ dewittj@chromium.org

[Lei Zhang, 2016-10-25 18:34:42]

On 2016/10/25 04:52:28, Lei Zhang wrote:
> yoshiki: Since you are in chrome/browser/notifications/OWNERS and
> ui/message_center/OWNERS, I believe you are familiar with the functions I'm
> calling here. Can you review?

+dewittj: Or can you review?

[dewittj, 2016-10-25 20:08:30]

https://codereview.chromium.org/2446043002/diff/1/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/privet_notifications.cc (right):

https://codereview.chromium.org/2446043002/diff/1/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/privet_notifications.cc:258: Profile*
profile = Profile::FromBrowserContext(profile_);
if you are storing |profile_| as a Profile*, why is this here?

https://codereview.chromium.org/2446043002/diff/1/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/privet_notifications.cc:364:
CloseNotification();
Since I don't know the details of this particular delegate, I'm a little wary of
closing the notification right in the observer method, especially since no tests
were added/changed.  Perhaps a post task is in order?

[Lei Zhang, 2016-10-25 22:22:53]

Please note I did not write the code originally, so some things are a bit
mysterious to me as well.

I will also admit I added this blindly, hoping it is a straight thing. Having a
privet printer broadcast its existence on the network means everyone near by
will get this notification. Last time I tried this on my workstation, I got some
funny reactions.

https://codereview.chromium.org/2446043002/diff/1/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/privet_notifications.cc (right):

https://codereview.chromium.org/2446043002/diff/1/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/privet_notifications.cc:258: Profile*
profile = Profile::FromBrowserContext(profile_);
On 2016/10/25 20:08:30, dewittj wrote:
> if you are storing |profile_| as a Profile*, why is this here?

This is PrivetNotificationService, not PrivetNotificationDelegate. I haven't
converted PrivetNotificationService yet.

https://codereview.chromium.org/2446043002/diff/1/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/privet_notifications.cc:364:
CloseNotification();
On 2016/10/25 20:08:30, dewittj wrote:
> Since I don't know the details of this particular delegate, I'm a little wary
of
> closing the notification right in the observer method, especially since no
tests
> were added/changed.  Perhaps a post task is in order?

Can you help me understand what the issue(s) are with closing the notification
early?

Once we reach ButtonClick() here, I don't think we need the notification for
anything else. Does CloseNotification() end up deleting |this|
PrivetNotificationDelegate?

privet_notifications_unittest.cc exists, but it does not appear to be testing
this code path. I can certainly fill in some of the test coverage now that I
noticed this.

[dewittj, 2016-10-25 23:35:56]

https://codereview.chromium.org/2446043002/diff/1/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/privet_notifications.cc (right):

https://codereview.chromium.org/2446043002/diff/1/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/privet_notifications.cc:258: Profile*
profile = Profile::FromBrowserContext(profile_);
On 2016/10/25 22:22:52, Lei Zhang wrote:
> On 2016/10/25 20:08:30, dewittj wrote:
> > if you are storing |profile_| as a Profile*, why is this here?
> 
> This is PrivetNotificationService, not PrivetNotificationDelegate. I haven't
> converted PrivetNotificationService yet.

Acknowledged.

https://codereview.chromium.org/2446043002/diff/1/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/privet_notifications.cc:364:
CloseNotification();
On 2016/10/25 22:22:52, Lei Zhang wrote:
> On 2016/10/25 20:08:30, dewittj wrote:
> > Since I don't know the details of this particular delegate, I'm a little
wary
> of
> > closing the notification right in the observer method, especially since no
> tests
> > were added/changed.  Perhaps a post task is in order?
> 
> Can you help me understand what the issue(s) are with closing the notification
> early?
> 
> Once we reach ButtonClick() here, I don't think we need the notification for
> anything else. Does CloseNotification() end up deleting |this|
> PrivetNotificationDelegate?
> 
> privet_notifications_unittest.cc exists, but it does not appear to be testing
> this code path. I can certainly fill in some of the test coverage now that I
> noticed this.
-warming up cache-

NotificationDelegate is refcounted, and a reference is held by the notification.
 It is possible that closing a notification right now will indeed destroy
|this|, since there don't appear to be any other objects holding references to
PrivetNotificationDelegate.

[Lei Zhang, 2016-10-26 00:22:09]

PTAL at patch set 2.

https://codereview.chromium.org/2446043002/diff/20001/chrome/browser/printing...
File chrome/browser/printing/cloud_print/privet_notifications.cc (right):

https://codereview.chromium.org/2446043002/diff/20001/chrome/browser/printing...
chrome/browser/printing/cloud_print/privet_notifications.cc:376:
CloseNotification();
If I move this to the top, like it was in patch set 1, I get a UAF. If I comment
this out, the new unit tests fail when checking the notification count at the
end.

[Lei Zhang, 2016-10-26 00:22:14]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 00:22:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 01:12:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 01:12:13]

Dry run: This issue passed the CQ dry run.

[dewittj, 2016-10-26 16:04:26]

lgtm, thanks!

https://codereview.chromium.org/2446043002/diff/20001/chrome/browser/printing...
File chrome/browser/printing/cloud_print/privet_notifications.cc (right):

https://codereview.chromium.org/2446043002/diff/20001/chrome/browser/printing...
chrome/browser/printing/cloud_print/privet_notifications.cc:376:
CloseNotification();
On 2016/10/26 00:22:09, Lei Zhang wrote:
> If I move this to the top, like it was in patch set 1, I get a UAF. If I
comment
> this out, the new unit tests fail when checking the notification count at the
> end.

Acknowledged.

[yoshiki, 2016-10-26 16:24:05]

lgtm with nit

https://codereview.chromium.org/2446043002/diff/20001/chrome/browser/printing...
File chrome/browser/printing/cloud_print/privet_notifications.cc (right):

https://codereview.chromium.org/2446043002/diff/20001/chrome/browser/printing...
chrome/browser/printing/cloud_print/privet_notifications.cc:258: Profile*
profile = Profile::FromBrowserContext(profile_);
I think you don't need to convert the profile.
Profile::FromBrowserContext(Profile) is just a static casting.

[Lei Zhang, 2016-10-26 16:52:41]

https://codereview.chromium.org/2446043002/diff/20001/chrome/browser/printing...
File chrome/browser/printing/cloud_print/privet_notifications.cc (right):

https://codereview.chromium.org/2446043002/diff/20001/chrome/browser/printing...
chrome/browser/printing/cloud_print/privet_notifications.cc:258: Profile*
profile = Profile::FromBrowserContext(profile_);
On 2016/10/26 16:24:05, yoshiki wrote:
> I think you don't need to convert the profile.
> Profile::FromBrowserContext(Profile) is just a static casting.

Profile is a BrowserContext, but a BrowserContext is not a Profile, so I cannot
pass it to the PrivetNotificationDelegate ctor as a BrowserContext. I will work
on storing a Profile* in a separate CL, to reduce the number of
Profile::FromBrowserContext() calls.

[Lei Zhang, 2016-10-26 16:52:56]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-10-26 16:53:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 16:59:46]

Description was changed from

==========
Close privet printer notifications when clicked.

BUG=643268
==========

to

==========
Close privet printer notifications when clicked.

BUG=643268
==========

[commit-bot: I haz the power, 2016-10-26 16:59:47]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-26 17:12:49]

Description was changed from

==========
Close privet printer notifications when clicked.

BUG=643268
==========

to

==========
Close privet printer notifications when clicked.

BUG=643268

Committed: https://crrev.com/84a0c256d7371bf0bd495f6d3ac3d0157e61fa44
Cr-Commit-Position: refs/heads/master@{#427720}
==========

[commit-bot: I haz the power, 2016-10-26 17:12:50]

Patchset 2 (id:??) landed as
https://crrev.com/84a0c256d7371bf0bd495f6d3ac3d0157e61fa44
Cr-Commit-Position: refs/heads/master@{#427720}