[Webkit-unassigned] [Bug 42976] Implement IDBKeyPath parser.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=42976





--- Comment #5 from Jeremy Orlow <jorlow at chromium.org>  2010-07-27 06:32:05 PST ---
(In reply to comment #4)
> thanks for the comments so far, Jeremy!
> 
> brief background / reasoning:
> 1. not sure whether or not we'll support arrays, but it seems that if we support named properties, we should support indexed values as well.
> 2. the reason for a separate class is only to expose for tests, since we have no way to exercise this code (yet).
> 3. just passing the array with a known struct will be fine, the API is mostly exposed for tests. however, the basic concept would still be the same: I think it'd be nice to have the "SerializedScriptValueExtractor" just traverse some kind of structure: get a name or index, de-serialize, repeat.. having this separation would allow us to decouple the the extractor from the parser.

I agree they should be separate.  But is there any possible way we can make the "deserialize" code within SerializedScriptValue use the same extractor?

> (In reply to comment #2)
> > (From update of attachment 62579 [details] [details])
> > 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.
> Done.
> 
> > 
> > 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)
> Done in the "check" function above.
> 
> > 
> > 
> > 
> > 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?
> done.
> 
> > 
> > 
> > 
> > WebCore/storage/IDBKeyPath.cpp:50
> >  +      Token token;
> > What does this do?
> duh, leftover. removed.
> 
> > 
> > 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.
> 
> Agreed. See above.
> 
> > 
> > WebCore/storage/IDBKeyPath.cpp:173
> >  +      return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z');
> > this is a subset of javascript identifiers
> Not yet done, I'll try to find the authoritative list of 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.
> 
> Indeed. The keyPath is not yet fully specced, so I'm flying a bit blind here.
> However, I think the main purpose of decoupling the "SerializedScriptValueExtractor" from the "KeyPathParser" is a "Good Thing", and will allow us to move to a point where the "Extractor"s (both JSC and V8) don't care about what a KeyPath is, as long as it provides an iterator of property names / indexes.
> thoughts?
> 
> ahn, I also exposed a "parserError", the caller can do whatever we decide (either throw an exception, ignore, use what was able to parse, ...)

But if there's an error, should we even return an object?

> > 
> > 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.
> 
> as mentioned somewhere else, there are two parts that worry me:
> 1. parsing the keypath (this bit).
> 2. extracting the value of a serializedscriptvalue..
> 
> both will work on unsafe, external data, and the more eyes (and tests!) we get the better.
> feel free to add anyone else to this bug.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.