[gin] Fire observer after added when recording is in progress.

[gin] Fire observer after added when recording is in progress.

When run benchmarks on Android, we don't get runtime statistics result because
we register an observer to trigger a flag to enable it, however, tracing is
started before observers are added to observer list in this case, thus we force
to fire observer when we register an observer through platform API when
recording is in progress.

BUG=682167
Review-Url: https://codereview.chromium.org/2650943008
Cr-Commit-Position: refs/heads/master@{#446713}
Committed: https://chromium.googlesource.com/chromium/src/+/48e4c328641ba109f490a7375418b5998c4955fb

[lpy, 2017-01-25 00:15:17]

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

[lpy, 2017-01-25 00:15:38]

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

[lpy, 2017-01-25 00:15:39]

alph@, ptal.

[commit-bot: I haz the power, 2017-01-25 00:15:56]

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, 2017-01-25 01:31:00]

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

[commit-bot: I haz the power, 2017-01-25 01:31:01]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-01-25 12:18:16]

shall we take this opportunity and start adding unit tests for the v8_platform?

[lpy, 2017-01-25 21:58:56]

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

[lpy, 2017-01-25 21:59:14]

On 2017/01/25 12:18:16, jochen (travelling til Feb 4) wrote:
> shall we take this opportunity and start adding unit tests for the
v8_platform?

Tests added. PTAL.

[commit-bot: I haz the power, 2017-01-25 21:59:30]

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

[alph, 2017-01-25 22:39:06]

lgtm % nit

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

https://codereview.chromium.org/2650943008/diff/20001/gin/v8_platform.cc#newc...
gin/v8_platform.cc:223: observers_.insert(observer);
Can you please move this line past 225. Just to be on the safe side if/when
AddEnabledStateObserver also implements similar logic.

[lpy, 2017-01-25 23:11:15]

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

https://codereview.chromium.org/2650943008/diff/20001/gin/v8_platform.cc#newc...
gin/v8_platform.cc:223: observers_.insert(observer);
On 2017/01/25 22:39:06, alph wrote:
> Can you please move this line past 225. Just to be on the safe side if/when
> AddEnabledStateObserver also implements similar logic.

I don't understand here, what do you mean similar logic? Do you mean if
`AddEnabledStateObserver` fires observer when observer is being added and then
we are in dead lock here?

Moving `observers_.insert(observer)` past 225 means EnabledStateObserverImpl
won't add itself to observer list in TraceLog the first time when `AddObserver`
is called.

[alph, 2017-01-25 23:26:32]

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

https://codereview.chromium.org/2650943008/diff/20001/gin/v8_platform.cc#newc...
gin/v8_platform.cc:223: observers_.insert(observer);
On 2017/01/25 23:11:15, lpy wrote:
> On 2017/01/25 22:39:06, alph wrote:
> > Can you please move this line past 225. Just to be on the safe side if/when
> > AddEnabledStateObserver also implements similar logic.
> 
> I don't understand here, what do you mean similar logic? Do you mean if
> `AddEnabledStateObserver` fires observer when observer is being added and then
> we are in dead lock here?
> 
> Moving `observers_.insert(observer)` past 225 means EnabledStateObserverImpl
> won't add itself to observer list in TraceLog the first time when
`AddObserver`
> is called.

My suggestion implied you change "observers_.size() == 1" to
"observers_.empty()"

[commit-bot: I haz the power, 2017-01-26 01:04:24]

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

[commit-bot: I haz the power, 2017-01-26 01:04:25]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[lpy, 2017-01-26 02:26:24]

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

[lpy, 2017-01-26 02:26:41]

Thanks, ptal.

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

https://codereview.chromium.org/2650943008/diff/20001/gin/v8_platform.cc#newc...
gin/v8_platform.cc:223: observers_.insert(observer);
On 2017/01/25 23:26:32, alph wrote:
> On 2017/01/25 23:11:15, lpy wrote:
> > On 2017/01/25 22:39:06, alph wrote:
> > > Can you please move this line past 225. Just to be on the safe side
if/when
> > > AddEnabledStateObserver also implements similar logic.
> > 
> > I don't understand here, what do you mean similar logic? Do you mean if
> > `AddEnabledStateObserver` fires observer when observer is being added and
then
> > we are in dead lock here?
> > 
> > Moving `observers_.insert(observer)` past 225 means EnabledStateObserverImpl
> > won't add itself to observer list in TraceLog the first time when
> `AddObserver`
> > is called.
> 
> My suggestion implied you change "observers_.size() == 1" to
> "observers_.empty()"

Done.

[commit-bot: I haz the power, 2017-01-26 02:27:29]

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, 2017-01-26 05:45:54]

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

[commit-bot: I haz the power, 2017-01-26 05:45:54]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jochen (gone - plz use gerrit), 2017-01-27 04:09:18]

awesome, thx

lgtm

[lpy, 2017-01-27 08:01:45]

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

[commit-bot: I haz the power, 2017-01-27 08:01:53]

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, 2017-01-27 08:54:33]

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

[commit-bot: I haz the power, 2017-01-27 08:54:34]

Dry run: This issue passed the CQ dry run.

[lpy, 2017-01-27 18:14:12]

The CQ bit was checked by lpy@chromium.org

[lpy, 2017-01-27 18:14:13]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org
Link to the patchset: https://codereview.chromium.org/2650943008/#ps40001
(title: "update")

[commit-bot: I haz the power, 2017-01-27 18:14:48]

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, 2017-01-27 18:22:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485540852980560,
"parent_rev": "9d62fd77957ef8e486ad77c57e05b717c5a037d3", "commit_rev":
"48e4c328641ba109f490a7375418b5998c4955fb"}

[commit-bot: I haz the power, 2017-01-27 18:23:06]

Description was changed from

==========
[gin] Fire observer after added when recording is in progress.

When run benchmarks on Android, we don't get runtime statistics result because
we register an observer to trigger a flag to enable it, however, tracing is
started before observers are added to observer list in this case, thus we force
to fire observer when we register an observer through platform API when
recording is in progress.

BUG=682167
==========

to

==========
[gin] Fire observer after added when recording is in progress.

When run benchmarks on Android, we don't get runtime statistics result because
we register an observer to trigger a flag to enable it, however, tracing is
started before observers are added to observer list in this case, thus we force
to fire observer when we register an observer through platform API when
recording is in progress.

BUG=682167

Review-Url: https://codereview.chromium.org/2650943008
Cr-Commit-Position: refs/heads/master@{#446713}
Committed:
https://chromium.googlesource.com/chromium/src/+/48e4c328641ba109f490a7375418...
==========

[commit-bot: I haz the power, 2017-01-27 18:23:07]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/48e4c328641ba109f490a7375418...