Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1162623002:
Implement FromDict/IntoDict for BackgroundTracingConfig. (Closed)
|
Created:
5 years, 6 months ago by shatch Modified:
5 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement FromDict/IntoDict for BackgroundTracingConfig.
This CL implements To/From JSON for preemptive and reactive configs.
Short Doc: https://docs.google.com/document/d/1N7iK2rL8Y7_qLrMYZf_EiU-Anezq6_hMiGoj0rs2T9I/edit#
BUG=493741
Committed: https://crrev.com/4d4bfe7b0b62ca497a921b0fbc570c6b271a50a6
Cr-Commit-Position: refs/heads/master@{#332673}
Patch Set 1 : #
Total comments: 21
Patch Set 2 : Oystein's comments addressed + cleanup. #
Total comments: 14
Patch Set 3 : Addressed dsinclair's comments. #Patch Set 4 : Rebase + fix asan. #Patch Set 5 : Rename file to fix mac gclient error. #
Messages
Total messages: 43 (22 generated)
Patchset #1 (id:1) has been deleted
oysteine@chromium.org changed reviewers: + oysteine@chromium.org
https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... File content/browser/tracing/background_tracing_config.cc (right): https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:28: "trace_on_trigger_or_10s_or_trigger_or_full"; Shouldn't this be trace_on_trigger_until_10s_or_trigger_or_full? https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:43: CHECK(false); NOTREACHED()? https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:47: bool IsStringValidCategoryPreset(const std::string& category_preset_string) { Feels like IsStringValidCategoryPreset() and StringToCategoryPreset() should be one function maybe? bool whatever(std::string* category). https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:63: CHECK(false); NOTREACHED()? https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:73: CHECK(false); NOTREACHED()? https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:117: // TODO(simonhatch): Implement UMA triggers. Brainstorming for later CLs: Let's add UMAs that count up when a dict results in a valid and activated scenario, and when we fail somewhere on the way. https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:174: base::ListValue* configs_list = new base::ListValue(); Is configs_list and config_dict leaking if we return false below, without appending them to the dict? Maybe they could be scoped_ptrs that get released on success instead? https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:204: base::DictionaryValue* config_dict = new base::DictionaryValue(); Same leak comment as above https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:217: default: Do we need the default? https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:234: if (!dict) Can this ever really be NULL? Feels like it should be a DCHECK and checked on the calling site. https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:256: switch (config->mode) { Switching on this and then static_casting() makes me a little bit nervous from a security perspective. Does the mode have to be a member variable? Can it be a pure virtual function instead implemented in each subclass?
https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... File content/browser/tracing/background_tracing_config.cc (right): https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:28: "trace_on_trigger_or_10s_or_trigger_or_full"; On 2015/05/28 17:00:46, Oystein wrote: > Shouldn't this be trace_on_trigger_until_10s_or_trigger_or_full? Ya was trying to shorten it since it was so long, but really no point since it's still super long. https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:43: CHECK(false); On 2015/05/28 17:00:46, Oystein wrote: > NOTREACHED()? Done. https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:47: bool IsStringValidCategoryPreset(const std::string& category_preset_string) { On 2015/05/28 17:00:46, Oystein wrote: > Feels like IsStringValidCategoryPreset() and StringToCategoryPreset() should be > one function maybe? bool whatever(std::string* category). Done. https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:63: CHECK(false); On 2015/05/28 17:00:46, Oystein wrote: > NOTREACHED()? Done. https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:73: CHECK(false); On 2015/05/28 17:00:46, Oystein wrote: > NOTREACHED()? Done. https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:174: base::ListValue* configs_list = new base::ListValue(); On 2015/05/28 17:00:47, Oystein wrote: > Is configs_list and config_dict leaking if we return false below, without > appending them to the dict? Maybe they could be scoped_ptrs that get released on > success instead? Done. https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:204: base::DictionaryValue* config_dict = new base::DictionaryValue(); On 2015/05/28 17:00:46, Oystein wrote: > Same leak comment as above Done. https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:217: default: On 2015/05/28 17:00:46, Oystein wrote: > Do we need the default? Yeah to error out for any enums we don't support yet. https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:234: if (!dict) On 2015/05/28 17:00:46, Oystein wrote: > Can this ever really be NULL? Feels like it should be a DCHECK and checked on > the calling site. Done. https://codereview.chromium.org/1162623002/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:256: switch (config->mode) { On 2015/05/28 17:00:46, Oystein wrote: > Switching on this and then static_casting() makes me a little bit nervous from a > security perspective. > > Does the mode have to be a member variable? Can it be a pure virtual function > instead implemented in each subclass? Chatted offline, maybe we can clean this up with a followup CL.
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
simonhatch@chromium.org changed reviewers: + nduca@chromium.org
I think this is ready for wider review, Nat ptal
simonhatch@chromium.org changed reviewers: + dsinclair@chromium.org
+dsinclair
https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... File content/browser/tracing/background_tracing_config.cc (right): https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:35: std::string CategoryPresetToString( Any reason these aren't all in the anonymous namespace? https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:72: scoped_ptr<BackgroundTracingPreemptiveConfig> config( ASSERT or DCHECK that dict is not null. https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:77: return NULL; s/NULL/nullptr/ (Does chromium use nullptr yet?) and below. https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:108: return NULL; Should this be a continue just to be on the safe side? We skip the UMA triggers but will process all non-UMA triggers. https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:142: if (type == kReactiveConfigRuleTraceOnTriggerOr10sOrTriggerOrFull) { if (type != kRea...) return nullptr; https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:180: return false; Should this be a continue? Right now, if one of these gets mixed in you'll lose all configs? https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:211: return false; continue? Or NOTREACHED and continue?
https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... File content/browser/tracing/background_tracing_config.cc (right): https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:35: std::string CategoryPresetToString( On 2015/06/01 14:29:54, dsinclair wrote: > Any reason these aren't all in the anonymous namespace? Done. https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:72: scoped_ptr<BackgroundTracingPreemptiveConfig> config( On 2015/06/01 14:29:54, dsinclair wrote: > ASSERT or DCHECK that dict is not null. Done. https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:77: return NULL; On 2015/06/01 14:29:53, dsinclair wrote: > s/NULL/nullptr/ (Does chromium use nullptr yet?) and below. Looks like this is recommended for new code: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/4mijeJHzxLg Done. https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:108: return NULL; On 2015/06/01 14:29:54, dsinclair wrote: > Should this be a continue just to be on the safe side? We skip the UMA triggers > but will process all non-UMA triggers. Yeah I guess there's no reason to completely bail out here. https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:142: if (type == kReactiveConfigRuleTraceOnTriggerOr10sOrTriggerOrFull) { On 2015/06/01 14:29:54, dsinclair wrote: > if (type != kRea...) > return nullptr; Done. https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:180: return false; On 2015/06/01 14:29:53, dsinclair wrote: > Should this be a continue? Right now, if one of these gets mixed in you'll lose > all configs? Ya true, done. https://codereview.chromium.org/1162623002/diff/80001/content/browser/tracing... content/browser/tracing/background_tracing_config.cc:211: return false; On 2015/06/01 14:29:53, dsinclair wrote: > continue? Or NOTREACHED and continue? Done.
lgtm
The CQ bit was checked by simonhatch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162623002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by simonhatch@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from dsinclair@chromium.org Link to the patchset: https://codereview.chromium.org/1162623002/#ps120001 (title: "Rebase + fix asan.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162623002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by simonhatch@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from dsinclair@chromium.org Link to the patchset: https://codereview.chromium.org/1162623002/#ps140001 (title: "Rename file to fix mac gclient error.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162623002/140001
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 simonhatch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162623002/140001
The CQ bit was unchecked by commit-bot@chromium.org
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 simonhatch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162623002/140001
The CQ bit was unchecked by commit-bot@chromium.org
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 simonhatch@chromium.org
The CQ bit was unchecked by simonhatch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162623002/140001
The CQ bit was unchecked by simonhatch@chromium.org
The CQ bit was checked by simonhatch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162623002/140001
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4d4bfe7b0b62ca497a921b0fbc570c6b271a50a6 Cr-Commit-Position: refs/heads/master@{#332673} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
