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

[bugzilla-daemon at webkit.org]

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


Jeremy Orlow <jorlow at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #63313|review?                     |review-
               Flag|                            |




--- Comment #11 from Jeremy Orlow <jorlow at chromium.org>  2010-08-03 06:24:35 PST ---
(From update of attachment 63313)
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.