Remove usage of SequencedWorkerPool::GetNamedSequenceToken from kiosk_app_data.cc.

Remove usage of SequencedWorkerPool::GetNamedSequenceToken from kiosk_app_data.cc.

SequencedWorkerPool is being deprecated in favor of TaskScheduler.

BUG=667892
Review-Url: https://codereview.chromium.org/2883273002
Cr-Commit-Position: refs/heads/master@{#473906}
Committed: https://chromium.googlesource.com/chromium/src/+/0a15ffb1835a5e36e3941fb0c71f23960a6d5e6e

[fdoray, 2017-05-16 14:41:37]

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

[commit-bot: I haz the power, 2017-05-16 14:42:02]

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-16 15:14:09]

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

[commit-bot: I haz the power, 2017-05-16 15:14:11]

Dry run: This issue passed the CQ dry run.

[fdoray, 2017-05-16 17:58:11]

Description was changed from

==========
KioskAppData

fix


fix
==========

to

==========
Remove usage of SequencedWorkerPool::GetNamedSequenceToken from
kiosk_app_data.cc.

SequencedWorkerPool is being deprecated in favor of TaskScheduler.

BUG=667892
==========

[fdoray, 2017-05-16 17:58:55]

fdoray@chromium.org changed reviewers:
+ stevenjb@chromium.org

[fdoray, 2017-05-16 17:58:56]

Please take a look. In particular, please make sure that appropriate TaskTraits
were used.

https://codereview.chromium.org/2883273002/diff/1/chrome/browser/chromeos/app...
File chrome/browser/chromeos/app_mode/kiosk_app_data.cc (left):

https://codereview.chromium.org/2883273002/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/kiosk_app_data.cc:87:
pool->GetNamedSequenceToken("KioskAppData.CrxLoaderWorker");
Before my CL, the named sequence token provided mutual exclusion between tasks
posted from different KioskAppData::CrxLoader instances.

With my CL, mutual exclusion is only between tasks posted from the same
KioskAppData::CrxLoader instance.

Is that ok?

[stevenjb, 2017-05-16 18:41:02]

stevenjb@chromium.org changed reviewers:
+ xiyuan@chromium.org

[stevenjb, 2017-05-16 18:41:02]

+xiyuan@

[xiyuan, 2017-05-16 19:41:46]

https://codereview.chromium.org/2883273002/diff/1/chrome/browser/chromeos/app...
File chrome/browser/chromeos/app_mode/kiosk_app_data.cc (left):

https://codereview.chromium.org/2883273002/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/kiosk_app_data.cc:87:
pool->GetNamedSequenceToken("KioskAppData.CrxLoaderWorker");
On 2017/05/16 17:58:56, fdoray wrote:
> Before my CL, the named sequence token provided mutual exclusion between tasks
> posted from different KioskAppData::CrxLoader instances.
> 
> With my CL, mutual exclusion is only between tasks posted from the same
> KioskAppData::CrxLoader instance.
> 
> Is that ok?

I would like to keep the existing behavior. Crx unpacking is heavy. If a kiosk
device gets a few kiosk apps pushed down via policy, we could be running crx
unpacking in parallel, which slows down the device.

We have a KioskAppManager::GetBackgroundTaskRunner, which always creates a new
task runner ATM. It is currently used in two places (for ExternalCache and USB
update monitoring). We probably can change it to only create one task runner and
use it everywhere (including for CrxLoader).

[fdoray, 2017-05-19 19:06:52]

PTAnL

https://codereview.chromium.org/2883273002/diff/1/chrome/browser/chromeos/app...
File chrome/browser/chromeos/app_mode/kiosk_app_data.cc (left):

https://codereview.chromium.org/2883273002/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/kiosk_app_data.cc:87:
pool->GetNamedSequenceToken("KioskAppData.CrxLoaderWorker");
On 2017/05/16 19:41:45, xiyuan wrote:
> On 2017/05/16 17:58:56, fdoray wrote:
> > Before my CL, the named sequence token provided mutual exclusion between
tasks
> > posted from different KioskAppData::CrxLoader instances.
> > 
> > With my CL, mutual exclusion is only between tasks posted from the same
> > KioskAppData::CrxLoader instance.
> > 
> > Is that ok?
> 
> I would like to keep the existing behavior. Crx unpacking is heavy. If a kiosk
> device gets a few kiosk apps pushed down via policy, we could be running crx
> unpacking in parallel, which slows down the device.
> 
> We have a KioskAppManager::GetBackgroundTaskRunner, which always creates a new
> task runner ATM. It is currently used in two places (for ExternalCache and USB
> update monitoring). We probably can change it to only create one task runner
and
> use it everywhere (including for CrxLoader).

The TaskScheduler is already limiting the rate at which
base::TaskPriority::BACKGROUND tasks run. We would prefer if tasks ran on the
same sequence only when there is a data dependency between them. Sharing a
sequence to limit load prevents the TaskScheduler from being smart (e.g. run
more BACKGROUND tasks when the browser is idle).

What do you think?

[xiyuan, 2017-05-19 19:51:44]

On 2017/05/19 19:06:52, fdoray wrote:
> PTAnL
> 
>
https://codereview.chromium.org/2883273002/diff/1/chrome/browser/chromeos/app...
> File chrome/browser/chromeos/app_mode/kiosk_app_data.cc (left):
> 
>
https://codereview.chromium.org/2883273002/diff/1/chrome/browser/chromeos/app...
> chrome/browser/chromeos/app_mode/kiosk_app_data.cc:87:
> pool->GetNamedSequenceToken("KioskAppData.CrxLoaderWorker");
> On 2017/05/16 19:41:45, xiyuan wrote:
> > On 2017/05/16 17:58:56, fdoray wrote:
> > > Before my CL, the named sequence token provided mutual exclusion between
> tasks
> > > posted from different KioskAppData::CrxLoader instances.
> > > 
> > > With my CL, mutual exclusion is only between tasks posted from the same
> > > KioskAppData::CrxLoader instance.
> > > 
> > > Is that ok?
> > 
> > I would like to keep the existing behavior. Crx unpacking is heavy. If a
kiosk
> > device gets a few kiosk apps pushed down via policy, we could be running crx
> > unpacking in parallel, which slows down the device.
> > 
> > We have a KioskAppManager::GetBackgroundTaskRunner, which always creates a
new
> > task runner ATM. It is currently used in two places (for ExternalCache and
USB
> > update monitoring). We probably can change it to only create one task runner
> and
> > use it everywhere (including for CrxLoader).
> 
> The TaskScheduler is already limiting the rate at which
> base::TaskPriority::BACKGROUND tasks run. We would prefer if tasks ran on the
> same sequence only when there is a data dependency between them. Sharing a
> sequence to limit load prevents the TaskScheduler from being smart (e.g. run
> more BACKGROUND tasks when the browser is idle).
> 
> What do you think?

SG. Thanks for the explanation. There are no data deps among the crx loaders. We
should be good as long as we don't try to run as many of them as possible.

lgtm

[fdoray, 2017-05-19 19:56:18]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-05-19 19:57:04]

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-19 20:00:25]

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

[commit-bot: I haz the power, 2017-05-19 20:00:27]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fdoray, 2017-05-23 14:49:32]

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

[fdoray, 2017-05-23 14:49:40]

The CQ bit was unchecked by fdoray@chromium.org

[fdoray, 2017-05-23 14:49:43]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-05-23 14:49:43]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2883273002/#ps20001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-23 14:49:47]

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-23 15:21:42]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495550983108730,
"parent_rev": "0fd77db4e8e5e8ad1ebdb2bd19bd0a1092c3ca3b", "commit_rev":
"0a15ffb1835a5e36e3941fb0c71f23960a6d5e6e"}

[commit-bot: I haz the power, 2017-05-23 15:21:54]

Description was changed from

==========
Remove usage of SequencedWorkerPool::GetNamedSequenceToken from
kiosk_app_data.cc.

SequencedWorkerPool is being deprecated in favor of TaskScheduler.

BUG=667892
==========

to

==========
Remove usage of SequencedWorkerPool::GetNamedSequenceToken from
kiosk_app_data.cc.

SequencedWorkerPool is being deprecated in favor of TaskScheduler.

BUG=667892

Review-Url: https://codereview.chromium.org/2883273002
Cr-Commit-Position: refs/heads/master@{#473906}
Committed:
https://chromium.googlesource.com/chromium/src/+/0a15ffb1835a5e36e3941fb0c71f...
==========

[commit-bot: I haz the power, 2017-05-23 15:21:55]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/0a15ffb1835a5e36e3941fb0c71f...