[gin] Add ConvertableToTraceFormatWrapper

[gin] Add ConvertableToTraceFormatWrapper

Do the conversion between v8::ConvertableToTraceFormat to
base::trace_event::ConvertableToTraceFormat

BUG=406277
Committed: https://crrev.com/92d855a4974fde0acad282b929b531fa6fca1a92
Cr-Commit-Position: refs/heads/master@{#421485}

[alph, 2016-09-23 00:51:30]

alph@chromium.org changed reviewers:
+ caseq@chromium.org, fmeawad@chromium.org, jochen@chromium.org

[alph, 2016-09-23 00:51:31]

Please take a look at the gin part.

[caseq, 2016-09-23 23:50:58]

lgtm

[jochen (gone - plz use gerrit), 2016-09-26 15:04:37]

lgtm with comments

https://codereview.chromium.org/2368483002/diff/20001/gin/public/v8_platform.h
File gin/public/v8_platform.h (right):

https://codereview.chromium.org/2368483002/diff/20001/gin/public/v8_platform....
gin/public/v8_platform.h:47: std::unique_ptr<v8::ConvertableToTraceFormat>*,
please add an args name

https://codereview.chromium.org/2368483002/diff/20001/gin/v8_platform.cc
File gin/v8_platform.cc (right):

https://codereview.chromium.org/2368483002/diff/20001/gin/v8_platform.cc#newc...
gin/v8_platform.cc:94: explicit ConvertableToTraceFormatWrapper(
please add a virtual dtor

https://codereview.chromium.org/2368483002/diff/20001/gin/v8_platform.cc#newc...
gin/v8_platform.cc:103: };
disallow copy / assign

https://codereview.chromium.org/2368483002/diff/20001/gin/v8_platform.cc#newc...
gin/v8_platform.cc:129: DCHECK(num_args <= 2);
DCHECK_LT

[fmeawad, 2016-09-26 15:11:23]

lgtm

[lpy, 2016-09-26 20:29:07]

Description was changed from

==========
[gin] Add ConvertableToTraceFormatWrapper

Do the conversion between v8::ConvertableToTraceFormat to
base::trace_event::ConvertableToTraceFormat

BUG=406277
==========

to

==========
[gin] Add ConvertableToTraceFormatWrapper

Do the conversion between v8::ConvertableToTraceFormat to
base::trace_event::ConvertableToTraceFormat

BUG=406277
==========

[alph, 2016-09-26 22:06:56]

https://codereview.chromium.org/2368483002/diff/20001/gin/public/v8_platform.h
File gin/public/v8_platform.h (right):

https://codereview.chromium.org/2368483002/diff/20001/gin/public/v8_platform....
gin/public/v8_platform.h:47: std::unique_ptr<v8::ConvertableToTraceFormat>*,
On 2016/09/26 15:04:37, jochen (slow) wrote:
> please add an args name

Done.

https://codereview.chromium.org/2368483002/diff/20001/gin/v8_platform.cc
File gin/v8_platform.cc (right):

https://codereview.chromium.org/2368483002/diff/20001/gin/v8_platform.cc#newc...
gin/v8_platform.cc:94: explicit ConvertableToTraceFormatWrapper(
On 2016/09/26 15:04:37, jochen (slow) wrote:
> please add a virtual dtor

I wonder why? I'm fine with the default destructor here. I didn't find anything
about it in the style guide either.

https://codereview.chromium.org/2368483002/diff/20001/gin/v8_platform.cc#newc...
gin/v8_platform.cc:103: };
On 2016/09/26 15:04:37, jochen (slow) wrote:
> disallow copy / assign
> 

Done.

https://codereview.chromium.org/2368483002/diff/20001/gin/v8_platform.cc#newc...
gin/v8_platform.cc:129: DCHECK(num_args <= 2);
On 2016/09/26 15:04:37, jochen (slow) wrote:
> DCHECK_LT

Done.

[jochen (gone - plz use gerrit), 2016-09-27 19:09:05]

https://codereview.chromium.org/2368483002/diff/20001/gin/v8_platform.cc
File gin/v8_platform.cc (right):

https://codereview.chromium.org/2368483002/diff/20001/gin/v8_platform.cc#newc...
gin/v8_platform.cc:94: explicit ConvertableToTraceFormatWrapper(
On 2016/09/26 at 22:06:56, alph wrote:
> On 2016/09/26 15:04:37, jochen (slow) wrote:
> > please add a virtual dtor
> 
> I wonder why? I'm fine with the default destructor here. I didn't find
anything about it in the style guide either.

Assuming you'd move this class to a header, it would be a non-trivial dtor, and
then it shouldn't be inline. I think it's good practice to apply the same rules
to all classes.

[alph, 2016-09-27 19:24:24]

https://codereview.chromium.org/2368483002/diff/20001/gin/v8_platform.cc
File gin/v8_platform.cc (right):

https://codereview.chromium.org/2368483002/diff/20001/gin/v8_platform.cc#newc...
gin/v8_platform.cc:94: explicit ConvertableToTraceFormatWrapper(
On 2016/09/27 19:09:04, jochen (slow) wrote:
> On 2016/09/26 at 22:06:56, alph wrote:
> > On 2016/09/26 15:04:37, jochen (slow) wrote:
> > > please add a virtual dtor
> > 
> > I wonder why? I'm fine with the default destructor here. I didn't find
> anything about it in the style guide either.
> 
> Assuming you'd move this class to a header, it would be a non-trivial dtor,
and
> then it shouldn't be inline. I think it's good practice to apply the same
rules
> to all classes.

Done. Though I don't see how it helps. :-)
If I move the class into a header, I'd still need to move the implementation out
of the class and keep it in .cc to avoid inlining.
Btw the same applies to the constructor.

[alph, 2016-09-28 08:39:57]

The CQ bit was checked by alph@chromium.org

[alph, 2016-09-28 08:39:58]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
jochen@chromium.org, fmeawad@chromium.org
Link to the patchset: https://codereview.chromium.org/2368483002/#ps60001
(title: "addressing jochen@ comment.")

[commit-bot: I haz the power, 2016-09-28 08:40:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 09:35:11]

Description was changed from

==========
[gin] Add ConvertableToTraceFormatWrapper

Do the conversion between v8::ConvertableToTraceFormat to
base::trace_event::ConvertableToTraceFormat

BUG=406277
==========

to

==========
[gin] Add ConvertableToTraceFormatWrapper

Do the conversion between v8::ConvertableToTraceFormat to
base::trace_event::ConvertableToTraceFormat

BUG=406277
==========

[commit-bot: I haz the power, 2016-09-28 09:35:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-28 09:36:49]

Description was changed from

==========
[gin] Add ConvertableToTraceFormatWrapper

Do the conversion between v8::ConvertableToTraceFormat to
base::trace_event::ConvertableToTraceFormat

BUG=406277
==========

to

==========
[gin] Add ConvertableToTraceFormatWrapper

Do the conversion between v8::ConvertableToTraceFormat to
base::trace_event::ConvertableToTraceFormat

BUG=406277

Committed: https://crrev.com/92d855a4974fde0acad282b929b531fa6fca1a92
Cr-Commit-Position: refs/heads/master@{#421485}
==========

[commit-bot: I haz the power, 2016-09-28 09:36:50]

Patchset 4 (id:??) landed as
https://crrev.com/92d855a4974fde0acad282b929b531fa6fca1a92
Cr-Commit-Position: refs/heads/master@{#421485}