Bug 43276 – Implements IDBKeyPath extractor.

• Component: New Bugs
• Status: RESOLVED FIXED

[Marcus Bulach]

Implements IDBKeyPath extractor.

[Marcus Bulach]

Created attachment 63116 [details]
Patch

[Marcus Bulach]

Created attachment 63117 [details]
Patch

[WebKit Review Bot]

Attachment 63117 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/tests/IDBKeyPathExtractorTest.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebKit/chromium/tests/IDBKeyPathExtractorTest.cpp:67:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
WebKit/chromium/tests/IDBKeyPathExtractorTest.cpp:84:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
WebKit/chromium/tests/IDBKeyPathExtractorTest.cpp:99:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
WebKit/chromium/tests/IDBKeyPathExtractorTest.cpp:100:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
WebKit/chromium/tests/IDBKeyPathExtractorTest.cpp:119:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
Total errors found: 6 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Marcus Bulach]

Hi,

This patch depends on https://bugs.webkit.org/show_bug.cgi?id=42976.
If you want to look at this, please focus on SerializedScriptValue, IDBKeyPathExtractorTest, as almost everything else is part of the patch above.

[Andrei Popescu]

> WebCore/bindings/v8/SerializedScriptValue.cpp
> String keyPathExtractor(const String& wireData, const String& keyPathString)

Does the above IDB-specific method belong to the SerializedScriptValue file? I think Jeremy added a IDBBindingsUtils class that seems more fit for your purpose.

Also, is this method used just for testing? Otherwise, I am a bit confused about why it always returns a string. 

And one last thing: is the current name the best choice? You're not extracting a key path. You're extracting a value from the serialized object at the given key path.

> void IDBKeyPath::parse()
> (...)
>  case Array : {
> (...)
> m_lexer.next();
 > token = m_lexer.currentToken();

You are only checking for dot or left bracket. But what if tokEnd follows? Seems like you're returning ParserErrorAfterArray in that case? Although looking at the unit tests, it seems you have cases for key paths that end indexing into an array.

> class LocalContext {

Umm, do you need this? Isn't this the same as v8::Context::Scope?

[Marcus Bulach]

Created attachment 63313 [details]
Patch

[WebKit Review Bot]

Attachment 63313 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/tests/IDBBindingUtilitiesTest.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Marcus Bulach]

Thanks, Andrei!
replies inline:

(In reply to comment #5)
> > WebCore/bindings/v8/SerializedScriptValue.cpp
> > String keyPathExtractor(const String& wireData, const String& keyPathString)
> 
> Does the above IDB-specific method belong to the SerializedScriptValue file? I think Jeremy added a IDBBindingsUtils class that seems more fit for your purpose.
moved to IDBBindingUtils.

> 
> Also, is this method used just for testing? Otherwise, I am a bit confused about why it always returns a string. 
this is the real method, returning string was a mistake :)
it now returns an IDBKey.

> 
> And one last thing: is the current name the best choice? You're not extracting a key path. You're extracting a value from the serialized object at the given key path.
as discussed, this is now called valueForKeyPath.

> 
> > void IDBKeyPath::parse()
> > (...)
> >  case Array : {
> > (...)
> > m_lexer.next();
>  > token = m_lexer.currentToken();
> 
> You are only checking for dot or left bracket. But what if tokEnd follows? Seems like you're returning ParserErrorAfterArray in that case? Although looking at the unit tests, it seems you have cases for key paths that end indexing into an array.

sorry, I think it wasn't clear. this bug depends on https://bugs.webkit.org/show_bug.cgi?id=42976, and in fact contains all the code from there.
as such, please avoid looking at any IDBKeyPath.* here. once that patch is in, I'll merge it here and this will only contain the binding utilities and related tests.
regardless: this comment has been applied in both places, thanks!

> 
> > class LocalContext {
> 
> Umm, do you need this? Isn't this the same as v8::Context::Scope?

apparently, it also needs a v8::HandleScope, so I put them both inside this LocalContext (and removed the HandleScope from individual tests).

another look please?

[Jeremy Orlow]

Comment on attachment 63313 [details]
Patch

WebKit/chromium/tests/IDBKeyPathTest.cpp:32
 +  #if ENABLE(INDEXED_DATABASE)
move this up...or....actually maybe just get rid of?

WebKit/chromium/tests/IDBBindingUtilitiesTest.cpp:35
 +  #if ENABLE(INDEXED_DATABASE)
ditto

WebCore/storage/IndexedDatabaseRequest.idl:31
 +          [CallWith=ScriptExecutionContext] IDBRequest open(in DOMString name, in DOMString description);
Umm....what's this file doing here?

WebCore/storage/IndexedDatabaseRequest.h:48
 +  class IndexedDatabaseRequest : public RefCounted<IndexedDatabaseRequest> {
ditto

WebCore/storage/IndexedDatabaseRequest.cpp:55
 +  PassRefPtr<IDBRequest> IndexedDatabaseRequest::open(ScriptExecutionContext* context, const String& name, const String& description)
and again

WebCore/storage/IDBKeyPath.cpp:33
 +  #if ENABLE(INDEXED_DATABASE)
move up

WebCore/bindings/v8/IDBBindingUtilities.h:40
 +  PassRefPtr<IDBKey> valueForKeyPath(const String& wireData, const String& keyPathString);
Should this just take a SerializedScriptValue?

WebCore/bindings/v8/IDBBindingUtilities.cpp:54
 +      RefPtr<IDBKeyPath> keyPath = IDBKeyPath::create(keyPathString);
Check whether it's valid before going on.

Of course....shouldn't we just compile once.  I.e. pass in an IDBKeyPath to the function?

WebCore/bindings/v8/IDBBindingUtilities.cpp:61
 +              v8::Local<v8::Object> object = v8Value->ToObject();
Is this the best way to do this?

WebCore/bindings/v8/IDBBindingUtilities.cpp:66
 +              if (!v8Value->IsObject())
Can this stuff be factored out?



You should probably do all the build files and JSC support in this patch as well.

Still haven't done a detailed review of the actual parser.

[Jeremy Orlow]

O..and make sure fishd reviews before you commit since it touches WebKit

[Jeremy Orlow]

Comment on attachment 63313 [details]
Patch

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

[Marcus Bulach]

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

[Jeremy Orlow]

(In reply to comment #12)
> 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.).

Ugh.  Please don't include multiple patches in one.  Do a diff between what's in the other change and this and post that.

[Marcus Bulach]

Created attachment 63638 [details]
Patch

this is a git diff between my branches, it should contain only the code for the valueForKeyPath and associated tests.
(it still depends on https://bugs.webkit.org/show_bug.cgi?id=42976, but hopefully can be reviewed in parallel..)

[Jeremy Orlow]

Comment on attachment 63638 [details]
Patch

WebKit/chromium/src/WebIDBKeyPath.cpp:34
 +  #include "Vector.h"
<wtf/Vector.h>

WebKit/chromium/src/WebIDBKeyPath.cpp:36
 +  using WebCore::IDBKeyPathElement;
Just using WebCore;

WebKit/chromium/src/WebIDBKeyPath.cpp:51
 +      delete m_private;
maybe check to see if m_private is !0...and then set it to 0?

WebKit/chromium/public/WebIDBKeyPath.h:53
 +      void assign(WTF::Vector<WebCore::IDBKeyPathElement, 0>&);
typically we just make these all implicit conversions.  Is that not possible?

WebKit/chromium/public/WebIDBKeyPath.h:58
 +      WTF::Vector<WebCore::IDBKeyPathElement, 0>* m_private;
I just asked hans to make a PrivateOwnPtr class (like PrivatePrt..maybe renaming it to PrivateRefPtr) for something similar.  Coordinate between yourselves?

WebKit/chromium/public/WebIDBKeyPath.h:32
 +  namespace WebCore {
#if WEBKIT_IMPLEMENTATION

WebKit/chromium/public/WebIDBKeyPath.h:61
 +  void WebIDBParseKeyPath(const WebString&, WebIDBKeyPath*, int* error);
Do we have other base methods like this?  Maybe it should be a static method of some class?

[Marcus Bulach]

Created attachment 64017 [details]
Patch

(this is still blocked by https://bugs.webkit.org/show_bug.cgi?id=42976)

more inline:

(In reply to comment #15)
> (From update of attachment 63638 [details])
> WebKit/chromium/src/WebIDBKeyPath.cpp:34
>  +  #include "Vector.h"
> <wtf/Vector.h>
done.

> 
> WebKit/chromium/src/WebIDBKeyPath.cpp:36
>  +  using WebCore::IDBKeyPathElement;
> Just using WebCore;
done.

> 
> WebKit/chromium/src/WebIDBKeyPath.cpp:51
>  +      delete m_private;
> maybe check to see if m_private is !0...and then set it to 0?
I used your suggestion and created a WebPrivateOwnPtr that takes care of it.

> 
> WebKit/chromium/public/WebIDBKeyPath.h:53
>  +      void assign(WTF::Vector<WebCore::IDBKeyPathElement, 0>&);
> typically we just make these all implicit conversions.  Is that not possible?
changed so that there's a factory method, in order to capture any parsing errors.

> 
> WebKit/chromium/public/WebIDBKeyPath.h:58
>  +      WTF::Vector<WebCore::IDBKeyPathElement, 0>* m_private;
> I just asked hans to make a PrivateOwnPtr class (like PrivatePrt..maybe renaming it to PrivateRefPtr) for something similar.  Coordinate between yourselves?

good idea! done.

> 
> WebKit/chromium/public/WebIDBKeyPath.h:32
>  +  namespace WebCore {
> #if WEBKIT_IMPLEMENTATION
hmm, I think such forward declarations are required regardless of which side?

> 
> WebKit/chromium/public/WebIDBKeyPath.h:61
>  +  void WebIDBParseKeyPath(const WebString&, WebIDBKeyPath*, int* error);
> Do we have other base methods like this?  Maybe it should be a static method of some class?
yep, created a factory method on WebIDBKeyPath.

[Jeremy Orlow]

Comment on attachment 64017 [details]
Patch

WebCore/bindings/v8/IDBBindingUtilities.cpp:51
 +  bool GetValueFrom(v8::Handle<v8::Value>& v8Value, T indexOrName)
lowercase g

WebCore/bindings/v8/IDBBindingUtilities.cpp:55
 +          return 0;
return false

WebCore/bindings/v8/IDBBindingUtilities.cpp:51
 +  bool GetValueFrom(v8::Handle<v8::Value>& v8Value, T indexOrName)
Hm...should the output param come second?

WebCore/bindings/v8/IDBBindingUtilities.cpp:68
 +          } else {
else if maybe...and maybe even add an else after this that ASSERT_NOT_REACHED?

Maybe use a switch?

WebCore/bindings/v8/IDBBindingUtilities.h:32
 +  #include <wtf/Vector.h>
alpha order

WebCore/bindings/v8/IDBBindingUtilities.h:38
 +  struct IDBKeyPathElement;
structs come after classes

WebKit/chromium/WebKit.gyp:180
 +                  'public/WebIDBBindingUtilities.h',
So this file is alpha, not ascii order?

WebKit/chromium/public/WebIDBBindingUtilities.h:35
 +  WebIDBKey WebValueForKeyPath(const WebSerializedScriptValue&, const WebIDBKeyPath&);
lower case w

WebKit/chromium/public/WebIDBKeyPath.h:33
 +  namespace WebCore {
these could be one liners if you wanted

WebKit/chromium/src/WebIDBBindingUtilities.cpp:40
 +  WebIDBKey WebValueForKeyPath(const WebSerializedScriptValue& serializedScriptValue, const WebIDBKeyPath& idbKeyPath)
I still kind of feel like this should just be a static method on WebIDBKeyPath or something.

WebKit/chromium/src/WebIDBKeyPath.cpp:44
 +      IDBParseKeyPath(keyPath, &idbElements, &idbError);
Hm....isn't the norm to just pass by reference, not pointer?

WebKit/chromium/src/WebIDBKeyPath.cpp:47
 +        return NULL;
0

and 4 spaces

WebKit/chromium/src/WebIDBKeyPath.cpp:40
 +  WebIDBKeyPath* WebIDBKeyPath::create(const WebString& keyPath, int* error)
Why do we need to return a *?  Just return a copy of this object.

WebKit/chromium/src/WebIDBKeyPath.cpp:58
 +      m_private.reset();
The destructor needn't call reset anymore now that you have a class to do it...so you could get rid of reset.

[Hans Wennborg]

(In reply to comment #16)
> Created an attachment (id=64017) [details]
> Patch

I'm only commenting on the WebPrivateOwnPtr part.

In WebKit/chromium/public/WebPrivateOwnPtr.h:
+#if WEBKIT_IMPLEMENTATION
+#include <wtf/OwnPtr.h>
+#include <wtf/PassOwnPtr.h>
+#endif

Those classes don't seem to be used.

The class should probably be Noncopyable, so hide copy ctor and assignment operator in the private section.

When WEBKIT_IMPLEMENTATION is not defined, code should probably not be allowed to construct objects of the class. To prevent this, maybe the WebPrivateOwnPtr() contructor should be declared in the private: section.

+    void reset()
+    {
+        // we own m_ptr.
+        delete m_ptr;
+        m_ptr = 0;
+    }
There is a subtle point to not defining reset() inline. If it is inline, then T must be defined when WebPrivatePtr<T> is instantiated, because it will try to bind the "delete m_ptr" call. If reset() is not inline, it's enough to have T forward-declared when instantiating the template, which is a good thing. OwnPtr does this too, for the same reason AFAIK.

Also, I thought you would do this in Bug 43709. If not, at least please update that when this goes in.

This is definitely a nice class to have, I think :)

[Hans Wennborg]

(In reply to comment #18)
> There is a subtle point to not defining reset() inline. If it is inline, then T must be defined when WebPrivatePtr<T> is instantiated, because it will try to bind the "delete m_ptr" call. If reset() is not inline, it's enough to have T forward-declared when instantiating the template, which is a good thing. OwnPtr does this too, for the same reason AFAIK.

I hadn't thought this through. Obviously it has to be inline, because it's in a template. I'm not sure what deleteOwnedPtr() in OwnPtrCommon.h does, but it looks tricky. Please disregard my comment.

[Marcus Bulach]

(In reply to comment #19)
> (In reply to comment #18)
> > There is a subtle point to not defining reset() inline. If it is inline, then T must be defined when WebPrivatePtr<T> is instantiated, because it will try to bind the "delete m_ptr" call. If reset() is not inline, it's enough to have T forward-declared when instantiating the template, which is a good thing. OwnPtr does this too, for the same reason AFAIK.
> 
> I hadn't thought this through. Obviously it has to be inline, because it's in a template. I'm not sure what deleteOwnedPtr() in OwnPtrCommon.h does, but it looks tricky. Please disregard my comment.

you're right, and the WEBKIT_API / IMPLEMENTATION and the templates are making me dizzy :)
I think we need to define the destructor inline and make it call "reset()", which will be declared as an API method, but defined inline under an IMPLEMENTATION guard. it must also COMPILE_ASSERT it knows sizeof(T). I'll address Jeremey's previous comment and try to get this one, will send a new patch shortly..

oh, and I'm happy to split and put in the separate bug as well, but I'd prefer to do that once we figure out the right usage here. how does it sound?

[Hans Wennborg]

(In reply to comment #20)
> oh, and I'm happy to split and put in the separate bug as well, but I'd prefer to do that once we figure out the right usage here. how does it sound?

Sounds good.

[Marcus Bulach]

Created attachment 64125 [details]
Patch

[Marcus Bulach]

Created attachment 64128 [details]
Patch

[Marcus Bulach]

thanks Jeremy, Hans! replies inline, another look please?

(In reply to comment #17)
> (From update of attachment 64017 [details])
> WebCore/bindings/v8/IDBBindingUtilities.cpp:51
>  +  bool GetValueFrom(v8::Handle<v8::Value>& v8Value, T indexOrName)
> lowercase g
done.

> 
> WebCore/bindings/v8/IDBBindingUtilities.cpp:55
>  +          return 0;
> return false
done

> 
> WebCore/bindings/v8/IDBBindingUtilities.cpp:51
>  +  bool GetValueFrom(v8::Handle<v8::Value>& v8Value, T indexOrName)
> Hm...should the output param come second?
done

> 
> WebCore/bindings/v8/IDBBindingUtilities.cpp:68
>  +          } else {
> else if maybe...and maybe even add an else after this that ASSERT_NOT_REACHED?
> 
> Maybe use a switch?
done

> 
> WebCore/bindings/v8/IDBBindingUtilities.h:32
>  +  #include <wtf/Vector.h>
> alpha order
done

> 
> WebCore/bindings/v8/IDBBindingUtilities.h:38
>  +  struct IDBKeyPathElement;
> structs come after classes
done.

> 
> WebKit/chromium/WebKit.gyp:180
>  +                  'public/WebIDBBindingUtilities.h',
> So this file is alpha, not ascii order?
not sure I understood this comment?

> 
> WebKit/chromium/public/WebIDBBindingUtilities.h:35
>  +  WebIDBKey WebValueForKeyPath(const WebSerializedScriptValue&, const WebIDBKeyPath&);
> lower case w
obsolete, this file no longer exists and this method is now WebIDBKey::createFromValueAndKeyPath

> 
> WebKit/chromium/public/WebIDBKeyPath.h:33
>  +  namespace WebCore {
> these could be one liners if you wanted
done  

> 
> WebKit/chromium/src/WebIDBBindingUtilities.cpp:40
>  +  WebIDBKey WebValueForKeyPath(const WebSerializedScriptValue& serializedScriptValue, const WebIDBKeyPath& idbKeyPath)
> I still kind of feel like this should just be a static method on WebIDBKeyPath or something.
as we chatted, this is now WebIDBKey::createFromValueAndKeyPath

> 
> WebKit/chromium/src/WebIDBKeyPath.cpp:44
>  +      IDBParseKeyPath(keyPath, &idbElements, &idbError);
> Hm....isn't the norm to just pass by reference, not pointer?
done.

> 
> WebKit/chromium/src/WebIDBKeyPath.cpp:47
>  +        return NULL;
> 0
done.

> 
> and 4 spaces
done.

> 
> WebKit/chromium/src/WebIDBKeyPath.cpp:40
>  +  WebIDBKeyPath* WebIDBKeyPath::create(const WebString& keyPath, int* error)
> Why do we need to return a *?  Just return a copy of this object.
done.  

> 
> WebKit/chromium/src/WebIDBKeyPath.cpp:58
>  +      m_private.reset();
> The destructor needn't call reset anymore now that you have a class to do it...so you could get rid of reset.
due to the template / WEBKIT_API, we need again the reset

(In reply to comment #18)
> (In reply to comment #16)
> > Created an attachment (id=64017) [details] [details]
> > Patch
> 
> I'm only commenting on the WebPrivateOwnPtr part.
> 
> In WebKit/chromium/public/WebPrivateOwnPtr.h:
> +#if WEBKIT_IMPLEMENTATION
> +#include <wtf/OwnPtr.h>
> +#include <wtf/PassOwnPtr.h>
> +#endif
> 
> Those classes don't seem to be used.
removed.

> 
> The class should probably be Noncopyable, so hide copy ctor and assignment operator in the private section.
done.

> 
> When WEBKIT_IMPLEMENTATION is not defined, code should probably not be allowed to construct objects of the class. To prevent this, maybe the WebPrivateOwnPtr() contructor should be declared in the private: section.
> 
> +    void reset()
> +    {
> +        // we own m_ptr.
> +        delete m_ptr;
> +        m_ptr = 0;
> +    }
> There is a subtle point to not defining reset() inline. If it is inline, then T must be defined when WebPrivatePtr<T> is instantiated, because it will try to bind the "delete m_ptr" call. If reset() is not inline, it's enough to have T forward-declared when instantiating the template, which is a good thing. OwnPtr does this too, for the same reason AFAIK.
yes, you're right. this is no longer _that_ useful, as the dtor needs to be inline, but reset() can't be... :(
so I used the same pattern as WebPrivatePtr, in the sense that the caller needs to call reset(), and the dtor assert it's null.

> 
> Also, I thought you would do this in Bug 43709. If not, at least please update that when this goes in.
Yep, I can split this at any point, just thought it'd be nicer to have a concrete usage. :)  

> 
> This is definitely a nice class to have, I think :)
:)

[Jeremy Orlow]

Comment on attachment 64128 [details]
Patch

need change log

WebCore/bindings/v8/IDBBindingUtilities.cpp:65
 +          switch (keyPath[i].type) {
Maybe this would be better if it were an if statement?

(Just kidding.  :-)

WebCore/bindings/v8/IDBBindingUtilities.h:33
 +  #include <wtf/Vector.h>
You don't need this.

WebKit/chromium/src/WebIDBKeyPath.cpp:49
 +      : m_private(new WTF::Vector<IDBKeyPathElement>(elements)),
comma on next line

WebKit/chromium/src/WebIDBKeyPath.cpp:41
 +  {
Initialize m_parseError?

[Jeremy Orlow]

forgot r=me

[Marcus Bulach]

(In reply to comment #26)
> forgot r=me

(In reply to comment #25)
> (From update of attachment 64128 [details])
> need change log
> 
> WebCore/bindings/v8/IDBBindingUtilities.cpp:65
>  +          switch (keyPath[i].type) {
> Maybe this would be better if it were an if statement?
> 
> (Just kidding.  :-)
> 
> WebCore/bindings/v8/IDBBindingUtilities.h:33
>  +  #include <wtf/Vector.h>
> You don't need this.
> 
> WebKit/chromium/src/WebIDBKeyPath.cpp:49
>  +      : m_private(new WTF::Vector<IDBKeyPathElement>(elements)),
> comma on next line
> 
> WebKit/chromium/src/WebIDBKeyPath.cpp:41
>  +  {
> Initialize m_parseError?

thanks, Jeremy! all comments addressed, landing it now.

[Marcus Bulach]

Committed r65439: <http://trac.webkit.org/changeset/65439>