PerformanceObserver.observe() needs 'target'

[mpb]

PerformanceObserver.observe(options) should really be PerformanceObserver.observe(target, options) , where 'target' is a Performance.
The API is a little vague otherwise.
Not sure if this means we want to introduce observer.disconnect(target) or just keep observer.disconnect() as a "disconnect all" function.

[igrigorik]

Hmm, why? I thought we intentionally omitted target because its always the same.

[mpb]

It's not always the same.. you can access window.performance for multiple (same domain) iframes. You need a way to distinguish which ones.

[igrigorik]

Ah, right.. I see. My guess is that the most common use case will be to observe window.performance.. makes me wonder if we could make this an optional attribute and default to that?

[mpb]

I'm fine with that. How do you specify it that in IDL?

[igrigorik]

I'm fine with that. How do you specify it that in IDL?

Good question, not sure. @bzbarsky is this doable?

[bzbarsky]

You have a few options, depending on the shape of the API you want:

Option 1:

void observe(PerformanceObserverInit options);
void observe(Performance target, PerformanceObserverInit options);

Option 2:

void observe(PerformanceObserverInit options, optional Performance target);

Option 3:

Add a member to PerformanceObserverInit that allows specifying a Performance to observe.

The first two options assume that the typeFilter member of PerformanceObserverInit will get marked required and step 1 of the processing model for observe() will therefore go away. If that's not done, then you need to throw in optional before all the trailing PerformanceObserverInit arguments, of course, but I don't see why you wouldn't mark it required.

[mpb]

Hm, any of those would work for me. @bzbarsky - any preference?

Also, just as a side-note, is it reasonable for

var observer  = new PerformanceObserver(callback);
o.observe({filterType:["resource"]});

to be limited to window.performance's events? Should it be explicit about what the target is?
Or maybe the API should be more like:

var observer = window.performance.createObserver(callback);

so that it's clear the scope.
Or just make target mandatory.

Thoughts?

[bzbarsky]

I think my preference would be to just add a dictionary member in this situation.

For the rest, I think having which things the observer observes be magic is pretty confusing; having those be explicit either via a constructor argument or factory function would make more sense to me...

[mpb]

Ok, so in the interest of "no magic" does
A:

Constructor(PerformanceObserverCallback callback)
void observe(Performance target, PerformanceObserverInit options);

B:

Constructor(Performance target, PerformanceObserverCallback callback)
void observe(PerformanceObserverInit options);

or C:

Constructor(PerformanceObserverCallback callback)
void observe(PerformanceObserverInit options);
dictionary PerformanceObserverInit {
  required sequence<DOMString> typeFilter;
  required Performance target;
};

sound better?

• one point to note: as spec'ed, both A and C let you create one observer that can listen to multiple performance timelines. Not sure if this is a good thing, bad thing or neither.
  I tested it out in my implementation in Chrome and it worked ok... the uri in 'name' let me disambiguate.

[bzbarsky]

Well, the first question is why there is this distinction between constructor and observe method at all. Can a single PerformanceObserver observe more than once? If so, do we want to allow it to observe multiple different Performance instances? That's basically your last question, I guess.

[mpb]

Yes..

So the way it's currently set up, you create one observer and it can observe multiple targets, but only have one set of filter criteria per target. And it can start and stop observations on those targets independently.

The other way to set it up would be create an observer that is hardcoded to a particular filter criteria and target, and have start & stop method that takes no arguments.

The only reason we picked the current direction is to keep it more in line with mutation observers (which function this way) and people more or less understand now. We don't yet have a use-case argument one way or the other.

[mpb]

(Currently, if you call observe with the same target, it will just adjust the filter criteria on that target to be the new ones you supplied.)

[igrigorik]

I can't think of any reasons why we would want to disallow observing multiple Performance instances.

(c) with target as part of the dictionary seems reasonable. Can we also make it default to window.performance?

[igrigorik]

one point to note: as spec'ed, both A and C let you create one observer that can listen to multiple performance timelines. Not sure if this is a good thing, bad thing or neither. I tested it out in my implementation in Chrome and it worked ok... the uri in 'name' let me disambiguate.

Actually, wait.. You're relying on the fact that "name" is reporting a URI, which is not true for all event types.

[mpb]

Well, it's true for resource, render and composite events. For user timing events it will be user-specified so I think that's fine as well.
Is there any event time this kind of thing would cause problems with?

[igrigorik]

• for Resource Timing & Server Timing, name is the resolved URL of the resource being fetched; there is no way to tell where it came from.
• for User Timing, name may clash between and there is no way disambiguate - e.g. I install a widget which emits same common names, and once again, there is no way to disambiguate them.

That said, I'm not sure if this is an issue, since this would be opt-in behavior.

Constructor(PerformanceObserverCallback callback)
void observe(PerformanceObserverInit options);
dictionary PerformanceObserverInit {
  required sequence<DOMString> typeFilter;
  optional Performance target;
};

Make target optional, and as part of processing define it as window.performance if left unspecified? I'm not sure if IDL allows us to define the default as "performance interface of current context".

[mpb]

"required" doesn't seem to be a keyword in http://www.w3.org/TR/WebIDL/
Also, is the preferred syntax

dictionary PerformanceObserverInit {
  required sequence<DOMString> typeFilter
  optional Performance target
}

or

dictionary PerformanceObserverInit {
  required sequence<DOMString> typeFilter
  Performance? target
}

?
Thanks!

[mpb]

Does this look any better?

[igrigorik]

This is a slight tangent, but since we're iterating on the dictionary, a couple of thoughts:

Instead of defining typeFilter as sequence of arbitrary strings, perhaps we can define an enum that contains valid types? This would help clarify what is supported both to web developers and implementers. E.g...

enum PerformanceEntryType {
    "navigation",
    "resource",   
    "mark",        
    "measure",
    "renderer",
    "composite",
    "server"
};

dictionary PerformanceObserverInit {
  required sequence<PerformanceEntryType> typeFilter
  ...
}

To address #30, we can then define processing as "typeFilter is a required field that must be a non-empty sequence". The benefit here is that this would nudge developers to be specific about what they want to observe and explicitly opt-in into each event.. which is something we want to encourage to avoid unnecessary overhead.

Thoughts?

/cc @hiikezoe @bzbarsky

[esprehn]

I don't think we should add this target argument, it's too easy for a developer to leak the whole context across frames and it's very strange to create an observer in one context and observe the entries in another. For example the entries in that other context will have properties with the wrong prototypes unexpectedly, (ex. Arrays will have a different Array.prototype), and the PerformanceEntry objects themselves will be of a different type.

[hiikezoe]

// create an observer var observer = window.performance.createObserver(callback); // then start monitoring observer.observe({typeFilter:["server","render"]);

I have no objection to this.

[eliperelman]

Are there any strong objections to keeping it scoped to the Performance object of the context in which the observer is created?

I much rather would go the route of having the context defaulted to the current context (however you define that in IDL), and provide the API to specify a different Performance context. The use case for this being in Firefox OS, we can observe in our System/parent process for performance entries generated in child processes, e.g. individual applications. This would give us the ability to observe without needing to create an observer in each individual application, especially for ones which we do not control.

[igrigorik]

I much rather would go the route of having the context defaulted to the current context (however you define that in IDL), and provide the API to specify a different Performance context. The use case for this being in Firefox OS, we can observe in our System/parent process for performance entries generated in child processes, e.g. individual applications.

As @esprehn noted earlier (#29 (comment)) this runs the risk of opening up some weird behaviors. As one further example, the timestamps will have different time origins and there is no way to tell which origin each event belongs to. FWIW, you can still get the behavior you want, you'll just need to create separate observers (~process.performance.createObserver) and then provide own logic for how/if you want to merge those and/or translate the timestamps.

This would give us the ability to observe without needing to create an observer in each individual application, especially for ones which we do not control.

This last bit sounds like a security hole? :)

[eliperelman]

This last bit sounds like a security hole? :)

Heh, possibly, I hadn't really thought it through, was more concerned with the API.

[mpb]

Anything you could have done with

var observer = new PerformanceObserver(callback);
observer.observe({target:otherwindow.performance, typeFilter:["server"]});

you can do with

var observer = otherwindow.performance.createObserver(callback);
observer.observe({typeFilter:["server"]});

It sounds like we have a consensus, more or less. Right?
Let me update the patch, and see how it looks.

[mpb]

Ok, I sync'ed with the w3c/performance-timeline gh-pages and updated the API from new PerformanceObserver to window.performance.createPerformanceObserver.
PTAL.

[mpb]

The merge history on this pull request is getting confusing. Moving request to #35, using the new interface. Thanks!

[igrigorik]

Resolved via #35, closing.

[esprehn]

Sorry I was on vacation, I disagree with bz that:

"That's weird magic-at-a-distance that I personally think we should try to avoid in API design."

That's how new Text() gets an ownerDocument, how new EventSource() finds an origin, how new Notification() works, how new Worker() gets an origin... most of the web platform is based around the idea of looking up objects in the current context.

Unless we plan to never add another object that looks up the document on the current window this shouldn't be any different about looking up the current performance instance.

[domenic]

Strong -1 for changing from new PerformanceObserver to window.performance.createObserver.

PerformanceObserver should lookup the |performance| object in the context on which it's defined.

That's weird magic-at-a-distance that I personally think we should try to avoid in API design.

It's no more magical than new Text() looking up the document object on the context in which its defined (for ownerDocument), or many other such things in the DOM hierarchy.

The way I think about it is that if we ever did want to fully expose what's going on here, we'd add a document or performance or whatever parameter, and have it default to the one determined via the context.