Bug 17649 - Address WebIDL comments from Kyle Huey and Ms2ger
Summary: Address WebIDL comments from Kyle Huey and Ms2ger
Status: RESOLVED FIXED
Alias: None
Product: WebAppsWG
Classification: Unclassified
Component: Indexed Database API (show other bugs)
Version: unspecified
Hardware: PC All
: P2 normal
Target Milestone: ---
Assignee: Joshua Bell
QA Contact: public-webapps-bugzilla
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-29 13:27 UTC by Jonas Sicking
Modified: 2013-03-01 22:27 UTC (History)
7 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonas Sicking 2012-06-29 13:27:33 UTC
http://lists.w3.org/Archives/Public/public-webapps/2012AprJun/1152.html

I'm not sure that we can address all of them, such as the enum ones, due to limitations in ReSpec
Comment 1 Eliot Graff 2012-07-23 23:45:00 UTC
Jonas,

I'm assigning this to you. I've taken care of some of Kyle's feedback, but I'll defer to you on things like the default values for the enums.


I've FIXED the following:

3.1.9

The return type of the static functions on IDBKeyRange is not 'static
IDBKeyRange', it is just 'IDBKeyRange'.

3.2.1

The correct type name is "object", not "Object" (note the capitalization).

IDBRequest.readyState should be an enum type, not a DOMString.

3.2.2

IDBVersionChangeEvent should probably reference whatever spec defines how
constructors work for DOM events.

3.2.7

"Object" should be "object".

3.2.8

IDBTransaction's mode attribute should be an enum type.



I DID NOT FIX the following items in the mail:

3.2.4

IDBDatabase.transaction's mode argument should be an enum type, with a
default value specified in IDL instead of in prose.

3.2.5

Is it intentional that IDBObjectStore.indexNames does not return the same
DOMStringList every time, unlike IDBDatabase.objectStoreNames (yes, I
realize that the circumstances under which the former can change are much
broader).

IDBObjectStore.openCursor's direction argument should be an enum type, with
a default value specified in IDL (right now it is unspecified).

3.2.6

IDBIndex.openCursor and IDBIndex.openKeyCursor's direction argument should
be an enum type, with a default value specified in IDL.



Also, it would be nice if we could tighten up keys from 'any' to a union.
Comment 2 Joshua Bell 2012-12-10 18:50:22 UTC
One more I didn't see in Kyle's list:

3.1.12 Options Object - IDBObjectStoreParameters is defined as:

dictionary IDBObjectStoreParameters {
  DOMString? keyPath = null;
  boolean autoIncrement = false;
};

But the prose says: "If the optionalParameters argument is specified and has a keyPath property which is not undefined or null, then set keyPath to the value of this property. If keyPath is an Array, then each item in the array is converted to a string. If keyPath is not an Array, it is converted to a string."

The IDL should either be:

  DOMString? keyPath = null;

or:

  (sequence<DOMString> or DOMString)? keyPath = null;
Comment 3 Joshua Bell 2013-01-22 17:07:28 UTC
Adding more IDL nits from Ms2ger - some overlap with items in this issue already, which is why I'm not filing a separate bug.

Original messages dated January 21, 2013: http://lists.w3.org/Archives/Public/public-webapps/2013JanMar/

(1) IDBKeyRange should have static functions

From the examples in the IDB specification (in [1], for example) and from existing implementations, it appears that the functions on the IDBKeyRange interface (only, lowerBound, upperBound and bound) should be static. However, there is no actual normative requirement to that effect; instead, the IDL snippet requires those functions to only be callable on IDBKeyRange instances. [2]

[1] https://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#key-generator-concept
[2] https://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#idl-def-IDBKeyRange


(2) Event handlers

The IDB specification still uses IDL like

  [TreatNonCallableAsNull] attribute Function? onsuccess;

for its event handlers. Current practice is to use

  attribute EventHandler onsuccess;


(3) Type of IDBRequest.source

IDBRequest.source has type "object" in the IDL snippet, but the definition claims the attribute can return null. If this is the case, the type should be "object?", as for all nullable types.


(4) Type of IDBRequest.readyState

interface IDBRequest : EventTarget {
    // …
    readonly attribute enum           readyState;
    // …
};

However, "enum" is not a type. The correct syntax would be something like

enum IDBReadyState { "pending", "done" };

interface IDBRequest : EventTarget {
    // …
    readonly attribute IDBReadyState  readyState;
    // …
};

(5) Type of IDBCursor.source

IDBCursor.source is described as returning "object". It would be better to return the union (IDBObjectStore or IDBIndex), to make it clear that that is what's intended.
Comment 4 Joshua Bell 2013-01-22 17:12:57 UTC
(In reply to comment #2)
> The IDL should either be:
> 
>   DOMString? keyPath = null;

I believe I meant:

    any? keyPath = null;

But the union is preferable.
Comment 5 Joshua Bell 2013-02-28 23:40:30 UTC
From Ms2ger's list:


(1) IDBKeyRange should have static functions
- Resolved as https://dvcs.w3.org/hg/IndexedDB/rev/bf71413d740c

(2) Event handlers
- Resolved as https://dvcs.w3.org/hg/IndexedDB/rev/09a89b16b4b2

(3) Type of IDBRequest.source
- Resolved as https://dvcs.w3.org/hg/IndexedDB/rev/0f8193ddb6af

(4) Type of IDBRequest.readyState
- Resolved as https://dvcs.w3.org/hg/IndexedDB/rev/18b6e04cc02c

#5 is not yet fixed, and #3 could be improved to be:
 readonly attribute (IDBObjectStore or IDBIndex or IDBCursor)? source;
... but I'm hitting a ReSpec.js bug (?) with unions
Comment 6 Joshua Bell 2013-03-01 00:10:19 UTC
(In reply to comment #1)
> 3.2.6
> 
> IDBIndex.openCursor and IDBIndex.openKeyCursor's direction argument should
> be an enum type, with a default value specified in IDL.

Resolved in: https://dvcs.w3.org/hg/IndexedDB/rev/47815a2eaed6
Comment 7 Joshua Bell 2013-03-01 00:11:41 UTC
(In reply to comment #6)
> (In reply to comment #1)
> > 3.2.6
> > 
> > IDBIndex.openCursor and IDBIndex.openKeyCursor's direction argument should
> > be an enum type, with a default value specified in IDL.
> 
> Resolved in: https://dvcs.w3.org/hg/IndexedDB/rev/47815a2eaed6

Whoops, that covers this as well:

> IDBObjectStore.openCursor's direction argument should be an enum type, with
> a default value specified in IDL (right now it is unspecified).
Comment 8 Joshua Bell 2013-03-01 00:28:19 UTC
(In reply to comment #1)
> 3.2.4
> 
> IDBDatabase.transaction's mode argument should be an enum type, with a
> default value specified in IDL instead of in prose.

Resolved in https://dvcs.w3.org/hg/IndexedDB/rev/8e121d53f2f6 (although I left the prose in there as well).
Comment 9 Joshua Bell 2013-03-01 00:47:52 UTC
> Also, it would be nice if we could tighten up keys from 'any' to a union.

This is tracked w/ suggested fix as: https://www.w3.org/Bugs/Public/show_bug.cgi?id=20582

...


Here's what remains:
 
> 3.2.5
> 
> Is it intentional that IDBObjectStore.indexNames does not return the same
> DOMStringList every time, unlike IDBDatabase.objectStoreNames (yes, I
> realize that the circumstances under which the former can change are much
> broader).

That requires thinking. "Next!"

The rest are all union types, blocked on a ReSpec bug (?) to get the IDL perfect:

> dictionary IDBObjectStoreParameters {
>   DOMString? keyPath = null;
>   boolean autoIncrement = false;
> };

Should be:  (sequence<DOMString> or DOMString)? keyPath = null;
Or as a fallback:   any? keyPath = null;

> Type of IDBRequest.source

Should be: (IDBObjectStore or IDBIndex or IDBCursor)?
 
> Type of IDBCursor.source

Should be: (IDBObjectStore or IDBIndex)
Comment 10 Ms2ger 2013-03-01 10:09:58 UTC
Robin?
Comment 11 Robin Berjon 2013-03-01 11:55:01 UTC
(In reply to comment #10)
> Robin?

This should all be fixed in 3.1.46 that just shipped.
Comment 12 Joshua Bell 2013-03-01 17:50:51 UTC
> > Type of IDBRequest.source
> 
> Should be: (IDBObjectStore or IDBIndex or IDBCursor)?

Fixed! https://dvcs.w3.org/hg/IndexedDB/rev/8a1cffca7f26
  
> > Type of IDBCursor.source
> 
> Should be: (IDBObjectStore or IDBIndex)

Fixed! https://dvcs.w3.org/hg/IndexedDB/rev/8a1cffca7f26

(In reply to comment #9)
> 
> > dictionary IDBObjectStoreParameters {
> >   DOMString? keyPath = null;
> >   boolean autoIncrement = false;
> > };
> 
> Should be:  (sequence<DOMString> or DOMString)? keyPath = null;

ReSpec.js is truncating "sequence<DOMString>" into "sequence". Mail to Robin written but not yet sent. :)
Comment 13 Joshua Bell 2013-03-01 18:00:34 UTC
(In reply to comment #12)
> 
> ReSpec.js is truncating "sequence<DOMString>" into "sequence". Mail to Robin
> written but not yet sent. :)

My bad, I wasn't escaping in the HTML. Derp.
Comment 14 Joshua Bell 2013-03-01 18:08:08 UTC
> > dictionary IDBObjectStoreParameters {
> >   DOMString? keyPath = null;
> >   boolean autoIncrement = false;
> > };
> 
> Should be:  (sequence<DOMString> or DOMString)? keyPath = null;

Fixed! https://dvcs.w3.org/hg/IndexedDB/rev/dffdbbe300ab

This leaves the DOMStringList question re: 3.2.5
Comment 15 Joshua Bell 2013-03-01 22:26:20 UTC
(In reply to comment #1)
> 3.2.5
> 
> Is it intentional that IDBObjectStore.indexNames does not return the same
> DOMStringList every time, unlike IDBDatabase.objectStoreNames (yes, I
> realize that the circumstances under which the former can change are much
> broader).

Is this question about the identity of the DOMStringList object returned by the property accesses, or about the values contained in the lists?

I'm choosing to focus on the inconsistencies between the property definitions. I've added the same text about snapshotting to indexNames as is present on objectStoreNames.

If that's not the intent of the question, can we file a new bug?

Resolved in https://dvcs.w3.org/hg/IndexedDB/rev/49660ca248b0
Comment 16 Joshua Bell 2013-03-01 22:27:11 UTC
I believe all issues mentioned in this bug have now been addressed. 

If not, we should probably file a new, much shorter bug. :)