[Webkit-unassigned] [Bug 43276] Implements IDBKeyPath extractor.

[bugzilla-daemon at webkit.org]

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





--- Comment #12 from Marcus Bulach <bulach at chromium.org>  2010-08-04 13:11:43 PST ---
thanks for this review, jeremy!
I'll reply to this comment specifically in https://bugs.webkit.org/show_bug.cgi?id=42976, which is the original patch for KeyPath.
I'll reply to the bindings / extractor later here.
(as mentioned above, this change *includes* 42976, I only split so that I could go ahead and not block with the implementation details of the parser.).

(In reply to comment #11)
> (From update of attachment 63313 [details])
> WebCore/storage/IDBKeyPath.cpp:101
>  +              if (token == TokIdentifier)
> Use a switch?
> 
> WebCore/storage/IDBKeyPath.cpp:46
>  +      , m_currentToken(0)
> Use the enum value
> 
> WebCore/storage/IDBKeyPath.cpp:58
>  +      return m_currentToken < m_tokens.size();
> I guess I'd suggest just converting over all this hasNext/next stuff to just returning the raw vector.  I don't feel strongly, but I just don't see much of a point of creating this interface.
> 
> WebCore/storage/IDBKeyPath.cpp:70
>  +          return "";
> Maybe you should just let it crash?  I.e. not do bounds checking?
> 
> WebCore/storage/IDBKeyPath.cpp:90
>  +      return m_parserError;
> Maybe all the methods should be ASSERTing that a parse error never happened?
> 
> WebCore/storage/IDBKeyPath.cpp:103
>  +              else if (token == TokLBracket)
> I'd lean towards spelling these all the way out.
> 
> WebCore/storage/IDBKeyPath.cpp:116
>  +              Token idbToken;
> these 2 token variable names are confusing.  The idb doesn't make it any less so.  Maybe 'outputToken' or something?
> 
> 
> 
> general note: you should talk to the guy who does fuzzers (skylined..?) and see if we can get some coverage on this
> 
> also, could we just do this more simply?  for example, in python pseudo code I could do this:
> 
> tokens = []
> if (path == ""):
>   return []
> for (component in path.split(".")):
>   if (component ~= /^([a-zA-Z0-9_]+)\[([0-9]+)\]$/):
>     tokens.append($1, $2)
>   else if (component ~= /^([a-zA-Z0-9_]+)$/):
>     tokens.append($1, null)
>   else:
>     return error
> return tokens
> 
> And that'd be about it.  Do we have any sort of regex support build into WebKit already?  The split would be pretty easy to do as well.  I'm just concerned that we're jumping the gun with all of this complexity.
> 
> Anyhow...
> 
> 
> WebCore/storage/IDBKeyPath.cpp:145
>  +              ASSERT(token.type == TokNumber);
> this seems mildly redundant, but I'm ok with it
> 
> WebCore/storage/IDBKeyPath.cpp:163
>  +                  state = Array;
> but this is an invalid state!
> 
> WebCore/storage/IDBKeyPath.cpp:147
>  +              idbToken.hasIndex = true;
> maybe assert it was false?
> 
> WebCore/storage/IDBKeyPath.cpp:190
>  +  
> default:
>   raise error
> 
> WebCore/storage/IDBKeyPath.cpp:99
>  +          case Start : {
> maybe pull this out of the state machine?
> 
> WebCore/storage/IDBKeyPath.cpp:278
>  +          return TokError;
> I'd just do an m_ptr >= m_end first and then return...then no need for an else
> 
> WebCore/storage/IDBKeyPath.cpp:292
>  +      return TokError;
> same again...test the error condition and return rather than putting everythign in a block
> 
> WebCore/storage/IDBKeyPath.cpp:252
>  +      if (m_ptr < m_end && isSafeIdentifierStartCharacter(*m_ptr))
> ditto
> 
> WebCore/storage/IDBKeyPath.h:84
>  +      class Lexer {
> Why does this need to be defined here?
> 
> WebCore/storage/IDBKeyPath.h:116
>  +          const UChar* m_ptr;
> should this be called m_cursor?
> 
> WebCore/storage/IDBKeyPath.h:100
>  +          TokenType next()
> these could all be on one line

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