[Mac] Wrap NSUserActivity begin/end so that it can be called from common files

[Mac] Wrap NSUserActivity begin/end so that it can be called from common files

This is prep for turning on AppNap as a variation of the
MacAllowBackgroundingProcesses experiment.

BUG=460102
Review-Url: https://codereview.chromium.org/2913883002
Cr-Commit-Position: refs/heads/master@{#476379}
Committed: https://chromium.googlesource.com/chromium/src/+/ece963bf6bf190b57620261673c6bc3d70f5ec97

[lgrey, 2017-05-31 14:25:23]

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

[commit-bot: I haz the power, 2017-05-31 14:25:35]

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, 2017-05-31 14:39:41]

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

[commit-bot: I haz the power, 2017-05-31 14:39:42]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[lgrey, 2017-05-31 15:12:53]

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

[commit-bot: I haz the power, 2017-05-31 15:13:11]

Dry run: CQ is trying da patch.

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

[lgrey, 2017-05-31 15:13:40]

The CQ bit was unchecked by lgrey@chromium.org

[lgrey, 2017-05-31 15:14:59]

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

[commit-bot: I haz the power, 2017-05-31 15:15:29]

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, 2017-05-31 16:41:33]

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

[commit-bot: I haz the power, 2017-05-31 16:41:34]

Dry run: This issue passed the CQ dry run.

[lgrey, 2017-05-31 17:34:52]

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

[lgrey, 2017-05-31 17:34:53]

PTAL :)

[Robert Sesek, 2017-05-31 18:11:27]

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
File content/common/mac/activity.h (right):

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.h:13: namespace mac {
use |namespace content|

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.h:13: namespace mac {
nit: blank lines surrounding the inside of the namespace declaration

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.h:24: class CONTENT_EXPORT Activity {
"Activity" is a bit broad of a name. What about "AppNapAssertion" or
"AppNapActivity" ?

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.h:28: void Begin();
nit: blank line after

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.h:31: ~Activity();
Put this after the ctor.

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
File content/common/mac/activity.mm (right):

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.mm:11: static NSString* const kActivityReason =
@"Process foregrounded";
optional: drop the static and move into the anonymous namespace, inside
|namespace content|

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.mm:20: class AssertionWrapper {
I think this could be a simple struct and just expose the member publicly, since
it's basically a private implementation detail.

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.mm:28: };
nit: blank line after

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.mm:33: Activity::~Activity(){};
DCHECK(!assertion_.get()) ?

[lgrey, 2017-05-31 18:37:25]

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

[commit-bot: I haz the power, 2017-05-31 18:38:22]

Dry run: CQ is trying da patch.

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

[lgrey, 2017-05-31 21:28:12]

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
File content/common/mac/activity.h (right):

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.h:13: namespace mac {
On 2017/05/31 18:11:26, Robert Sesek wrote:
> nit: blank lines surrounding the inside of the namespace declaration

Done.

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.h:13: namespace mac {
On 2017/05/31 18:11:26, Robert Sesek wrote:
> use |namespace content|

Done.

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.h:24: class CONTENT_EXPORT Activity {
On 2017/05/31 18:11:26, Robert Sesek wrote:
> "Activity" is a bit broad of a name. What about "AppNapAssertion" or
> "AppNapActivity" ?

Done.

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.h:28: void Begin();
On 2017/05/31 18:11:26, Robert Sesek wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.h:31: ~Activity();
On 2017/05/31 18:11:26, Robert Sesek wrote:
> Put this after the ctor.

Done.

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
File content/common/mac/activity.mm (right):

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.mm:11: static NSString* const kActivityReason =
@"Process foregrounded";
On 2017/05/31 18:11:27, Robert Sesek wrote:
> optional: drop the static and move into the anonymous namespace, inside
> |namespace content|

Done.

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.mm:20: class AssertionWrapper {
On 2017/05/31 18:11:26, Robert Sesek wrote:
> I think this could be a simple struct and just expose the member publicly,
since
> it's basically a private implementation detail.

Done. Renamed the member |obj_| to avoid awkward
"assertion_->assertion_->whatever" though IMO it still looks really awkward. Any
suggestions for how to clean it up or do you think it's OK like this?

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.mm:28: };
On 2017/05/31 18:11:26, Robert Sesek wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.mm:33: Activity::~Activity(){};
On 2017/05/31 18:11:26, Robert Sesek wrote:
> DCHECK(!assertion_.get()) ?

Done.

[commit-bot: I haz the power, 2017-05-31 22:14:59]

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

[commit-bot: I haz the power, 2017-05-31 22:15:00]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-05-31 22:21:57]

LGTM w/ some nits

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
File content/common/mac/activity.mm (right):

https://codereview.chromium.org/2913883002/diff/60001/content/common/mac/acti...
content/common/mac/activity.mm:20: class AssertionWrapper {
On 2017/05/31 21:28:12, lgrey wrote:
> On 2017/05/31 18:11:26, Robert Sesek wrote:
> > I think this could be a simple struct and just expose the member publicly,
> since
> > it's basically a private implementation detail.
> 
> Done. Renamed the member |obj_| to avoid awkward
> "assertion_->assertion_->whatever" though IMO it still looks really awkward.
Any
> suggestions for how to clean it up or do you think it's OK like this?

Nope, I think that's fine. |obj| or |ptr| would be appropriate.

https://codereview.chromium.org/2913883002/diff/80001/content/common/mac/app_...
File content/common/mac/app_nap_activity.mm (right):

https://codereview.chromium.org/2913883002/diff/80001/content/common/mac/app_...
content/common/mac/app_nap_activity.mm:24: public:
struct members are public by default, so you can omit

https://codereview.chromium.org/2913883002/diff/80001/content/common/mac/app_...
content/common/mac/app_nap_activity.mm:25: base::scoped_nsobject<id> obj_;
You can also omit trailing _ on public struct members.

https://codereview.chromium.org/2913883002/diff/80001/content/common/mac/app_...
content/common/mac/app_nap_activity.mm:48: }
nit: blank line after

[lgrey, 2017-06-01 15:51:10]

Thanks!

https://codereview.chromium.org/2913883002/diff/80001/content/common/mac/app_...
File content/common/mac/app_nap_activity.mm (right):

https://codereview.chromium.org/2913883002/diff/80001/content/common/mac/app_...
content/common/mac/app_nap_activity.mm:24: public:
On 2017/05/31 22:21:56, Robert Sesek wrote:
> struct members are public by default, so you can omit

Done.

https://codereview.chromium.org/2913883002/diff/80001/content/common/mac/app_...
content/common/mac/app_nap_activity.mm:25: base::scoped_nsobject<id> obj_;
On 2017/05/31 22:21:56, Robert Sesek wrote:
> You can also omit trailing _ on public struct members.

Done.

https://codereview.chromium.org/2913883002/diff/80001/content/common/mac/app_...
content/common/mac/app_nap_activity.mm:48: }
On 2017/05/31 22:21:56, Robert Sesek wrote:
> nit: blank line after

Done.

[lgrey, 2017-06-01 15:51:15]

The CQ bit was checked by lgrey@chromium.org

[lgrey, 2017-06-01 15:51:15]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2913883002/#ps120001
(title: "Whitespace")

[commit-bot: I haz the power, 2017-06-01 15:51:55]

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, 2017-06-01 16:02:08]

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

[commit-bot: I haz the power, 2017-06-01 16:02:09]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lgrey, 2017-06-01 17:30:43]

lgrey@chromium.org changed reviewers:
+ avi@chromium.org

[lgrey, 2017-06-01 17:30:44]

+avi@ for content owner (guess who read the wrong OWNERS file? :D)

[Avi (use Gerrit), 2017-06-01 18:19:44]

lgtm

[lgrey, 2017-06-01 18:20:26]

The CQ bit was checked by lgrey@chromium.org

[commit-bot: I haz the power, 2017-06-01 18:21:03]

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, 2017-06-01 19:06:13]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496341226296600,
"parent_rev": "7d0b5dfa22bbc761575c3627e7627819ec8b4c8c", "commit_rev":
"ece963bf6bf190b57620261673c6bc3d70f5ec97"}

[commit-bot: I haz the power, 2017-06-01 19:06:20]

Description was changed from

==========
[Mac] Wrap NSUserActivity begin/end so that it can be called from common files

This is prep for turning on AppNap as a variation of the
MacAllowBackgroundingProcesses experiment.

BUG=460102
==========

to

==========
[Mac] Wrap NSUserActivity begin/end so that it can be called from common files

This is prep for turning on AppNap as a variation of the
MacAllowBackgroundingProcesses experiment.

BUG=460102

Review-Url: https://codereview.chromium.org/2913883002
Cr-Commit-Position: refs/heads/master@{#476379}
Committed:
https://chromium.googlesource.com/chromium/src/+/ece963bf6bf190b57620261673c6...
==========

[commit-bot: I haz the power, 2017-06-01 19:06:21]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ece963bf6bf190b57620261673c6...