[webkit-reviews] review denied: [Bug 42976] Implement IDBKeyPath parser. : [Attachment 62579] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Marcus Bulach
<bulach at chromium.org>'s request for review:
Bug 42976: Implement IDBKeyPath parser.
https://bugs.webkit.org/show_bug.cgi?id=42976

Attachment 62579: Patch
https://bugs.webkit.org/attachment.cgi?id=62579&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
WebCore/storage/IndexedDatabaseRequest.idl:36
 +	    // FIXME: IDBKeyPath is public only to allow testing.
FIXMEs should have an action...i.e. delete as soon as we can because it's only
here for testing...fix elsewhere.

LayoutTests/storage/indexeddb/idb-keypath-expected.txt:17
 +  indexedDB.makeKeyPath('a[34][20].foo[2].bar')
maybe insert a newline via "debug('');" between test cases? (here and
elsewhere)



LayoutTests/storage/indexeddb/script-tests/idb-keypath.js:9
 +	for (;keyPath.hasNext(); keyPath.next(), ++i) {
space before keyPath...but why not put the var i = 0 in there?



WebCore/storage/IDBKeyPath.cpp:50
 +	Token token;
What does this do?

WebCore/storage/IDBKeyPath.cpp:63
 +  void IDBKeyPath::next()
I guess this next interface stuff makes sense in terms of easily testing
(without building a lot of infrastructure that's just going to go away) but for
the real implementation, I don't see why we don't just give a pointer to the
raw array + size check.  This stuff seems overly complicated.

WebCore/storage/IDBKeyPath.cpp:173
 +	return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z');
this is a subset of javascript identifiers




Do we really want to support array syntax within keyRanges?  Also, what do we
do when we see malformed keyPaths?  Right now it seems we try our best to parse
them, but I don't think this is the right behavior.  I think we should raise an
exception on parse errors.  (The create function should return 0 and the caller
would raise.)  All of this really needs to be specced out tho.

Haven't looked at the parser or tokenizer in detail yet.  To be honest, it
feels a little heavyweight for our needs, but I'm guessing it'll pay off in the
long run.  We'll probably want multiple eyes looking at it though...since it'd
be one of the easier ways to introduce security issues, I'd think.