Closed Bug 1165796 Opened 5 years ago Closed 5 years ago

Implement Performance Observer

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(4 files, 7 obsolete files)

808 bytes, patch
baku
: review+
Details | Diff | Splinter Review
41.66 KB, patch
hiro
: review+
Details | Diff | Splinter Review
4.56 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.88 KB, patch
hiro
: review+
Details | Diff | Splinter Review
No description provided.
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.
QA Contact: hiikezoe
Depends on: 1178172
Assignee: nobody → hiikezoe
QA Contact: hiikezoe
Depends on: 1178642
No longer depends on: 1178642
To implement a new method named nsPerformance::IsObserverEnabled we need to modify current PrefEnabledRunnable to be used for other preference names.
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.
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).
Attachment #8627548 - Attachment is obsolete: true
Attachment #8628081 - Flags: review?(amarchesini)
Attachment #8627543 - Flags: review?(amarchesini)
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 ?
Attachment #8627543 - Flags: review?(amarchesini) → review+
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?
Attachment #8628081 - Flags: review?(amarchesini) → review-
Addressed all comments.
Carrying over review+.
Attachment #8627543 - Attachment is obsolete: true
Attachment #8628610 - Flags: review+
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.
Attachment #8628081 - Attachment is obsolete: true
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?
Flags: needinfo?(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...
Flags: needinfo?(bzbarsky)
Thanks, I did misunderstand the spec. I thought getEntries in the invalid attribute case must return an empty array.
Depends on: 1184845
No longer depends on: 1184845
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.
Attachment #8628613 - Attachment is obsolete: true
Attachment #8647465 - Flags: review?(amarchesini)
Unfortunately there is another unified build error.
Attachment #8647466 - Flags: review?(amarchesini)
Attachment #8647466 - Flags: review?(amarchesini) → review+
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?
Attachment #8647465 - Flags: review?(amarchesini) → review+
(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.
Attachment #8647465 - Attachment is obsolete: true
Attachment #8647807 - Flags: review+
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.
Attachment #8647813 - Flags: review?(amarchesini)
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
Attachment #8647813 - Attachment is obsolete: true
Attachment #8647813 - Flags: review?(amarchesini)
Attachment #8647817 - Flags: review?(amarchesini)
Attachment #8647817 - Flags: review?(amarchesini) → review+
Rebased to master.
Attachment #8628610 - Attachment is obsolete: true
Attachment #8648288 - Flags: review+
Thank you, Andrea!
Keywords: checkin-needed
Depends on: 1195700
Are we going to ship this?
Flags: needinfo?(hiikezoe)
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.
Flags: needinfo?(hiikezoe)
(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.
Blocks: 1271487
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.