[webkit-reviews] review denied: [Bug 43276] Implements IDBKeyPath extractor. : [Attachment 63313] 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 43276: Implements IDBKeyPath extractor.
https://bugs.webkit.org/show_bug.cgi?id=43276

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

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
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