Ensure the xpc transaction is ended

Ensure the xpc transaction is ended

This is a best effort activity since there are situations where we cannot
detect that a given alert has been closed. This will however ensure that
1) only one transaction at most is kept opened
2) whenever an alert is detected to be closed end the transaction
if no other alerts are displayed.

BUG=652237
Committed: https://crrev.com/d624e939a8d3cf2ae751980c5178ff087a7aefba
Cr-Commit-Position: refs/heads/master@{#428397}

[Miguel Garcia, 2016-10-15 09:44:35]

Description was changed from

==========
Ensure the xpc transaction is closed once the last alert is
closed.

This is a best effort thing since there are situations where we cannot
detect that a given alert has been closed. This will however ensure that
1) only one transaction is kept opened
2) whenever an alert is detected to be closed shut the connection
if no other alerts are displaed.

BUG=652237
==========

to

==========
Ensure the xpc transaction is ended

This is a best effort activity since there are situations where we cannot
detect that a given alert has been closed. This will however ensure that
1) only one transaction at most is kept opened
2) whenever an alert is detected to be closed end the transaction
if no other alerts are displayed.

BUG=652237
==========

[Miguel Garcia, 2016-10-15 09:44:58]

miguelg@chromium.org changed reviewers:
+ rsesek@chromium.org

[Miguel Garcia, 2016-10-20 10:01:16]

Friendly ping on this one.

[Robert Sesek, 2016-10-24 23:11:05]

I'm not sure this is going to work correctly in all cases. XPC tries to abstract
the notion of process lifetime to a connection object. The reference count of
the connection is (mostly) tied to the lifetime of the process, whereby when the
connection object refcount goes to 0, the system can kill it at will. We are
going to artificially increase the reference count of the connection by
initiating a transaction (whose main purpose is to keep an XPC connection alive
across a series of XPC messages, rather than just a simple a 1:1
transaction:message scenario). So there are then two approaches we could take:

- Balance begin/end with every deliverNotification: and didDismissAlert:. This
requires perfect accounting to make sure the connection lifetime is correct.
- Balance begin/end once for the first deliverNotification: and then once again
when didDismissAlert: is called and deliveredNotifications.count is 0.

This CL is closer to the first option, but I'm not sure it's balancing
completely correct. I'm also concerned about the issue we brought up on the
other CL about the reliability of macOS telling us about notifications being
dismissed. So maybe the second option is better?

https://codereview.chromium.org/2419213003/diff/1/chrome/browser/ui/cocoa/not...
File chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm (right):

https://codereview.chromium.org/2419213003/diff/1/chrome/browser/ui/cocoa/not...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:12: Boolean
transactionOpen;
Use BOOL or bool.

https://codereview.chromium.org/2419213003/diff/1/chrome/browser/ui/cocoa/not...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:12: Boolean
transactionOpen;
naming: needs trailing underscore

https://codereview.chromium.org/2419213003/diff/1/chrome/browser/ui/cocoa/not...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:22:
@synchronized(self) {
Why @synchronized here? I assume we're avoiding base::Lock because of base?

[Robert Sesek, 2016-10-25 20:15:05]

On 2016/10/24 23:11:05, Robert Sesek wrote:
> I'm not sure this is going to work correctly in all cases. XPC tries to
abstract
> the notion of process lifetime to a connection object. The reference count of
> the connection is (mostly) tied to the lifetime of the process, whereby when
the
> connection object refcount goes to 0, the system can kill it at will. We are
> going to artificially increase the reference count of the connection by
> initiating a transaction (whose main purpose is to keep an XPC connection
alive
> across a series of XPC messages, rather than just a simple a 1:1
> transaction:message scenario). So there are then two approaches we could take:
> 
> - Balance begin/end with every deliverNotification: and didDismissAlert:. This
> requires perfect accounting to make sure the connection lifetime is correct.
> - Balance begin/end once for the first deliverNotification: and then once
again
> when didDismissAlert: is called and deliveredNotifications.count is 0.
> 
> This CL is closer to the first option, but I'm not sure it's balancing
> completely correct. I'm also concerned about the issue we brought up on the
> other CL about the reliability of macOS telling us about notifications being
> dismissed. So maybe the second option is better?

Ignore all of this. You're basically doing this already. I'm not sure why I got
confused.

[Miguel Garcia, 2016-10-26 15:56:39]

PTAL

https://codereview.chromium.org/2419213003/diff/1/chrome/browser/ui/cocoa/not...
File chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm (right):

https://codereview.chromium.org/2419213003/diff/1/chrome/browser/ui/cocoa/not...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:12: Boolean
transactionOpen;
On 2016/10/24 23:11:05, Robert Sesek wrote:
> Use BOOL or bool.

Done.

https://codereview.chromium.org/2419213003/diff/1/chrome/browser/ui/cocoa/not...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:12: Boolean
transactionOpen;
On 2016/10/24 23:11:05, Robert Sesek wrote:
> naming: needs trailing underscore

Done.

https://codereview.chromium.org/2419213003/diff/1/chrome/browser/ui/cocoa/not...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:22:
@synchronized(self) {
On 2016/10/24 23:11:05, Robert Sesek wrote:
> Why @synchronized here? I assume we're avoiding base::Lock because of base?

We need a synchronized block because alerts are added/removed in different
threads.
With that in mind yes, I think we want to stay away from base as much as
possible so doing it natively seemed better.

[Robert Sesek, 2016-10-26 18:49:41]

Just some nits.

https://codereview.chromium.org/2419213003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm (right):

https://codereview.chromium.org/2419213003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:3: // found in
the LICENSE file.
nit: blank line after

https://codereview.chromium.org/2419213003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:13: }
nit: blank line after

https://codereview.chromium.org/2419213003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:16:
transactionOpen_ = NO;
NO -> false

https://codereview.chromium.org/2419213003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:27:
transactionOpen_ = YES;
YES -> true

https://codereview.chromium.org/2419213003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:35: NSInteger
showing = [[notificationCenter deliveredNotifications] count];
-[NSArray count] returns NSUInteger

https://codereview.chromium.org/2419213003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:38:
transactionOpen_ = NO;
NO -> false

[Miguel Garcia, 2016-10-28 14:53:29]

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

[Miguel Garcia, 2016-10-28 14:53:57]

https://codereview.chromium.org/2419213003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm (right):

https://codereview.chromium.org/2419213003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:3: // found in
the LICENSE file.
On 2016/10/26 18:49:41, Robert Sesek wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/2419213003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:13: }
On 2016/10/26 18:49:41, Robert Sesek wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/2419213003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:16:
transactionOpen_ = NO;
On 2016/10/26 18:49:41, Robert Sesek wrote:
> NO -> false

Done.

https://codereview.chromium.org/2419213003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:27:
transactionOpen_ = YES;
On 2016/10/26 18:49:41, Robert Sesek wrote:
> YES -> true

Done.

https://codereview.chromium.org/2419213003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:35: NSInteger
showing = [[notificationCenter deliveredNotifications] count];
On 2016/10/26 18:49:41, Robert Sesek wrote:
> -[NSArray count] returns NSUInteger

Done.

https://codereview.chromium.org/2419213003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/xpc_transaction_handler.mm:38:
transactionOpen_ = NO;
On 2016/10/26 18:49:41, Robert Sesek wrote:
> NO -> false

Done.

[commit-bot: I haz the power, 2016-10-28 14:54:00]

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-28 15:17:09]

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

[commit-bot: I haz the power, 2016-10-28 15:17:10]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Robert Sesek, 2016-10-28 16:28:06]

LGTM

[Miguel Garcia, 2016-10-28 16:34:57]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2016-10-28 16:35:24]

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-28 17:04:11]

Description was changed from

==========
Ensure the xpc transaction is ended

This is a best effort activity since there are situations where we cannot
detect that a given alert has been closed. This will however ensure that
1) only one transaction at most is kept opened
2) whenever an alert is detected to be closed end the transaction
if no other alerts are displayed.

BUG=652237
==========

to

==========
Ensure the xpc transaction is ended

This is a best effort activity since there are situations where we cannot
detect that a given alert has been closed. This will however ensure that
1) only one transaction at most is kept opened
2) whenever an alert is detected to be closed end the transaction
if no other alerts are displayed.

BUG=652237
==========

[commit-bot: I haz the power, 2016-10-28 17:04:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-28 17:21:57]

Description was changed from

==========
Ensure the xpc transaction is ended

This is a best effort activity since there are situations where we cannot
detect that a given alert has been closed. This will however ensure that
1) only one transaction at most is kept opened
2) whenever an alert is detected to be closed end the transaction
if no other alerts are displayed.

BUG=652237
==========

to

==========
Ensure the xpc transaction is ended

This is a best effort activity since there are situations where we cannot
detect that a given alert has been closed. This will however ensure that
1) only one transaction at most is kept opened
2) whenever an alert is detected to be closed end the transaction
if no other alerts are displayed.

BUG=652237

Committed: https://crrev.com/d624e939a8d3cf2ae751980c5178ff087a7aefba
Cr-Commit-Position: refs/heads/master@{#428397}
==========

[commit-bot: I haz the power, 2016-10-28 17:21:58]

Patchset 3 (id:??) landed as
https://crrev.com/d624e939a8d3cf2ae751980c5178ff087a7aefba
Cr-Commit-Position: refs/heads/master@{#428397}