Add builders for tracing event's structural arguments

Add builders for tracing event's structural arguments

The new classes allow building JSON-like structural arguments. Current implementation uses base::Value as backing store but that can be replaced in the future with something more efficient without changing client code.

All clients of cc/debug/traced_value.h should eventually switch to use the new builders.

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

R=alph@chromium.org, dsinclair@chromium.org, nduca@chromium.org, willchan@chromium.org

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

[yurys, 2014-07-09 15:01:16]

This code is actually ported from Blink:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[alph, 2014-07-09 15:03:53]

lgtm

https://codereview.chromium.org/380763002/diff/1/base/debug/trace_event_argum...
File base/debug/trace_event_argument_unittest.cc (right):

https://codereview.chromium.org/380763002/diff/1/base/debug/trace_event_argum...
base/debug/trace_event_argument_unittest.cc:31: .SetInteger("i1", 2014)
Maybe use paddings to express the structure.

[yurys, 2014-07-09 15:04:54]

https://codereview.chromium.org/380763002/diff/1/base/debug/trace_event_argum...
File base/debug/trace_event_argument_unittest.cc (right):

https://codereview.chromium.org/380763002/diff/1/base/debug/trace_event_argum...
base/debug/trace_event_argument_unittest.cc:31: .SetInteger("i1", 2014)
On 2014/07/09 15:03:53, alph wrote:
> Maybe use paddings to express the structure.

I would love to do so but 'git cl format' produced this output.

[yurys, 2014-07-10 07:23:51]

@nduca: could you review this?

[caseq, 2014-07-10 12:42:44]

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
File base/debug/trace_event_argument.h (right):

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
base/debug/trace_event_argument.h:166: static const size_t nestingLevel = 1;
nit: kNestingLevel?
Also, since it's never used as memory size or index, I guess int would be more
appropriate.

[yurys, 2014-07-10 12:50:50]

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
File base/debug/trace_event_argument.h (right):

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
base/debug/trace_event_argument.h:166: static const size_t nestingLevel = 1;
On 2014/07/10 12:42:44, caseq wrote:
> nit: kNestingLevel?
> Also, since it's never used as memory size or index, I guess int would be more
> appropriate.
std::vector::size will return size_t and we compare it to nestingLevel, so the
type is size_t to avoid static_cast<size_t>.

[dsinclair, 2014-07-10 13:32:14]

What do we template the array and dictionary since, as far as I can see they're
both always TracedValue. If we want to swap the storage out later, why not just
do it behind the scenes?

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
File base/debug/trace_event_argument.cc (right):

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
base/debug/trace_event_argument.cc:30: }
Nit: blank line before }
nit: }  // namespace

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
File base/debug/trace_event_argument.h (right):

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
base/debug/trace_event_argument.h:43: void EndCurrentArray();
nit: Move this up with EndCurrentDictionary() above.

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
File base/debug/trace_event_argument_unittest.cc (right):

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
base/debug/trace_event_argument_unittest.cc:13: TEST_F(TraceEventArgumentTest,
FlatDitctionary) {
nit: s/FlatDitctionary/FlatDictionary

[yurys, 2014-07-10 15:06:29]

On 2014/07/10 13:32:14, dsinclair wrote:
> What do we template the array and dictionary since, as far as I can see
they're
> both always TracedValue. If we want to swap the storage out later, why not
just
> do it behind the scenes?
> 
I'm not sure I understand the question, can you elaborate? All actual work is
happening in non-template TracedValueBase which can be changed to use different
storage without touching client code.

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
File base/debug/trace_event_argument.cc (right):

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
base/debug/trace_event_argument.cc:30: }
On 2014/07/10 13:32:14, dsinclair wrote:
> Nit: blank line before }
> nit: }  // namespace

Done.

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
File base/debug/trace_event_argument.h (right):

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
base/debug/trace_event_argument.h:43: void EndCurrentArray();
On 2014/07/10 13:32:14, dsinclair wrote:
> nit: Move this up with EndCurrentDictionary() above.

Done.

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
File base/debug/trace_event_argument_unittest.cc (right):

https://codereview.chromium.org/380763002/diff/40001/base/debug/trace_event_a...
base/debug/trace_event_argument_unittest.cc:13: TEST_F(TraceEventArgumentTest,
FlatDitctionary) {
On 2014/07/10 13:32:14, dsinclair wrote:
> nit: s/FlatDitctionary/FlatDictionary

Done.

[dsinclair, 2014-07-10 15:17:28]

On 2014/07/10 15:06:29, yurys wrote:
> On 2014/07/10 13:32:14, dsinclair wrote:
> > What do we template the array and dictionary since, as far as I can see
> they're
> > both always TracedValue. If we want to swap the storage out later, why not
> just
> > do it behind the scenes?
> > 
> I'm not sure I understand the question, can you elaborate? All actual work is
> happening in non-template TracedValueBase which can be changed to use
different
> storage without touching client code.
> 


So, my question is, why are TracedArray and TracedDictionary templated? When you
do BeginDictionary, why does it return:

TracedDictionary<TracedArray<OwnerType> >& BeginDictionary()

instead of just TracedDictionary& BeginDictionary? Why do we need the
TracedArray<OwnerType> template specified to the dictionary? Is it just for the
ability to chain method calls after the EndDictionary call?

[yurys, 2014-07-10 15:24:28]

On 2014/07/10 15:17:28, dsinclair wrote:
> On 2014/07/10 15:06:29, yurys wrote:
> > On 2014/07/10 13:32:14, dsinclair wrote:
> > > What do we template the array and dictionary since, as far as I can see
> > they're
> > > both always TracedValue. If we want to swap the storage out later, why not
> > just
> > > do it behind the scenes?
> > > 
> > I'm not sure I understand the question, can you elaborate? All actual work
is
> > happening in non-template TracedValueBase which can be changed to use
> different
> > storage without touching client code.
> > 
> 
> 
> So, my question is, why are TracedArray and TracedDictionary templated? When
you
> do BeginDictionary, why does it return:
> 
> TracedDictionary<TracedArray<OwnerType> >& BeginDictionary()
> 
> instead of just TracedDictionary& BeginDictionary? Why do we need the
> TracedArray<OwnerType> template specified to the dictionary? Is it just for
the
> ability to chain method calls after the EndDictionary call?

Knowing OwnerType allows us to control type-safety after closing a container
when using chained calls. E.g. following code will be a compile-time error:
json.beginArray("MyArray")
    .beginDictionary()
    .endDictionary()
    .setInteger("foo", 1)

We are discussing whether it makes sense to preserve the template version or
come up with something simpler in Blink, see
https://codereview.chromium.org/381653002/. Let's agree on the approach there
and I will replicated it in this change.

[yurys, 2014-07-11 15:34:28]

Updated patch, ptal.

[yurys, 2014-07-14 12:04:50]

@dsinclair: please do OWNERS review.

[dsinclair, 2014-07-14 13:42:19]

lgtm

[yurys, 2014-07-14 14:35:40]

willchan@chromium.org: Please review changes in 
base/BUILD.gn
base/base.gyp
base/base.gypi

[willchan no longer on Chromium, 2014-07-14 15:21:46]

I just skimmed it, but it looks like there are still some webkitisms in the
code.

https://codereview.chromium.org/380763002/diff/80001/base/debug/trace_event_a...
File base/debug/trace_event_argument.h (right):

https://codereview.chromium.org/380763002/diff/80001/base/debug/trace_event_a...
base/debug/trace_event_argument.h:50: std::vector<Value*> m_stack;
Should be private and named stack_ instead of m_stack.

https://codereview.chromium.org/380763002/diff/80001/base/debug/trace_event_a...
base/debug/trace_event_argument.h:138: static const size_t nestingLevel =
OwnerType::nestingLevel + 1;
kNestingLevel

https://codereview.chromium.org/380763002/diff/80001/base/debug/trace_event_a...
base/debug/trace_event_argument.h:262:
scoped_refptr<base::debug::ConvertableToTraceFormat> finish();
Finish()

[yurys, 2014-07-14 16:16:30]

https://codereview.chromium.org/380763002/diff/80001/base/debug/trace_event_a...
File base/debug/trace_event_argument.h (right):

https://codereview.chromium.org/380763002/diff/80001/base/debug/trace_event_a...
base/debug/trace_event_argument.h:50: std::vector<Value*> m_stack;
On 2014/07/14 15:21:46, willchan wrote:
> Should be private
It is used in the descendants. I can make it private and add a getter but I
don't think it makes much sense.

> and named stack_ instead of m_stack.
Done.

https://codereview.chromium.org/380763002/diff/80001/base/debug/trace_event_a...
base/debug/trace_event_argument.h:138: static const size_t nestingLevel =
OwnerType::nestingLevel + 1;
On 2014/07/14 15:21:46, willchan wrote:
> kNestingLevel

Done.

https://codereview.chromium.org/380763002/diff/80001/base/debug/trace_event_a...
base/debug/trace_event_argument.h:262:
scoped_refptr<base::debug::ConvertableToTraceFormat> finish();
On 2014/07/14 15:21:46, willchan wrote:
> Finish()

Done.

[willchan no longer on Chromium, 2014-07-14 16:49:22]

https://codereview.chromium.org/380763002/diff/80001/base/debug/trace_event_a...
File base/debug/trace_event_argument.h (right):

https://codereview.chromium.org/380763002/diff/80001/base/debug/trace_event_a...
base/debug/trace_event_argument.h:50: std::vector<Value*> m_stack;
On 2014/07/14 16:16:30, yurys wrote:
> On 2014/07/14 15:21:46, willchan wrote:
> > Should be private
> It is used in the descendants. I can make it private and add a getter but I
> don't think it makes much sense.

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
says:

"""
Make data members private, and provide access to them through accessor functions
as needed (for technical reasons, we allow data members of a test fixture class
to be protected when using gUnit). Typically a variable would be called foo_ and
the accessor function foo(). You may also want a mutator function set_foo().
"""
> 
> > and named stack_ instead of m_stack.
> Done.

[willchan no longer on Chromium, 2014-07-14 16:50:40]

https://codereview.chromium.org/380763002/diff/80001/base/debug/trace_event_a...
File base/debug/trace_event_argument.h (right):

https://codereview.chromium.org/380763002/diff/80001/base/debug/trace_event_a...
base/debug/trace_event_argument.h:50: std::vector<Value*> m_stack;
On 2014/07/14 16:49:22, willchan wrote:
> On 2014/07/14 16:16:30, yurys wrote:
> > On 2014/07/14 15:21:46, willchan wrote:
> > > Should be private
> > It is used in the descendants. I can make it private and add a getter but I
> > don't think it makes much sense.
> 
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Oops, meant to use the public link :)

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Access_Control

> says:
> 
> """
> Make data members private, and provide access to them through accessor
functions
> as needed (for technical reasons, we allow data members of a test fixture
class
> to be protected when using gUnit). Typically a variable would be called foo_
and
> the accessor function foo(). You may also want a mutator function set_foo().
> """
> > 
> > > and named stack_ instead of m_stack.
> > Done.
>

[nduca, 2014-07-15 02:09:40]

you're not moving cc's traced value over to this and instead leaving that as
debt against someone unspecified. you? someone else? you're missing pieces from
cc's traced value for object tracing... i think you should move cc/ over to this
to show that it still works.

[yurys, 2014-07-15 09:11:26]

On 2014/07/14 16:50:40, willchan wrote:
>
https://codereview.chromium.org/380763002/diff/80001/base/debug/trace_event_a...
> File base/debug/trace_event_argument.h (right):
> 
>
https://codereview.chromium.org/380763002/diff/80001/base/debug/trace_event_a...
> base/debug/trace_event_argument.h:50: std::vector<Value*> m_stack;
> On 2014/07/14 16:49:22, willchan wrote:
> > On 2014/07/14 16:16:30, yurys wrote:
> > > On 2014/07/14 15:21:46, willchan wrote:
> > > > Should be private
> > > It is used in the descendants. I can make it private and add a getter but
I
> > > don't think it makes much sense.
> > 
> >
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
> 
> Oops, meant to use the public link :)
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Access_Control
> 
Done. Thanks for the link!

[yurys, 2014-07-15 15:38:03]

On 2014/07/15 02:09:40, nduca wrote:
> you're not moving cc's traced value over to this and instead leaving that as
> debt against someone unspecified. you? someone else? you're missing pieces
from
> cc's traced value for object tracing... i think you should move cc/ over to
this
> to show that it still works.

I thought it would make sense to do this in a separate CL due to a number of
files it would affect. Now that I started doing it as part of this change I feel
like the client code may be too verbose.


There are a few options to replace the following code under cc/:
"
cc/layers/layer_impl.cc:
...
void LayerImpl::AsValueInto(base::DictionaryValue* state) const {
...
state->Set("position", MathUtil::AsValue(position_).release());
...

cc/base/math_util.h:
scoped_ptr<base::Value> MathUtil::AsValue(const gfx::Size& s) {
  scoped_ptr<base::DictionaryValue> res(new base::DictionaryValue());
  res->SetDouble("width", s.width());
  res->SetDouble("height", s.height());
  return res.PassAs<base::Value>();
}

"

1) This is quite verbose but ensures that MathUtil::AsTracedValue will add
properties to a dictionary and can't treat it as an array.

void LayerImpl::AsValueInto(base::debug::TracedDictionaryBase* state) const {
...
  TracedDictionaryBase& d =
      state->BeginDictionaryNamed("position");
  MathUtil::AsTracedValue(position_, &d);
  d.EndDictionary();
...
void MathUtil::AsTracedValue(const gfx::Size& s,
                             base::debug::TracedDictionaryBase* res) {
  res->SetDouble("width", s.width());
  res->SetDouble("height", s.height());
}

2)
...
   MathUtil::AsTracedValue(state->BeginDictionaryNamed("position"), position_)
      ->EndDictionary();
....
base::debug::TracedDictionaryBase& MathUtil::AsTracedValue(
    base::debug::TracedDictionaryBase& res,
    const gfx::Size& s) {
  res->SetDouble("width", s.width());
  res->SetDouble("height", s.height());
  return res;
}

3) The call site is quite clean but in this case we have to pass dictionary name
into AsTracedValue and the same routine cannot be reused for serializing
gfx::Size into an object contained in an array.
...
  MathUtil::AsTracedValue(position_, state, "position");
...
void MathUtil::AsTracedValue(const gfx::Size& s,
                             base::debug::TracedDictionaryBase* dict,
                             const char* name) {
  dict->BeginDictionaryNamed(name)
      .SetDouble("width", s.width())
      .SetDouble("height", s.height())
      .EndDictionary();
}

4) With this approach we have addToDictionary overloaded for each type that
needs to be serialized into a traced value. The call site looks good but the
implementation may seem overcomplicated.
...
 
state->BeginDictionaryNamed("position").addToDictionary(position_).EndDictionary();
...

  TracedDictionaryBase& addToDictionary(TracedDictionaryBase* dict, const
gfx::Size& s) {
    dict->SetDouble("width", s.width());
    dict->SetDouble("height", s.height());
    return *dict;
  }

...
class BASE_EXPORT TracedDictionaryBase : public TracedValueBase {
...
  template <typename T>
  SelfType& addToDictionary(T t) {
    addToDictionary(this, t);
    return *this;
  }
...


5) As an extreme alternative we could drop all templated builders and make all
methods on TracedValueBase public. All call sites would be as clean and concise
as in the current implementation. Compiler wouldn't be able to catch the case
when e.g. SetInteger("foo", 10) is called while writing into an array but in
debug mode an assert would fail.
...
  state->BeginDictionaryNamed("position");
  MathUtil::AsTracedValue(position_, state);
  state->EndDictionary();
...
Personally I lean towards 1) or 5) options. What do you think about this?


Another problem is that LayerImpl::AsValueInto parses JSON string into a
base::Value and then adds it into the trace event argument[1]. That works fine
when base::Value is used for building trace event arguments but with the new
typed builder interface we would have to traverse base::Value recursively and
just to duplicate its structure using the new classes. Is that fine with you or
should we add SetValue/PushValue instead?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer_im...

[dsinclair, 2014-07-15 16:07:54]

On 2014/07/15 at 15:38:03, yurys wrote:
> On 2014/07/15 02:09:40, nduca wrote:
> > you're not moving cc's traced value over to this and instead leaving that as
> > debt against someone unspecified. you? someone else? you're missing pieces
from
> > cc's traced value for object tracing... i think you should move cc/ over to
this
> > to show that it still works.
> 
> I thought it would make sense to do this in a separate CL due to a number of
files it would affect. Now that I started doing it as part of this change I feel
like the client code may be too verbose.
> 
> 
> There are a few options to replace the following code under cc/:
> "
> cc/layers/layer_impl.cc:
> ...
> void LayerImpl::AsValueInto(base::DictionaryValue* state) const {
> ...
> state->Set("position", MathUtil::AsValue(position_).release());
> ...
> 
> cc/base/math_util.h:
> scoped_ptr<base::Value> MathUtil::AsValue(const gfx::Size& s) {
>   scoped_ptr<base::DictionaryValue> res(new base::DictionaryValue());
>   res->SetDouble("width", s.width());
>   res->SetDouble("height", s.height());
>   return res.PassAs<base::Value>();
> }
> 
> "
> 
> 1) This is quite verbose but ensures that MathUtil::AsTracedValue will add
properties to a dictionary and can't treat it as an array.
> 
> void LayerImpl::AsValueInto(base::debug::TracedDictionaryBase* state) const {
> ...
>   TracedDictionaryBase& d =
>       state->BeginDictionaryNamed("position");
>   MathUtil::AsTracedValue(position_, &d);
>   d.EndDictionary();
> ...
> void MathUtil::AsTracedValue(const gfx::Size& s,
>                              base::debug::TracedDictionaryBase* res) {
>   res->SetDouble("width", s.width());
>   res->SetDouble("height", s.height());
> }
> 
> 2)
> ...
>    MathUtil::AsTracedValue(state->BeginDictionaryNamed("position"), position_)
>       ->EndDictionary();
> ....
> base::debug::TracedDictionaryBase& MathUtil::AsTracedValue(
>     base::debug::TracedDictionaryBase& res,
>     const gfx::Size& s) {
>   res->SetDouble("width", s.width());
>   res->SetDouble("height", s.height());
>   return res;
> }
> 
> 3) The call site is quite clean but in this case we have to pass dictionary
name into AsTracedValue and the same routine cannot be reused for serializing
gfx::Size into an object contained in an array.
> ...
>   MathUtil::AsTracedValue(position_, state, "position");
> ...
> void MathUtil::AsTracedValue(const gfx::Size& s,
>                              base::debug::TracedDictionaryBase* dict,
>                              const char* name) {
>   dict->BeginDictionaryNamed(name)
>       .SetDouble("width", s.width())
>       .SetDouble("height", s.height())
>       .EndDictionary();
> }
> 
> 4) With this approach we have addToDictionary overloaded for each type that
needs to be serialized into a traced value. The call site looks good but the
implementation may seem overcomplicated.
> ...
>  
state->BeginDictionaryNamed("position").addToDictionary(position_).EndDictionary();
> ...
> 
>   TracedDictionaryBase& addToDictionary(TracedDictionaryBase* dict, const
gfx::Size& s) {
>     dict->SetDouble("width", s.width());
>     dict->SetDouble("height", s.height());
>     return *dict;
>   }
> 
> ...
> class BASE_EXPORT TracedDictionaryBase : public TracedValueBase {
> ...
>   template <typename T>
>   SelfType& addToDictionary(T t) {
>     addToDictionary(this, t);
>     return *this;
>   }
> ...
> 
> 
> 5) As an extreme alternative we could drop all templated builders and make all
methods on TracedValueBase public. All call sites would be as clean and concise
as in the current implementation. Compiler wouldn't be able to catch the case
when e.g. SetInteger("foo", 10) is called while writing into an array but in
debug mode an assert would fail.
> ...
>   state->BeginDictionaryNamed("position");
>   MathUtil::AsTracedValue(position_, state);
>   state->EndDictionary();
> ...
> Personally I lean towards 1) or 5) options. What do you think about this?
> 
> 
> Another problem is that LayerImpl::AsValueInto parses JSON string into a
base::Value and then adds it into the trace event argument[1]. That works fine
when base::Value is used for building trace event arguments but with the new
typed builder interface we would have to traverse base::Value recursively and
just to duplicate its structure using the new classes. Is that fine with you or
should we add SetValue/PushValue instead?
> 
> [1]
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer_im...



My preference would be to do 5 over 1. The code seems cleaner to me and we still
have the ASSERTs in debug mode which should cover us I believe.

[alph, 2014-07-16 14:20:17]

On 2014/07/15 15:38:03, yurys wrote:
> On 2014/07/15 02:09:40, nduca wrote:
> > you're not moving cc's traced value over to this and instead leaving that as
> > debt against someone unspecified. you? someone else? you're missing pieces
> from
> > cc's traced value for object tracing... i think you should move cc/ over to
> this
> > to show that it still works.
> 
> I thought it would make sense to do this in a separate CL due to a number of
> files it would affect. Now that I started doing it as part of this change I
feel
> like the client code may be too verbose.
> 
> 
> There are a few options to replace the following code under cc/:
> "
> cc/layers/layer_impl.cc:
> ...
> void LayerImpl::AsValueInto(base::DictionaryValue* state) const {
> ...
> state->Set("position", MathUtil::AsValue(position_).release());
> ...
> 
> cc/base/math_util.h:
> scoped_ptr<base::Value> MathUtil::AsValue(const gfx::Size& s) {
>   scoped_ptr<base::DictionaryValue> res(new base::DictionaryValue());
>   res->SetDouble("width", s.width());
>   res->SetDouble("height", s.height());
>   return res.PassAs<base::Value>();
> }
> 
> "
> 
> 1) This is quite verbose but ensures that MathUtil::AsTracedValue will add
> properties to a dictionary and can't treat it as an array.
> 
> void LayerImpl::AsValueInto(base::debug::TracedDictionaryBase* state) const {
> ...
>   TracedDictionaryBase& d =
>       state->BeginDictionaryNamed("position");
>   MathUtil::AsTracedValue(position_, &d);
>   d.EndDictionary();
> ...
> void MathUtil::AsTracedValue(const gfx::Size& s,
>                              base::debug::TracedDictionaryBase* res) {
>   res->SetDouble("width", s.width());
>   res->SetDouble("height", s.height());
> }
> 
> 2)
> ...
>    MathUtil::AsTracedValue(state->BeginDictionaryNamed("position"), position_)
>       ->EndDictionary();
> ....
> base::debug::TracedDictionaryBase& MathUtil::AsTracedValue(
>     base::debug::TracedDictionaryBase& res,
>     const gfx::Size& s) {
>   res->SetDouble("width", s.width());
>   res->SetDouble("height", s.height());
>   return res;
> }
> 
> 3) The call site is quite clean but in this case we have to pass dictionary
name
> into AsTracedValue and the same routine cannot be reused for serializing
> gfx::Size into an object contained in an array.
> ...
>   MathUtil::AsTracedValue(position_, state, "position");
> ...
> void MathUtil::AsTracedValue(const gfx::Size& s,
>                              base::debug::TracedDictionaryBase* dict,
>                              const char* name) {
>   dict->BeginDictionaryNamed(name)
>       .SetDouble("width", s.width())
>       .SetDouble("height", s.height())
>       .EndDictionary();
> }
> 
> 4) With this approach we have addToDictionary overloaded for each type that
> needs to be serialized into a traced value. The call site looks good but the
> implementation may seem overcomplicated.
> ...
>  
>
state->BeginDictionaryNamed("position").addToDictionary(position_).EndDictionary();
> ...
> 
>   TracedDictionaryBase& addToDictionary(TracedDictionaryBase* dict, const
> gfx::Size& s) {
>     dict->SetDouble("width", s.width());
>     dict->SetDouble("height", s.height());
>     return *dict;
>   }
> 
> ...
> class BASE_EXPORT TracedDictionaryBase : public TracedValueBase {
> ...
>   template <typename T>
>   SelfType& addToDictionary(T t) {
>     addToDictionary(this, t);
>     return *this;
>   }
> ...
> 
> 
> 5) As an extreme alternative we could drop all templated builders and make all
> methods on TracedValueBase public. All call sites would be as clean and
concise
> as in the current implementation. Compiler wouldn't be able to catch the case
> when e.g. SetInteger("foo", 10) is called while writing into an array but in
> debug mode an assert would fail.
> ...
>   state->BeginDictionaryNamed("position");
>   MathUtil::AsTracedValue(position_, state);
>   state->EndDictionary();
> ...
> Personally I lean towards 1) or 5) options. What do you think about this?
> 
> 
> Another problem is that LayerImpl::AsValueInto parses JSON string into a
> base::Value and then adds it into the trace event argument[1]. That works fine
> when base::Value is used for building trace event arguments but with the new
> typed builder interface we would have to traverse base::Value recursively and
> just to duplicate its structure using the new classes. Is that fine with you
or
> should we add SetValue/PushValue instead?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer_im...

6)
I can suggest a slightly modified 1) that reduces the number of word
"Dictionary":

TracedDictionary d(parent, "position");
MathUtil::AsTracedValue(position_, &d);
d.End();

[yurys, 2014-07-16 14:24:17]

On 2014/07/16 14:20:17, alph wrote:
> 6)
> I can suggest a slightly modified 1) that reduces the number of word
> "Dictionary":
> 
> TracedDictionary d(parent, "position");
> MathUtil::AsTracedValue(position_, &d);
> d.End();

This still requires introducing a variable for each dictionary and doesn't catch
the following error:

TracedArray a(parent, "position");
  TracedDictionary d(parent, "position");
  MathUtil::AsTracedValue(position_, &d);
  a.AppendInteger(1);
  d.End();
parent.End();

So I'd rather proceed with the simplest solution (5).

[yurys, 2014-07-16 14:25:56]

@nduca: are you OK with option (5)? If so I will update cc/ code to use that
interface instead of base::Value.

[yurys, 2014-07-25 14:54:40]

On 2014/07/15 02:09:40, nduca wrote:
> you're not moving cc's traced value over to this and instead leaving that as
> debt against someone unspecified. you? someone else? you're missing pieces
from
> cc's traced value for object tracing... i think you should move cc/ over to
this
> to show that it still works.

@nduca: I updated most of the tracing code under cc/ to use the new builder
instead of cc::debug::TracedValue. As the next step I'm going to remove
cc/debug/traced_value.*. Please review.

[nduca, 2014-07-30 07:52:33]

im very very sorry for the delayed review. lgtm.

https://codereview.chromium.org/380763002/diff/240001/cc/base/math_util.h
File cc/base/math_util.h (left):

https://codereview.chromium.org/380763002/diff/240001/cc/base/math_util.h#old...
cc/base/math_util.h:186: static scoped_ptr<base::Value> AsValue(const gfx::BoxF&
box);
do we need these now that we have AddToTracedValue?

[yurys, 2014-07-30 09:44:58]

On 2014/07/30 07:52:33, nduca wrote:
> im very very sorry for the delayed review. lgtm.
> 
Thanks for review!

https://codereview.chromium.org/380763002/diff/240001/cc/base/math_util.h
File cc/base/math_util.h (left):

https://codereview.chromium.org/380763002/diff/240001/cc/base/math_util.h#old...
cc/base/math_util.h:186: static scoped_ptr<base::Value> AsValue(const gfx::BoxF&
box);
On 2014/07/30 07:52:32, nduca wrote:
> do we need these now that we have AddToTracedValue?

Hopefully not. I will remove all unused utility functions and TracedValue from
cc/debug/traced_value.h

[yurys, 2014-07-30 14:41:17]

https://codereview.chromium.org/380763002/diff/240001/cc/base/math_util.h
File cc/base/math_util.h (left):

https://codereview.chromium.org/380763002/diff/240001/cc/base/math_util.h#old...
cc/base/math_util.h:186: static scoped_ptr<base::Value> AsValue(const gfx::BoxF&
box);
On 2014/07/30 07:52:32, nduca wrote:
> do we need these now that we have AddToTracedValue?

Some of them need to stay as they are used in LayerImpl::LayerTreeAsJson and its
overrides. I removed the rest.

[yurys, 2014-07-30 14:42:11]

@willchan: Please review changes in 
base/BUILD.gn
base/base.gyp
base/base.gypi

[willchan no longer on Chromium, 2014-07-30 15:56:17]

lgtm, only have nits

https://codereview.chromium.org/380763002/diff/280001/base/debug/trace_event_...
File base/debug/trace_event_argument.cc (right):

https://codereview.chromium.org/380763002/diff/280001/base/debug/trace_event_...
base/debug/trace_event_argument.cc:18: DCHECK(stack_.size() == 1);
You should use DCHECK_EQ() and DCHECK_GT() and those set of macros, so the log
message prints the bad value too.

https://codereview.chromium.org/380763002/diff/280001/base/debug/trace_event_...
File base/debug/trace_event_argument.h (right):

https://codereview.chromium.org/380763002/diff/280001/base/debug/trace_event_...
base/debug/trace_event_argument.h:22: class BASE_EXPORT TracedValue : public
base::debug::ConvertableToTraceFormat {
Nit: I think it's unnecessary to use base:: and base::debug in this file, since
everything's already in the namespace. If there's a reason to prefer this,
that's fine, but otherwise it seems like it should get removed.

https://codereview.chromium.org/380763002/diff/280001/base/debug/trace_event_...
File base/debug/trace_event_argument_unittest.cc (right):

https://codereview.chromium.org/380763002/diff/280001/base/debug/trace_event_...
base/debug/trace_event_argument_unittest.cc:13: TEST_F(TraceEventArgumentTest,
FlatDictionary) {
If the test fixture is just going to be empty, you don't need a test fixture.
Delete the empty class and just use the TEST macro instead of the TEST_F macro.

[yurys, 2014-07-31 12:06:10]

https://codereview.chromium.org/380763002/diff/280001/base/debug/trace_event_...
File base/debug/trace_event_argument.cc (right):

https://codereview.chromium.org/380763002/diff/280001/base/debug/trace_event_...
base/debug/trace_event_argument.cc:18: DCHECK(stack_.size() == 1);
On 2014/07/30 15:56:16, willchan wrote:
> You should use DCHECK_EQ() and DCHECK_GT() and those set of macros, so the log
> message prints the bad value too.

Done.

https://codereview.chromium.org/380763002/diff/280001/base/debug/trace_event_...
File base/debug/trace_event_argument.h (right):

https://codereview.chromium.org/380763002/diff/280001/base/debug/trace_event_...
base/debug/trace_event_argument.h:22: class BASE_EXPORT TracedValue : public
base::debug::ConvertableToTraceFormat {
On 2014/07/30 15:56:16, willchan wrote:
> Nit: I think it's unnecessary to use base:: and base::debug in this file,
since
> everything's already in the namespace. If there's a reason to prefer this,
> that's fine, but otherwise it seems like it should get removed.

Done. Overlooked that. Thanks.

https://codereview.chromium.org/380763002/diff/280001/base/debug/trace_event_...
File base/debug/trace_event_argument_unittest.cc (right):

https://codereview.chromium.org/380763002/diff/280001/base/debug/trace_event_...
base/debug/trace_event_argument_unittest.cc:13: TEST_F(TraceEventArgumentTest,
FlatDictionary) {
On 2014/07/30 15:56:16, willchan wrote:
> If the test fixture is just going to be empty, you don't need a test fixture.
> Delete the empty class and just use the TEST macro instead of the TEST_F
macro.

Done.

[yurys, 2014-07-31 12:06:23]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-07-31 12:07:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yurys@chromium.org/380763002/290001

[yurys, 2014-07-31 12:43:08]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-07-31 12:44:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yurys@chromium.org/380763002/310001

[commit-bot: I haz the power, 2014-07-31 16:06:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[commit-bot: I haz the power, 2014-07-31 17:59:37]

Change committed as 286849

[erikchen, 2014-07-31 18:52:46]

A revert of this CL has been created in
https://codereview.chromium.org/421183003/ by erikchen@chromium.org.

The reason for reverting is: linux ASAN errors.

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....

[yurys, 2014-08-01 09:35:42]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-08-01 09:38:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yurys@chromium.org/380763002/350001

[yurys, 2014-08-01 13:10:56]

Committed patchset #19 manually as 286984 (presubmit successful).