|
|
Created:
3 years, 7 months ago by fdoray Modified:
3 years, 7 months ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, jshin+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove usage of SequencedWorkerPool::GetNamedSequenceToken from GetKeyboardLayoutsForLocale.
SequencedWorkerPool is being deprecated in favor of TaskScheduler.
BUG=667892
TBR=achuith@chromium.org
Review-Url: https://codereview.chromium.org/2884973004
Cr-Commit-Position: refs/heads/master@{#473270}
Committed: https://chromium.googlesource.com/chromium/src/+/b8de8c51c48c9bf5cc72c3e7363d14cd8b5b66c6
Patch Set 1 #Patch Set 2 : fix-build-error #Patch Set 3 : rebase #
Total comments: 7
Patch Set 4 : CR-emaxx-18 #
Messages
Total messages: 34 (24 generated)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fdoray@chromium.org changed reviewers: + achuith@chromium.org
Please take a look. https://codereview.chromium.org/2884973004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/l10n_util.cc (right): https://codereview.chromium.org/2884973004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/l10n_util.cc:583: base::PostTaskWithTraitsAndReplyWithResult( Is it important to have mutual exclusion between tasks posted by different GetKeyboardLayoutsForLocale() calls? Previously, this mutual exclusion was provided by using a SequencedTaskRunner. In the TaskScheduler world, we prefer not to create sequences for no reason.
achuith@chromium.org changed reviewers: + bartfab@chromium.org, emaxx@chromium.org, tnagel@chromium.org
Looks like GetKeyboardLayoutsForResolvedLocale was introduced here: https://codereview.chromium.org/417103003 Bartosz is no longer working on this, but maybe Maksim or Thiemo may be able to chime in? https://codereview.chromium.org/2884973004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/l10n_util.cc (left): https://codereview.chromium.org/2884973004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/l10n_util.cc:354: // DCHECK(task_runner->RunsTasksInCurrentSequence()); Is there an appropriate DCHECK to assert the right thread that we could use here?
> Bartosz is no longer working on this, but maybe Maksim or Thiemo may be able to > chime in? He might still have kept some memories, though?
LGTM. Although it may still make sense to ask bartfab@ - maybe he has some faint memories regarding this code. (You may want to ping him via chat and/or e-mail.) https://codereview.chromium.org/2884973004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/l10n_util.cc (left): https://codereview.chromium.org/2884973004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/l10n_util.cc:355: base::ThreadRestrictions::AssertIOAllowed(); nit: While you're here - please add #include "base/threading/thread_restrictions.h" https://codereview.chromium.org/2884973004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/l10n_util.cc:586: base::SequencedWorkerPool::SKIP_ON_SHUTDOWN); Is there a reason for not keeping the SKIP_ON_SHUTDOWN flag? https://codereview.chromium.org/2884973004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/l10n_util.cc (right): https://codereview.chromium.org/2884973004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/l10n_util.cc:583: base::PostTaskWithTraitsAndReplyWithResult( On 2017/05/17 17:52:16, fdoray wrote: > Is it important to have mutual exclusion between tasks posted by different > GetKeyboardLayoutsForLocale() calls? Previously, this mutual exclusion was > provided by using a SequencedTaskRunner. In the TaskScheduler world, we prefer > not to create sequences for no reason. Looking at implementation of l10n_util::GetApplicationLocale, I don't think there's any kind of global state used by it that can lead to concurrency issues. This function doesn't modify the ICU default locale as long as the set_icu_locale parameter is passed as false. It also uses some singletons (base::i18n::IllegalCharacters, ResourceBundle), but they seem to be fine.
I pinged bartfab@ on IM. https://codereview.chromium.org/2884973004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/l10n_util.cc (left): https://codereview.chromium.org/2884973004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/l10n_util.cc:355: base::ThreadRestrictions::AssertIOAllowed(); On 2017/05/19 17:58:16, emaxx wrote: > nit: While you're here - please add #include > "base/threading/thread_restrictions.h" Done. https://codereview.chromium.org/2884973004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/l10n_util.cc:586: base::SequencedWorkerPool::SKIP_ON_SHUTDOWN); On 2017/05/19 17:58:16, emaxx wrote: > Is there a reason for not keeping the SKIP_ON_SHUTDOWN flag? No. Added it back.
I pinged bartfab@ on IM. Landing now and will make changes later if required.
The CQ bit was checked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove usage of SequencedWorkerPool::GetNamedSequenceToken from GetKeyboardLayoutsForLocale. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 ========== to ========== Remove usage of SequencedWorkerPool::GetNamedSequenceToken from GetKeyboardLayoutsForLocale. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 TBR=achuith@chromium.org ==========
I pinged bartfab@ on IM. Landing now and will make changes later if required. TBR achuith@ who delegated to other reviewers.
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org Link to the patchset: https://codereview.chromium.org/2884973004/#ps60001 (title: "CR-emaxx-18")
The CQ bit was checked by fdoray@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495219034548600, "parent_rev": "d5a340817ee90fb66e5e23316311987e8f92e1ee", "commit_rev": "b8de8c51c48c9bf5cc72c3e7363d14cd8b5b66c6"}
Message was sent while issue was closed.
Description was changed from ========== Remove usage of SequencedWorkerPool::GetNamedSequenceToken from GetKeyboardLayoutsForLocale. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 TBR=achuith@chromium.org ========== to ========== Remove usage of SequencedWorkerPool::GetNamedSequenceToken from GetKeyboardLayoutsForLocale. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 TBR=achuith@chromium.org Review-Url: https://codereview.chromium.org/2884973004 Cr-Commit-Position: refs/heads/master@{#473270} Committed: https://chromium.googlesource.com/chromium/src/+/b8de8c51c48c9bf5cc72c3e7363d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b8de8c51c48c9bf5cc72c3e7363d... |