- From: Andrei Popescu <andreip@google.com>
- Date: Tue, 25 Aug 2009 15:43:26 +0100
- To: public-geolocation@w3.org
Hi Cameron,
Many thanks for the comments! Please see my responses inline:
On Fri, Jul 10, 2009 at 3:19 AM, Cameron McCormack<cam@mcc.id.au> wrote:
> Across the whole document
> -------------------------
>
> s/User Agent/user agent/g, since according to the W3C Manual of Style,
> it should be written in lowercase.
>
> http://www.w3.org/2001/06/manual/#Terms
>
Fixed.
>
>
> 2 Introduction
> --------------
>
> The following code extract illustrates how to obtain basic location
> information:
>
> s/extract/extracts/, since there are multiple examples following.
>
Fixed.
>
>
> // By using a 'timeout' of 0 milliseconds, if there is
> // no suitable cached position available, the User Agent
> // will immediately invoke the error callback with code
> // TIMEOUT and will not initiate a new position
> // acquisition process.
>
> The comment say that the UA will immediately invoke the error callback,
> but the description of getCurrentPosition() in section 5.1 says:
>
> When called, it must immediately return and then asynchronously
> acquire a new Position object.
>
> So these seem to be in conflict. It should be made clear in the comment
> whether the error callback is invoked before or after
> getCurrentPosition() returns. The same applies to the comment in the
> following example.
>
>
> navigator.geolocation.getCurrentPosition(successCallback,
> errorCallback,
> {maximumAge:Infinity, timeout:0});
>
Ok, I've rewritten the description of the API methods, hopefully
making it more clear. There aren't any changes in functionality or
behavior, it's just making things explicit for the sake of clarity. I
think this refactoring addresses the issues you mentioned above.
Please have another look.
> The type of the maximumAge attribute on the PositionOptions interface is
> long. According to Web IDL, Infinity will be converted to an IDL long
> of value 0.
>
> http://dev.w3.org/2006/webapi/WebIDL/#es-long
>
> If you want to allow Infinity as a value, then you’ll need to either
> make maximumAge be of type float or any (and if any, you’d need to
> define behaviour when any other type is given). Alternatively, you
> could recommend to pass in the maximum signed 32-bit value to
> maximumAge, 2**31-1.
>
As per your later comment, I've left this as-is.
>
>
> 4.3 Additional implementation considerations
> --------------------------------------------
>
> However, in designing these measures, implementers are advised to
> enable user awareness of location sharing, and to provide easy access
> to interfaces that enable revocation of permissions.
>
> s/implementers/implementors/.
>
Fixed.
>
>
> 5.1 Geolocation interface
> -------------------------
>
> Objects implementing the Navigator interface (e.g. the
> window.navigator object) must also implement the NavigatorGeolocation
> interface [NAVIGATOR].
>
> The current editor’s draft of Web IDL supports asserting this in the IDL
> itself with this statement:
>
> Navigator implements NavigatorGeolocation;
>
Added.
>
> The indenting of the second block of IDL in 5.1 is a bit strange. (The
> closing brace of the Geolocation interface is not in the first column,
> and the following two interfaces aren’t in the first column either.)
>
>
Fixed.
>
> I’ll also point out that in the current Web IDL editor’s draft a number
> of changes to IDL syntax have been made. These haven’t been decided on
> yet, and probably won’t be until we get to Last Call, but it might be
> useful for you to know that optional parameters will probably be
> supported using real IDL syntax and not with an [Optional] extended
> attribute. So with the new syntax, the getCurrentPosition() operation
> on the Geolocation interface would be rewritten as:
>
> void getCurrentPosition(in PositionCallback successCallback,
> in optional PositionErrorCallback errorCallback,
> in optional PositionOptions options);
>
>
>
> It would be good to wrap those operations, since the lines are long and
> have white-space:nowrap applied to them currently.
>
Done.
>
>
> void clearWatch(in int watchId);
>
> s/int/long/.
>
Fixed.
>
>
> If successful, this method must invoke its associated successCallback
> argument with a Position object as an argument.
>
> I would say s/with a/with that/, since you want to return the object
> that was just acquired.
>
Fixed.
> Also, I think you should say that it invokes the handleEvent() operation
> of the successCallback object that was passed in, to be language binding
> neutral here. That, or explain somewhere that when it’s written to
> invoke one of these callback objects that it means to invoke the
> handleEvent() operation on the object.
>
>
Added.
>
> 5.2 PositionOptions interface
> -----------------------------
>
> PositionOptions objects are regular ECMAScript objects that have the
> following properties:
>
> They are only ECMAScript objects in the ECMAScript binding. I would
> instead say something like:
>
> In ECMAScript, PositionOptions objects are represented using regular
> native objects with optional properties named "enableHighAccuracy",
> "timeout" and "maximumAge" on them.
>
> and put it below the IDL.
>
Done.
>
>
> attribute long timeout;
> attribute long maximumAge;
>
> Should these be unsigned longs instead? Does it make sense to use
> negative values for these attributes? If not changed, you should define
> what happens when negative values are specified for these.
>
I think they should be unsigned longs. But looking at
window.setTimeout, that seems to take a long, not an unsigned long. If
I remember correctly, I think we wanted our timeout to be the same as
window.setTimeout. In any case, I think it's now clear what happens if
a negative value is specified.
>
>
> enableHighAccuracy, timeout and maximumAge attributes are all
> optional: when creating a PositionOptions object, the developer may
> specify any of these attributes.
>
> It doesn’t make sense to say that the attributes (at the IDL level) are
> optional. An object that implements an interface must have those
> attributes. But in ECMAScript, native objects that implement an
> interface needn’t have properties to represent those attributes, since
> all that happens is that a [[Get]] on the object is performed with the
> relevant property name.
>
Yes, that was a comment aimed at ECMAScript developers. It wasn't
explanatory note for the IDL. Clarified.
>
>
> The user might also deny this capability, or the device might not be
> able to provide more accurate results than if the flag wasn't
> specified.
>
> This implies that omitting the property on an ECMAScript object
> corresponds to enableHighAccuracy = false, which certainly is the case
> because Web IDL specifies that the undefined value (which is what would
> be returned from positionOptionsObject.enableHighAccuracy) converts to
> the boolean false, but you may want to point this out explicitly to be
> clear.
>
Done. I think we generally had a problem with default values being
vaguely (or not at all) specified. E.g. we hit this in
https://bugs.webkit.org/show_bug.cgi?id=27254
>
>
> If the implementation is unable to successfully acquire a new Position
> before the given timeout elapses, and no other errors have occurred in
> this interval, then the corresponding errorCallback must be invoked
> with a PositionError object whose code attribute is set to TIMEOUT.
>
> Make “code” link to the attribute definition.
>
Made.
>
>
> Is the default behaviour of converting undefined to 0 appropriate for
> the timeout attribute? You should point out this default behaviour for
> when the timeout property isn’t specified on the ECMAScript object.
>
Done.
>
>
> Note: Before executing a call to getCurrentPosition() or
> watchPosition(), an implementation may first need to acquire the
> user's permission to disclose location information to the current
> application origin. In such a case, the timeout interval starts
> immediately after the user's permission has been successfully
> acquired.
>
> This is a note, so it’s non-normative, but it makes requirements of the
> timeout interval. I think this should be normative.
>
Done.
>
> In case of a getCurrentPosition() call, the errorCallback would be
> invoked exactly once.
>
> s/exactly/at most/, IIUC.
>
Fixed.
>
> In case of a watchPosition(), the errorCallback could be invoked
> repeatedly: the first timeout is relative to the moment
> watchPosition() was called, …
>
> I guess it should be relative to the moment watchPosition() was called
> or the moment the user’s permission was obtained, if that was necessary.
>
>
That's right, done.
>
> If maximumAge is not specified or set to 0, the implementation must
> immediately attempt to acquire a new position object.
>
> At the IDL level, an attribute can’t be not specified. It’s the
> ECMAScript property that can be omitted. It would be more accurate to
> state something like:
>
> If maximumAge is set to 0, the implementation must immediately attempt
> to acquire a new position object. Note that the maximumAge attribute
> will default to 0 if an ECMAScript object without a "maximumAge"
> property is used.
>
Done.
>
>
> Setting the maximumAge to Infinity will force the implementation to
> return a cached position regardless of its age.
>
> As mentioned above, ∞ isn’t a value of the IDL long type.
>
>
>
> 5.3 Position interface
> ----------------------
>
> This version of the specification allows one attribute of type
> Coordinates and a timestamp.
>
> “timestamp” is underlined in blue for some reason.
>
Well spotted :) Fixed.
>
>
> The altitude attribute denotes the height of the position, specified
> in meters above the [WGS84] ellipsoid. If the implementation cannot
> provide altitude information, the value of this attribute must be
> null.
>
> null isn’t a value of the double type. If you want to allow null, then
> you need to have a type that supports double values or the null value.
> In the most recent WD of Web IDL, such a type is defined using the
> valuetype syntax:
>
> valuetype OptionalDouble double;
>
> Then declare the attribute to be of this OptionalDouble type.
>
> See:
>
> http://www.w3.org/TR/WebIDL/#idl-valuetypes
>
> In recent editor’s drafts, the syntax for this has changed to be a bit
> simpler. There is now a concept of a nullable type (effectively the
> same as the boxed valuetypes mentioned above), specified by placing a
> “?” after a type name. So with this new syntax, you would write:
>
> readonly attribute double? altitude;
>
> But as with the changes to the optionl argument syntax, this hasn’t been
> agreed upon yet.
>
Well, I went ahead and used it anyway. I'll try to keep a closer look
at WebIDL and adjust our spec accordingly if things change.
> Note that if you wanted to stay within the range of the double type, you
> could define the attribute to have a NaN value when the altitude
> information is not available.
>
> (These comments apply to altitudeAccuracy, heading and speed, too.)
>
I think using null for such cases is fine.
>
>
> 5.5 PositionError interface
> ---------------------------
>
> The specified maximum length of time has elapsed before the
> implementation could successfully acquire a new Position object.
>
> “Position” should be linked to its definition.
>
Done.
>
>
> 6.2 Requirements
> ----------------
>
> I don’t think the requirements should be section headings with no
> content in each section. Just use an <ol> instead.
>
> References
> ----------
>
> You should link to dated URLs for W3C specifications, such as Web IDL.
> Otherwise, a subsequent publication of the dependent spec could change
> incompatibly so that this draft of the Geolocation API effectively
> breaks.
>
Sure. I think the only URL that wasn't dated was the one to WebIDL.
But since we're now using features from the WebIDL Editor's draft, I
changed the link point to that draft. We'll make sure we revisit this
section before we publish again.
Thanks again for the great comments. Please have another look.
http://dev.w3.org/geo/api/spec-source.html
http://dev.w3.org/cvsweb/geo/api/spec-source.html.diff?r1=1.65&r2=1.66&f=h
Andrei
Received on Tuesday, 25 August 2009 14:44:06 UTC