base: Add Is* functions to Value class.

base: Add Is* functions to Value class.

This will simplify a lot the code in the clients of the Values API.

This replaces all the uses of IsType(Value::TYPE_*);

The clients now are be able to say (in a nice way), for example:

value->IsNull();

Instead of:

value->IsType(Value::TYPE_NULL);

or

value->GetType() == Value::TYPE_NULL

BUG=None
TEST=None

R=evan@chromium.org,tony@chromium.org

[tfarina, 2011-08-19 14:36:46]

+tony for review.

Hi Tony, could you review this to me?

[Evan Martin, 2011-08-19 17:22:23]

On 2011/08/19 14:36:46, tfarina wrote:
> +tony for review.
> 
> Hi Tony, could you review this to me?

Hm, I swear I wrote a comment before but I don't see it on here!

There's a lot of code like:
if (!root_val->IsList())
  return false;
ListValue* root_list = static_cast<ListValue*>(root_val);


I think a better pattern is to provide a function like
virtual ListValue* AsList();
which returns NULL if the Value isn't a list.

That would allow users to do things like:

ListValue* root_list = root_val->AsList();
if (!root_list)
  return;

Or even:

if (ListValue* root_list = root_val->AsList()) {
  ... use root_list here ...
}

[tony, 2011-08-19 17:24:20]

I like this change-- seems to be easier to read than IsType().

http://codereview.chromium.org/7661009/diff/4012/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/command.cc (right):

http://codereview.chromium.org/7661009/diff/4012/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/command.cc:44: value->IsNull();
Nit: Maybe we can fit this on 2 lines?  Also, the indenting is the wrong amount
(should only be 4 spaces in from the previous line, so 6 spaces total).

http://codereview.chromium.org/7661009/diff/4012/remoting/host/plugin/policy_...
File remoting/host/plugin/policy_hack/nat_policy_linux.cc (right):

http://codereview.chromium.org/7661009/diff/4012/remoting/host/plugin/policy_...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:212:
scoped_ptr<base::DictionaryValue> new_policy(Load());
Why is this necessary here?  There are lots of other references to
DictionaryValue that you didn't change. I would leave this unchanged for now.

[tony, 2011-08-19 17:26:15]

Sorry, didn't see Evan's comment before I replied.  I agree with Evan, but it
could be done as a follow up patch since it seems like we still need the Is*()
functions in many places.

[Evan Martin, 2011-08-19 17:30:36]

AsFoo() returns non-zero only for Foos, so it can also be used in
place of IsFoo.

On Fri, Aug 19, 2011 at 10:26 AM,  <tony@chromium.org> wrote:
> Sorry, didn't see Evan's comment before I replied.  I agree with Evan, but
> it
> could be done as a follow up patch since it seems like we still need the
> Is*()
> functions in many places.
>
> http://codereview.chromium.org/7661009/
>

[tony, 2011-08-19 17:33:38]

Ah, I see.  You're right, we wouldn't need the Is*() methods.  Yeah, that sounds
better.

On 2011/08/19 17:30:36, Evan Martin wrote:
> AsFoo() returns non-zero only for Foos, so it can also be used in
> place of IsFoo.
> 
> On Fri, Aug 19, 2011 at 10:26 AM,  <mailto:tony@chromium.org> wrote:
> > Sorry, didn't see Evan's comment before I replied. &nbsp;I agree with Evan,
but
> > it
> > could be done as a follow up patch since it seems like we still need the
> > Is*()
> > functions in many places.
> >
> > http://codereview.chromium.org/7661009/
> >

[tfarina, 2011-08-19 18:40:47]

http://codereview.chromium.org/7661009/diff/4012/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/command.cc (right):

http://codereview.chromium.org/7661009/diff/4012/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/command.cc:44: value->IsNull();
On 2011/08/19 17:24:21, tony wrote:
> Nit: Maybe we can fit this on 2 lines?  Also, the indenting is the wrong
amount
> (should only be 4 spaces in from the previous line, so 6 spaces total).

Done.

http://codereview.chromium.org/7661009/diff/4012/remoting/host/plugin/policy_...
File remoting/host/plugin/policy_hack/nat_policy_linux.cc (right):

http://codereview.chromium.org/7661009/diff/4012/remoting/host/plugin/policy_...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:212:
scoped_ptr<base::DictionaryValue> new_policy(Load());
On 2011/08/19 17:24:21, tony wrote:
> Why is this necessary here?  There are lots of other references to
> DictionaryValue that you didn't change. I would leave this unchanged for now.

Done.

[tfarina, 2011-08-19 19:59:50]

On 2011/08/19 17:22:23, Evan Martin wrote:
> On 2011/08/19 14:36:46, tfarina wrote:
> > +tony for review.
> > 
> > Hi Tony, could you review this to me?
> 
> Hm, I swear I wrote a comment before but I don't see it on here!
> 
> There's a lot of code like:
> if (!root_val->IsList())
>   return false;
> ListValue* root_list = static_cast<ListValue*>(root_val);
> 
> 
> I think a better pattern is to provide a function like
> virtual ListValue* AsList();
> which returns NULL if the Value isn't a list.
> 
I can think in three reasons to not do this (at least not in this patch). As a
non-native speaker Is sounds better than As and it's easier to understand.
Second is that implementing As* functions will required much more code than the
Is* functions. And using As* throughout would be much more invasive and error
prone (given the big number of entries of IsType) to fix than converting
everything to Is*. Which is nice to read.

> That would allow users to do things like:
> 
> ListValue* root_list = root_val->AsList();
> if (!root_list)
>   return;
>
That pattern, at least, has the same number of lines as the Is version.
 
> Or even:
> 
> if (ListValue* root_list = root_val->AsList()) {
>   ... use root_list here ...
> }

That scares me, in some languages (like C#) that is not even allowed.

[tfarina, 2011-08-22 19:13:22]

Tony, Evan, can I go with the current route?

[tony, 2011-08-23 17:21:41]

I think we should do evan's recommendation (the As* functions).  If you are
worried about the change being too big, you can convert one type at a time
(separate patches for AsBool, AsString, etc).

[tfarina, 2011-08-23 17:45:36]

On 2011/08/23 17:21:41, tony wrote:
> I think we should do evan's recommendation (the As* functions).  If you are
> worried about the change being too big, you can convert one type at a time
> (separate patches for AsBool, AsString, etc).
Yeah, I'm worried about that. It's very big to do everything at once. I've
started the AsList() and pushed it here http://codereview.chromium.org/7714004/.