Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2553953002:
Split initialization_util into a Hermetic Library and a Variations Library (Closed)
DescriptionSplit initialization_util into a Hermetic Library and a Variations Library
This split allows callers to easily use some of the task_scheduler_init
structures without taking a dependency on variations.
Also adds unit tests for some of the functions.
BUG=662052
TBR=asvitkine
Committed: https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381
Cr-Commit-Position: refs/heads/master@{#437113}
Patch Set 1 #
Total comments: 48
Patch Set 2 : CR Feedback #
Total comments: 1
Patch Set 3 : Quick Wording Fix #
Total comments: 18
Patch Set 4 : CR Feedback #
Total comments: 4
Patch Set 5 : Quick Adjustments #
Total comments: 4
Dependent Patchsets: Messages
Total messages: 49 (29 generated)
The CQ bit was checked by robliao@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...
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
Here's the initialization_util split that will allow us to use components in content. I've updated the content CL to depend on this CL as well.
Description was changed from ========== Split initialization_util to be Usable by Content and Browser To allow for reuse of parameters between content, chrome/browser, and iOS, this change splits initialization_util into parts that can be included into content. The initialization directory can be included by content as it is relatively self contained. The variations portions can be referenced by chrome/browser and iOS. BUG=662052 ========== to ========== Split initialization_util to be Usable by content and chrome/browser To allow for reuse of parameters between content, chrome/browser, and iOS, this change splits initialization_util into parts that can be included into content. The initialization directory can be included by content as it is relatively self contained. The variations portions can be referenced by chrome/browser and iOS. BUG=662052 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
blundell@chromium.org changed reviewers: + blundell@chromium.org
Drive-by: We're trying to move away from including //components in //content, as it introduces a confusing layering structure. Did you talk with John about the approach here? (I see he's CC'd on the bug, but it doesn't like he chimed in at least there).
On 2016/12/06 08:54:49, blundell wrote: > Drive-by: > > We're trying to move away from including //components in //content, as it > introduces a confusing layering structure. Did you talk with John about the > approach here? (I see he's CC'd on the bug, but it doesn't like he chimed in at > least there). Yes: https://groups.google.com/a/google.com/d/topic/lucky-luke-dev/zAPenctn3G8/dis...
On 2016/12/06 15:48:50, gab wrote: > On 2016/12/06 08:54:49, blundell wrote: > > Drive-by: > > > > We're trying to move away from including //components in //content, as it > > introduces a confusing layering structure. Did you talk with John about the > > approach here? (I see he's CC'd on the bug, but it doesn't like he chimed in > at > > least there). > > Yes: > https://groups.google.com/a/google.com/d/topic/lucky-luke-dev/zAPenctn3G8/dis... Thanks! I replied on that email thread.
https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/initialization/browser_util.cc (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:9: #include <vector> <vector> already included in .h file https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:36: constexpr SchedulerWorkerPoolPredefinedParams kAllPredefinedParams[] = { You could add a SingleWorkerPoolConfiguration* member to SchedulerWorkerPoolPredefinedParams and rename it to SingleWorkerPoolPredefinedAndVariationsParams: struct SingleWorkerPoolPredefinedAndVariationsParams { const char* name; ThreadPriority priority_hint; SingleWorkerPoolVariationParams* config; } {"Background", ThreadPriority::BACKGROUND, &config.background}, {"BackgroundFileIO", ThreadPriority::BACKGROUND, &config.background_file_io}, {"Foreground", ThreadPriority::NORMAL, &config.foreground}, {"ForegroundFileIO", ThreadPriority::NORMAL, &config.foreground_file_io}, kAllPredefinedParams could no longer be constexpr, but it would be easier to verify that SingleWorkerPoolConfiguration* are associated with the right predefined params and you could use a range-based loop below. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:47: sizeof(SingleWorkerPoolConfiguration); Compute this the same way as in BrowserWorkerPoolsConfiguration(). constexpr size_t kNumWorkerPoolsDefined = sizeof(BrowserWorkerPoolsConfiguration) / sizeof(SingleWorkerPoolConfiguration); https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:55: static_assert(arraysize(kAllPredefinedParams) == arraysize(all_pools), Since most of the code in this file can be reused in child processes, I would: 1. Rename browser_util.cc -> util.cc. 2. Move the code below to a helper function: std::vector<base::SchedulerWorkerPoolParams> WorkerPoolPredefinedAndVariationsParamsToSchedulerWorkerPoolParams( WorkerPoolPredefinedAndVariationsParams* worker_pool_predefined_and_variations_params, size_t worker_pool_predefined_and_variations_params_size); 3. In separate CLs, reuse the helper function to create vectors of SchedulerWorkerPoolParams for child processes. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:71: // FOREGROUND or FOREGROUND_FILE_IO on unknown wrap https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:76: if (traits.with_file_io()) { no braces https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/initialization/browser_util.h (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.h:28: struct SingleWorkerPoolConfiguration { SingleWorkerPoolVariationParams? https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.h:34: struct BrowserWorkerPoolsConfiguration { BrowserWorkerPoolsVariationParams? https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/initialization_util.cc (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization_util.cc:17: void InitializeDefaultBrowserTaskScheduler() { This code should be moved to ios/ (in this CL or TODO).
Very nice, much cleaner IMO :), mostly nits below. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/DEPS (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/DEPS:1: include_rules = [] No DEPS file required if empty? https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/initialization/browser_util.cc (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:36: constexpr SchedulerWorkerPoolPredefinedParams kAllPredefinedParams[] = { static per https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/-2PSZ... https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:60: const auto& pool_config = all_pools[i]; s/&/*/ https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:71: // FOREGROUND or FOREGROUND_FILE_IO on unknown "unknown priorities"? "non-background" priorities" maybe? https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:76: if (traits.with_file_io()) { nit: no {} https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/initialization/browser_util.h (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.h:15: } // namespace base // namespace comments not required on short fwd-decl blocks. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.h:50: // GetDefaultBrowserSchedulerWorkerPoolParams(). This sentence makes it sound like the "index" is returned by those methods. Maybe "to the index of a browser worker pool vector provided by..."? https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/variations/browser_variations_util.cc (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.cc:48: const std::vector<std::string> tokens = SplitString( Use SplitStringPiece to get a StringPiece vector :) https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.cc:70: return config; Is there a way to static_assert here that there isn't a new unhandled field in SingleWorkerPoolConfiguration? https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.cc:95: const auto& worker_pool_name = kWorkerPoolNames[i]; s/&/*/ (always prefer auto* when referring to a pointer) https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.cc:102: auto& pool_config = *all_pools[i]; auto* pool_config = all_pools[i] is less surprising IMO (then you can deref for assignment and -> to read members) https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.cc:104: if (pool_config.threads <= 0 || pool_config.detach_period.is_zero()) { Negative detach_period would also be invalid, right? https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.cc:137: base::SequencedWorkerPool::EnableForProcess(); Hadn't previously noticed but it's pretty weird to have this critical call hidden all the way down here. I guess it's fine for now as it'll go away soonish but it's still unexpected that the whole browser won't work without this (when no experiment is involved). https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/variations/browser_variations_util.h (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.h:21: // based off of input from the variations parameters. // Redirects zero-to-many PostTask APIs to the browser task scheduler // based off of input from the variations parameters. (i.e. avoid "PostTask style")
Description was changed from ========== Split initialization_util to be Usable by content and chrome/browser To allow for reuse of parameters between content, chrome/browser, and iOS, this change splits initialization_util into parts that can be included into content. The initialization directory can be included by content as it is relatively self contained. The variations portions can be referenced by chrome/browser and iOS. BUG=662052 ========== to ========== Split initialization_util into a Hermetic Library and a Variations Library This split allows callers to easily use some of the task_scheduler_init structures without taking a dependency on variations. Also adds unit tests for some of the functions. BUG=662052 ==========
Patchset #2 (id:20001) has been deleted
Thanks for the comments! https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/DEPS (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/DEPS:1: include_rules = [] On 2016/12/06 19:16:07, gab wrote: > No DEPS file required if empty? Done. Seems to have the same effect either way. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/initialization/browser_util.cc (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:9: #include <vector> On 2016/12/06 17:22:23, fdoray wrote: > <vector> already included in .h file Done. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:36: constexpr SchedulerWorkerPoolPredefinedParams kAllPredefinedParams[] = { On 2016/12/06 17:22:23, fdoray wrote: > You could add a SingleWorkerPoolConfiguration* member to > SchedulerWorkerPoolPredefinedParams and rename it to > SingleWorkerPoolPredefinedAndVariationsParams: > > struct SingleWorkerPoolPredefinedAndVariationsParams { > const char* name; > ThreadPriority priority_hint; > SingleWorkerPoolVariationParams* config; > } > > {"Background", ThreadPriority::BACKGROUND, &config.background}, > {"BackgroundFileIO", ThreadPriority::BACKGROUND, &config.background_file_io}, > {"Foreground", ThreadPriority::NORMAL, &config.foreground}, > {"ForegroundFileIO", ThreadPriority::NORMAL, &config.foreground_file_io}, > > kAllPredefinedParams could no longer be constexpr, but it would be easier to > verify that SingleWorkerPoolConfiguration* are associated with the right > predefined params and you could use a range-based loop below. I can live with that. Done. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:36: constexpr SchedulerWorkerPoolPredefinedParams kAllPredefinedParams[] = { On 2016/12/06 19:16:07, gab wrote: > static per > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/-2PSZ... This can't be static with the array inclusion change now. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:47: sizeof(SingleWorkerPoolConfiguration); On 2016/12/06 17:22:23, fdoray wrote: > Compute this the same way as in BrowserWorkerPoolsConfiguration(). > > constexpr size_t kNumWorkerPoolsDefined = > sizeof(BrowserWorkerPoolsConfiguration) / > sizeof(SingleWorkerPoolConfiguration); Done https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:55: static_assert(arraysize(kAllPredefinedParams) == arraysize(all_pools), On 2016/12/06 17:22:23, fdoray wrote: > Since most of the code in this file can be reused in child processes, I would: > > 1. Rename browser_util.cc -> util.cc. > 2. Move the code below to a helper function: > > std::vector<base::SchedulerWorkerPoolParams> > WorkerPoolPredefinedAndVariationsParamsToSchedulerWorkerPoolParams( > WorkerPoolPredefinedAndVariationsParams* > worker_pool_predefined_and_variations_params, > size_t worker_pool_predefined_and_variations_params_size); > > 3. In separate CLs, reuse the helper function to create vectors of > SchedulerWorkerPoolParams for child processes. We can't do the rename because initialization/util.h clashes with initialization_util.h. The include once guards are the same. COMPONENTS_TASK_SCHEDULER_UTIL_INITIALIZATION_UTIL_H_. Let's table the name until the helper refactor. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:60: const auto& pool_config = all_pools[i]; On 2016/12/06 19:16:07, gab wrote: > s/&/*/ Done. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:71: // FOREGROUND or FOREGROUND_FILE_IO on unknown On 2016/12/06 17:22:23, fdoray wrote: > wrap Done. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:71: // FOREGROUND or FOREGROUND_FILE_IO on unknown On 2016/12/06 19:16:07, gab wrote: > "unknown priorities"? "non-background" priorities" maybe? Went with "any other" priorities. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:76: if (traits.with_file_io()) { On 2016/12/06 17:22:23, fdoray wrote: > no braces Done. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.cc:76: if (traits.with_file_io()) { On 2016/12/06 19:16:07, gab wrote: > nit: no {} Done. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/initialization/browser_util.h (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.h:15: } // namespace base On 2016/12/06 19:16:07, gab wrote: > // namespace comments not required on short fwd-decl blocks. Done. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.h:28: struct SingleWorkerPoolConfiguration { On 2016/12/06 17:22:24, fdoray wrote: > SingleWorkerPoolVariationParams? I intentionally avoided the word "variation" to make this work better with content as variations is more of a browser thing. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.h:34: struct BrowserWorkerPoolsConfiguration { On 2016/12/06 17:22:24, fdoray wrote: > BrowserWorkerPoolsVariationParams? See above. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization/browser_util.h:50: // GetDefaultBrowserSchedulerWorkerPoolParams(). On 2016/12/06 19:16:07, gab wrote: > This sentence makes it sound like the "index" is returned by those methods. > > Maybe "to the index of a browser worker pool vector provided by..."? Done. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/initialization_util.cc (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization_util.cc:17: void InitializeDefaultBrowserTaskScheduler() { On 2016/12/06 17:22:24, fdoray wrote: > This code should be moved to ios/ (in this CL or TODO). Still used by both for now. The goal of this refactor was to move things without impacting the other callers. A TODO that removes this function after the last caller goes seems unnecessary. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/variations/browser_variations_util.cc (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.cc:48: const std::vector<std::string> tokens = SplitString( On 2016/12/06 19:16:07, gab wrote: > Use SplitStringPiece to get a StringPiece vector :) Nice, but there's no StringPiece equivalent of base::StringToDouble. That will have to happen first. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.cc:70: return config; On 2016/12/06 19:16:08, gab wrote: > Is there a way to static_assert here that there isn't a new unhandled field in > SingleWorkerPoolConfiguration? I figured one out last night as I drove home and was going to add it to the default function. I'll add it here too. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.cc:95: const auto& worker_pool_name = kWorkerPoolNames[i]; On 2016/12/06 19:16:07, gab wrote: > s/&/*/ > > (always prefer auto* when referring to a pointer) Done. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.cc:102: auto& pool_config = *all_pools[i]; On 2016/12/06 19:16:07, gab wrote: > auto* pool_config = all_pools[i] > > is less surprising IMO (then you can deref for assignment and -> to read > members) Done. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.cc:104: if (pool_config.threads <= 0 || pool_config.detach_period.is_zero()) { On 2016/12/06 19:16:08, gab wrote: > Negative detach_period would also be invalid, right? Done. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.cc:137: base::SequencedWorkerPool::EnableForProcess(); On 2016/12/06 19:16:07, gab wrote: > Hadn't previously noticed but it's pretty weird to have this critical call > hidden all the way down here. I guess it's fine for now as it'll go away soonish > but it's still unexpected that the whole browser won't work without this (when > no experiment is involved). Yep, but that's also similar to task scheduler init like we had before. Either way. https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/variations/browser_variations_util.h (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.h:21: // based off of input from the variations parameters. On 2016/12/06 19:16:08, gab wrote: > // Redirects zero-to-many PostTask APIs to the browser task scheduler > // based off of input from the variations parameters. > > (i.e. avoid "PostTask style") Done.
The CQ bit was checked by robliao@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...
https://codereview.chromium.org/2553953002/diff/40001/components/task_schedul... File components/task_scheduler_util/DEPS (left): https://codereview.chromium.org/2553953002/diff/40001/components/task_schedul... components/task_scheduler_util/DEPS:1: include_rules = [ This is showing up as a move for me in git .../task_scheduler_util/{ => variations}/DEPS | 0 Rietveld oddity?
lgtm w/ comments https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... File components/task_scheduler_util/initialization/browser_util.cc (right): https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:28: const SingleWorkerPoolConfiguration& config; SchedulerWorkerPoolConfiguration vs. SingleWorkerPoolConfiguration: these names are similar but refer to different things. I would rename SingleWorkerPoolConfiguration -> SchedulerWorkerPoolCustomizableConfiguration to highlight the fact that this is the dynamically customizable part. I did my best to avoid using the word "variations" :) https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:66: for (size_t i = 0; i < arraysize(worker_pool_config); ++i) { for (const auto& config : worker_pool_config) { https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... File components/task_scheduler_util/initialization/browser_util_unittest.cc (right): https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util_unittest.cc:36: std::vector<base::SchedulerWorkerPoolParams> params_vector = const
https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/variations/browser_variations_util.cc (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.cc:48: const std::vector<std::string> tokens = SplitString( On 2016/12/07 00:03:54, robliao wrote: > On 2016/12/06 19:16:07, gab wrote: > > Use SplitStringPiece to get a StringPiece vector :) > > Nice, but there's no StringPiece equivalent of base::StringToDouble. That will > have to happen first. Lame, but you can still do this and use StringPiece::as_string() for that one call. Or, seems like s/std::string/StringPiece on StringToDouble() should just work (TM)?! https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... File components/task_scheduler_util/initialization/browser_util.cc (right): https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:30: SchedulerWorkerPoolConfiguration( constructors go before members in style-guide for both structs and classes https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:33: const SingleWorkerPoolConfiguration& config) To avoid name clash since structs to use _ suffixed members. I like the paradigm of suffixing the in-params with "_in". (I know it resolves correctly in the initializer list but readability > correctness :)) https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:67: const auto* const config = &worker_pool_config[i]; const auto& is fine (and prefered) in general, what I meant earlier is that const auto* should be used when the type is clearly a pointer (i.e. const auto& foo ...; foo->... is weird as the arrow operator comes out of nowhere yet encodes that auto has to resolve to a pointer so might as well be explicit) Either way, the for-each loop proposed by francois is the best solution here. https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:70: config->config.standby_thread_policy, config->config.threads, config->config is weird, can we change the name of one of the two members? https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:100: static_assert(kSizeAssignedFields == sizeof(config.background), Does this work if the struct is padded by the compiler? https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... File components/task_scheduler_util/variations/browser_variations_util.cc (right): https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/variations/browser_variations_util.cc:110: pool_config->detach_period < base::TimeDelta()) { <= and drop is_zero() check
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/variations/browser_variations_util.cc (right): https://codereview.chromium.org/2553953002/diff/1/components/task_scheduler_u... components/task_scheduler_util/variations/browser_variations_util.cc:48: const std::vector<std::string> tokens = SplitString( On 2016/12/07 15:53:06, gab wrote: > On 2016/12/07 00:03:54, robliao wrote: > > On 2016/12/06 19:16:07, gab wrote: > > > Use SplitStringPiece to get a StringPiece vector :) > > > > Nice, but there's no StringPiece equivalent of base::StringToDouble. That will > > have to happen first. > > Lame, but you can still do this and use StringPiece::as_string() for that one > call. > > Or, seems like s/std::string/StringPiece on StringToDouble() should just work > (TM)?! Ah, missed as_string(). I haven't done a deep reading on StringToDouble, but it seems like it should work. Most (if not all) of the equivalent required calls are there. https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... File components/task_scheduler_util/initialization/browser_util.cc (right): https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:28: const SingleWorkerPoolConfiguration& config; On 2016/12/07 13:34:11, fdoray wrote: > SchedulerWorkerPoolConfiguration vs. SingleWorkerPoolConfiguration: these names > are similar but refer to different things. I would rename > SingleWorkerPoolConfiguration -> SchedulerWorkerPoolCustomizableConfiguration to > highlight the fact that this is the dynamically customizable part. I did my best > to avoid using the word "variations" :) Done. https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:30: SchedulerWorkerPoolConfiguration( On 2016/12/07 15:53:06, gab wrote: > constructors go before members in style-guide for both structs and classes Done. https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:33: const SingleWorkerPoolConfiguration& config) On 2016/12/07 15:53:06, gab wrote: > To avoid name clash since structs to use _ suffixed members. I like the paradigm > of suffixing the in-params with "_in". > > (I know it resolves correctly in the initializer list but readability > > correctness :)) Done. https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:66: for (size_t i = 0; i < arraysize(worker_pool_config); ++i) { On 2016/12/07 13:34:11, fdoray wrote: > for (const auto& config : worker_pool_config) { Done. https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:67: const auto* const config = &worker_pool_config[i]; On 2016/12/07 15:53:06, gab wrote: > const auto& is fine (and prefered) in general, what I meant earlier is that > const auto* should be used when the type is clearly a pointer (i.e. const auto& > foo ...; foo->... is weird as the arrow operator comes out of nowhere yet > encodes that auto has to resolve to a pointer so might as well be explicit) > > Either way, the for-each loop proposed by francois is the best solution here. Acknowledged. https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:70: config->config.standby_thread_policy, config->config.threads, On 2016/12/07 15:53:06, gab wrote: > config->config is weird, can we change the name of one of the two members? config.config -> config.single_worker_pool_config https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:100: static_assert(kSizeAssignedFields == sizeof(config.background), On 2016/12/07 15:53:06, gab wrote: > Does this work if the struct is padded by the compiler? Padding does make this go out the window. I guess there isn't a way to easily determine without indirecting all of the parameters through a pointer type (and even then it's not a sure fire guarantee since padding is up to the compiler). The only way seems to be to indirect all of the parameters through another struct to add consistent padding, but that seems overkill. Removed for now. https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... File components/task_scheduler_util/initialization/browser_util_unittest.cc (right): https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/initialization/browser_util_unittest.cc:36: std::vector<base::SchedulerWorkerPoolParams> params_vector = On 2016/12/07 13:34:11, fdoray wrote: > const Done. https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... File components/task_scheduler_util/variations/browser_variations_util.cc (right): https://codereview.chromium.org/2553953002/diff/60001/components/task_schedul... components/task_scheduler_util/variations/browser_variations_util.cc:110: pool_config->detach_period < base::TimeDelta()) { On 2016/12/07 15:53:06, gab wrote: > <= and drop is_zero() check Done.
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.
lgtm w/ comments https://codereview.chromium.org/2553953002/diff/80001/components/task_schedul... File components/task_scheduler_util/initialization/browser_util.cc (right): https://codereview.chromium.org/2553953002/diff/80001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:78: // FOREGROUND_FILE_IO on any other priorities. "other" than what? https://codereview.chromium.org/2553953002/diff/80001/components/task_schedul... File components/task_scheduler_util/variations/browser_variations_util.cc (right): https://codereview.chromium.org/2553953002/diff/80001/components/task_schedul... components/task_scheduler_util/variations/browser_variations_util.cc:52: // as_string() call immediately above the declarations works around that. Hmm that seems incorrect to me, i.e. say we fix StringToDouble to use StringPiece then this goes away and causes unrelated issues below... Just initialize the numbers to zero at decl-site and inline as_string()
robliao@chromium.org changed reviewers: + asvitkine@chromium.org
TBR=asvitkine for trivial change in testing/variations/fieldtrial_testing_config.json https://codereview.chromium.org/2553953002/diff/80001/components/task_schedul... File components/task_scheduler_util/initialization/browser_util.cc (right): https://codereview.chromium.org/2553953002/diff/80001/components/task_schedul... components/task_scheduler_util/initialization/browser_util.cc:78: // FOREGROUND_FILE_IO on any other priorities. On 2016/12/07 20:17:22, gab wrote: > "other" than what? Done. // Returns the worker pool index for |traits| defaulting to FOREGROUND or // FOREGROUND_FILE_IO on any priorities other than background. https://codereview.chromium.org/2553953002/diff/80001/components/task_schedul... File components/task_scheduler_util/variations/browser_variations_util.cc (right): https://codereview.chromium.org/2553953002/diff/80001/components/task_schedul... components/task_scheduler_util/variations/browser_variations_util.cc:52: // as_string() call immediately above the declarations works around that. On 2016/12/07 20:17:22, gab wrote: > Hmm that seems incorrect to me, i.e. say we fix StringToDouble to use > StringPiece then this goes away and causes unrelated issues below... > > Just initialize the numbers to zero at decl-site and inline as_string() Initialized to 0 + added a comment. Changing StringToDouble to handle StringPieces has some subtleties that this doesn't need to wait on.
The CQ bit was checked by robliao@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 checked by robliao@chromium.org to run a CQ dry run
Patchset #5 (id:100001) has been deleted
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.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2553953002/#ps120001 (title: "Quick Adjustments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Split initialization_util into a Hermetic Library and a Variations Library This split allows callers to easily use some of the task_scheduler_init structures without taking a dependency on variations. Also adds unit tests for some of the functions. BUG=662052 ========== to ========== Split initialization_util into a Hermetic Library and a Variations Library This split allows callers to easily use some of the task_scheduler_init structures without taking a dependency on variations. Also adds unit tests for some of the functions. BUG=662052 TBR=asvitkine ==========
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481157485345270,
"parent_rev": "0f54fdc85e515626c21e39f2b08503406be6285b", "commit_rev":
"82494e61c72f6ab461a5281928ad29ed83862f98"}
Message was sent while issue was closed.
Description was changed from ========== Split initialization_util into a Hermetic Library and a Variations Library This split allows callers to easily use some of the task_scheduler_init structures without taking a dependency on variations. Also adds unit tests for some of the functions. BUG=662052 TBR=asvitkine ========== to ========== Split initialization_util into a Hermetic Library and a Variations Library This split allows callers to easily use some of the task_scheduler_init structures without taking a dependency on variations. Also adds unit tests for some of the functions. BUG=662052 TBR=asvitkine ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Split initialization_util into a Hermetic Library and a Variations Library This split allows callers to easily use some of the task_scheduler_init structures without taking a dependency on variations. Also adds unit tests for some of the functions. BUG=662052 TBR=asvitkine ========== to ========== Split initialization_util into a Hermetic Library and a Variations Library This split allows callers to easily use some of the task_scheduler_init structures without taking a dependency on variations. Also adds unit tests for some of the functions. BUG=662052 TBR=asvitkine Committed: https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381 Cr-Commit-Position: refs/heads/master@{#437113} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381 Cr-Commit-Position: refs/heads/master@{#437113}
Message was sent while issue was closed.
Still lgtm https://codereview.chromium.org/2553953002/diff/120001/components/task_schedu... File components/task_scheduler_util/variations/browser_variations_util.cc (right): https://codereview.chromium.org/2553953002/diff/120001/components/task_schedu... components/task_scheduler_util/variations/browser_variations_util.cc:52: // complain about uninitialized varaibles due to the as_string() call below. Curious, why does the as_string() have something to do with the warning?
Message was sent while issue was closed.
https://codereview.chromium.org/2553953002/diff/120001/components/task_schedu... File components/task_scheduler_util/initialization/DEPS (right): https://codereview.chromium.org/2553953002/diff/120001/components/task_schedu... components/task_scheduler_util/initialization/DEPS:1: # This component may be included into content and should follow the same rules Nit: This comment is out-of-date now, right?
Message was sent while issue was closed.
https://codereview.chromium.org/2553953002/diff/120001/components/task_schedu... File components/task_scheduler_util/initialization/DEPS (right): https://codereview.chromium.org/2553953002/diff/120001/components/task_schedu... components/task_scheduler_util/initialization/DEPS:1: # This component may be included into content and should follow the same rules On 2016/12/08 09:15:15, blundell wrote: > Nit: This comment is out-of-date now, right? Not yet. I weakened the statement from the original. When the dust settles and all is committed, I'll remove it. Thanks! https://codereview.chromium.org/2553953002/diff/120001/components/task_schedu... File components/task_scheduler_util/variations/browser_variations_util.cc (right): https://codereview.chromium.org/2553953002/diff/120001/components/task_schedu... components/task_scheduler_util/variations/browser_variations_util.cc:52: // complain about uninitialized varaibles due to the as_string() call below. On 2016/12/08 03:41:08, gab wrote: > Curious, why does the as_string() have something to do with the warning? Speculation: I suspect it's because it's the first call that doesn't do anything with the variable, oddly enough. It might very well be an MSVC bug because it shouldn't really care.
Message was sent while issue was closed.
fieldtrial_testing_config.json lgtm |
