Prioritize ARC instance only when it is needed

Prioritize ARC instance only when it is needed

Previously, the instance was prioritized when the OS fully started.
However, it turned out that the prioritization caused Chrome UI jank
on some boards.

This CL changes the way of prioritization more dynamic to prevent
such a jank.

BUG=chrome-os-partner:60946
TEST=try

Review-Url: https://codereview.chromium.org/2624513004
Cr-Commit-Position: refs/heads/master@{#442768}
Committed: https://chromium.googlesource.com/chromium/src/+/9ef6c424a7a2e2fe5f8c24a4e47602a7942877d6

[Yusuke Sato, 2017-01-10 03:37:50]

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

[commit-bot: I haz the power, 2017-01-10 03:38:25]

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-01-10 05:50:04]

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

[commit-bot: I haz the power, 2017-01-10 05:50:05]

Dry run: This issue passed the CQ dry run.

[Yusuke Sato, 2017-01-10 22:13:58]

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

[commit-bot: I haz the power, 2017-01-10 22:14:43]

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

[Yusuke Sato, 2017-01-10 22:14:45]

Description was changed from

==========
arc: WIP

BUG=
==========

to

==========
Prioritize ARC instance only when it is needed

Previously, the instance was prioritized when the OS fully started.
However, it turned out that the prioritization caused Chrome UI jank
on some boards.

This CL changes the way of prioritization more dynamic to prevent
such a jank.

BUG=chrome-os-partner:60946
TEST=try
==========

[Yusuke Sato, 2017-01-10 22:19:26]

yusukes@chromium.org changed reviewers:
+ lhchavez@chromium.org

[Yusuke Sato, 2017-01-10 22:19:27]

PTAL

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/boot_phase_monitor/arc_instance_throttle.cc
(right):

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/boot_phase_monitor/arc_instance_throttle.cc:38: //
TODO(yusukes): Call session_manager_client->UnprioritizeArcInstance()
It's under review at https://chromium-review.googlesource.com/#/c/425450/. Once
this is submitted, I'll pull the dbus constant to Chrome and add the method to
Chrome's SessionManagerClient.

[commit-bot: I haz the power, 2017-01-10 23:39:08]

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

[commit-bot: I haz the power, 2017-01-10 23:39:09]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2017-01-11 00:07:28]

lgtm with a few nits.

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h
(right):

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:40:
std::unique_ptr<ArcInstanceThrottle> throttle_;
nit: s/throttle_/throttler_/

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/boot_phase_monitor/arc_instance_throttle.h
(right):

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/boot_phase_monitor/arc_instance_throttle.h:13: // A
class that watches window activations and throttle the ARC instance when
nit: s/throttle/throttles/

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/boot_phase_monitor/arc_instance_throttle.h:14: //
needed.
maybe expand this comment a bit and mention that ARC is prioritized when one of
its windows is focused.

Also, when you unprioritize ARC, does it lower its priority below the baseline?
If so, maybe a better name for this would be ArcInstancePrioritizer.

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/boot_phase_monitor/arc_instance_throttle.h:15: class
ArcInstanceThrottle : public ash::WmActivationObserver {
Maybe "Throttler"? I've seen that Throttle has a different meaning in UI
context.

[Luis Héctor Chávez, 2017-01-11 00:10:19]

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/boot_phase_monitor/arc_instance_throttle.h
(right):

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/boot_phase_monitor/arc_instance_throttle.h:15: class
ArcInstanceThrottle : public ash::WmActivationObserver {
On 2017/01/11 00:07:28, Luis Héctor Chávez wrote:
> Maybe "Throttler"? I've seen that Throttle has a different meaning in UI
> context.

Hah, TIL that Throttle is the correct word and we've been using it wrong (in
favor of Throttler) more or less consistently throughout the codebase :P

[Yusuke Sato, 2017-01-11 00:31:55]

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h
(right):

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:40:
std::unique_ptr<ArcInstanceThrottle> throttle_;
On 2017/01/11 00:07:28, Luis Héctor Chávez wrote:
> nit: s/throttle_/throttler_/

(see my next comment)

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/boot_phase_monitor/arc_instance_throttle.h
(right):

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/boot_phase_monitor/arc_instance_throttle.h:13: // A
class that watches window activations and throttle the ARC instance when
On 2017/01/11 00:07:28, Luis Héctor Chávez wrote:
> nit: s/throttle/throttles/

Done.

https://codereview.chromium.org/2624513004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/boot_phase_monitor/arc_instance_throttle.h:14: //
needed.
On 2017/01/11 00:07:28, Luis Héctor Chávez wrote:
> maybe expand this comment a bit and mention that ARC is prioritized when one
of
> its windows is focused.
> 
> Also, when you unprioritize ARC, does it lower its priority below the
baseline?
> If so, maybe a better name for this would be ArcInstancePrioritizer.

Done. Re class name, chatted offline and decided to keep the current class name.

[Yusuke Sato, 2017-01-11 00:32:15]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2017-01-11 00:32:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2624513004/#ps40001
(title: "address comments")

[commit-bot: I haz the power, 2017-01-11 00:33:29]

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-01-11 01:57:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484094735467240,
"parent_rev": "de31123249d664dc1b9f96a924aa31b5d332a488", "commit_rev":
"9ef6c424a7a2e2fe5f8c24a4e47602a7942877d6"}

[commit-bot: I haz the power, 2017-01-11 01:58:12]

Description was changed from

==========
Prioritize ARC instance only when it is needed

Previously, the instance was prioritized when the OS fully started.
However, it turned out that the prioritization caused Chrome UI jank
on some boards.

This CL changes the way of prioritization more dynamic to prevent
such a jank.

BUG=chrome-os-partner:60946
TEST=try
==========

to

==========
Prioritize ARC instance only when it is needed

Previously, the instance was prioritized when the OS fully started.
However, it turned out that the prioritization caused Chrome UI jank
on some boards.

This CL changes the way of prioritization more dynamic to prevent
such a jank.

BUG=chrome-os-partner:60946
TEST=try

Review-Url: https://codereview.chromium.org/2624513004
Cr-Commit-Position: refs/heads/master@{#442768}
Committed:
https://chromium.googlesource.com/chromium/src/+/9ef6c424a7a2e2fe5f8c24a4e476...
==========

[commit-bot: I haz the power, 2017-01-11 01:58:14]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9ef6c424a7a2e2fe5f8c24a4e476...