Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add entryTypes method to enumerate type of entries observable via PerformanceObserver #77

Closed
spanicker opened this issue May 23, 2017 · 22 comments
Assignees
Milestone

Comments

@spanicker
Copy link

Currently developers have to "just know" the names of entryTypes for observe:
...observer.observe({entryTypes: ["foobar"]});

Would be nice if there was a method to enumerate them.
(Maybe there is a way and I just don't know)

@igrigorik
Copy link
Member

Would be nice if there was a method to enumerate them.
(Maybe there is a way and I just don't know)

There isn't. However, one can feature detect support pretty easily.. see: #64

@spanicker
Copy link
Author

Right this is not about feature detection but rather about discoverability of what types exist.
So it's a smaller use-case - that's about convenience (although current types can be guessed)

@igrigorik
Copy link
Member

Yep. So far we've said that it's already possible to script on your own and hence the utility of exposing a native API for it is ~low.. Happy to be convinced otherwise. /cc @toddreifsteck @plehegar

@igrigorik
Copy link
Member

A quick proof of concept to test support for various perf interfaces:

perfTypes = {
  "PerformanceResourceTiming": "resource",      // Resource Timing
  "PerformanceNavigationTiming": "navigation",  // Navigation Timing 
  "PerformanceMark": "mark",                    // User Timing
  "PerformanceMeasure": "measure",              // User Timing
  "PerformanceLongTaskTiming": "longtask",      // Long Tasks
  "PerformancePaintTiming": "paint",            // Paint Timing
  "PerformanceServerTiming": "server",          // Server Timing
}

var supportedPerfInterfaces = Object.keys(perfTypes)
  .filter(interface => typeof window[interface] === "function")
  .reduce((acc, key) => acc.concat(perfTypes[key]), []);

console.log(supportedPerfInterfaces)

Note: one gotcha to the above is that availability of interface != it can be subscribed to by PerformanceObserver today. For that we'd probably need UA support.

@jfsiii
Copy link

jfsiii commented May 31, 2017

If N userland scripts all have this snippet, N must be updated when the list changes.

How likely is this list of interfaces to change? How would a developer know the list has changed?

@igrigorik
Copy link
Member

@jfsiii all good questions, but none of them are new as far as polyfills go.. yes interfaces can be updated, as long as you periodically pull in the snippet from some canonical source, you'd be fine.

(That's not argument for not exposing this natively, but just pointing out that these are not new concerns)

@igrigorik igrigorik added this to the Level 2 milestone Jul 21, 2017
@igrigorik igrigorik changed the title Add a method to enumerate available types of entries Add a method to enumerate type of entries observable via PerformanceObserver Jul 21, 2017
@hiikezoe
Copy link

FWIW, mozilla has a bug report[1] that Google Cloud Platform does not work at all because the site obverses 'longtask' but Gecko does not support it yet. Also I did skim the WebKit source, they don't support 'longtask' either, so the Goole Cloud Platform will not work on the WebKit.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1398477

@JosephPecoraro
Copy link

Yep. So far we've said that it's already possible to script on your own and hence the utility of exposing a native API for it is ~low.

With #87 there would be utility for getting a list. But if that goes away (which I believe it will) then we are back to not needing it.

For feature detection, checking for an interface type is a safe and direct solution. I'd argue that is safer and better than checking for a string in a list or providing an explicit isSupported() API. This requires that new PerformanceEntry types get added with each new feature and the same interface not be shared for different entry types (all of which has been the case so far). This could lead to an explosion of PerformanceEntry types, but I don't think that is realistic.

If a developer is interested in using a particular feature of a specific entry type then they will almost certainly be feature detecting it anyways. If that is the case then checking for the interface type is sufficient and natural.


This brings to mind a related point. Given the growing list of entry types, a wildcard, such as "*" may make sense. A developer may want to gather all entries, and analyze their basic info (type and time). Providing a way to register for "all types" would be convenient. However, I think spec authors will probably side with sticking to an explicit list as a performance consideration. Registering for a particular event type may impact page performance, so requiring explicit registration for each type discourages unnecessary resource usage. I don't hold any strong opinion here.

@igrigorik igrigorik self-assigned this Oct 7, 2017
igrigorik added a commit that referenced this issue Oct 7, 2017
@igrigorik igrigorik changed the title Add a method to enumerate type of entries observable via PerformanceObserver Add entryTypes method to enumerate type of entries observable via PerformanceObserver May 7, 2018
@igrigorik
Copy link
Member

Initial proposal and discussion: 11c7e1c

@npm1
Copy link
Contributor

npm1 commented Jul 23, 2018

Are we adding the method to expose supported entryTypes, or shall we close this issue?

@igrigorik
Copy link
Member

We are. Spec work is currently assigned to me and unfortunately I'm the bottleneck.. If someone else is willing or interested in driving that, I'd be happy to review+support it.

npm1 added a commit to npm1/performance-timeline that referenced this issue Oct 5, 2018
Add supportedEntryTypes and include an example of how it would be used. Solves w3c#77
npm1 added a commit to npm1/performance-timeline that referenced this issue Oct 5, 2018
Add supportedEntryTypes and include an example of how it would be used. Solves w3c#77
npm1 added a commit to npm1/performance-timeline that referenced this issue Oct 5, 2018
Add supportedEntryTypes and include an example of how it would be used. Solves w3c#77
@bakulf
Copy link

bakulf commented Oct 25, 2018

I have 2 different approaches to suggest.

Can we obtain basically the same result changing 3.3.1.2:
"Remove all unsupported types from entry types. The user agent SHOULD notify developers if entry types is modified. For example, a console warning listing removed types might be appropriate."
to:
"Throw an exception if entry types contain an unsupported type." ?

Second approach:

interface PerformanceObserver {
 sequence<DOMString> observe(PerformanceObserverInit options);
}

where observe() returns the list of observed entry types.

@igrigorik @npm1

@npm1
Copy link
Contributor

npm1 commented Oct 25, 2018

Throwing an exception could break current usage since currently there should be no exceptions when observing unsupported types.

And returning the list of observed entry types is an interesting option, but what is the benefit of this option over the current proposal? I think a con is that it will only allow discovering entries when calling observe() (ideally we want to know about supported entries before this). Another con is that it's a bit harder to get a full list of all supported entry types via this approach.

@npm1
Copy link
Contributor

npm1 commented Dec 3, 2018

This issue can be closed since supportedEntryTypes has been added. I don't have issue-closing power.

@yoavweiss
Copy link
Contributor

Closing

@bzbarsky
Copy link

bzbarsky commented Mar 27, 2019

@igrigorik @yoavweiss @spanicker May I ask why this added an attribute that is defined to return a new value on each get? That is a well-known antipattern, to the point where Web IDL goes out of its way to prevent it (by disallowing sequence as the type of an attribute). Did this change end up using FrozenArray just to work around that antipattern prevention? :(

The right thing to do if you want to return a new value on each get is to define a method, not an attribute.

@npm1
Copy link
Contributor

npm1 commented Mar 27, 2019

No, it did not use FrozenArray just to work around it. I personally was just not aware that this was an anti-pattern, but given what this attribute is meant for, we can make it not return a new copy each time.

@bzbarsky
Copy link

I'm a bit confused now. The whole point of FrozenArray is to use it for things that keep returning the same array (so it has to be frozen, so consumers can't add stuff to it). It should be really hard to use FrozenArray for a "return a new thing each time" situation... If it's not in Chrome, could we get issues filed on that, please, by someone more familiar than I with the Chrome bindings code?

@npm1
Copy link
Contributor

npm1 commented Mar 27, 2019

No, that's actually not the whole point. It is frozen to a consumer. This does not in any way imply that the attribute getter must always return that same FrozenArray? If an object changes and needs to return different values, it should be able to create and return a new FrozenArray.

@bzbarsky
Copy link

bzbarsky commented Mar 27, 2019

No, that's actually not the whole point. It is frozen to a consumer.

Right, but the point of freezing it to the consumer is to be able to keep returning the same value over and over. As you say, if there is an actual change to the state of the world (e.g. an assignment to an attribute setter, for a non-readonly attribute), the value returned can change. But the default behavior absent such a world-state change for attributes returning FrozenArray should be to keep returning the same value, and it should take effort on the part of an implementor to change the value. Having it accidentally end up a different value each time should not be a thing that can happen, basically; you should have to go out of your way to explicitly change the value in response to some world-state change.

@npm1
Copy link
Contributor

npm1 commented Mar 27, 2019

Agree that we should be returning the same array whenever possible. I'm pointing this out because I'm not sure how Chrome bindings would enforce this 'no change in the world means you should return the same array'.

@bzbarsky
Copy link

bzbarsky commented Mar 27, 2019

I'm not actually asking for that. But even that is not that hard. The Firefox bindings do it, for non-static attributes. You do it by caching the FrozenArray object in the bindings layer itself, and not even calling into the spec-defined steps if the cache is filled. Changes that should lead to the value changing need to empty out the cache, so the next get calls back into the spec-defined steps.

But I was specifically talking about the fact that it should be clear in the spec-defined steps that a new object is being created on every get, and that if that's not clear in Chrome that's a problem. Doing that is quite simple For example, it could be done by just requiring that people invoke https://heycam.github.io/webidl/#dfn-create-frozen-array manually whenever they want to create one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants