Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2020413002:
Refactor out a new class for syncing field trials in child processes. (Closed)
|
Created:
4 years, 6 months ago by Alexei Svitkine (slow) Modified:
4 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor out a new class for syncing field trials in child processes.
This is split off out of chrome_render_thread_observer.cc so that
we can use it in other processes than the renderer. For example, in
the GPU process which now receives field trial and feature state
on the command-line. (That will be done in a follow-up CL.)
BUG=616171
Committed: https://crrev.com/23974972332452d270ba2af7fad725ce1a5e397c
Cr-Commit-Position: refs/heads/master@{#397316}
Patch Set 1 : #Patch Set 2 : Tweak param names & comments. #
Total comments: 12
Patch Set 3 : Address comments. #
Total comments: 3
Messages
Total messages: 27 (14 generated)
Description was changed from ========== . Merge branch 'master' of https://chromium.googlesource.com/chromium/src into ft_notify . BUG= ========== to ========== Refactor out a new class for syncing field trials in child processes. This is split off out of chrome_render_thread_observer.cc so that we can use it in other process than the renderer. For example, in the GPU process which now receives field trial and feature state on the command-line. BUG= ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Refactor out a new class for syncing field trials in child processes. This is split off out of chrome_render_thread_observer.cc so that we can use it in other process than the renderer. For example, in the GPU process which now receives field trial and feature state on the command-line. BUG= ========== to ========== Refactor out a new class for syncing field trials in child processes. This is split off out of chrome_render_thread_observer.cc so that we can use it in other process than the renderer. For example, in the GPU process which now receives field trial and feature state on the command-line. BUG=616171 ==========
asvitkine@chromium.org changed reviewers: + rkaplow@chromium.org, thestig@chromium.org
rkaplow: chrome/common/variations thestig: chrome/ OWNERS
Description was changed from ========== Refactor out a new class for syncing field trials in child processes. This is split off out of chrome_render_thread_observer.cc so that we can use it in other process than the renderer. For example, in the GPU process which now receives field trial and feature state on the command-line. BUG=616171 ========== to ========== Refactor out a new class for syncing field trials in child processes. This is split off out of chrome_render_thread_observer.cc so that we can use it in other processes than the renderer. For example, in the GPU process which now receives field trial and feature state on the command-line. (That will be done in a follow-up CL.) BUG=616171 ==========
lgtm https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... File chrome/common/variations/child_process_field_trial_syncer.cc (right): https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.cc:34: std::set<std::string> initially_active_trials_set; IWYU: <set> https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.cc:36: initially_active_trials_set.insert(std::move(entry.trial_name)); IYWU: <utility> https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.cc:41: for (const auto& trial : current_active_trials) { Side question as this is just refactoring existing code... How many times do we typically call OnFieldTrialGroupFinalized() which then sends an IPC message? Can ChromeViewHostMsg_FieldTrialActivated take a std::set/vector to reduce the number of messages? https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... File chrome/common/variations/child_process_field_trial_syncer.h (right): https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.h:8: #include <memory> not needed https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.h:11: #include "base/compiler_specific.h" probably not needed https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.h:49: IPC::Sender* ipc_sender_; IPC::Sender* const ipc_sender_;
Thanks! rkaplow: friendly ping https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... File chrome/common/variations/child_process_field_trial_syncer.cc (right): https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.cc:34: std::set<std::string> initially_active_trials_set; On 2016/06/01 20:52:51, Lei Zhang wrote: > IWYU: <set> Done. https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.cc:36: initially_active_trials_set.insert(std::move(entry.trial_name)); On 2016/06/01 20:52:51, Lei Zhang wrote: > IYWU: <utility> Done. https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.cc:41: for (const auto& trial : current_active_trials) { On 2016/06/01 20:52:51, Lei Zhang wrote: > Side question as this is just refactoring existing code... > > How many times do we typically call OnFieldTrialGroupFinalized() which then > sends an IPC message? Can ChromeViewHostMsg_FieldTrialActivated take a > std::set/vector to reduce the number of messages? The initial set is actually sent on the command-line at process start up as a list. The entries sent individually via IPC are sent later as they get activated, so it's not really possible to batch them (without e.g. delaying them and having some threshold for batching them). But I don't think the latter is frequent enough to warrant this. (i.e. individual activations happen rarely and depend on usage) https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... File chrome/common/variations/child_process_field_trial_syncer.h (right): https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.h:8: #include <memory> On 2016/06/01 20:52:51, Lei Zhang wrote: > not needed Done. https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.h:11: #include "base/compiler_specific.h" On 2016/06/01 20:52:51, Lei Zhang wrote: > probably not needed Done. https://codereview.chromium.org/2020413002/diff/60001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.h:49: IPC::Sender* ipc_sender_; On 2016/06/01 20:52:51, Lei Zhang wrote: > IPC::Sender* const ipc_sender_; Done.
https://codereview.chromium.org/2020413002/diff/80001/chrome/common/variation... File chrome/common/variations/child_process_field_trial_syncer.h (right): https://codereview.chromium.org/2020413002/diff/80001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.h:43: // base::FieldTrialList::Observer: ?
https://codereview.chromium.org/2020413002/diff/80001/chrome/common/variation... File chrome/common/variations/child_process_field_trial_syncer.h (right): https://codereview.chromium.org/2020413002/diff/80001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.h:43: // base::FieldTrialList::Observer: On 2016/06/01 21:08:38, rkaplow wrote: > ? That's the comment syntax to indicate that the methods below are overrides of that parent class.
lgtm https://codereview.chromium.org/2020413002/diff/80001/chrome/common/variation... File chrome/common/variations/child_process_field_trial_syncer.h (right): https://codereview.chromium.org/2020413002/diff/80001/chrome/common/variation... chrome/common/variations/child_process_field_trial_syncer.h:43: // base::FieldTrialList::Observer: On 2016/06/01 21:13:58, Alexei Svitkine (slow) wrote: > On 2016/06/01 21:08:38, rkaplow wrote: > > ? > > That's the comment syntax to indicate that the methods below are overrides of > that parent class. Acknowledged.
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2020413002/#ps80001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020413002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2020413002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020413002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2020413002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020413002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2020413002/80001
Message was sent while issue was closed.
Description was changed from ========== Refactor out a new class for syncing field trials in child processes. This is split off out of chrome_render_thread_observer.cc so that we can use it in other processes than the renderer. For example, in the GPU process which now receives field trial and feature state on the command-line. (That will be done in a follow-up CL.) BUG=616171 ========== to ========== Refactor out a new class for syncing field trials in child processes. This is split off out of chrome_render_thread_observer.cc so that we can use it in other processes than the renderer. For example, in the GPU process which now receives field trial and feature state on the command-line. (That will be done in a follow-up CL.) BUG=616171 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Refactor out a new class for syncing field trials in child processes. This is split off out of chrome_render_thread_observer.cc so that we can use it in other processes than the renderer. For example, in the GPU process which now receives field trial and feature state on the command-line. (That will be done in a follow-up CL.) BUG=616171 ========== to ========== Refactor out a new class for syncing field trials in child processes. This is split off out of chrome_render_thread_observer.cc so that we can use it in other processes than the renderer. For example, in the GPU process which now receives field trial and feature state on the command-line. (That will be done in a follow-up CL.) BUG=616171 Committed: https://crrev.com/23974972332452d270ba2af7fad725ce1a5e397c Cr-Commit-Position: refs/heads/master@{#397316} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/23974972332452d270ba2af7fad725ce1a5e397c Cr-Commit-Position: refs/heads/master@{#397316} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
