1197003 - Implement processing algorithm for PerformanceObserver to notify a batch of entries

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

https://github.com/w3c/performance-timeline/pull/46

The spec change is still in discussion though.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP

All tests has been rewritten with promise_test[1] because all tests need to be run sequentially to avoid being mixed of user timing entries.

[1] https://github.com/w3c/testharness.js/commit/cf0b67dff57c3140c882b1ffd2b5f4c7adc2ba44

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Processing algorithm has been merged now.
https://github.com/w3c/performance-timeline/commit/3cd10a8f1dff7f7d9d2fc52ba2ab38d0ee6ce093

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: PerformanceObserver::Disconenct() should be called before mPerformance is destroyed.

I realized that Disconnect() should be called before mPerformance is destroyed in case of the reference of the mPerformance is the last one.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Implement processing algorithm for PerformanceObserver to notify a batch of entries

The previous patch (attachment 8650776 [details] [diff] [review]) did not work on worker threads. I did not know that nsCancelableRunnable is needed for worker threads. 

Changes for test parts needs promise_test[1] to run each test sequentially.
sequential promise_test has been already merged in mozilla-central[2].

[1] https://github.com/w3c/testharness.js/commit/cf0b67dff57c3140c882b1ffd2b5f4c7adc2ba44
[2] https://hg.mozilla.org/mozilla-central/rev/4c5c368b1adf

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8652556 [details] [diff] [review]
Part 1: PerformanceObserver::Disconenct() should be called before mPerformance is destroyed.

Review of attachment 8652556 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/PerformanceObserver.cpp
@@ -54,5 @@
>  }
>  
>  PerformanceObserver::~PerformanceObserver()
>  {
> -  Disconnect();

I would not remove this. Keep this too if Disconnect() has:

if (!mConnected) { return.. or something similar.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: PerformanceObserver::Disconenct() should be called before mPerformance is destroyed v2

Addressed comment#5.
Carrying over review+.
Thanks Andrea!

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8652559 [details] [diff] [review]
Part 2: Implement processing algorithm for PerformanceObserver to notify a batch of entries

Review of attachment 8652559 [details] [diff] [review]:
-----------------------------------------------------------------

Interdiff for this small issue?

::: dom/base/nsPerformance.cpp
@@ +1111,5 @@
> +  NS_IMETHOD Run() override
> +  {
> +    mPerformance->NotifyObservers();
> +    return NS_OK;
> +  }

NS_IMETHOD Cancel() override
{
  We should inform mPerformance that this task has been canceled.
}

@@ +1126,5 @@
> +PerformanceBase::RunNotificationObserversTask()
> +{
> +  mPendingNotificationObserversTask = true;
> +  nsCOMPtr<nsIRunnable> task = new NotifyObserversTask(this);
> +  NS_DispatchToCurrentThread(task);

This can fail. Do:

nsresult rv = NS_DispatchToCurrentThread(task);
if (NS_WARN_IF(NS_FAILED(rv))) {
  mPendingNotificationObserversTask = false;
}

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Cancel notification to observers

This is the diff to address comment#7.
Thanks!

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8655850 [details] [diff] [review]
Cancel notification to observers

Review of attachment 8655850 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsPerformance.cpp
@@ +1110,5 @@
>  {
>  public:
>    explicit NotifyObserversTask(PerformanceBase* aPerformance)
>      : mPerformance(aPerformance)
>    {

MOZ_ASSERT(mPerformance);

@@ +1114,5 @@
>    {
>    }
>  
>    NS_IMETHOD Run() override
>    {

MOZ_ASSERT(mPerformance);

@@ +1121,5 @@
>    }
>  
> +  NS_IMETHOD Cancel() override
> +  {
> +    mPerformance->CancelNotificationObservers();

mPerformance = nullptr;

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Implement processing algorithm for PerformanceObserver to notify a batch of entries v2

Carrying over review+.

Unified attachment #8655850 [details] [diff] [review].

Comment 11

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9451b0f0437b
Thanks!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e099f2b5d8d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6112374608d

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e099f2b5d8d8
https://hg.mozilla.org/mozilla-central/rev/b6112374608d