NotificationController::clientFrom() should return a reference

NotificationController::clientFrom() should return a reference

NotificationController has maintained NotificationClient with OwnPtr since r173468. And, NotificationClient should not be a null. We can return a reference on NotificationController::clientFrom().

BUG=none

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175795

[gyuyoung-inactive, 2014-05-08 04:39:31]

Add haraken to reviewer

[haraken, 2014-05-08 05:12:04]

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
File Source/modules/notifications/Notification.cpp (left):

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.cpp:196: m_client = 0;

I don't understand why you can remove this. I agree that m_client should not be
0 when the Notification object is created, but m_client should become 0 when the
Notification stops.

[Peter Beverloo, 2014-05-08 12:59:34]

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
File Source/modules/notifications/Notification.cpp (left):

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.cpp:196: m_client = 0;
On 2014/05/08 05:12:04, haraken wrote:
> 
> I don't understand why you can remove this. I agree that m_client should not
be
> 0 when the Notification object is created, but m_client should become 0 when
the
> Notification stops.

Are you worried about the case where LocalFrame gets destroyed, thus
NotificationController gets destroyed, freeing the NotificationClient, which
would leave Notification (for a very brief time) with a stale reference to freed
memory?

The class carefully guards against this by checking |m_state| where it matters,
so part of the risk is mitigated. However, we shouldn't refer to such memory in
the first place, which means Notification::m_client can't be a reference.

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.cpp:91: if
(NotificationController::from(toDocument(executionContext())->frame())->client().checkPermission(executionContext())
!= NotificationClient::PermissionAllowed) {
You can use m_client.checkPermission(executionContext()) here.

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.cpp:166: return
permissionString(NotificationController::from(toDocument(context)->frame())->client().checkPermission(context));
NotificationController::clientFrom()?

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.cpp:172:
NotificationController::from(toDocument(context)->frame())->client().requestPermission(context,
callback);
NotificationController::clientFrom()?

[gyuyoung-inactive, 2014-05-08 15:50:27]

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
File Source/modules/notifications/Notification.cpp (left):

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.cpp:196: m_client = 0;
On 2014/05/08 12:59:34, Peter Beverloo wrote:
> On 2014/05/08 05:12:04, haraken wrote:
> > 
> > I don't understand why you can remove this. I agree that m_client should not
> be
> > 0 when the Notification object is created, but m_client should become 0 when
> the
> > Notification stops.
> 
> Are you worried about the case where LocalFrame gets destroyed, thus
> NotificationController gets destroyed, freeing the NotificationClient, which
> would leave Notification (for a very brief time) with a stale reference to
freed
> memory?
> 
> The class carefully guards against this by checking |m_state| where it
matters,
> so part of the risk is mitigated. However, we shouldn't refer to such memory
in
> the first place, which means Notification::m_client can't be a reference.

Though I'm not 100% sure, it looks you guys worry about the case m_client can't
be a reference because of some situations(to become 0 when the Notificaiton stop
and so on). So, I continue to keep Notification::m_client as a pointer.
Notification just refers to a pointer of NotificationClient in
NotificationController. If I'm still wrong, please let me know.

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.cpp:91: if
(NotificationController::from(toDocument(executionContext())->frame())->client().checkPermission(executionContext())
!= NotificationClient::PermissionAllowed) {
On 2014/05/08 12:59:34, Peter Beverloo wrote:
> You can use m_client.checkPermission(executionContext()) here.

Done.

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.cpp:166: return
permissionString(NotificationController::from(toDocument(context)->frame())->client().checkPermission(context));
On 2014/05/08 12:59:34, Peter Beverloo wrote:
> NotificationController::clientFrom()?

Done.

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.cpp:172:
NotificationController::from(toDocument(context)->frame())->client().requestPermission(context,
callback);
On 2014/05/08 12:59:34, Peter Beverloo wrote:
> NotificationController::clientFrom()?

Done.

[gyuyoung-inactive, 2014-05-09 02:28:49]

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
File Source/modules/notifications/Notification.cpp (left):

https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.cpp:196: m_client = 0;
On 2014/05/08 15:50:27, gyuyoung wrote:
> On 2014/05/08 12:59:34, Peter Beverloo wrote:
> > On 2014/05/08 05:12:04, haraken wrote:
> > > 
> > > I don't understand why you can remove this. I agree that m_client should
not
> > be
> > > 0 when the Notification object is created, but m_client should become 0
when
> > the
> > > Notification stops.
> > 
> > Are you worried about the case where LocalFrame gets destroyed, thus
> > NotificationController gets destroyed, freeing the NotificationClient, which
> > would leave Notification (for a very brief time) with a stale reference to
> freed
> > memory?
> > 
> > The class carefully guards against this by checking |m_state| where it
> matters,
> > so part of the risk is mitigated. However, we shouldn't refer to such memory
> in
> > the first place, which means Notification::m_client can't be a reference.
> 
> Though I'm not 100% sure, it looks you guys worry about the case m_client
can't
> be a reference because of some situations(to become 0 when the Notificaiton
stop
> and so on). So, I continue to keep Notification::m_client as a pointer.
> Notification just refers to a pointer of NotificationClient in
> NotificationController. If I'm still wrong, please let me know.

Peter and Kentaro, any comment ?

[gyuyoung-inactive, 2014-06-09 04:58:32]

On 2014/05/09 02:28:49, gyuyoung wrote:
>
https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
> File Source/modules/notifications/Notification.cpp (left):
> 
>
https://codereview.chromium.org/268353004/diff/1/Source/modules/notifications...
> Source/modules/notifications/Notification.cpp:196: m_client = 0;
> On 2014/05/08 15:50:27, gyuyoung wrote:
> > On 2014/05/08 12:59:34, Peter Beverloo wrote:
> > > On 2014/05/08 05:12:04, haraken wrote:
> > > > 
> > > > I don't understand why you can remove this. I agree that m_client should
> not
> > > be
> > > > 0 when the Notification object is created, but m_client should become 0
> when
> > > the
> > > > Notification stops.
> > > 
> > > Are you worried about the case where LocalFrame gets destroyed, thus
> > > NotificationController gets destroyed, freeing the NotificationClient,
which
> > > would leave Notification (for a very brief time) with a stale reference to
> > freed
> > > memory?
> > > 
> > > The class carefully guards against this by checking |m_state| where it
> > matters,
> > > so part of the risk is mitigated. However, we shouldn't refer to such
memory
> > in
> > > the first place, which means Notification::m_client can't be a reference.
> > 
> > Though I'm not 100% sure, it looks you guys worry about the case m_client
> can't
> > be a reference because of some situations(to become 0 when the Notificaiton
> stop
> > and so on). So, I continue to keep Notification::m_client as a pointer.
> > Notification just refers to a pointer of NotificationClient in
> > NotificationController. If I'm still wrong, please let me know.
> 
> Peter and Kentaro, any comment ?

Beverloo, I think I fixed all thinks you pointed out. So, can you take look
again ? This cl is too old...

[haraken, 2014-06-09 05:21:13]

LGTM, but beverloo want to confirm.

https://codereview.chromium.org/268353004/diff/40001/Source/modules/notificat...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/268353004/diff/40001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:196: m_client = nullptr;

We use 0 for raw pointers. (We use nullptr for RefPtrs and OwnPtrs.) Otherwise
we'll hit compile errors in some compilers.

[gyuyoung-inactive, 2014-06-09 05:38:31]

https://codereview.chromium.org/268353004/diff/40001/Source/modules/notificat...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/268353004/diff/40001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:196: m_client = nullptr;
On 2014/06/09 05:21:12, haraken wrote:
> 
> We use 0 for raw pointers. (We use nullptr for RefPtrs and OwnPtrs.) Otherwise
> we'll hit compile errors in some compilers.

Ok, Fixed.

[Peter Beverloo, 2014-06-09 12:43:29]

lgtm

[gyuyoung-inactive, 2014-06-09 12:50:48]

The CQ bit was checked by gyuyoung.kim@samsung.com

[gyuyoung-inactive, 2014-06-09 12:50:59]

On 2014/06/09 12:43:29, Peter Beverloo wrote:
> lgtm

Thanks Peter !

[commit-bot: I haz the power, 2014-06-09 12:51:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gyuyoung.kim@samsung.com/268353004/60001

[commit-bot: I haz the power, 2014-06-09 13:53:52]

Change committed as 175795