New issue
Advanced search Search tips

Issue 725835 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Use scoped_refptr for delegate parameter in Notification constructor

Project Member Reported by tnagel@chromium.org, May 24 2017

Issue description

Afaics, the Notification() constructor would be easier to use if the |delegate| parameter was a scoped_refptr and thus ownership of the delegate would be obvious.

As a quick fix, it would probably help to add a comment to the constructor that explains ownership.
 

Comment 1 Deleted

Comment 2 by peter@chromium.org, Jul 10 2017

Owner: peter@chromium.org
Status: Started (was: Untriaged)
Agreed. Put a quick CL up here:
https://chromium-review.googlesource.com/c/565416
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/89a570f5b843acf4fe9284d0cade48752a3e3265

commit 89a570f5b843acf4fe9284d0cade48752a3e3265
Author: Peter Beverloo <peter@chromium.org>
Date: Tue Jul 11 22:37:05 2017

Use scoped_refptr for the delegate in the Notification constructors

Because they are. On a tangent, we should probably figure out how to
delete the Chrome-level NotificationDelegate override altogether.

BUG= 725835 

Change-Id: I9d092fd627ec009ff3ddd1d6dc01a246ce427971
Reviewed-on: https://chromium-review.googlesource.com/565416
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Commit-Queue: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485715}
[modify] https://crrev.com/89a570f5b843acf4fe9284d0cade48752a3e3265/chrome/browser/notifications/notification.cc
[modify] https://crrev.com/89a570f5b843acf4fe9284d0cade48752a3e3265/chrome/browser/notifications/notification.h
[modify] https://crrev.com/89a570f5b843acf4fe9284d0cade48752a3e3265/ui/message_center/notification.cc
[modify] https://crrev.com/89a570f5b843acf4fe9284d0cade48752a3e3265/ui/message_center/notification.h

Comment 4 by peter@chromium.org, Jul 12 2017

Status: Fixed (was: Started)

Comment 5 by tnagel@chromium.org, Jul 12 2017

Many thanks!

Sign in to add a comment