[Tracing] Make TracingCategoryObserver v8 internal.

[Tracing] Make TracingCategoryObserver v8 internal.

This patch removes TracingCategoryObserver API and moves the creation of
observer inside platform initialization, by assuming that either
Platform::AddTraceStateObserver is implemented correctly to add observer to
tracing controller that implemented by embedders, or default tracing controller
has already been set up and attached to platform before
v8::V8::InitializePlatform is called.

BUG=v8:5590
Committed: https://crrev.com/2525b0573b28e929a25a41ac7b57bdb84d9abf40
Cr-Commit-Position: refs/heads/master@{#40739}

[lpy, 2016-11-02 18:22:04]

The CQ bit was checked by lpy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 18:22:12]

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

[lpy, 2016-11-02 18:24:32]

lpy@chromium.org changed reviewers:
+ alph@chromium.org, fmeawad@chromium.org

[lpy, 2016-11-02 18:24:32]

PTAL.

[fmeawad, 2016-11-02 18:37:21]

I like this approach better because I believe that Embedders should not be aware
of the TraceCategoryObserver which is what this CL accomplish.

On the other hand, since we need a V8 internal code to initialize the observer,
we need to make sure that this code is called after Creating the
TracingController and setting it in the profiler. 

So the choice here for that is v8::V8::InitializePlatform.

If the V8 team is aware of another piece of code that is also internal in V8 and
that is guaranteed to run early enough but after the platform creation, please
let us know.

lgtm

[commit-bot: I haz the power, 2016-11-02 18:51:58]

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

[commit-bot: I haz the power, 2016-11-02 18:51:59]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15524)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)

[lpy, 2016-11-02 19:04:36]

The CQ bit was checked by lpy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 19:04:38]

Dry run: 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-11-02 19:08:48]

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

[commit-bot: I haz the power, 2016-11-02 19:08:49]

Dry run: Try jobs failed on following builders:
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)

[lpy, 2016-11-02 19:11:13]

The CQ bit was checked by lpy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 19:11:18]

Dry run: 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-11-02 19:15:28]

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

[commit-bot: I haz the power, 2016-11-02 19:15:29]

Dry run: Try jobs failed on following builders:
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)

[lpy, 2016-11-02 19:24:34]

The CQ bit was checked by lpy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 19:24:38]

Dry run: 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-11-02 19:27:08]

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

[commit-bot: I haz the power, 2016-11-02 19:27:09]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[lpy, 2016-11-02 19:32:55]

The CQ bit was checked by lpy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 19:33:03]

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

[lpy, 2016-11-02 19:34:26]

The CQ bit was checked by lpy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 19:34:33]

Dry run: 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-11-02 19:38:03]

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

[commit-bot: I haz the power, 2016-11-02 19:38:04]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[lpy, 2016-11-02 19:51:39]

The CQ bit was checked by lpy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 19:51:45]

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

[lpy, 2016-11-02 19:54:19]

Patchset #7 (id:120001) has been deleted

[lpy, 2016-11-02 19:54:26]

Patchset #6 (id:100001) has been deleted

[lpy, 2016-11-02 19:54:34]

Patchset #5 (id:80001) has been deleted

[lpy, 2016-11-02 19:54:40]

Patchset #4 (id:60001) has been deleted

[lpy, 2016-11-02 19:54:44]

Patchset #3 (id:40001) has been deleted

[lpy, 2016-11-02 19:54:51]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-11-02 19:55:35]

Dry run: Try jobs failed on following builders:
  v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/17197)

[lpy, 2016-11-02 19:56:00]

The CQ bit was checked by lpy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 19:56:04]

Dry run: 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-11-02 20:00:54]

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

[commit-bot: I haz the power, 2016-11-02 20:00:55]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/17199)

[alph, 2016-11-02 20:18:23]

Thank you. lgtm

https://codereview.chromium.org/2471583004/diff/1/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2471583004/diff/1/src/d8.cc#newcode2834
src/d8.cc:2834: platform::SetTracingController(g_platform, tracing_controller);
who is responsible for deleting tracing_controller under
FLAG_verify_predictable?

https://codereview.chromium.org/2471583004/diff/1/src/tracing/tracing-categor...
File src/tracing/tracing-category-observer.cc (right):

https://codereview.chromium.org/2471583004/diff/1/src/tracing/tracing-categor...
src/tracing/tracing-category-observer.cc:15:
std::unique_ptr<TracingCategoryObserver> g_tracing_category_observer;
nit: You can make it a static field inside the class.

https://codereview.chromium.org/2471583004/diff/1/src/tracing/tracing-categor...
File src/tracing/tracing-category-observer.h (right):

https://codereview.chromium.org/2471583004/diff/1/src/tracing/tracing-categor...
src/tracing/tracing-category-observer.h:28: ~TracingCategoryObserver() =
default;
nit: not needed

[lpy, 2016-11-02 21:48:51]

The CQ bit was checked by lpy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 21:48:54]

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

[lpy, 2016-11-02 21:49:05]

Patchset #2 (id:140001) has been deleted

[lpy, 2016-11-02 21:52:27]

Thanks, I updated the comments.

https://codereview.chromium.org/2471583004/diff/1/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2471583004/diff/1/src/d8.cc#newcode2834
src/d8.cc:2834: platform::SetTracingController(g_platform, tracing_controller);
On 2016/11/02 20:18:23, alph wrote:
> who is responsible for deleting tracing_controller under
> FLAG_verify_predictable?

Done.

https://codereview.chromium.org/2471583004/diff/1/src/tracing/tracing-categor...
File src/tracing/tracing-category-observer.cc (right):

https://codereview.chromium.org/2471583004/diff/1/src/tracing/tracing-categor...
src/tracing/tracing-category-observer.cc:15:
std::unique_ptr<TracingCategoryObserver> g_tracing_category_observer;
On 2016/11/02 20:18:23, alph wrote:
> nit: You can make it a static field inside the class.

Done.

https://codereview.chromium.org/2471583004/diff/1/src/tracing/tracing-categor...
File src/tracing/tracing-category-observer.h (right):

https://codereview.chromium.org/2471583004/diff/1/src/tracing/tracing-categor...
src/tracing/tracing-category-observer.h:28: ~TracingCategoryObserver() =
default;
On 2016/11/02 20:18:23, alph wrote:
> nit: not needed

Done.

[lpy, 2016-11-02 21:53:19]

On 2016/11/02 21:52:27, lpy wrote:
> Thanks, I updated the comments.
> 
ah sorry, I meant I updated the patch.

[commit-bot: I haz the power, 2016-11-02 22:23:31]

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

[commit-bot: I haz the power, 2016-11-02 22:23:32]

Dry run: This issue passed the CQ dry run.

[lpy, 2016-11-02 22:23:54]

The CQ bit was checked by lpy@chromium.org

[lpy, 2016-11-02 22:23:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
fmeawad@chromium.org, alph@chromium.org
Link to the patchset: https://codereview.chromium.org/2471583004/#ps160001
(title: "address alph's comments")

[commit-bot: I haz the power, 2016-11-02 22:24:02]

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-11-02 22:27:31]

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

[commit-bot: I haz the power, 2016-11-02 22:27:32]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/27708)

[lpy, 2016-11-02 22:28:14]

lpy@chromium.org changed reviewers:
+ jochen@chromium.org

[lpy, 2016-11-02 22:28:15]

+ jochen@ for review.
PTAL.

[alph, 2016-11-02 22:31:52]

https://codereview.chromium.org/2471583004/diff/160001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2471583004/diff/160001/src/d8.cc#newcode2955
src/d8.cc:2955: delete tracing_controller;
nit: using unique_ptr would help you avoid dealing with delete here.

[lpy, 2016-11-02 22:34:41]

On 2016/11/02 22:31:52, alph wrote:
> https://codereview.chromium.org/2471583004/diff/160001/src/d8.cc
> File src/d8.cc (right):
> 
> https://codereview.chromium.org/2471583004/diff/160001/src/d8.cc#newcode2955
> src/d8.cc:2955: delete tracing_controller;
> nit: using unique_ptr would help you avoid dealing with delete here.

platform holds tracing controller in unique_ptr, see
https://cs.chromium.org/chromium/src/v8/src/libplatform/default-platform.cc?s...

is it safe to have another unique_ptr here to hold the tracing controller?

[jochen (gone - plz use gerrit), 2016-11-03 11:55:27]

lgtm

[alph, 2016-11-03 15:07:39]

On 2016/11/02 22:34:41, lpy wrote:
> On 2016/11/02 22:31:52, alph wrote:
> > https://codereview.chromium.org/2471583004/diff/160001/src/d8.cc
> > File src/d8.cc (right):
> > 
> > https://codereview.chromium.org/2471583004/diff/160001/src/d8.cc#newcode2955
> > src/d8.cc:2955: delete tracing_controller;
> > nit: using unique_ptr would help you avoid dealing with delete here.
> 
> platform holds tracing controller in unique_ptr, see
>
https://cs.chromium.org/chromium/src/v8/src/libplatform/default-platform.cc?s...
> 
> is it safe to have another unique_ptr here to hold the tracing controller?

It is safe. You call release on the pointer when you pass ownership to the
platform.

[lpy, 2016-11-03 16:47:04]

The CQ bit was checked by lpy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 16:47:14]

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

[lpy, 2016-11-03 16:47:31]

On 2016/11/03 15:07:39, alph wrote:
> On 2016/11/02 22:34:41, lpy wrote:
> > On 2016/11/02 22:31:52, alph wrote:
> > > https://codereview.chromium.org/2471583004/diff/160001/src/d8.cc
> > > File src/d8.cc (right):
> > > 
> > >
https://codereview.chromium.org/2471583004/diff/160001/src/d8.cc#newcode2955
> > > src/d8.cc:2955: delete tracing_controller;
> > > nit: using unique_ptr would help you avoid dealing with delete here.
> > 
> > platform holds tracing controller in unique_ptr, see
> >
>
https://cs.chromium.org/chromium/src/v8/src/libplatform/default-platform.cc?s...
> > 
> > is it safe to have another unique_ptr here to hold the tracing controller?
> 
> It is safe. You call release on the pointer when you pass ownership to the
> platform.

Thanks, done.

[commit-bot: I haz the power, 2016-11-03 17:10:17]

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

[commit-bot: I haz the power, 2016-11-03 17:10:18]

Dry run: This issue passed the CQ dry run.

[lpy, 2016-11-03 18:00:56]

The CQ bit was checked by lpy@chromium.org

[lpy, 2016-11-03 18:00:56]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
alph@chromium.org, fmeawad@chromium.org
Link to the patchset: https://codereview.chromium.org/2471583004/#ps180001
(title: "use unique_ptr")

[commit-bot: I haz the power, 2016-11-03 18:01:02]

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-11-03 18:03:31]

Committed patchset #3 (id:180001)

[alph, 2016-11-03 20:19:27]

https://codereview.chromium.org/2471583004/diff/180001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2471583004/diff/180001/src/d8.cc#newcode2834
src/d8.cc:2834: platform::SetTracingController(g_platform,
tracing_controller.get());
you can use release right here. it returns raw pointer.

[commit-bot: I haz the power, 2016-11-17 22:21:21]

Description was changed from

==========
[Tracing] Make TracingCategoryObserver v8 internal.

This patch removes TracingCategoryObserver API and moves the creation of
observer inside platform initialization, by assuming that either
Platform::AddTraceStateObserver is implemented correctly to add observer to
tracing controller that implemented by embedders, or default tracing controller
has already been set up and attached to platform before
v8::V8::InitializePlatform is called.

BUG=v8:5590
==========

to

==========
[Tracing] Make TracingCategoryObserver v8 internal.

This patch removes TracingCategoryObserver API and moves the creation of
observer inside platform initialization, by assuming that either
Platform::AddTraceStateObserver is implemented correctly to add observer to
tracing controller that implemented by embedders, or default tracing controller
has already been set up and attached to platform before
v8::V8::InitializePlatform is called.

BUG=v8:5590

Committed: https://crrev.com/2525b0573b28e929a25a41ac7b57bdb84d9abf40
Cr-Commit-Position: refs/heads/master@{#40739}
==========

[commit-bot: I haz the power, 2016-11-17 22:21:23]

Patchset 3 (id:??) landed as
https://crrev.com/2525b0573b28e929a25a41ac7b57bdb84d9abf40
Cr-Commit-Position: refs/heads/master@{#40739}