Decrement CreateDoubleValue count

Decrement CreateDoubleValue count

BUG=160586
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284304

[Evan Stade, 2014-07-15 19:54:21]

bartfab:
  components/policy/*
ajuma:
  cc/base/math_util.cc
battre:
  base/prefs/*
  components/pref_registry/pref_registry_syncable.cc

[ajuma, 2014-07-15 20:07:52]

cc lgtm

[battre, 2014-07-16 07:37:12]

https://codereview.chromium.org/390233003/diff/1/base/prefs/pref_value_map.h
File base/prefs/pref_value_map.h (right):

https://codereview.chromium.org/390233003/diff/1/base/prefs/pref_value_map.h#...
base/prefs/pref_value_map.h:75: void SetDouble(const std::string& key, const
double value);
Can you add a GetDouble as well so that we have full symmetry?

https://codereview.chromium.org/390233003/diff/1/base/prefs/pref_value_map_un...
File base/prefs/pref_value_map_unittest.cc (right):

https://codereview.chromium.org/390233003/diff/1/base/prefs/pref_value_map_un...
base/prefs/pref_value_map_unittest.cc:48: EXPECT_EQ(5.5, double_value);
EXPECT_DOUBLE_EQ?

https://codereview.chromium.org/390233003/diff/1/components/policy/core/commo...
File components/policy/core/common/registry_dict_win.cc (right):

https://codereview.chromium.org/390233003/diff/1/components/policy/core/commo...
components/policy/core/common/registry_dict_win.cc:92: new
base::FundamentalValue(int_value));
Do you want to cast to double here? Otherwise you change the type of the result.
I did not check whether this has an impact anywhere downstream.

[bartfab (slow), 2014-07-16 09:07:55]

policy LGTM

https://codereview.chromium.org/390233003/diff/1/components/policy/core/commo...
File components/policy/core/common/registry_dict_win.cc (right):

https://codereview.chromium.org/390233003/diff/1/components/policy/core/commo...
components/policy/core/common/registry_dict_win.cc:92: new
base::FundamentalValue(int_value));
On 2014/07/16 07:37:12, battre wrote:
> Do you want to cast to double here? Otherwise you change the type of the
result.
> I did not check whether this has an impact anywhere downstream.

Yes, you definitely want to cast. Otherwise, the policy will have an incorrect
type and there will be a CHECK or DCHECK failure later on.

[battre, 2014-07-16 09:53:21]

https://codereview.chromium.org/390233003/diff/1/components/policy/core/commo...
File components/policy/core/common/registry_dict_win.cc (right):

https://codereview.chromium.org/390233003/diff/1/components/policy/core/commo...
components/policy/core/common/registry_dict_win.cc:92: new
base::FundamentalValue(int_value));
On 2014/07/16 09:07:55, bartfab wrote:
> On 2014/07/16 07:37:12, battre wrote:
> > Do you want to cast to double here? Otherwise you change the type of the
> result.
> > I did not check whether this has an impact anywhere downstream.
> 
> Yes, you definitely want to cast. Otherwise, the policy will have an incorrect
> type and there will be a CHECK or DCHECK failure later on.

Maybe that is an indication that going for an overloaded constructor is a bad
move.

[Evan Stade, 2014-07-16 22:15:25]

https://codereview.chromium.org/390233003/diff/1/base/prefs/pref_value_map.h
File base/prefs/pref_value_map.h (right):

https://codereview.chromium.org/390233003/diff/1/base/prefs/pref_value_map.h#...
base/prefs/pref_value_map.h:75: void SetDouble(const std::string& key, const
double value);
On 2014/07/16 07:37:12, battre wrote:
> Can you add a GetDouble as well so that we have full symmetry?

I don't think it's a good idea to add dead code.

https://codereview.chromium.org/390233003/diff/1/base/prefs/pref_value_map_un...
File base/prefs/pref_value_map_unittest.cc (right):

https://codereview.chromium.org/390233003/diff/1/base/prefs/pref_value_map_un...
base/prefs/pref_value_map_unittest.cc:48: EXPECT_EQ(5.5, double_value);
On 2014/07/16 07:37:12, battre wrote:
> EXPECT_DOUBLE_EQ?

Done.

https://codereview.chromium.org/390233003/diff/1/components/policy/core/commo...
File components/policy/core/common/registry_dict_win.cc (right):

https://codereview.chromium.org/390233003/diff/1/components/policy/core/commo...
components/policy/core/common/registry_dict_win.cc:92: new
base::FundamentalValue(int_value));
On 2014/07/16 09:53:20, battre wrote:
> On 2014/07/16 09:07:55, bartfab wrote:
> > On 2014/07/16 07:37:12, battre wrote:
> > > Do you want to cast to double here? Otherwise you change the type of the
> > result.
> > > I did not check whether this has an impact anywhere downstream.
> > 
> > Yes, you definitely want to cast. Otherwise, the policy will have an
incorrect
> > type and there will be a CHECK or DCHECK failure later on.
> 
> Maybe that is an indication that going for an overloaded constructor is a bad
> move.

Equally, I think it's an indication that this code should not have been relying
on an implicit cast. Luckily, several tests caught this as well. Going forward,
I don't think the overloaded constructor is more likely to cause bugs (although
I will grant it is a potential source of regressions in the switch over).

[battre, 2014-07-17 15:58:39]

lgtm

https://codereview.chromium.org/390233003/diff/1/base/prefs/pref_value_map.h
File base/prefs/pref_value_map.h (right):

https://codereview.chromium.org/390233003/diff/1/base/prefs/pref_value_map.h#...
base/prefs/pref_value_map.h:75: void SetDouble(const std::string& key, const
double value);
On 2014/07/16 22:15:25, Evan Stade wrote:
> On 2014/07/16 07:37:12, battre wrote:
> > Can you add a GetDouble as well so that we have full symmetry?
> 
> I don't think it's a good idea to add dead code.

I'll leave it up to you but feel that you are pushing the burden to get another
owner's approval to the next person who will wonder why everything has a getter
and setter except for double.

[Evan Stade, 2014-07-17 16:07:06]

https://codereview.chromium.org/390233003/diff/1/base/prefs/pref_value_map.h
File base/prefs/pref_value_map.h (right):

https://codereview.chromium.org/390233003/diff/1/base/prefs/pref_value_map.h#...
base/prefs/pref_value_map.h:75: void SetDouble(const std::string& key, const
double value);
On 2014/07/17 15:58:39, battre wrote:
> On 2014/07/16 22:15:25, Evan Stade wrote:
> > On 2014/07/16 07:37:12, battre wrote:
> > > Can you add a GetDouble as well so that we have full symmetry?
> > 
> > I don't think it's a good idea to add dead code.
> 
> I'll leave it up to you but feel that you are pushing the burden to get
another
> owner's approval to the next person who will wonder why everything has a
getter
> and setter except for double.

Thanks for your review. Hopefully such a small change will be quick and painless
for both the reviewer and reviewee.

[Evan Stade, 2014-07-17 16:07:09]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-07-17 16:09:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/390233003/40001

[commit-bot: I haz the power, 2014-07-17 16:22:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/29775)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/34187)

[commit-bot: I haz the power, 2014-07-17 16:24:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-17 16:24:52]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/29786)

[Evan Stade, 2014-07-19 01:21:31]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-07-19 01:22:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/390233003/60001

[commit-bot: I haz the power, 2014-07-19 04:40:35]

Change committed as 284304