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

Tue Jul 27 10:33:14 PDT 2010

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

--- Comment #9 from Marcus Bulach <bulach at chromium.org>  2010-07-27 10:33:14 PST ---
thanks everyone!
this is not yet final: I'll remove the IDLs and add support via TestShell / DRT wrappers soon, but would appreciate if you could take another look at the parser?

replies inline:

jorlow:
(In reply to comment #5)
> (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?

yep, this is going to be the second step.
either:
1. homogeneize the SerializedScriptValue interfaces to provide something like
"getNamedPropertyValue()" and "getIndexedPropertyValue()" so that the extractor doesn't care if it's talking to V8 or JSC.

2. pass this array (or iterator) of identifiers, and let the SerializedScriptValues deal with it.

I'm not there yet to decide, but would like to go with (1) rather than (2)..

> > 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?

this is pending decision on the spec.

also: I fixed the JS identifiers.

andreip:

(In reply to comment #6)
> > indexedDB.makeKeyPath('foo.bar.zoo')
> 
> Hmm, makeKeyPath() is not a method that exists in the spec. Rather than extending the indexedDB object with methods that are intended solely for testing, perhaps we should add a couple of methods to LayoutTestController instead?

yep, I'll do this shortly (need to create WebKit wrappers, etc..)
but meanwhile, could you please take another look at the parsing bits?
I hope the next patches will only add the wrappers and not touch the logic.

> 
> > module storage {
> >     // FIXME: IDBKeyPath is public only to allow testing, remove as soon as we have more comprehensive > test infrastructure for IndexedDB. 
> >     interface [
> >         Conditional=INDEXED_DATABASE
> >     ] IDBKeyPath {
> 
> Oh, I see. Looking around, I cannot find any precedent for this. So maybe it's better to have a LTC method that returns true / false depending on whether the input is a valid keypath and was successfully parsed or not? I realize this doesn't allow as extensive testing, but I am not sure it's ok to add non-standard interfaces for testing, either.
> 
> >168     else
> > 169        state = Start;
> 
> Here we are in an array, after the right bracket and we jump to the Start state, which allows an identifier. Does this mean that the following sequence will parse successfully?
> 
> identifier[100]anotherIdentifier

good point, added test case.

> 
> Looks great, otherwise!
thanks!

abarth:

(In reply to comment #7)
> (From update of attachment 62593 [details])
> WebCore/storage/IDBKeyPath.cpp:193
>  +      return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z');
> Should we use the version of this function in ASCIITypes.h?
done.

> 
> WebCore/storage/IDBKeyPath.cpp:203
>  +          token.start = token.end = m_ptr;
> Please don't use this form of assignment.
fixed.

> 
> WebCore/storage/IDBKeyPath.cpp:206
>  +      ASSERT(m_ptr <= m_end);
> Should this be m_ptr < m_end ?
done.

> 
> WebCore/storage/IDBKeyPath.cpp:237
>  +      return TokError;
> I'd move this into the default case, but that's just a stylistic point.
not sure I understood this..

> 
> WebCore/storage/IDBKeyPath.cpp:242
>  +      const UChar* runStart = m_ptr;
> I'd just call that "start"
done.

> 
> WebCore/storage/IDBKeyPath.cpp:249
>  +      token.stringToken = builder.toString();
> Why are we using a string builder here?  The append is outside the loop.
left over, using String directly.

> 
> WebCore/storage/IDBKeyPath.cpp:265
>  +          return TokError;
> so 01 only tokenizes the "0" and leaves the 1.  I presume that's what you mean.
it was an error, added a test case and fixed.

> 
> WebCore/storage/IDBKeyPath.cpp:271
>  +      for (i = 0; i < token.end - token.start; i++) {
> Please break "token.end - token.start" out as a length variable since you're using it more than once.
see below.

> 
> WebCore/storage/IDBKeyPath.cpp:275
>  +      buffer[i] = 0;
> Yuck.  This seems error prone.  I can see that it's correct, but just computing the int directly seems way easier.
I completely changed it to use to String::toIntStrict(), looks much simpler and safer now.

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