[webkit-reviews] review denied: [Bug 43276] Implements IDBKeyPath extractor. : [Attachment 64017] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied  review:
Bug 43276: Implements IDBKeyPath extractor.
https://bugs.webkit.org/show_bug.cgi?id=43276

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

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