Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1928863002:
Enable FeatureList for the GPU process. (Closed)
|
Created:
4 years, 7 months ago by erikchen Modified:
4 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable FeatureList for the GPU process.
This CL plumbs feature flags from the browser process to the gpu process.
This CL moves FeatureList initialization into content_main_runner.cc, for the
gpu, utility and ppapi plugin processes. Eventually, all non-browser FeatureList
initialization should live in content_main_runner.cc.
This CL sets the entropy provider to NULL, and creates trials from
kForceFieldTrials for the gpu, utility, and ppapi plugin processes.
BUG=606850, 563705
Committed: https://crrev.com/55edbffd354ade26373730bcb2f744c46b46b1ed
Cr-Commit-Position: refs/heads/master@{#391403}
Patch Set 1 #Patch Set 2 : #
Total comments: 7
Patch Set 3 : Comments from asvitkine. #
Total comments: 4
Patch Set 4 : Comments from asvitkine. #Patch Set 5 : More comments. #
Total comments: 2
Patch Set 6 : Comments from asvitkine. #Patch Set 7 : Zygote process! #
Total comments: 6
Patch Set 8 : #Patch Set 9 : #
Total comments: 6
Patch Set 10 : Comments from asvitkine. #
Total comments: 5
Patch Set 11 : Comments from jam. #Patch Set 12 : Comments from jam. #
Total comments: 4
Patch Set 13 : Comments from jam. #
Total comments: 2
Messages
Total messages: 58 (22 generated)
Description was changed from ========== Enable FeatureList for the GPU process. This CL plumbs feature flags from the browser process to the gpu process. It also moves FeatureList initialization into content_main_runner.cc, for the GPU, utility and ppapi plugin processes. Eventually, all non-browser FeatureList initialization should live in content_main_runner.cc. patch from issue 1927433002 at patchset 80001 (http://crrev.com/1927433002#ps80001) ========== to ========== Enable FeatureList for the GPU process. This CL plumbs feature flags from the browser process to the gpu process. It also moves FeatureList initialization into content_main_runner.cc, for the GPU, utility and ppapi plugin processes. Eventually, all non-browser FeatureList initialization should live in content_main_runner.cc. BUG= ==========
Description was changed from ========== Enable FeatureList for the GPU process. This CL plumbs feature flags from the browser process to the gpu process. It also moves FeatureList initialization into content_main_runner.cc, for the GPU, utility and ppapi plugin processes. Eventually, all non-browser FeatureList initialization should live in content_main_runner.cc. BUG= ========== to ========== Enable FeatureList for the GPU process. This CL plumbs feature flags from the browser process to the gpu process. This CL moves FeatureList initialization into content_main_runner.cc, for the gpu, utility and ppapi plugin processes. Eventually, all non-browser FeatureList initialization should live in content_main_runner.cc. This CL sets the entropy provider to NULL, and creates trials from kForceFieldTrials for the gpu, utility, and ppapi plugin processes. BUG= ==========
Description was changed from ========== Enable FeatureList for the GPU process. This CL plumbs feature flags from the browser process to the gpu process. This CL moves FeatureList initialization into content_main_runner.cc, for the gpu, utility and ppapi plugin processes. Eventually, all non-browser FeatureList initialization should live in content_main_runner.cc. This CL sets the entropy provider to NULL, and creates trials from kForceFieldTrials for the gpu, utility, and ppapi plugin processes. BUG= ========== to ========== Enable FeatureList for the GPU process. This CL plumbs feature flags from the browser process to the gpu process. This CL moves FeatureList initialization into content_main_runner.cc, for the gpu, utility and ppapi plugin processes. Eventually, all non-browser FeatureList initialization should live in content_main_runner.cc. This CL sets the entropy provider to NULL, and creates trials from kForceFieldTrials for the gpu, utility, and ppapi plugin processes. BUG=606850 ==========
erikchen@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine: Please review. I tried moving renderer FeatureList initialization into content_main_runner.cc, but ran into difficulties, and didn't want to spend too much time looking into it. Hopefully this will make it easier for you to enable FeatureList in all child processes.
Thanks for working on this! https://codereview.chromium.org/1928863002/diff/20001/content/app/content_mai... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1928863002/diff/20001/content/app/content_mai... content/app/content_main_runner.cc:735: // TODO(asvitkine): This logic should run for all child processes. Thanks! You can add 563705 to the BUG= line of this CL. https://codereview.chromium.org/1928863002/diff/20001/content/app/content_mai... content/app/content_main_runner.cc:743: base::FieldTrialList field_trial_list(NULL); This is not correct. The FieldTrialList should stay alive for the duration of the process. In this case, if you put it outside of the loop, it should be fine since this calls RunNamedProcessTypeMain() and so the lifetime of FieldTrialList will outlive whatever else the process does. You can make it a unique_ptr above the loop and create the object in the loop. https://codereview.chromium.org/1928863002/diff/20001/content/app/content_mai... content/app/content_main_runner.cc:747: if (command_line.HasSwitch(switches::kForceFieldTrials)) { It would be good to check whether we propagate this flag when launching child processes. Not sure if you've done that already.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Description was changed from ========== Enable FeatureList for the GPU process. This CL plumbs feature flags from the browser process to the gpu process. This CL moves FeatureList initialization into content_main_runner.cc, for the gpu, utility and ppapi plugin processes. Eventually, all non-browser FeatureList initialization should live in content_main_runner.cc. This CL sets the entropy provider to NULL, and creates trials from kForceFieldTrials for the gpu, utility, and ppapi plugin processes. BUG=606850 ========== to ========== Enable FeatureList for the GPU process. This CL plumbs feature flags from the browser process to the gpu process. This CL moves FeatureList initialization into content_main_runner.cc, for the gpu, utility and ppapi plugin processes. Eventually, all non-browser FeatureList initialization should live in content_main_runner.cc. This CL sets the entropy provider to NULL, and creates trials from kForceFieldTrials for the gpu, utility, and ppapi plugin processes. BUG=606850, 563705 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1928863002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928863002/40001
https://codereview.chromium.org/1928863002/diff/20001/content/app/content_mai... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1928863002/diff/20001/content/app/content_mai... content/app/content_main_runner.cc:735: // TODO(asvitkine): This logic should run for all child processes. On 2016/04/28 15:23:33, Alexei Svitkine wrote: > Thanks! You can add 563705 to the BUG= line of this CL. Done. https://codereview.chromium.org/1928863002/diff/20001/content/app/content_mai... content/app/content_main_runner.cc:743: base::FieldTrialList field_trial_list(NULL); On 2016/04/28 15:23:33, Alexei Svitkine wrote: > This is not correct. The FieldTrialList should stay alive for the duration of > the process. > > In this case, if you put it outside of the loop, it should be fine since this > calls RunNamedProcessTypeMain() and so the lifetime of FieldTrialList will > outlive whatever else the process does. > > You can make it a unique_ptr above the loop and create the object in the loop. You keep talking about "the loop". Are you referring to the loop in RunNamedProcessTypeMain()? I moved the object constructor to line 739, where it should stay alive until RunNamedProcessTypeMain() returns. I prefer to run that code as early as possible. https://codereview.chromium.org/1928863002/diff/20001/content/app/content_mai... content/app/content_main_runner.cc:747: if (command_line.HasSwitch(switches::kForceFieldTrials)) { On 2016/04/28 15:23:33, Alexei Svitkine wrote: > It would be good to check whether we propagate this flag when launching child > processes. Not sure if you've done that already. We weren't propagating it. I added it to ChildProcessHostImpl::CopyEnableDisableFeatureFlags.
https://codereview.chromium.org/1928863002/diff/20001/content/app/content_mai... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1928863002/diff/20001/content/app/content_mai... content/app/content_main_runner.cc:743: base::FieldTrialList field_trial_list(NULL); On 2016/04/29 17:45:04, erikchen wrote: > On 2016/04/28 15:23:33, Alexei Svitkine wrote: > > This is not correct. The FieldTrialList should stay alive for the duration of > > the process. > > > > In this case, if you put it outside of the loop, it should be fine since this > > calls RunNamedProcessTypeMain() and so the lifetime of FieldTrialList will > > outlive whatever else the process does. > > > > You can make it a unique_ptr above the loop and create the object in the loop. > > You keep talking about "the loop". Are you referring to the loop in > RunNamedProcessTypeMain()? I moved the object constructor to line 739, where it > should stay alive until RunNamedProcessTypeMain() returns. I prefer to run that > code as early as possible. Woops, when I said "loop" I meant "if". Sorry for the confusion. https://codereview.chromium.org/1928863002/diff/40001/content/app/content_mai... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1928863002/diff/40001/content/app/content_mai... content/app/content_main_runner.cc:736: // provider to NULL to disallow non-browser processes from creating their Nit: NULL -> null and NULL -> nullptr below. https://codereview.chromium.org/1928863002/diff/40001/content/app/content_mai... content/app/content_main_runner.cc:739: base::FieldTrialList field_trial_list(NULL); How does this interact with: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... i.e. It would be incorrect to have two FieldTrialList's active in that case. If the renderer also runs the code, then we should make this a scoped_ptr and create it inside the if. (But maybe I'm wrong and renderers don't run this code path?)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
https://codereview.chromium.org/1928863002/diff/40001/content/app/content_mai... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1928863002/diff/40001/content/app/content_mai... content/app/content_main_runner.cc:736: // provider to NULL to disallow non-browser processes from creating their On 2016/04/29 18:06:32, Alexei Svitkine wrote: > Nit: NULL -> null > > and NULL -> nullptr below. Done. https://codereview.chromium.org/1928863002/diff/40001/content/app/content_mai... content/app/content_main_runner.cc:739: base::FieldTrialList field_trial_list(NULL); On 2016/04/29 18:06:32, Alexei Svitkine wrote: > How does this interact with: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > i.e. It would be incorrect to have two FieldTrialList's active in that case. If > the renderer also runs the code, then we should make this a scoped_ptr and > create it inside the if. (But maybe I'm wrong and renderers don't run this code > path?) whoops! I had meant to remove that.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1928863002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928863002/80001
lgtm with one final suggestion thanks again! https://codereview.chromium.org/1928863002/diff/80001/content/renderer/render... File content/renderer/renderer_main.cc (left): https://codereview.chromium.org/1928863002/diff/80001/content/renderer/render... content/renderer/renderer_main.cc:174: base::FeatureList::SetInstance(std::move(feature_list)); How about removing this too and adding kRendererProcess to the switch?
https://codereview.chromium.org/1928863002/diff/80001/content/renderer/render... File content/renderer/renderer_main.cc (left): https://codereview.chromium.org/1928863002/diff/80001/content/renderer/render... content/renderer/renderer_main.cc:174: base::FeatureList::SetInstance(std::move(feature_list)); On 2016/04/29 18:42:13, Alexei Svitkine wrote: > How about removing this too and adding kRendererProcess to the switch? And by "switch", I of course mean "if" because apparently I confuse all the constructs!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1928863002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928863002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1928863002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928863002/120001
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 tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1928863002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928863002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_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 erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1928863002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928863002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
asvitkine: PTAL
https://codereview.chromium.org/1928863002/diff/120001/content/common/child_p... File content/common/child_process_host_impl.cc (right): https://codereview.chromium.org/1928863002/diff/120001/content/common/child_p... content/common/child_process_host_impl.cc:204: void ChildProcessHostImpl::CopyEnableDisableFeatureFlags( Nit: CopyFeatureAndFieldTrialFlags() since now this also doing field_trial_states. https://codereview.chromium.org/1928863002/diff/120001/content/common/child_p... content/common/child_process_host_impl.cc:217: // histograms relating to the base::FieldTrial states. Nit: I'd remove the ", or record histograms relating to the base::FieldTrial states." part of the sentence. I think this was referring to the old field-trial-suffixed histograms which we don't use anymore. https://codereview.chromium.org/1928863002/diff/120001/content/renderer/rende... File content/renderer/renderer_main.cc (left): https://codereview.chromium.org/1928863002/diff/120001/content/renderer/rende... content/renderer/renderer_main.cc:174: base::FeatureList::SetInstance(std::move(feature_list)); What happened to remove this? I don't see it in the latest patchset. https://codereview.chromium.org/1928863002/diff/160001/content/app/content_ma... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1928863002/diff/160001/content/app/content_ma... content/app/content_main_runner.cc:131: namespace { Nit: Add an empty line below. You can remove the empty line above it. https://codereview.chromium.org/1928863002/diff/160001/content/app/content_ma... content/app/content_main_runner.cc:132: void InitializeFieldTrialAndFeatureList( Nit: Please add a short comment about what this does. https://codereview.chromium.org/1928863002/diff/160001/content/app/content_ma... content/app/content_main_runner.cc:168: } // namespace Nit: Add an empty line above it.
asvitkine: PTAL https://codereview.chromium.org/1928863002/diff/120001/content/common/child_p... File content/common/child_process_host_impl.cc (right): https://codereview.chromium.org/1928863002/diff/120001/content/common/child_p... content/common/child_process_host_impl.cc:204: void ChildProcessHostImpl::CopyEnableDisableFeatureFlags( On 2016/05/03 18:59:47, Alexei Svitkine wrote: > Nit: CopyFeatureAndFieldTrialFlags() since now this also doing > field_trial_states. Done. https://codereview.chromium.org/1928863002/diff/120001/content/common/child_p... content/common/child_process_host_impl.cc:217: // histograms relating to the base::FieldTrial states. On 2016/05/03 18:59:47, Alexei Svitkine wrote: > Nit: I'd remove the ", or record histograms relating to the base::FieldTrial > states." part of the sentence. I think this was referring to the old > field-trial-suffixed histograms which we don't use anymore. Done. https://codereview.chromium.org/1928863002/diff/120001/content/renderer/rende... File content/renderer/renderer_main.cc (left): https://codereview.chromium.org/1928863002/diff/120001/content/renderer/rende... content/renderer/renderer_main.cc:174: base::FeatureList::SetInstance(std::move(feature_list)); On 2016/05/03 18:59:47, Alexei Svitkine wrote: > What happened to remove this? I don't see it in the latest patchset. Moving this logic to contain_main_runner causes problems with the global descriptor table for renderer zygote processes on Linux. I spent some time looking into it and couldn't figure it out, and didn't want to pour too much time into this. https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... https://codereview.chromium.org/1928863002/diff/160001/content/app/content_ma... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1928863002/diff/160001/content/app/content_ma... content/app/content_main_runner.cc:131: namespace { On 2016/05/03 18:59:47, Alexei Svitkine wrote: > Nit: Add an empty line below. You can remove the empty line above it. Done. https://codereview.chromium.org/1928863002/diff/160001/content/app/content_ma... content/app/content_main_runner.cc:132: void InitializeFieldTrialAndFeatureList( On 2016/05/03 18:59:47, Alexei Svitkine wrote: > Nit: Please add a short comment about what this does. Done. https://codereview.chromium.org/1928863002/diff/160001/content/app/content_ma... content/app/content_main_runner.cc:168: } // namespace On 2016/05/03 18:59:47, Alexei Svitkine wrote: > Nit: Add an empty line above it. Done.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1928863002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928863002/180001
lgtm
erikchen@chromium.org changed reviewers: + jam@chromium.org
jam: Please review.
https://codereview.chromium.org/1928863002/diff/180001/content/app/content_ma... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1928863002/diff/180001/content/app/content_ma... content/app/content_main_runner.cc:143: // TODO(asvitkine): This logic should run for all child processes. why isn't this change doing it for all processes? I don't see a particular reason not to do this now. https://codereview.chromium.org/1928863002/diff/180001/content/common/child_p... File content/common/child_process_host_impl.h (right): https://codereview.chromium.org/1928863002/diff/180001/content/common/child_p... content/common/child_process_host_impl.h:68: static void CopyFeatureAndFieldTrialFlags(base::CommandLine* cmd_line); this code is only called from content/browser, so it doesn't below in content/common (i.e. code shared between browser and child processes). you can add this static method to BrowserChildProcessHostImpl or create new files for it in content/browser
jam: PTAL https://codereview.chromium.org/1928863002/diff/180001/content/app/content_ma... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1928863002/diff/180001/content/app/content_ma... content/app/content_main_runner.cc:143: // TODO(asvitkine): This logic should run for all child processes. On 2016/05/03 19:45:08, jam wrote: > why isn't this change doing it for all processes? I don't see a particular > reason not to do this now. Moving this logic to contain_main_runner causes problems with the global descriptor table for renderer zygote processes on Linux. I spent some time looking into it and couldn't figure it out, and didn't want to pour too much time into this. https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... https://codereview.chromium.org/1928863002/diff/180001/content/common/child_p... File content/common/child_process_host_impl.h (right): https://codereview.chromium.org/1928863002/diff/180001/content/common/child_p... content/common/child_process_host_impl.h:68: static void CopyFeatureAndFieldTrialFlags(base::CommandLine* cmd_line); On 2016/05/03 19:45:08, jam wrote: > this code is only called from content/browser, so it doesn't below in > content/common (i.e. code shared between browser and child processes). you can > add this static method to BrowserChildProcessHostImpl or create new files for it > in content/browser I added a static method to BrowserChildProcessHostImpl.
https://codereview.chromium.org/1928863002/diff/180001/content/app/content_ma... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1928863002/diff/180001/content/app/content_ma... content/app/content_main_runner.cc:143: // TODO(asvitkine): This logic should run for all child processes. On 2016/05/03 19:55:05, erikchen wrote: > On 2016/05/03 19:45:08, jam wrote: > > why isn't this change doing it for all processes? I don't see a particular > > reason not to do this now. > > Moving this logic to contain_main_runner causes problems with the global > descriptor table for renderer zygote processes on Linux. I spent some time > looking into it and couldn't figure it out, and didn't want to pour too much > time into this. In general when we plumb stuff like this to child processes, we treat all child processes the same. Otherwise it kicks the can down the road and we'll have to solve this problem later (after some time debugging why a feature doesn't work in a child process). I really prefer to keep child processes as similar as possible for this code in base/. Thanks for the link, I looked at it and nothing stuck out as obvious. Can you share how far you got? We can also loop in folks that are most familiar with zygote. > > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
On 2016/05/03 20:07:38, jam wrote: > https://codereview.chromium.org/1928863002/diff/180001/content/app/content_ma... > File content/app/content_main_runner.cc (right): > > https://codereview.chromium.org/1928863002/diff/180001/content/app/content_ma... > content/app/content_main_runner.cc:143: // TODO(asvitkine): This logic should > run for all child processes. > On 2016/05/03 19:55:05, erikchen wrote: > > On 2016/05/03 19:45:08, jam wrote: > > > why isn't this change doing it for all processes? I don't see a particular > > > reason not to do this now. > > > > Moving this logic to contain_main_runner causes problems with the global > > descriptor table for renderer zygote processes on Linux. I spent some time > > looking into it and couldn't figure it out, and didn't want to pour too much > > time into this. > > In general when we plumb stuff like this to child processes, we treat all child > processes the same. Otherwise it kicks the can down the road and we'll have to > solve this problem later (after some time debugging why a feature doesn't work > in a child process). > > I really prefer to keep child processes as similar as possible for this code in > base/. > > Thanks for the link, I looked at it and nothing stuck out as obvious. Can you > share how far you got? We can also loop in folks that are most familiar with > zygote. > > > > > > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... jam: PTAL It turns out that the previous patch set had a bug on LInux Zygote processes, which is why it wasn't working. [the command line doesn't get updated to reflect the field trial flags until after some zygote logic runs).
lgtm https://codereview.chromium.org/1928863002/diff/220001/content/app/content_ma... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1928863002/diff/220001/content/app/content_ma... content/app/content_main_runner.cc:777: if (process_type != "" && process_type != switches::kZygoteProcess) nit !process_type.empty() which is convention https://codereview.chromium.org/1928863002/diff/220001/content/common/child_p... File content/common/child_process_host_impl.h (right): https://codereview.chromium.org/1928863002/diff/220001/content/common/child_p... content/common/child_process_host_impl.h:26: class CommandLine; nit: undo
https://codereview.chromium.org/1928863002/diff/220001/content/app/content_ma... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1928863002/diff/220001/content/app/content_ma... content/app/content_main_runner.cc:777: if (process_type != "" && process_type != switches::kZygoteProcess) On 2016/05/03 21:33:48, jam wrote: > nit !process_type.empty() which is convention Done. https://codereview.chromium.org/1928863002/diff/220001/content/common/child_p... File content/common/child_process_host_impl.h (right): https://codereview.chromium.org/1928863002/diff/220001/content/common/child_p... content/common/child_process_host_impl.h:26: class CommandLine; On 2016/05/03 21:33:48, jam wrote: > nit: undo Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/1928863002/#ps240001 (title: "Comments from jam.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1928863002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928863002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Enable FeatureList for the GPU process. This CL plumbs feature flags from the browser process to the gpu process. This CL moves FeatureList initialization into content_main_runner.cc, for the gpu, utility and ppapi plugin processes. Eventually, all non-browser FeatureList initialization should live in content_main_runner.cc. This CL sets the entropy provider to NULL, and creates trials from kForceFieldTrials for the gpu, utility, and ppapi plugin processes. BUG=606850, 563705 ========== to ========== Enable FeatureList for the GPU process. This CL plumbs feature flags from the browser process to the gpu process. This CL moves FeatureList initialization into content_main_runner.cc, for the gpu, utility and ppapi plugin processes. Eventually, all non-browser FeatureList initialization should live in content_main_runner.cc. This CL sets the entropy provider to NULL, and creates trials from kForceFieldTrials for the gpu, utility, and ppapi plugin processes. BUG=606850, 563705 Committed: https://crrev.com/55edbffd354ade26373730bcb2f744c46b46b1ed Cr-Commit-Position: refs/heads/master@{#391403} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/55edbffd354ade26373730bcb2f744c46b46b1ed Cr-Commit-Position: refs/heads/master@{#391403}
Message was sent while issue was closed.
lgtm % nit Maybe send a follow-up CL to clean that up? Thanks! https://codereview.chromium.org/1928863002/diff/240001/content/app/content_ma... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1928863002/diff/240001/content/app/content_ma... content/app/content_main_runner.cc:141: command_line.GetSwitchValueASCII(switches::kProcessType); Nit: This line is no longer needed, right?
Message was sent while issue was closed.
https://codereview.chromium.org/1928863002/diff/240001/content/app/content_ma... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1928863002/diff/240001/content/app/content_ma... content/app/content_main_runner.cc:141: command_line.GetSwitchValueASCII(switches::kProcessType); On 2016/05/04 15:08:08, Alexei Svitkine wrote: > Nit: This line is no longer needed, right? https://codereview.chromium.org/1948663003/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
