errorCallback parameter to getPosition() and watchPosition() is nullable in all major browser engines

[cdumez]

errorCallback parameter to getPosition() and watchPosition() is nullable in all major browser engines (tested WebKit, Gecko and Blink). However, it is not marked as nullable in the specification. Changing this behavior in browser engines would be a compatibility risk so I suggest we update the specification instead.

Note that there is already a web-platform-test relying on errorCallback being nullable:
https://w3c-test.org/geolocation-API/PositionOptions.https.html

[reillyeon]

Thanks for pointing this out. My knowledge of WebIDL always feels lacking but it seems invalid to have an optional callback argument which isn't nullable.

[anssiko]

Per whatwg/webidl#774 (comment) nullable optional dictionary is invalid IDL.

The discussion on that issue seems to imply nullable optional callback function is valid IDL?

@bzbarsky is well-versed in WebIDL and can probably advise us on this detail.

[bzbarsky]

The questions of nullability and optionality are completely independent. That is, in general you can have a thing that is neither one, a thing that is nullable but not optional, a thing that is optional but not nullable, or a thing that is both nullable and optional.

Dictionaries are a little weird in that dictionary arguments are required to be optional and forbidden from being nullable (because null is a value that converts to an empty dictionary anyway). But for most other types, the setup above applies, and you just pick the option that represents the behavior you want.

So the questions to answer to decide on nullable/optional bits are:

1. If null is passed, should this throw an exception? If not, the type needs to be nullable, generally.
2. Is omitting the argument entirely OK? If it is, the type should be optional.

In this case, if both watchPosition(() => {}) and watchPosition(() => {}, null) are meant to be acceptable, you want the argument both nullable and optional. If the first should be OK but the second not, you want optional and not nullable. And so forth.

Past that, if it's optional you may want to define a default value, or just define the behavior for the missing-argument case in prose; the former is preferred if the desired behavior for missing-argument case is the same as the behavior for some value of the argument.

Conceretely, the IDL in Gecko for watchPosition looks like this:

  long watchPosition(PositionCallback successCallback,
                     optional PositionErrorCallback? errorCallback = null,
                     optional PositionOptions options = {});

and the IDL in Blink looks like this:

  long watchPosition(
        PositionCallback successCallback,
        // TODO(crbug.com/841185): |errorCallback| is not nullable in the spec.
        optional PositionErrorCallback? errorCallback,
        optional PositionOptions options = {});

If you read https://bugs.chromium.org/p/chromium/issues/detail?id=841185 it says:

Web IDL optional is meant to accept only ES undefined, and Blink follows this rule in general except for callback arguments. Historically Blink has not been throwing a TypeError when ES null is passed to optional non-nullable callback arguments.

which probably explains how the spec ended up in the state it's in: the IDL was copied from Blink (or added by a Blink engineer), and more or less depended on the Blink IDL bug described above. The bug got fixed in their bindings, and the IDL was edited to preserve behavior, but the spec never got updated, apparently.

Now Blink still has a bug in their bindings, as far as I can tell: a nullable optional callback is defaulted to null even if the IDL doesn't have a default value defined. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1151949 on that.

[anssiko]

@bzbarsky thank you for clarifying these details!

@reillyeon can probably help make sure the Blink bindings bugs get triaged.

[reillyeon]

It will forever confuse me that Javascript has two nullish types and so something which is not nullable can still be undefined.

Edit: After reading https://crbug.com/1151949 I see that I am still confused and optional parameters use the special value "missing" instead of "undefined".

[reillyeon]

As for what to do for this bug, pragmatically we should just update the specification to mark the parameter as nullable unless we want to do a compatibility analysis to see how many times the errorCallback parameter is null instead of undefined. Given the agreement between engines I'm inclined to do the former and add a note explaining the historical significance so that people don't copy it for future specifications.

[anssiko]

I'm supportive of @reillyeon's proposal and see @cdumez's thumbs up so I think we have consensus on this issue.

Given related crbugs have active discussion, do you @reillyeon want to take a stab at the spec change when settled with Blink changes?

[reillyeon]

My team is tracking the Blink changes (and web compat impact) on Chromium issue 1153081. I will updated when we've come to a conclusion on whether Blink's IDL can be changed or if the specification should be updated to match what is implemented.

[bzbarsky]

If there is currently interop across browsers, changing the spec, rather than browsers, is probably the right thing to do...