Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2151793002:
Add base::Value::GetTypeName(). (Closed)
|
Created:
4 years, 5 months ago by Lei Zhang Modified:
4 years, 5 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, oshima+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd base::Value::GetTypeName().
Committed: https://crrev.com/617092468b16c01673ab8a06cd3c2d4472ff905e
Cr-Commit-Position: refs/heads/master@{#406161}
Patch Set 1 #
Total comments: 7
Patch Set 2 : address comments, fix build #
Total comments: 2
Patch Set 3 : DCHECK array bounds #Patch Set 4 : win build #
Dependent Patchsets: Messages
Total messages: 41 (24 generated)
The CQ bit was checked by thestig@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...
thestig@chromium.org changed reviewers: + pneubeck@chromium.org
pneubeck: I know it says no reviews, but this is for your TODO. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2151793002/diff/1/chromeos/network/onc/onc_va... File chromeos/network/onc/onc_validator.cc (right): https://codereview.chromium.org/2151793002/diff/1/chromeos/network/onc/onc_va... chromeos/network/onc/onc_validator.cc:65: return base::DictionaryValue::From(result_value); forgot a std::move(), will load new patch set later.
great that someone cleans up base::Value ! https://codereview.chromium.org/2151793002/diff/1/chromeos/network/onc/onc_va... File chromeos/network/onc/onc_validator.cc (right): https://codereview.chromium.org/2151793002/diff/1/chromeos/network/onc/onc_va... chromeos/network/onc/onc_validator.cc:65: return base::DictionaryValue::From(result_value); On 2016/07/14 10:26:41, Lei Zhang (Slow Thursday) wrote: > forgot a std::move(), will load new patch set later. Slight difference to the old version is the dropped CHECK(result_dict). Probably OK to drop. https://codereview.chromium.org/2151793002/diff/1/components/policy/core/brow... File components/policy/core/browser/configuration_policy_handler.cc (right): https://codereview.chromium.org/2151793002/diff/1/components/policy/core/brow... components/policy/core/browser/configuration_policy_handler.cc:31: std::string ConfigurationPolicyHandler::ValueTypeToString( maybe inline and remove this function? https://codereview.chromium.org/2151793002/diff/1/extensions/common/api/decla... File extensions/common/api/declarative/declarative_manifest_data.cc (right): https://codereview.chromium.org/2151793002/diff/1/extensions/common/api/decla... extensions/common/api/declarative/declarative_manifest_data.cc:134: if (!list->GetDictionary(i, &dict)) { this whole block is rather confusing. maybe change to: for (const auto& element : *list) { const base::DictionaryValue* dict = nullptr; if (!element->GetAsDictionary(&dict)) { error_builder.Append(....element->GetType()); return; } ... }
https://codereview.chromium.org/2151793002/diff/1/chromeos/network/onc/onc_va... File chromeos/network/onc/onc_validator.cc (right): https://codereview.chromium.org/2151793002/diff/1/chromeos/network/onc/onc_va... chromeos/network/onc/onc_validator.cc:65: return base::DictionaryValue::From(result_value); On 2016/07/14 18:51:37, pneubeck (no reviews) wrote: > On 2016/07/14 10:26:41, Lei Zhang (Slow Thursday) wrote: > > forgot a std::move(), will load new patch set later. > > Slight difference to the old version is the dropped CHECK(result_dict). > Probably OK to drop. base::DictionaryValue::From() internally does the same thing, and understands the CHECK is not necessary. https://codereview.chromium.org/2151793002/diff/1/components/policy/core/brow... File components/policy/core/browser/configuration_policy_handler.cc (right): https://codereview.chromium.org/2151793002/diff/1/components/policy/core/brow... components/policy/core/browser/configuration_policy_handler.cc:31: std::string ConfigurationPolicyHandler::ValueTypeToString( On 2016/07/14 18:51:37, pneubeck (no reviews) wrote: > maybe inline and remove this function? It has a lot of callers. Added a TODO to make another pass. https://codereview.chromium.org/2151793002/diff/1/extensions/common/api/decla... File extensions/common/api/declarative/declarative_manifest_data.cc (right): https://codereview.chromium.org/2151793002/diff/1/extensions/common/api/decla... extensions/common/api/declarative/declarative_manifest_data.cc:134: if (!list->GetDictionary(i, &dict)) { On 2016/07/14 18:51:37, pneubeck (no reviews) wrote: > this whole block is rather confusing. > maybe change to: > > for (const auto& element : *list) { > const base::DictionaryValue* dict = nullptr; > if (!element->GetAsDictionary(&dict)) { > error_builder.Append(....element->GetType()); > return; > } > ... > } Sure. You are absolutely right, the else case can't happen.
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thestig@chromium.org changed reviewers: + benwells@chromium.org, cschuet@chromium.org, dcheng@chromium.org
dcheng: base/ cschuet: c*/ benwells: extensions/
base lgtm https://codereview.chromium.org/2151793002/diff/20001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2151793002/diff/20001/base/values.cc#newcode87 base/values.cc:87: return kTypeNames[type]; Or perhaps just DCHECK(type < arraysize(kTypeNames)) here.
On 2016/07/15 07:05:12, dcheng wrote: > base lgtm > > https://codereview.chromium.org/2151793002/diff/20001/base/values.cc > File base/values.cc (right): > > https://codereview.chromium.org/2151793002/diff/20001/base/values.cc#newcode87 > base/values.cc:87: return kTypeNames[type]; > Or perhaps just DCHECK(type < arraysize(kTypeNames)) here. lgtm
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
https://codereview.chromium.org/2151793002/diff/20001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2151793002/diff/20001/base/values.cc#newcode87 base/values.cc:87: return kTypeNames[type]; On 2016/07/15 07:05:12, dcheng wrote: > Or perhaps just DCHECK(type < arraysize(kTypeNames)) here. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/15 01:27:18, Lei Zhang wrote: > https://codereview.chromium.org/2151793002/diff/1/chromeos/network/onc/onc_va... > File chromeos/network/onc/onc_validator.cc (right): > > https://codereview.chromium.org/2151793002/diff/1/chromeos/network/onc/onc_va... > chromeos/network/onc/onc_validator.cc:65: return > base::DictionaryValue::From(result_value); > On 2016/07/14 18:51:37, pneubeck (no reviews) wrote: > > On 2016/07/14 10:26:41, Lei Zhang (Slow Thursday) wrote: > > > forgot a std::move(), will load new patch set later. > > > > Slight difference to the old version is the dropped CHECK(result_dict). > > Probably OK to drop. > > base::DictionaryValue::From() internally does the same thing, and understands > the CHECK is not necessary. If I read it correctly, DictionaryValue::From returns a nullptr if the value is not a dictionary. The old version did a CHECK, if that's the case, i.e. CHECK(value == null || IsDictionary(value)); > > https://codereview.chromium.org/2151793002/diff/1/components/policy/core/brow... > File components/policy/core/browser/configuration_policy_handler.cc (right): > > https://codereview.chromium.org/2151793002/diff/1/components/policy/core/brow... > components/policy/core/browser/configuration_policy_handler.cc:31: std::string > ConfigurationPolicyHandler::ValueTypeToString( > On 2016/07/14 18:51:37, pneubeck (no reviews) wrote: > > maybe inline and remove this function? > > It has a lot of callers. Added a TODO to make another pass. ok. I just found only 3 callers in code search (sometimes incomplete, i know): https://cs.chromium.org/search/?q=ConfigurationPolicyHandler+ValueTypeToStrin... > > https://codereview.chromium.org/2151793002/diff/1/extensions/common/api/decla... > File extensions/common/api/declarative/declarative_manifest_data.cc (right): > > https://codereview.chromium.org/2151793002/diff/1/extensions/common/api/decla... > extensions/common/api/declarative/declarative_manifest_data.cc:134: if > (!list->GetDictionary(i, &dict)) { > On 2016/07/14 18:51:37, pneubeck (no reviews) wrote: > > this whole block is rather confusing. > > maybe change to: > > > > for (const auto& element : *list) { > > const base::DictionaryValue* dict = nullptr; > > if (!element->GetAsDictionary(&dict)) { > > error_builder.Append(....element->GetType()); > > return; > > } > > ... > > } > > Sure. You are absolutely right, the else case can't happen. lgtm (from wrong account, but I can't login currently)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/15 07:45:17, pneubeck2 wrote: > ok. I just found only 3 callers in code search (sometimes incomplete, i know): > https://cs.chromium.org/search/?q=ConfigurationPolicyHandler+ValueTypeToStrin... There's more. https://codereview.chromium.org/2149253003/ will follow up to this. > lgtm (from wrong account, but I can't login currently) Thanks for helping out!
thestig@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+devlin for extensions/ as well.
lgtm
The CQ bit was checked by thestig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, cschuet@chromium.org, pneubeck@google.com Link to the patchset: https://codereview.chromium.org/2151793002/#ps60001 (title: "win build")
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add base::Value::GetTypeName(). ========== to ========== Add base::Value::GetTypeName(). Committed: https://crrev.com/617092468b16c01673ab8a06cd3c2d4472ff905e Cr-Commit-Position: refs/heads/master@{#406161} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/617092468b16c01673ab8a06cd3c2d4472ff905e Cr-Commit-Position: refs/heads/master@{#406161} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
