Add support for int64 and uint64 in values.h's API.

Add support for int64 and uint64 in values.h's API.

A lot of callers -- including PrefService and PrefRegistry -- have custom
code to handle (u)int64 inside DictionaryValue/ListValue.
Introduce support directly in values.h's API to avoid code duplication at
every call site.

This CL cleans an example call site in PrefHashFilter (since it's also under the prefs/ umbrella), but doesn't clean
up all other call sites (https://code.google.com/p/chromium/codesearch#search/&q=base::Int64ToString) those should be cleaned over time.

Motivated by a request @ https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_engines/default_search_manager.cc#newcode73

BUG=None

[gab, 2014-04-23 23:20:19]

Bernhard, PTAL.

Thanks!
Gab

[Bernhard Bauer, 2014-04-24 11:54:18]

I don't think this will fly. I'm not a base/ OWNER (I'm flattered that you
continuously think I am though :D ), so I'm not going to make the final decision
anyway, but AFAIU, base::Value is meant to only contain types that can be
represented in JSON (TYPE_BINARY notwithstanding, but that already creates its
own set of problems). It's not meant to be a generic value type.

We do have base/value_conversions.* to pack/unpack Values with non-JSON types.
That's probably where this code should live.

[gab, 2014-04-24 12:35:49]

On 2014/04/24 11:54:18, Bernhard Bauer wrote:
> I don't think this will fly. I'm not a base/ OWNER (I'm flattered that you
> continuously think I am though :D ), so I'm not going to make the final
decision anyways

Duh... I somehow always forget that values.h is outside of prefs haha (I mean, I
know it's file path is base/values.h, but I still think prefs when modifying
that API...)!
 
> but AFAIU, base::Value is meant to only contain types that can be
> represented in JSON (TYPE_BINARY notwithstanding, but that already creates its
> own set of problems). It's not meant to be a generic value type.
> 
> We do have base/value_conversions.* to pack/unpack Values with non-JSON types.
> That's probably where this code should live.

@Nico, WDYT? 

I'm essentially tired of code doing:

dict->GetString("foo", &foo_str);
base::StringToInt64(foo_str, &foo_int64);

(and similar two-line setters) all over the place... I'd like to abstract away
the fact that a string is used as the internal representation of int64 Values.

The cleanest way for the callers is for values.h to support int64s in its API
(such that Get/Set become one-liners for int64 as it is for all native types).
If this is not an option, then I guess at least hiding the int64<->string logic
in value_conversions.h is an option (although that will still make for a two
line call on the caller side).

Thanks!
Gab

[Bernhard Bauer, 2014-04-24 12:52:26]

On 2014/04/24 12:35:49, gab wrote:
> On 2014/04/24 11:54:18, Bernhard Bauer wrote:
> > I don't think this will fly. I'm not a base/ OWNER (I'm flattered that you
> > continuously think I am though :D ), so I'm not going to make the final
> decision anyways
> 
> Duh... I somehow always forget that values.h is outside of prefs haha (I mean,
I
> know it's file path is base/values.h, but I still think prefs when modifying
> that API...)!
>  
> > but AFAIU, base::Value is meant to only contain types that can be
> > represented in JSON (TYPE_BINARY notwithstanding, but that already creates
its
> > own set of problems). It's not meant to be a generic value type.
> > 
> > We do have base/value_conversions.* to pack/unpack Values with non-JSON
types.
> > That's probably where this code should live.
> 
> @Nico, WDYT? 
> 
> I'm essentially tired of code doing:
> 
> dict->GetString("foo", &foo_str);
> base::StringToInt64(foo_str, &foo_int64);
> 
> (and similar two-line setters) all over the place... I'd like to abstract away
> the fact that a string is used as the internal representation of int64 Values.
> 
> The cleanest way for the callers is for values.h to support int64s in its API
> (such that Get/Set become one-liners for int64 as it is for all native types).
> If this is not an option, then I guess at least hiding the int64<->string
logic
> in value_conversions.h is an option (although that will still make for a two
> line call on the caller side).

We could also add additional methods for getting int64's out of a
DictionaryValue or a ListValue to value_conversions.h. That's essentially the
same API surface that you're adding in this CL.

> Thanks!
> Gab

[gab, 2014-04-24 12:57:47]

On 2014/04/24 12:35:49, gab wrote:
> On 2014/04/24 11:54:18, Bernhard Bauer wrote:
> > I don't think this will fly. I'm not a base/ OWNER (I'm flattered that you
> > continuously think I am though :D ), so I'm not going to make the final
> decision anyways
> 
> Duh... I somehow always forget that values.h is outside of prefs haha (I mean,
I
> know it's file path is base/values.h, but I still think prefs when modifying
> that API...)!
>  
> > but AFAIU, base::Value is meant to only contain types that can be
> > represented in JSON (TYPE_BINARY notwithstanding, but that already creates
its
> > own set of problems). It's not meant to be a generic value type.
> > 
> > We do have base/value_conversions.* to pack/unpack Values with non-JSON
types.
> > That's probably where this code should live.
> 
> @Nico, WDYT? 
> 
> I'm essentially tired of code doing:
> 
> dict->GetString("foo", &foo_str);
> base::StringToInt64(foo_str, &foo_int64);
> 
> (and similar two-line setters) all over the place... I'd like to abstract away
> the fact that a string is used as the internal representation of int64 Values.
> 
> The cleanest way for the callers is for values.h to support int64s in its API
> (such that Get/Set become one-liners for int64 as it is for all native types).
> If this is not an option, then I guess at least hiding the int64<->string
logic
> in value_conversions.h is an option (although that will still make for a two
> line call on the caller side).

Actually, it's not even the duplicated two-liners at every call site that bug me
most. What bugs me most is the fact that PrefService supports int64s natively
(i.e. has GetInt64/SetInt64) while DictionaryValue doesn't.

This makes it such that if you get/set a registered int64 pref you can do it
directly, but if you are setting an int64 inside a registered dictionary, you
can't...

i.e.

pref_service->SetInt64("foo", 1234);

works,
but

pref_service->GetDictionary("bar", &dict);
dict->SetInt64("myint64", 1234);

doesn't... (while similar patterns work for Boolean/String/Integer/Double).


> 
> Thanks!
> Gab

[gab, 2014-04-24 13:00:06]

On 2014/04/24 12:52:26, Bernhard Bauer wrote:
> On 2014/04/24 12:35:49, gab wrote:
> > On 2014/04/24 11:54:18, Bernhard Bauer wrote:
> > > I don't think this will fly. I'm not a base/ OWNER (I'm flattered that you
> > > continuously think I am though :D ), so I'm not going to make the final
> > decision anyways
> > 
> > Duh... I somehow always forget that values.h is outside of prefs haha (I
mean,
> I
> > know it's file path is base/values.h, but I still think prefs when modifying
> > that API...)!
> >  
> > > but AFAIU, base::Value is meant to only contain types that can be
> > > represented in JSON (TYPE_BINARY notwithstanding, but that already creates
> its
> > > own set of problems). It's not meant to be a generic value type.
> > > 
> > > We do have base/value_conversions.* to pack/unpack Values with non-JSON
> types.
> > > That's probably where this code should live.
> > 
> > @Nico, WDYT? 
> > 
> > I'm essentially tired of code doing:
> > 
> > dict->GetString("foo", &foo_str);
> > base::StringToInt64(foo_str, &foo_int64);
> > 
> > (and similar two-line setters) all over the place... I'd like to abstract
away
> > the fact that a string is used as the internal representation of int64
Values.
> > 
> > The cleanest way for the callers is for values.h to support int64s in its
API
> > (such that Get/Set become one-liners for int64 as it is for all native
types).
> > If this is not an option, then I guess at least hiding the int64<->string
> logic
> > in value_conversions.h is an option (although that will still make for a two
> > line call on the caller side).
> 
> We could also add additional methods for getting int64's out of a
> DictionaryValue or a ListValue to value_conversions.h. That's essentially the
> same API surface that you're adding in this CL.

That could work too. The only downside to this instead of having it in values.h
is that developer have to know to look for these helpers (i.e., I didn't know
about them until today).

> 
> > Thanks!
> > Gab

[Bernhard Bauer, 2014-04-24 13:13:22]

On 2014/04/24 13:00:06, gab wrote:
> On 2014/04/24 12:52:26, Bernhard Bauer wrote:
> > On 2014/04/24 12:35:49, gab wrote:
> > > On 2014/04/24 11:54:18, Bernhard Bauer wrote:
> > > > I don't think this will fly. I'm not a base/ OWNER (I'm flattered that
you
> > > > continuously think I am though :D ), so I'm not going to make the final
> > > decision anyways
> > > 
> > > Duh... I somehow always forget that values.h is outside of prefs haha (I
> mean,
> > I
> > > know it's file path is base/values.h, but I still think prefs when
modifying
> > > that API...)!
> > >  
> > > > but AFAIU, base::Value is meant to only contain types that can be
> > > > represented in JSON (TYPE_BINARY notwithstanding, but that already
creates
> > its
> > > > own set of problems). It's not meant to be a generic value type.
> > > > 
> > > > We do have base/value_conversions.* to pack/unpack Values with non-JSON
> > types.
> > > > That's probably where this code should live.
> > > 
> > > @Nico, WDYT? 
> > > 
> > > I'm essentially tired of code doing:
> > > 
> > > dict->GetString("foo", &foo_str);
> > > base::StringToInt64(foo_str, &foo_int64);
> > > 
> > > (and similar two-line setters) all over the place... I'd like to abstract
> away
> > > the fact that a string is used as the internal representation of int64
> Values.
> > > 
> > > The cleanest way for the callers is for values.h to support int64s in its
> API
> > > (such that Get/Set become one-liners for int64 as it is for all native
> types).
> > > If this is not an option, then I guess at least hiding the int64<->string
> > logic
> > > in value_conversions.h is an option (although that will still make for a
two
> > > line call on the caller side).
> > 
> > We could also add additional methods for getting int64's out of a
> > DictionaryValue or a ListValue to value_conversions.h. That's essentially
the
> > same API surface that you're adding in this CL.
> 
> That could work too. The only downside to this instead of having it in
values.h
> is that developer have to know to look for these helpers (i.e., I didn't know
> about them until today).

There *is* a comment at the top of values.h pointing out what I said above...
O:-) We could probably add a reference to value_conversions.h there though.

> > 
> > > Thanks!
> > > Gab

[Nico, 2014-04-24 19:10:02]

(Sounds like you don't need anything from me here?

The usual answer to this proposal is "values is supposed to be like json
which is like javascript which requires 64bit numbers to be stored as ints
as its numbers are doubles")


On Thu, Apr 24, 2014 at 6:13 AM, <bauerb@chromium.org> wrote:

> On 2014/04/24 13:00:06, gab wrote:
>
>> On 2014/04/24 12:52:26, Bernhard Bauer wrote:
>> > On 2014/04/24 12:35:49, gab wrote:
>> > > On 2014/04/24 11:54:18, Bernhard Bauer wrote:
>> > > > I don't think this will fly. I'm not a base/ OWNER (I'm flattered
>> that
>>
> you
>
>> > > > continuously think I am though :D ), so I'm not going to make the
>> final
>> > > decision anyways
>> > >
>> > > Duh... I somehow always forget that values.h is outside of prefs haha
>> (I
>> mean,
>> > I
>> > > know it's file path is base/values.h, but I still think prefs when
>>
> modifying
>
>> > > that API...)!
>> > >
>> > > > but AFAIU, base::Value is meant to only contain types that can be
>> > > > represented in JSON (TYPE_BINARY notwithstanding, but that already
>>
> creates
>
>> > its
>> > > > own set of problems). It's not meant to be a generic value type.
>> > > >
>> > > > We do have base/value_conversions.* to pack/unpack Values with
>> non-JSON
>> > types.
>> > > > That's probably where this code should live.
>> > >
>> > > @Nico, WDYT?
>> > >
>> > > I'm essentially tired of code doing:
>> > >
>> > > dict->GetString("foo", &foo_str);
>> > > base::StringToInt64(foo_str, &foo_int64);
>> > >
>> > > (and similar two-line setters) all over the place... I'd like to
>> abstract
>> away
>> > > the fact that a string is used as the internal representation of int64
>> Values.
>> > >
>> > > The cleanest way for the callers is for values.h to support int64s in
>> its
>> API
>> > > (such that Get/Set become one-liners for int64 as it is for all native
>> types).
>> > > If this is not an option, then I guess at least hiding the
>> int64<->string
>> > logic
>> > > in value_conversions.h is an option (although that will still make
>> for a
>>
> two
>
>> > > line call on the caller side).
>> >
>> > We could also add additional methods for getting int64's out of a
>> > DictionaryValue or a ListValue to value_conversions.h. That's
>> essentially
>>
> the
>
>> > same API surface that you're adding in this CL.
>>
>
>  That could work too. The only downside to this instead of having it in
>>
> values.h
>
>> is that developer have to know to look for these helpers (i.e., I didn't
>> know
>> about them until today).
>>
>
> There *is* a comment at the top of values.h pointing out what I said
> above...
> O:-) We could probably add a reference to value_conversions.h there though.
>
>  >
>> > > Thanks!
>> > > Gab
>>
>
>
>
> https://codereview.chromium.org/254473002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gab, 2014-04-24 19:14:40]

On 2014/04/24 19:10:02, Nico wrote:
> (Sounds like you don't need anything from me here?
> 
> The usual answer to this proposal is "values is supposed to be like json
> which is like javascript which requires 64bit numbers to be stored as ints
> as its numbers are doubles")
> 

s/as ints/as strings ?

It's currently stored as strings everywhere.

My concern is mostly that it's possible to use PrefService::Get/SetInt64 for
registered int64 prefs, but there is no corresponding
DictionaryValue::Get/SetInt64 for int64 values inside registered dictionary
pref. This makes the API confusing as they're typically used in conjunction.

[Nico, 2014-04-24 19:16:19]

On Thu, Apr 24, 2014 at 12:14 PM, <gab@chromium.org> wrote:

> On 2014/04/24 19:10:02, Nico wrote:
>
>> (Sounds like you don't need anything from me here?
>>
>
>  The usual answer to this proposal is "values is supposed to be like json
>> which is like javascript which requires 64bit numbers to be stored as ints
>> as its numbers are doubles")
>>
>
>
> s/as ints/as strings ?
>

Yes, sorry.


>
> It's currently stored as strings everywhere.
>
> My concern is mostly that it's possible to use PrefService::Get/SetInt64
> for
> registered int64 prefs, but there is no corresponding
> DictionaryValue::Get/SetInt64 for int64 values inside registered dictionary
> pref. This makes the API confusing as they're typically used in
> conjunction.
>
> https://codereview.chromium.org/254473002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gab, 2014-04-24 19:20:43]

On 2014/04/24 19:16:19, Nico wrote:
> On Thu, Apr 24, 2014 at 12:14 PM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote:
> 
> > On 2014/04/24 19:10:02, Nico wrote:
> >
> >> (Sounds like you don't need anything from me here?
> >>
> >
> >  The usual answer to this proposal is "values is supposed to be like json
> >> which is like javascript which requires 64bit numbers to be stored as ints
> >> as its numbers are doubles")
> >>
> >
> >
> > s/as ints/as strings ?
> >
> 
> Yes, sorry.

Okay, so my proposal doesn't break this (i.e. int64 are still stored as strings
internally); it just adds direct support for it to avoid code duplication all
over the place for this.

> 
> 
> >
> > It's currently stored as strings everywhere.
> >
> > My concern is mostly that it's possible to use PrefService::Get/SetInt64
> > for
> > registered int64 prefs, but there is no corresponding
> > DictionaryValue::Get/SetInt64 for int64 values inside registered dictionary
> > pref. This makes the API confusing as they're typically used in
> > conjunction.
> >
> > https://codereview.chromium.org/254473002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....