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

[bugzilla-daemon at webkit.org]

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





--- Comment #24 from Marcus Bulach <bulach at chromium.org>  2010-08-11 09:43:28 PST ---
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)
 --> (https://bugs.webkit.org/attachment.cgi?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 :)
:)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.