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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Nov 25 03:48:14 PST 2010


Jeremy Orlow <jorlow at chromium.org> has canceled Jeremy Orlow
<jorlow at chromium.org>'s request 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 #11)
> (From update of attachment 74844 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=74844&action=review
> 
> > LayoutTests/ChangeLog:5
> > +	     Add an OptionsObject class for IndexedDB (and later GeoLocation)
> 
> Nit: Geolocation

done

> > LayoutTests/ChangeLog:8
> > +	     Existing test covers this case.
> 
> It looks like you have added a new test?

oops

> > LayoutTests/storage/indexeddb/createObjectStoreOptions.html:1
> > +<html>
> 
> This test is named incorrectly - create-object-store-options.html

oops
 
> > WebCore/ChangeLog:13
> > +	     This first patch implements it in V8 and makes createObjectStore
use it.
> 
> It would aid reviewing if you used separate patches for the addition of the
new OptionsObject and for using it in IDB. Same goes for when you do it for JSC
and then use it for Geolocation.

I thought we generally weren't supposed to add dead code?  I tried to make this
as minimal as possible.

> > WebCore/bindings/v8/OptionsObject.cpp:53
> > +	 if (m_options.IsEmpty())
> 
> I guess this is safe even if m_options has never been initialized?

It isn't.
 
> > WebCore/bindings/v8/OptionsObject.cpp:83
> > +	 ASSERT(!v8Value.IsEmpty());
> 
> Doesn't this make more sense at line 78? Why no similar assert in
getKeyBool() ?

Was supposed to be on value, not v8Value.

> > WebCore/bindings/v8/OptionsObject.cpp:89
> > +	 if (undefinedOrNull())
> 
> I don't think is the right check. The caller might want to distinguish
between null and undefined. For example, in Geolocation, for
PositionOptions.timeout, if the property is unset or set to undefined we apply
a default value. For any other value, we apply the generic JS conversion to
Number, which includes null -> 0. One can imagine a similar situation for a
boolean property which defaults to true, but where we want to allow null ->
false. Can we test for this kind of thing?

Will address in later patch.
 
> > WebCore/bindings/v8/OptionsObject.cpp:91
> > +	 v8::Local<v8::Object> options = m_options->ToObject();
> 
> Is it possible to do the conversion to v8::Object once only, in the
constructor, and cache the result?

We could with the current code, but I'm working on a patch where this is no
longer true so I'm not sure it's worth the effort.

> > WebCore/bindings/v8/OptionsObject.cpp:94
> > +	 return !value.IsEmpty();
> 
> As Andrei mentioned, it's more readable to return true here.

No, he mentioned it earlier.  It isn't necessarily true here.  But if I do a
Has check first, then it should always be true.  And that does seem better.

> > WebCore/bindings/v8/OptionsObject.h:43
> > +	 bool undefinedOrNull() const;
> 
> Shouldn't this be isUndefinedOrNull() ?

done


More information about the webkit-reviews mailing list