Bug 50030 – Add an OptionsObject class for IndexedDB (and later GeoLocation)

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

GeoLocation and IndexedDB both have the concept of an optional parameter that has various options supplied with it.  In GeoLocation this was done with custom bindings, but I'm trying to avoid that for IndexedDB.

Patch1: Implement this for v8, add it to the bindings tests, and utilize it for IDBDatabase.createObjectStore

Patch2: Implement for JSC, change GeoLocation to use it instead of custom code

[Jeremy Orlow]

Created attachment 74766 [details]
patch

[Eric Seidel (no email)]

Attachment 74766 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6395030

[Jeremy Orlow]

The bots show red only because of style.  And the style is that of the generated bindings, but actual code.

[Jeremy Orlow]

Created attachment 74771 [details]
Added another layout test

[Eric Seidel (no email)]

Attachment 74771 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6375029

[Build Bot]

Attachment 74771 [details] did not build on win:
Build output: http://queues.webkit.org/results/6372031

[Andrei Popescu]

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

>  bool getKeyBool(const String& key, bool& value) const;
>  bool getKeyString(const String& key, String& value) const;

Geolocation also uses a long.


> 83     return !v8Value.IsEmpty();

Should this be return true; instead? If v8Value was empty you would have already returned false on line 77, no?

[Jeremy Orlow]

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

[Jeremy Orlow]

Created attachment 74844 [details]
andrei's changes

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

[Eric Seidel (no email)]

Attachment 74844 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6348048

[Steve Block]

Comment on attachment 74844 [details]
andrei's changes

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

> LayoutTests/ChangeLog:8
> +        Existing test covers this case.

It looks like you have added a new test?

> LayoutTests/storage/indexeddb/createObjectStoreOptions.html:1
> +<html>

This test is named incorrectly - create-object-store-options.html

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

> WebCore/bindings/v8/OptionsObject.cpp:53
> +    if (m_options.IsEmpty())

I guess this is safe even if m_options has never been initialized?

> WebCore/bindings/v8/OptionsObject.cpp:83
> +    ASSERT(!v8Value.IsEmpty());

Doesn't this make more sense at line 78? Why no similar assert in getKeyBool() ?

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

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

> WebCore/bindings/v8/OptionsObject.cpp:94
> +    return !value.IsEmpty();

As Andrei mentioned, it's more readable to return true here.

> WebCore/bindings/v8/OptionsObject.h:43
> +    bool undefinedOrNull() const;

Shouldn't this be isUndefinedOrNull() ?

[Jeremy Orlow]

Created attachment 74849 [details]
address steve's coments

(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

[Andrei Popescu]

LGTM

> static void* operator new(size_t); 
> static void operator delete(void *);


I'd add the array new (void operator new[] (size_t)) just in case someone decides to allocate an array of these objects.

[Jeremy Orlow]

(In reply to comment #13)
> LGTM
> 
> > static void* operator new(size_t); 
> > static void operator delete(void *);
> 
> 
> I'd add the array new (void operator new[] (size_t)) just in case someone decides to allocate an array of these objects.

Done in my local copy.

[Jeremy Orlow]

Created attachment 74852 [details]
andrei's comment

[Jeremy Orlow]

Created attachment 74853 [details]
even better

[Eric Seidel (no email)]

Attachment 74853 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6342039

[Steve Block]

> I thought we generally weren't supposed to add dead code?  I tried to make
> this as minimal as possible.
Sure, but when the the need for the temporarily dead code is clear (eg bug dependencies), I think it's best to keep things as simple 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.
So what if somebody calls the default constructor then one of the getter methods?

> Will address in later patch.
Sure, if you're confident it doesn't make sense to do it now.

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

The bots seems to be red because of a missing header, which needs fixing.

[Jeremy Orlow]

(In reply to comment #18)
> > I thought we generally weren't supposed to add dead code?  I tried to make
> > this as minimal as possible.
> Sure, but when the the need for the temporarily dead code is clear (eg bug dependencies), I think it's best to keep things as simple 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.
> So what if somebody calls the default constructor then one of the getter methods?

Nothing will be returned.  Which makes sense, because the option was not provided.
 
> > Will address in later patch.
> Sure, if you're confident it doesn't make sense to do it now.

What benefit does it give besides a slight perf advantage that would never be measurable and maybe a slight code cleanliness bump?
 
> > > 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.
> OK
> 
> The bots seems to be red because of a missing header, which needs fixing.

Will add an empty file for JSC with a fixme.

[Jeremy Orlow]

Created attachment 74858 [details]
updated

should compile now

[Jeremy Orlow]

(In reply to comment #19)
> (In reply to comment #18)
> > > I thought we generally weren't supposed to add dead code?  I tried to make
> > > this as minimal as possible.
> > Sure, but when the the need for the temporarily dead code is clear (eg bug dependencies), I think it's best to keep things as simple 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.
> > So what if somebody calls the default constructor then one of the getter methods?
> 
> Nothing will be returned.  Which makes sense, because the option was not provided.

Because getKey will call this class's isUndefinedOrNull which does the check.
 
> > > Will address in later patch.
> > Sure, if you're confident it doesn't make sense to do it now.
> 
> What benefit does it give besides a slight perf advantage that would never be measurable and maybe a slight code cleanliness bump?
> 
> > > > 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.
> > OK
> > 
> > The bots seems to be red because of a missing header, which needs fixing.
> 
> Will add an empty file for JSC with a fixme.

done

[Andrei Popescu]

(In reply to comment #19)
> (In reply to comment #18)
> > > I thought we generally weren't supposed to add dead code?  I tried to make
> > > this as minimal as possible.
> > Sure, but when the the need for the temporarily dead code is clear (eg bug dependencies), I think it's best to keep things as simple 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.
> > So what if somebody calls the default constructor then one of the getter methods?
> 
> Nothing will be returned.  Which makes sense, because the option was not provided.
> 
> > > Will address in later patch.
> > Sure, if you're confident it doesn't make sense to do it now.
> 
> What benefit does it give besides a slight perf advantage that would never be measurable and maybe a slight code cleanliness bump?
> 


I think Steve's comment is about something else altogether. Say someone calls

geolocation.getCurrentPosition(successCallback, errorCallback, {timeout:null});

What should happen here is that the null parameter gets coerced to a Number, which will be 0 in this case. That then sets a timeout of 0ms, overriding the default value (Infinitiy).

Now if we were to use the OptionsObject in this patch, getKey() would early out after the isUndefinedOrNull() check, since the value is null. This would then mean that the extraction of the timeout attribute would fail, so the default value would apply (Infinity). But this is wrong since 0 should have been used.

[Steve Block]

> > > > > I guess this is safe even if m_options has never been initialized?
> > > > It isn't.
> > > So what if somebody calls the default constructor then one of the getter methods?
> > 
> > Nothing will be returned.  Which makes sense, because the option was not provided.
OK, so it is safe. I think there was just a misunderstanding to start with.

[Jeremy Orlow]

(In reply to comment #23)
> > > > > > I guess this is safe even if m_options has never been initialized?
> > > > > It isn't.
> > > > So what if somebody calls the default constructor then one of the getter methods?
> > > 
> > > Nothing will be returned.  Which makes sense, because the option was not provided.
> OK, so it is safe. I think there was just a misunderstanding to start with.

Let's concentrate on this patch.  I'll modify it to work for Geolocation later.  I prototyped what it would look like and I think this is on the right track, but we can discuss it in that patch.  k?

Can I please get a r+?

[Steve Block]

Comment on attachment 74858 [details]
updated

> > > > > I don't think is the right check. ...
> > > > Will address in later patch.
> > > Sure, if you're confident it doesn't make sense to do it now.
> > What benefit does it give besides a slight perf advantage that would never be measurable and maybe a slight code cleanliness bump?
> Let's concentrate on this patch.  I'll modify it to work for Geolocation later.
I pointed it out in this review because it might have made sense to fix it here. If you'd like to fix it in a later patch, that's fine with me, as I said above. I was then was confused by your comment about performance, which seems unrelated.

[Jeremy Orlow]

Committed r72728: <http://trac.webkit.org/changeset/72728>