[webkit-reviews] review requested: [Bug 50030] Add an OptionsObject class for IndexedDB (and later GeoLocation) : [Attachment 74844] andrei's changes

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has asked  for review:
Bug 50030: Add an OptionsObject class for IndexedDB (and later GeoLocation)
https://bugs.webkit.org/show_bug.cgi?id=50030

Attachment 74844: andrei's changes
https://bugs.webkit.org/attachment.cgi?id=74844&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
(In reply to comment #8)
> (In reply to comment #7)
> > Nice!
> > 
> > > // IMPORTANT: This should only be allocated on the stack given that it
stores the
> > > //	    object as a v8::Local which is only allowed to be on the
stack.
> > 
> > Rather than this, maybe it's more efficient to just declare the new
operator as private? This wouldn't work if you derive from this class tho...
> 
> Good idea.

Done.
 
> > >  bool getKeyBool(const String& key, bool& value) const;
> > >  bool getKeyString(const String& key, String& value) const;
> > 
> > Geolocation also uses a long.
> 
> And other stuff.  Will add it in patch 2.
> 
> > > 83     return !v8Value.IsEmpty();
> > 
> > Should this be return true; instead? If v8Value was empty you would have
already returned false on line 77, no?
> 
> I guess I can assert that it's not empty and return true.

Done

Also, I added something to the WebKit API for another patch I'm working on.