API review comments for Navigation Error Logging

Hi all! [www-tag in bcc]

The TAG has meant for a while now to take a look at the Navigation Error Logging spec. I couldn't find a bug tracker for Navigation Error Logging, so I thought I'd send our comments to the list. They're overall pretty minor things, and my apologies if any of them have been discussed to death before.

- First the positive feedback: the spec is well-motivated, with a good introduction that explains the problem statement. As a web developer I never even think of this problem, but now I am quite interested in solving it. Also, the privacy and security section is especially great to see.

- NavigationErrorEntry should have a constructor; otherwise it is hard for developers to understand how instances of NavigationErrorEntry exist at all. (The answer you have to explain to them is: there *is* a constructor, but it takes a secret parameter which only the user agent knows the value of, so whenever you as a web developer try to call it, you get a TypeError. That is not a very good answer and should be reserved for objects that really do contain complicated internal secrets, e.g. Window or SubtleCrypto.)

- I was curious why the "name" attribute was called "name" instead of "url"?

- As explained in [1], for URLs the IDL type USVString should be used, instead of DOMString. This impacts NavigationErrorEntry.prototype.name's return type and Performance.prototype.enableNavigationErrorReporting's parameter type.

- Boolean parameters are in general an antipattern (see e.g. [2]), and besides, it is very strange to call `navigator.performance.enableNavigationErrorLogging(false)` to *disable* navigation error logging. Especially given that to disable navigation error *reporting*, you just do `navigator.performance.disableNavigationErrorReporting()`. I think it would be better to just split the method into two methods? There is of course no loss of expressive power, since eg. `n.p.eNEL(myBool)` could be rewritten as `n.p[myBool ? "eNEL" : "dNEL"]()`, which is a fairly familiar pattern from other web APIs. Alternately if there are strong feelings that this kind of Boolean parameter is desired, a better name (drawing from ClassList and its jQuery predecessor) would be "toggle".

- The WebIDL is slightly invalid in that you are not allowed to simply say `Promise myMethod()`; you must give the promise a type. So I would instead say `Promise<sequence<NavigationErrorEntry>> getNavigationErrors()`.

- A minor nit, but I find the typedef sequence <NavigationErrorEntry> NavigationErrorEntryList more obfuscating than helpful; I had to hunt down the definition, whereas I already know what a sequence<T> is.

- The getNavigationErrors method should be sure to do expensive steps "in parallel" [3]

- The "Monotonic Clock" clock section is interesting. I'm not sure I entirely understand it though. How do you expect to test this requirement? How do you expect implementations to meet it?

Thanks!

[1]: https://url.spec.whatwg.org/#url-apis-elsewhere
[2]: http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html
[3]: http://www.w3.org/2001/tag/doc/promises-guide#explicit-async-steps

Received on Wednesday, 7 January 2015 21:50:19 UTC