entryType enum + consistent use

[igrigorik]

This is a followup to discussion in https://github.com/w3c/performance-timeline/pull/29#issuecomment-119754195.

• defined entryType enum with links to appropriate specs/interfaces
• changed method signatures to use PerformanceEntryType enum
• changed typeFilter -> entryType to be consistent with getEntriesBy*

Preview: https://rawgit.com/w3c/performance-timeline/entryType/index.html#the-performanceentry-interface

[igrigorik]

@plehegar @toddreifsteck @mpb @hiikezoe @bzbarsky ptal.

[bzbarsky]

Seems reasonable.

[mpb]

Should FilterOptions be more explicit about what it's filtering? PerformanceFilterOptions or PerformanceEntryFilterOptions?

[igrigorik]

No, I think we want to reserve the right to add other (i.e. non "type") filters in the future, hence the dictionary. If we only wanted type then we could simplify the whole thing to a sequence.

[mpb]

But is there any chance of "FilterOptions" colliding with other APIs?

[igrigorik]

/me shrugs... I think that's orthogonal to this PR, but if there is concern over collisions we can definitely rename it. s/FilterOptions/PerformanceObserverOptions/ or some such?

[mpb]

sounds good. LGTM.

[igrigorik]

Renamed to PerformanceObserverOptions.

[mpb]

performance.getEntries(options) uses the same dictionary though. My preference would be FilterOptions or PerformanceEntryFilterOptions

[igrigorik]

Ah, good catch - thanks. Fixed.

[mpb]

lgtm

[hiikezoe]

Looks good to me too.

[bzbarsky]

Actually, I do see a possible problem here. Consider what happens when a new entry type is added and a page starts using it for getEntriesByType calls.

If the entry type argument is a string, it will just get back an empty list in browsers that don't support the new type yet. If it's an enum it will get an exception. So this change makes the forward compat story here a lot more complicated.

[bzbarsky]

I suppose https://www.w3.org/Bugs/Public/show_bug.cgi?id=19936 could be useful here if we wanted to avoid the throwing behavior, though.

[igrigorik]

If the entry type argument is a string, it will just get back an empty list in browsers that don't support the new type yet. If it's an enum it will get an exception. So this change makes the forward compat story here a lot more complicated.

Hmm, but this can also be a feature. As in, today we return an empty array but that does not provide any mechanism to distinguish "we've not observed any instances of such event" from "we do not support or log such events". FWIW, I don't think it's completely unreasonable to throw the exception? That said, curious to hear other's thoughts on this.. /cc @toddreifsteck @plehegar

[bzbarsky]

If we never plan to add any new types, then throwing is reasonable.

If we plan to add them, it's not clear to me how the adoption story would work, short of everyone who uses them knowing they have to try-catch around all the uses....

I do agree there is value in querying whether a particular event type is supported for people who want to double-check that; I'm not 100% sure that combining that with the getter is the right approach.

[toddreifsteck]

A developer must either be able to query an enum for existence of entries to avoid making the call OR make the call and not get an exception.

@bzbarsky @igrigorik @plehegar Which of these methods is the most web-compatible?

[igrigorik]

I do agree there is value in querying whether a particular event type is supported for people who want to double-check that; I'm not 100% sure that combining that with the getter is the right approach.

That's fair. A couple of notes while I'm looking at our current logic...

• Current definition of observe method will throw a TypeError if list is empty or it does not contain any valid names -- although, does that even work with enum definition, would we get an error earlier by declaring an unsupported type?
  • We do want the error behavior on empty list because if we don't then you'd end up registering an empty observer that would never fire any events; providing a default opt-in is also not the intended outcome as you'd end up receiving records you don't care about + add monitoring overhead. /cc @mpb
• For getEntriesByType(type), we define that via getEntries({entryType: type}), which defines own processing algorithm.

Practically speaking, if we can't make the enum work I guess we can drop that part of the PR and defer to processing algorithms to invoke whatever behavior we think is appropriate. As a separate discussion, we should have a broader "how do I feature test support for X perf API" thread.

[bzbarsky]

@toddreifsteck Not throwing on unknown entry types is obviously most web-compatible, since that's the current browser behavior.

[igrigorik]

@bzbarsky any thoughts on my comment above?

Current definition of observe method will throw a TypeError if list is empty or it does not contain any valid names -- although, does that even work with enum definition, would we get an error earlier by declaring an unsupported type?

[bzbarsky]

I'm not sure what you're asking...

[igrigorik]

Sorry, let me try again. As a result of this pull we have:

• observe accepts an sequence<PerformanceEntryType>
  • If you look at current processing definition, it's meant to take that sequence and filter out unsupported types (i.e. we accept invalid types but we do throw an error if all types are invalid)

However, I don't think the above works? As in, from reading the IDL spec, I think we'd never get to the filter step since providing an invalid type in the sequence would cause a TypeError?

[bzbarsky]

Yes, if PerformanceEntryType is an enum.

[igrigorik]

Ok, in which case the proposed enum approach is not compatible with either getEntries* or observe() unless https://www.w3.org/Bugs/Public/show_bug.cgi?id=19936 allows us to set a default value. Which is unfortunate, since it'd be nice to have a well defined list of perf entry types, instead of having to hunt for them across various specs.

That said, in terms of moving forward, I think I'll propose dropping the enum for now:

• I can add a note citing valid entry types, but it won't be enforced through IDL
• Both getEntries* and observe can retain their current behavior
  • getEntries* will return an empty array on unknown type
  • observe will filter provided list and ignore unknown types, unless all types are invalid, in which case it'll throw a TypeError

The feature testing question (i.e. "how do I know if this UA supports monitoring X") is something we can split into a separate issue.

Does that sound reasonable?

[bzbarsky]

That sounds fine. And if https://www.w3.org/Bugs/Public/show_bug.cgi?id=19936 ever happens we can reevaluate at that point. @heycam, do you think that's likely to happen or not?

[igrigorik]

@bzbarsky @toddreifsteck @mpb @hiikezoe reverted entryType back to DOMString. PTAL at the updated pull.

[mpb]

LGTM

[hiikezoe]

observe will filter provided list and ignore unknown types, unless all types are invalid, in which case it'll throw a TypeError

I'm wondering this inconsistency is acceptable for developers. i.e.

observe({entryType:['unknown']}); // throws exception observe({entryType:['unknown', 'unsupported', 'invalid', 'mark']}); doesn't throw exception

[igrigorik]

@hiikezoe what's the alternative? Throw the exception if there is any invalid type? That would make it tricky to observe across different implementations which might support different types. Current behavior just lets you know that none of the specified types are supported.

[hiikezoe]

@hiikezoe what's the alternative? Throw the exception if there is any invalid type? That would make it tricky to observe across different implementations which might support different types. Current behavior just lets you know that none of the specified types are supported.

I'd suggest that observe method should not throw exception even if there is any invalid entry type name.

[hiikezoe]

Another thing is not clear to me is that performance.getEntries({entryType: ['render']) throws exception or not once Frame-Timing API is migrated to PerformanceObserver.

I think we should return an empty array in that case. And the case on worker thread, I think observe({entryType:['render']}) on worker should not throw exception. Actually I believe |render| is invalid on worker thread.

[igrigorik]

@hiikezoe if we don't throw an error in the case where all entries are invalid or unsupported you'll end up with an zombie observer object.. which doesn't seem right.

Another thing is not clear to me is that performance.getEntries({entryType: ['render']) throws exception or not once Frame-Timing API is migrated to PerformanceObserver.

Yes. The plan is to migrate away from the global buffer model and encourage use of PerfObserver.

And the case on worker thread, I think observe({entryType:['render']}) on worker should not throw exception. Actually I believe |render| is invalid on worker thread.

Ah, interesting.. haven't thought about the worker case. You're right, that's an invalid event on worker.

[hiikezoe]

if we don't throw an error in the case where all entries are invalid or unsupported you'll end up with an zombie observer object.. which doesn't seem right.

Ah, we should prevent zombie observers. Then we should throw TypeError in the case of frame-timing on worker?

== OFF TOPIC == we can create zombie MutationObserver as well. var observer = new MutationObserver(function() {}); observer.observe(target, { attributes: true, attributeFilter: [{}] }); == OFF TOPIC ==

[igrigorik]

Ah, we should prevent zombie observers. Then we should throw TypeError in the case of frame-timing on worker?

To be consistent, we'd treat frame timing ("renderer", "composite") events as invalid within a Worker, so if there are no other valid event types specified when calling observe then we'd throw an error.

---

Note that this discussion is orthogonal to the pull in this thread. The goal of this pull is to converge on same filter attribute names. @hiikezoe any objections from your end on that? We should move the exception + worker discussion into a separate thread.

[hiikezoe]

@igrigorik no objection now. Thanks.