Add entryType-specific parameters for observe()

[npm1]

This PR adds a PerformanceEntryObserverOptions dictionary where entryType-specific parameters are used. The buffered flag is moved to this dictionary. Some <i> tags are changed to <var>. Observe() is updated to consider dictionaries. Queueing an entry is modified to allow passing in a closure. This is in preparation for adding thresholds to certain entry types. Solves #103
@igrigorik @tdresser @toddreifsteck PTAL

---

Preview | Diff

[tdresser]

What is "this dictionary"?

[npm1]

This is in the section titled 'PerformanceObserverInit dictionary' so I think it's clear enough? To reduce ambiguity I could replace that with 'the PerformanceObserverInit dictionary'.

[tdresser]

I'm not following the grammar of this sentence.

[npm1]

Sorry, which? The one starting with Specifications, or with 'The queue'

[npm1]

Definitely The queue sentence is incorrect. The first word should be 'To'.

[tdresser]

For cloning steps, I thought there was an inheritance like approach taken, where specs could override the default. That feels a bit slicker than what we've got here.

[npm1]

Hm no, I don't think there is a default. The closure approach has a default, the cloning steps approach says that a specification may specify additional steps that should be run at a certain point in time.

[tdresser]

You're right!

[tdresser]

Thanks, this LGTM.

We should get other folks to take a look though.

[tdresser]

You're right!

[npm1]

Ping Ilya and Todd (or who else could be a good reviewer for this?)

[igrigorik]

It's probably worth spelling out explicitly what's going on here. Specially, that providing names to entryTypes enables monitoring for said type, initialized with some default settings (e.g. buffered = false). Alternatively, you can specify the entryType name as a key and provide a dictionary with custom settings — e.g. resource : {buffered: true}.

On that note, @toddreifsteck @yoavweiss would love to get your sanity check and review on this pattern.

[igrigorik]

Not sure I grok what this is doing. What is an example of "each dictionary of name entryType"? :-)

I think you're enumerating the keys in options and adding them to entryTypes? As an aside, can we actually do this without first cloning the options? As in, I don't think we should be modifying the passed in object.

[npm1]

Yes, maybe enumarating keys in options is clearer? Or enumerating keys whose values are dictionaries (to exclude entryTypes itself for example)?

[igrigorik]

Per later comment, I don't think we should be modifying this object in place.. But if we're cloning it first, we could: pluck entryTypes out first and <do-stuff>, then enumerate the remaining keys and <do-more-stuff>, arriving at a consistent internal object that we can use later in the algorithm?

[npm1]

Yes, so I propose adding something like "Each PerformanceObserver gets an associated |entry types| object". Then here we would modify observer's |entry types| instead of some variable of option's entryTypes array. Does that sound reasonable?

[igrigorik]

We don't do this today for existing types. If we follow this recommendation, than we should do so.

That said, we've also tried to explicitly avoid enumerating "valid types" in Performance Timeline, as that makes a hard dependency for the perf timeline to be revved whenever we add a new type. Per discussion in #77, the idea is to have other specs call into Perf Timeline to register available types. The logic here should then enumerate the dictionary members, compare against registered + known types and proceed accordingly. This also means that we ignore any unknown types in this algorithm.

WDYT, does that make sense?

[npm1]

I'm not forcing PerformanceTimeline to be modified whenever a new entry type is added. I was thinking the new spec could do something along the lines of 'partial interface' to add the new dictionary corresponding to its entry type. We already "remove all unsupported types", which is a bit hand-wavy but I think it's clear what it means. If we want to make it a bit more formal, we can go with your registration approach, I just don't know if it adds much clarity or if it makes speccing new entry types easier.

[igrigorik]

Ah, I'm with you now. I like that, that's much cleaner than I what I had in mind before. The one bit I'd change in this proposal is, instead of extending the PerformanceObserverInit we wanted + need to introduce performance.entryTypes and that's the interface the various specs should be extending. In turn, we can then filter passed options to PerformanceObserverInit against said interface.

WDYT, does that make sense? If you're on board with that.. the changes are coupled, and we would need to land the performance.entryTypes proposal first, and then rebase this against that.

[npm1]

Sorry, I don't follow what you're suggesting here... performance.entryTypes will only help in knowing which are the entry types that are supported globally (implemented by the browser), right? So we still need a way to specify what filters can be used for each entry type, unless we want to support the same set of filters for all entry types? Even if we have the same set of filters on all entry types, we still need to augment PerformanceObserverInit with the dictionaries per entry type (I don't think we want to allow it to contain dictionaries like 'longtask' and 'mark' without specifying them anywhere explicitly). The only alternative I can think of that avoids this would be to have something like this:

dictionary PerformanceObserverInit {
  required sequence<DOMString> entryTypes;
  PerformanceEntryObserverOptions filters;
}

But this would force an observer to apply the same set of filters to all of the observed entry types that it is observing. This is kind of what we're trying to avoid here, since ideally we'd like entryType-specific parameters. If we don't have that, we might as well just keep adding the filters to PerformanceObserverInit directly. WDYT?

[igrigorik]

I believe our plan was to have this on by default for all types prior to onload, and no buffering afterwards. If that's the case, I believe we wouldn't need this — correct?

[npm1]

This is about the queue algorithm, not about buffering. An example of where we need this is LongTasks thresholds: we need the spec to provide an algorithm to determine if an entry should be added to an observer (look at the entry's duration, compare with the observer's threshold).

[igrigorik]

This step should be with respect to before or after onload, not the passed in flag.

For context, we ruled out the flag approach because it would, once again, require updating every upstream spec to have this on by default.. and I can't think of cases where we'd want this to be off by default for before-onload.

[npm1]

Sure, but I haven't added this flag, it's currently in the spec. How about we do a separate PR for this independent change?

[igrigorik]

Why are we doing this here again, if we're modifying entryTypes and adding names to it earlier? Could we preprocess this earlier (either unroll entryTypes into the array with default dicts, or the reverse) and reuse that here?

[npm1]

We did this in observe() to the |options| parameter, and now that is gone. I would propose having an internal variable within PerformanceObserver that consists of a list of supported entry types. The list could be populated during observe() and then it could be used here, in the queue an entry algorithm. This is probably how implementations do it anyways. WDYT?

[npm1]

I'm thinking we may want to remove the 'required' from |entryTypes| as well as you could specify what you want to observe using the other dictionaries.

[npm1]

By the way I've added the 'supported entry types' and addressed all comments.
@yoavweiss @toddreifsteck any thoughts on the pattern used on observe() here?

[igrigorik]

s/supported entry types/observed entry types/?

I think we should reserve supported entry types for the global "which entry types does performance timeline support"?

[npm1]

Done

[yoavweiss]

Where's the resource-timing part? It's no longer observed, is it?

[npm1]

You mean in the description, or in the code? In the description, we have at the end "Also subscribe to and retrieve buffered events from Resource-Timing." In the code, per this PR the existence of a dictionary 'resource' implies that this PerformanceObserver observes that entry type.

[yoavweiss]

s/dictonary/dictionary/

[yoavweiss]

I think it'd be better to explicitly state what the options variable is a line above that.
e.g. "Let options be the input PerformanceObserverInit object" or something along those lines

[yoavweiss]

Also, I think you meant "observed entry types" to be a var, not a link

[npm1]

I've added what observer() receives. I really do mean a link to the associated concept of the PerformanceObserver, not a local var. So is correct in this context.

[npm1]

*so <a> is correct

[yoavweiss]

s/dictonary/dictionary/

[npm1]

Done

[yoavweiss]

It's not clear to me what this does and why. Can you explain?

[npm1]

What I mean is: add the entry types that are listed as dictionaries in 'options' to the set of entry types that are observed by this observer. In the example in the code, we had:

observer.observe({
  entryTypes: ["mark", "measure"],
  resource : {buffered: true}
});

In this case, there is a resource dictionary. So this step means: since the key resource is present, add 'resource' to the list of observed entry types.

I think @igrigorik wanted your opinion on whether this is sane or not. If we think this is too ugly, we can remove this part, so that all entry types to be observed must still be listed in the entryTypes array.

[yoavweiss]

Do we define somewhere what are the supported types? Can we point to such a definition?
Also, can you rewrite that as an iteration over the sequence?

[npm1]

We do not define this (this is browser-dependent). If/when we expose performance.entryTypes then we can point to that, but for now I think we don't have a definition. Rewrote as an iteration (although note that this wording was already present before this PR).

[yoavweiss]

options is a PerformanceObserverInit, right? Or is it PerformanceEntryObserverOptions?

[npm1]

Yes, but buffered would be inside the PerformanceEntryObserverOptions corresponding to that entry type (if it exists), which is itself inside the PerformanceObserverInit options. I moved this flag so that it can be an entryType-specific parameter.

[yoavweiss]

It's not clear what you mean by "the name of the new entryType". Can you be more explicit?

[npm1]

What I mean is that PerformanceObserverInit needs to be augmented for example if we were adding EventTiming with entries that have entryType 'event':

partial dictionary PerformanceObserverInit {
   optional PerformanceEntryObserverOptions event;
}

I reworded that last part, is it better now?

[rniwa]

I don't think this approach fits *Observer pattern well. See my comment in #103 (comment).

[npm1]

I'm open to changing the shape of the API but we probably want something that is backwards compatible. Changing observe() to only allow specifying one type per observe() call isn't.

[rniwa]

We can support both. Either specify a list of types to observe, or specify a type and it's options.

[npm1]

I'm closing this PR and later sending a new one based on the discussion in #103