Bug 43276

Created attachment 63116 [details] Patch

Created attachment 63117 [details] Patch

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.

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.

> 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?

Created attachment 63313 [details] Patch

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.

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?

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.

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

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

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

(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.

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..)

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?

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.

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.

(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 :)

(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.

(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?

(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.

Created attachment 64125 [details] Patch

Created attachment 64128 [details] Patch

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 :) :)

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?

forgot r=me

(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.

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