1165796 - Implement Performance Observer

Status: RESOLVED FIXED

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

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I've decided to implement this without consideration of Frame Timing API for now.
 
I suppose that all performance timings except frame timing can notify each entry to observers when a new entry is recorded respectively. But in case of frame timing, each notification is invoked with a bunch of entries. In my understandings, it is not yet defined how often the notification is invoked.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Make PrefEnabledRunnable usable for other preference names

To implement a new method named nsPerformance::IsObserverEnabled we need to modify current PrefEnabledRunnable to be used for other preference names.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Implement PerformanceObserver

This patch is just for reference.

Actually this patch almost works fine except on worker thread because of bug 1178642.
I will rewrite this without fix for bug 1178642 soon.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Implement PerformanceObserver v2

PerformanceObserver class and two inheritances for each threads are implemented in a single file in dom/base. I'd tried to use separate files in dom/base and dom/workers, but it did not work well due to a constrain of binding code generator. See bug 1178642.

Anyway the implementation in this patch became much simpler than before (attachment 8627548 [details] [diff] [review]).

Note about tests in this patch:
The tests use testharness.js instead of SimpleTest.js because testharness.js provides a useful method in worker named fetch_tests_from_worker. With fetch_tests_from_worker we can use any assertions in worker thread as well as main thread.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0983a759fa4a
The try server results seem good (all known oranges).

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8627543 [details] [diff] [review]
Part 1: Make PrefEnabledRunnable usable for other preference names

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

::: dom/base/nsPerformance.cpp
@@ +686,5 @@
>  
>  class PrefEnabledRunnable final : public WorkerMainThreadRunnable
>  {
>  public:
> +  explicit PrefEnabledRunnable(WorkerPrivate* aWorkerPrivate,

no explicit when there are more than 1 param.

@@ +687,5 @@
>  class PrefEnabledRunnable final : public WorkerMainThreadRunnable
>  {
>  public:
> +  explicit PrefEnabledRunnable(WorkerPrivate* aWorkerPrivate,
> +                               const char* aPrefName)

can you make this a const nsCString& aPrefName ?

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8628081 [details] [diff] [review]
Part 2: Implement PerformanceObserver v2

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

looks good! Can you apply these comments and answer to my questions? Thanks!
Then send me a new version of the patch and I'll review it asap.

::: dom/base/LazyPerformanceEntryList.cpp
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsPerformance.h"
> +#include "nsString.h"
> +#include "LazyPerformanceEntryList.h"

move this on top of the headers.

::: dom/base/LazyPerformanceEntryList.h
@@ +7,5 @@
> +#ifndef LazyPerformanceEntryList_h
> +#define LazyPerformanceEntryList_h
> +
> +#include "mozilla/dom/LazyPerformanceEntryListBinding.h"
> +#include "mozilla/dom/PerformanceEntry.h"

these 2 are not needed.

@@ +19,5 @@
> +  virtual ~LazyPerformanceEntryList();
> +
> +public:
> +  explicit LazyPerformanceEntryList(nsISupports* aOwner)
> +  : mOwner(aOwner)

2 spaces.

::: dom/base/PerformanceObserver.cpp
@@ +78,5 @@
> +  }
> +
> +  virtual PerformanceBase* GetPerformance() override;
> +
> +  virtual nsISupports* GetParentObject() const override;

in workers this can simple returns nullptr.

@@ +149,5 @@
> +    }
> +    MOZ_ASSERT(ownerWindow->IsInnerWindow());
> +
> +    nsRefPtr<PerformanceObserver> observer =
> +      new MainThreadPerformanceObserver(ownerWindow, aCb);

instead doing this, implement just 1 class "PerformanceObserver" and when you need to know in which thread you are, you can simple do:

if (NS_IsMainThread()) {
  ... something
} else {
  MOZ_ASSERT(mWorkerPrivate);
  mWorkerPrivate->AssertInWorkerThread();
  ... something else
}

And maybe, just add 2 CTOR, 1 for window and 1 for WorkerPrivate.

@@ +187,5 @@
> +};
> +
> +void
> +PerformanceObserver::Notify(PerformanceEntry* aEntry)
> +{

MOZ_ASSERT(aEntry);

@@ +190,5 @@
> +PerformanceObserver::Notify(PerformanceEntry* aEntry)
> +{
> +  nsAutoString entryType;
> +  aEntry->GetEntryType(entryType);
> +  if (!mTypeFilter.Contains(entryType, PerformanceStringComparator())) {

Why do you need this custom comparison class?

@@ +194,5 @@
> +  if (!mTypeFilter.Contains(entryType, PerformanceStringComparator())) {
> +    return;
> +  }
> +
> +  LazyPerformanceEntryList* list = new LazyPerformanceEntryList(this);

It seems to me that we are leaking this list.
What about:

nsRefPtr<LazyPerformanceEntryList> list = ...

@@ +211,5 @@
> +    return;
> +  }
> +
> +  mTypeFilter.Clear();
> +  for (nsString typeName : aOptions.mTypeFilter) {

can it be:

const nsString& typeName ?

or nsAutoString ?

::: dom/base/PerformanceObserver.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef PerformanceObserver_h

ifndef mozilla_dom_PerformanceObserver_h

@@ +6,5 @@
> +
> +#ifndef PerformanceObserver_h
> +#define PerformanceObserver_h
> +
> +#include "nsPerformance.h"

you don't need this include.

@@ +7,5 @@
> +#ifndef PerformanceObserver_h
> +#define PerformanceObserver_h
> +
> +#include "nsPerformance.h"
> +#include "mozilla/dom/PerformanceObserverBinding.h"

remove this too and use a forward declaration of PerformanceObserverCallback

@@ +8,5 @@
> +#define PerformanceObserver_h
> +
> +#include "nsPerformance.h"
> +#include "mozilla/dom/PerformanceObserverBinding.h"
> +

include nsWrappercache.h and nsISupports.h

@@ +21,5 @@
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(PerformanceObserver)
> +
> +  explicit PerformanceObserver(PerformanceObserverCallback& aCb)

can it be a private ctor?

@@ +22,5 @@
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(PerformanceObserver)
> +
> +  explicit PerformanceObserver(PerformanceObserverCallback& aCb)
> +  : mCallback(&aCb)

2 spaces before :

::: dom/base/nsPerformance.cpp
@@ +1046,4 @@
>  }
> +
> +void
> +PerformanceBase::AddObserver(PerformanceObserver* observer)

aObserver

@@ +1046,5 @@
>  }
> +
> +void
> +PerformanceBase::AddObserver(PerformanceObserver* observer)
> +{

what about:

MOZ_ASSERT(aObserver);
MOZ_ASSERT(!mObservers.Contains(aObserver))
mObservers.AppendElement(aObserver);

@@ +1052,5 @@
> +}
> +
> +bool
> +PerformanceBase::RemoveObserver(PerformanceObserver* observer)
> +{

aObserver

and then:

MOZ_ASSERT(aObserver);
MOZ_ASSERT(mObservers.Contains(aObserver);

::: dom/webidl/LazyPerformanceEntryList.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is
> + * http://dom.spec.whatwg.org

wrong URL.

@@ +7,5 @@
> + * http://dom.spec.whatwg.org
> + */
> +
> +[Func="nsPerformance::IsObserverEnabled", Exposed=(Window,Worker)]
> +interface LazyPerformanceEntryList {

Where is this interface from? Can you give me the spec URL?

::: dom/webidl/PerformanceObserver.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is
> + * http://dom.spec.whatwg.org

this is the wrong URL, correct?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Make PrefEnabledRunnable usable for other preference names v2

Addressed all comments.
Carrying over review+.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Implement PerformanceObserver v3

Some part of this patch has been changed considerably because I did not catch up recent changes in the spec. Renaming LazyPerformanceEntryList [1] and Filtering of getEntries [2].

[1] https://github.com/w3c/performance-timeline/commit/587e2803f7e354104d57d05072d5bf61452d0a92
[2] https://github.com/w3c/performance-timeline/commit/32b0420d7ff52ce8aec3adeb7b0ac49a7d46064a

And this patch is not ready for review because there is an issue about filtering of getEntries(). I will post about the issue later.

(In reply to Andrea Marchesini (:baku) from comment #6)
> ::: dom/base/LazyPerformanceEntryList.cpp
> @@ +5,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +#include "nsPerformance.h"
> > +#include "nsString.h"
> > +#include "LazyPerformanceEntryList.h"
> 
> move this on top of the headers.

Fixed.

> ::: dom/base/LazyPerformanceEntryList.h
> @@ +7,5 @@
> > +#ifndef LazyPerformanceEntryList_h
> > +#define LazyPerformanceEntryList_h
> > +
> > +#include "mozilla/dom/LazyPerformanceEntryListBinding.h"
> > +#include "mozilla/dom/PerformanceEntry.h"
> 
> these 2 are not needed.

Fixed.

> @@ +19,5 @@
> > +  virtual ~LazyPerformanceEntryList();
> > +
> > +public:
> > +  explicit LazyPerformanceEntryList(nsISupports* aOwner)
> > +  : mOwner(aOwner)
> 
> 2 spaces.

Fixed.

> ::: dom/base/PerformanceObserver.cpp
> @@ +78,5 @@
> > +  }
> > +
> > +  virtual PerformanceBase* GetPerformance() override;
> > +
> > +  virtual nsISupports* GetParentObject() const override;
> 
> in workers this can simple returns nullptr.

Thanks. I did not notice this fact.

> @@ +149,5 @@
> > +    }
> > +    MOZ_ASSERT(ownerWindow->IsInnerWindow());
> > +
> > +    nsRefPtr<PerformanceObserver> observer =
> > +      new MainThreadPerformanceObserver(ownerWindow, aCb);
> 
> instead doing this, implement just 1 class "PerformanceObserver" and when
> you need to know in which thread you are, you can simple do:
> 
> if (NS_IsMainThread()) {
>   ... something
> } else {
>   MOZ_ASSERT(mWorkerPrivate);
>   mWorkerPrivate->AssertInWorkerThread();
>   ... something else
> }

Actually I wanted to avoid this pattern. So bz suggested me to create two inheriting classes in bug 1178642.

> And maybe, just add 2 CTOR, 1 for window and 1 for WorkerPrivate.

How do I do that? I really wanted to create two separate constructors.

> @@ +187,5 @@
> > +};
> > +
> > +void
> > +PerformanceObserver::Notify(PerformanceEntry* aEntry)
> > +{
> 
> MOZ_ASSERT(aEntry);

Fixed.

> @@ +190,5 @@
> > +PerformanceObserver::Notify(PerformanceEntry* aEntry)
> > +{
> > +  nsAutoString entryType;
> > +  aEntry->GetEntryType(entryType);
> > +  if (!mTypeFilter.Contains(entryType, PerformanceStringComparator())) {
> 
> Why do you need this custom comparison class?

Unfortunately there is no Comparator for nsString.

> @@ +194,5 @@
> > +  if (!mTypeFilter.Contains(entryType, PerformanceStringComparator())) {
> > +    return;
> > +  }
> > +
> > +  LazyPerformanceEntryList* list = new LazyPerformanceEntryList(this);
> 
> It seems to me that we are leaking this list.
> What about:
> 
> nsRefPtr<LazyPerformanceEntryList> list = ...

Fixed.

> @@ +211,5 @@
> > +    return;
> > +  }
> > +
> > +  mTypeFilter.Clear();
> > +  for (nsString typeName : aOptions.mTypeFilter) {
> 
> can it be:
> 
> const nsString& typeName ?
> 
> or nsAutoString ?

Fixed to use const nsString&.

> ::: dom/base/PerformanceObserver.h
> @@ +3,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +#ifndef PerformanceObserver_h
> 
> ifndef mozilla_dom_PerformanceObserver_h

Fixed.

> @@ +6,5 @@
> > +
> > +#ifndef PerformanceObserver_h
> > +#define PerformanceObserver_h
> > +
> > +#include "nsPerformance.h"
> 
> you don't need this include.

Fixed.
It took a while for me to recognize nsPerformance (and PerformanceBase) is not in mozilla::dom namespace!

> @@ +7,5 @@
> > +#ifndef PerformanceObserver_h
> > +#define PerformanceObserver_h
> > +
> > +#include "nsPerformance.h"
> > +#include "mozilla/dom/PerformanceObserverBinding.h"
> 
> remove this too and use a forward declaration of PerformanceObserverCallback

Fixed.

> @@ +8,5 @@
> > +#define PerformanceObserver_h
> > +
> > +#include "nsPerformance.h"
> > +#include "mozilla/dom/PerformanceObserverBinding.h"
> > +
> 
> include nsWrappercache.h and nsISupports.h

Fixed.

> @@ +21,5 @@
> > +public:
> > +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> > +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(PerformanceObserver)
> > +
> > +  explicit PerformanceObserver(PerformanceObserverCallback& aCb)
> 
> can it be a private ctor?

It can't. So I change it protected. 

> @@ +22,5 @@
> > +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> > +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(PerformanceObserver)
> > +
> > +  explicit PerformanceObserver(PerformanceObserverCallback& aCb)
> > +  : mCallback(&aCb)
> 
> 2 spaces before :

Fixed.

> ::: dom/base/nsPerformance.cpp
> @@ +1046,4 @@
> >  }
> > +
> > +void
> > +PerformanceBase::AddObserver(PerformanceObserver* observer)
> 
> aObserver

Fixed.

> @@ +1046,5 @@
> >  }
> > +
> > +void
> > +PerformanceBase::AddObserver(PerformanceObserver* observer)
> > +{
> 
> what about:
> 
> MOZ_ASSERT(aObserver);
> MOZ_ASSERT(!mObservers.Contains(aObserver))
> mObservers.AppendElement(aObserver);
> 
> @@ +1052,5 @@
> > +}
> > +
> > +bool
> > +PerformanceBase::RemoveObserver(PerformanceObserver* observer)
> > +{
> 
> aObserver
> 
> and then:
> 
> MOZ_ASSERT(aObserver);
> MOZ_ASSERT(mObservers.Contains(aObserver);

I'd like to make Add/RemoveObserver harmless. That means user can call those methods any number of times.

> ::: dom/webidl/LazyPerformanceEntryList.webidl
> @@ +3,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/.
> > + *
> > + * The origin of this IDL file is
> > + * http://dom.spec.whatwg.org
> 
> wrong URL.

Fixed. Sorry for copy&paste...

> @@ +7,5 @@
> > + * http://dom.spec.whatwg.org
> > + */
> > +
> > +[Func="nsPerformance::IsObserverEnabled", Exposed=(Window,Worker)]
> > +interface LazyPerformanceEntryList {
> 
> Where is this interface from? Can you give me the spec URL?

I mentioned first, I did not catch up recent changes in the spec.

> ::: dom/webidl/PerformanceObserver.webidl
> @@ +3,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/.
> > + *
> > + * The origin of this IDL file is
> > + * http://dom.spec.whatwg.org
> 
> this is the wrong URL, correct?

Fixed.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Codegen.py seems not to use Optional template if the optional argument is a dictionary type. So how can I distinguish whether no argument is specified or attribute name in the dictionary is invalid?

Here is the webidl for PerformanceObserverEntryList:

dictionary FilterOptions {
  DOMString name;
  DOMString entryType;
  DOMString initiatorType;
};

[Func="nsPerformance::IsObserverEnabled", Exposed=(Window,Worker)]
interface PerformanceObserverEntryList {
  PerformanceEntryList getEntries(optional FilterOptions filter);
};

Then Codegen.py generates like this:

static bool
getEntries(JSContext* cx, JS::Handle<JSObject*> obj, mozilla::dom::PerformanceObserverEntryList* self, const JSJitMethodCallArgs& args)
{
  binding_detail::FastFilterOptions arg0;
  if (!arg0.Init(cx, (args.hasDefined(0)) ? args[0] : JS::NullHandleValue, "Argument 1 of PerformanceObserverEntryList.getEntries", false)) {
    return false;
  }
  nsTArray<StrongPtrForMember<mozilla::dom::PerformanceEntry>::Type> result;
  self->GetEntries(Constify(arg0), result);

In this case, we can define GetEntries as below:

void GetEntries(const FilterOptions& aFilterOptions, ...);

(I wrote Optional<FilterOptions>& there first but it can not be compiled.)

So below codes in caller side do not work as expected.

 getEntries(); it should return all entries.

 getEntries({"invalid": "xxxx"}) it should return an empty string.

Is there any way to handle there differences?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> seems not to use Optional template if the optional argument is a dictionary type.

Right, because per the Web IDL spec, null, undefined, {}, and "not passed" are all the same thing for dictionary types.

> getEntries(); it should return all entries.
> getEntries({"invalid": "xxxx"}) it should return an empty string.

Then it shouldn't be using a dictionary argument in the IDL.  If that argument is a dictionary that doesn't have a member named "invalid", those two calls will have identical behavior.

But where are you getting your desired behavior for the {"invalid": "xxx"} case from?  The spec at http://w3c.github.io/performance-timeline/#widl-Performance-getEntries-PerformanceEntryList-FilterOptions-filter doesn't seem to say anything like that...

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Thanks, I did misunderstand the spec. I thought getEntries in the invalid attribute case must return an empty array.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Implement PerformanceObserver v4

The spec has been updated. Now it's time to update patches too.

There are mainly two changes in the spec.

1) Valid entryType values are introduced;
https://w3c.github.io/performance-timeline/#widl-PerformanceEntry-entryType
2) PerformanceObserverInit member has been renamed, entryType -> entryTypes;
https://w3c.github.io/performance-timeline/#widl-PerformanceObserverInit-entryTypes

This patch addressed comment #6 and includes additional test cases corresponding to the spec changes and some tests cleanup.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 0: Unified build fix

Unfortunately there is another unified build error.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4a108b9b23c
Not finished yet.

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 8647465 [details] [diff] [review]
Part 2: Implement PerformanceObserver v4

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

looks good. Give me an answer to the question about multiple observe() calls.

::: dom/base/PerformanceObserver.cpp
@@ +19,5 @@
> +#include "WorkerPrivate.h"
> +#include "WorkerScope.h"
> +
> +using namespace mozilla;
> +using namespace mozilla::dom;

using namespace mozilla::dom::workers;

and remove all the workers::something.

@@ +36,5 @@
> +PerformanceObserver::PerformanceObserver(nsPIDOMWindow* aOwner,
> +                                         PerformanceObserverCallback& aCb)
> +  : mOwner(aOwner)
> +  , mCallback(&aCb)
> +{

MOZ_ASSERT(mOwner);

@@ +40,5 @@
> +{
> +  mPerformance = aOwner->GetPerformance();
> +}
> +
> +PerformanceObserver::PerformanceObserver(workers::WorkerPrivate* workerPrivate,

aWorkerPrivate

@@ +43,5 @@
> +
> +PerformanceObserver::PerformanceObserver(workers::WorkerPrivate* workerPrivate,
> +                                         PerformanceObserverCallback& aCb)
> +  : mCallback(&aCb)
> +{

MOZ_ASSERT(aWorkerPrivate);

@@ +72,5 @@
> +    return observer.forget();
> +  }
> +
> +  JSContext* cx = aGlobal.Context();
> +  mozilla::dom::workers::WorkerPrivate* workerPrivate =

remove 'mozilla::dom::'

@@ +102,5 @@
> +  nsRefPtr<PerformanceObserverEntryList> list = new PerformanceObserverEntryList(this);
> +  list->AppendEntry(aEntry);
> +
> +  ErrorResult rv;
> +  mCallback->Call(this, *list, *this, rv);

NS_WARN_IF(rv.Failed());

@@ +138,5 @@
> +    aRv.Throw(NS_ERROR_DOM_TYPE_ERR);
> +    return;
> +  }
> +
> +  mEntryTypes = validEntryTypes;

what about if I call 'observe' more than once?
What does the spec say about it? I see 2 options:

1. we append the entrytypes
2. we overwrite them.

in both cases I would like to have a mochitest.

::: dom/base/PerformanceObserver.h
@@ +45,5 @@
> +
> +  PerformanceObserver(nsPIDOMWindow* aOwner,
> +                      PerformanceObserverCallback& aCb);
> +
> +  PerformanceObserver(mozilla::dom::workers::WorkerPrivate* workerPrivate,

1. aWorkerPrivate
2. remove mozilla::dom::

@@ +61,5 @@
> +
> +  void Notify(PerformanceEntry* entry);
> +
> +private:
> +  virtual ~PerformanceObserver();

this class is 'final'. Remove virtual.

::: dom/base/PerformanceObserverEntryList.cpp
@@ +45,5 @@
> +  for (const nsRefPtr<PerformanceEntry>& entry : mEntries) {
> +    if (aFilter.mInitiatorType.WasPassed() &&
> +        entry->GetEntryType().EqualsLiteral("resource")) {
> +      nsRefPtr<PerformanceResourceTiming> resourceEntry =
> +        static_cast<PerformanceResourceTiming*>(entry.get());

this seems very fragile. What about if you add a:

virtual PerformanceResourceTiming* ToResourceTiming() const { return nullptr; }

in PerformanceResourceTiming you do:

virtual PerformanceResourceTiming* ToResourceTiming() const overwrite { return this; }

here you can then do:

PerformanceResourceTiming* resouceEntry = entry->ToResourceTiming();
if (aFilter.mInitiatorType.WasPassed() && resourceEntry) {
  ...

::: dom/base/PerformanceObserverEntryList.h
@@ +22,5 @@
> +
> +class PerformanceObserverEntryList final : public nsISupports,
> +                                           public nsWrapperCache
> +{
> +  virtual ~PerformanceObserverEntryList();

no virtual for final classes.

::: dom/webidl/Performance.webidl
@@ +70,5 @@
>    void measure(DOMString measureName, optional DOMString startMark, optional DOMString endMark);
>    [Func="nsPerformance::IsEnabled"]
>    void clearMeasures(optional DOMString measureName);
>  };
> +

why this extra line?

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Implement PerformanceObserver v5

(In reply to Andrea Marchesini (:baku) from comment #15)
> @@ +138,5 @@
> > +    aRv.Throw(NS_ERROR_DOM_TYPE_ERR);
> > +    return;
> > +  }
> > +
> > +  mEntryTypes = validEntryTypes;
> 
> what about if I call 'observe' more than once?
> What does the spec say about it? I see 2 options:
> 
> 1. we append the entrytypes
> 2. we overwrite them.
> 
> in both cases I would like to have a mochitest.

I think the spec says 2.

 If the performance observer is already registered with a Performance Timeline,
 update the performance observer options with options.


The previous patch has already a test case;

  observer.observe({entryTypes: ['mark', 'measure']});
  observer.observe({entryTypes: ['mark']});

I added another test case for safety in this patch;

  observer.observe({entryTypes: ['mark']});
  observer.observe({entryTypes: ['measure']});

> ::: dom/base/PerformanceObserverEntryList.cpp
> @@ +45,5 @@
> > +  for (const nsRefPtr<PerformanceEntry>& entry : mEntries) {
> > +    if (aFilter.mInitiatorType.WasPassed() &&
> > +        entry->GetEntryType().EqualsLiteral("resource")) {
> > +      nsRefPtr<PerformanceResourceTiming> resourceEntry =
> > +        static_cast<PerformanceResourceTiming*>(entry.get());
> 
> this seems very fragile. What about if you add a:
> 
> virtual PerformanceResourceTiming* ToResourceTiming() const { return
> nullptr; }
> 
> in PerformanceResourceTiming you do:
> 
> virtual PerformanceResourceTiming* ToResourceTiming() const overwrite {
> return this; }
> 
> here you can then do:
> 
> PerformanceResourceTiming* resouceEntry = entry->ToResourceTiming();
> if (aFilter.mInitiatorType.WasPassed() && resourceEntry) {
>   ...

Thanks! This looks much better! And this code noticed me that initiatorType filtering against !resourceEntry does not work well.  I will post a follow up patch now.

This patch fixed all the other comments.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Fix getEntries filtering for initiatorType

Part 2 has an issue that filtering by initiatorType. 

The issue is that if a PerformanceObserverEntryList does not have any resource timing entry;

 performanceObserverEntryList.getEntries({initiatorType: 'link'});

does not filter at all. 

This patch added a check of this issue for user timing and a couple of checks for resource timing.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Fix getEntries filtering for initiatorType v2

The previous patch does have unnecessary .length there.

+    assert_array_equals(observedEntryList.getEntries({initiatorType: "link"}).length,

This assertion changed from assert_equals() just before posting patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47a385a88efe

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Make PrefEnabledRunnable usable for other preference names v3

Rebased to master.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Thank you, Andrea!

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd56d26a2ec9
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6f22fa46223
https://hg.mozilla.org/integration/mozilla-inbound/rev/12cb95969689
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e1b1e7995c4

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dd56d26a2ec9
https://hg.mozilla.org/mozilla-central/rev/d6f22fa46223
https://hg.mozilla.org/mozilla-central/rev/12cb95969689
https://hg.mozilla.org/mozilla-central/rev/1e1b1e7995c4

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Are we going to ship this?

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Yes.  But now I noticed I forgot to add the preference entry in all.js.  So I think we should add it and enable it in nightly by default.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> Yes.  But now I noticed I forgot to add the preference entry in all.js.  So
> I think we should add it and enable it in nightly by default.

Filed bug 1271487.