Re: API review comments for Navigation Error Logging

Hi Domenic. You're cc'ed on the other thread, but for the public record...
Please take a look at:
http://lists.w3.org/Archives/Public/public-web-perf/2015Jan/0021.html - new
proposal that, I believe, should address all of the above points.

ig

On Wed, Jan 7, 2015 at 1:49 PM, Domenic Denicola <d@domenic.me> wrote:

> 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
>
>
>