917295 - GC: Handlify public date APIs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jon Coppeard (:jonco)]

Attachment: handlify-date-apis

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 806587 [details] [diff] [review]
handlify-date-apis

Review of attachment 806587 [details] [diff] [review]:
-----------------------------------------------------------------

The JSAPI change looks good. Thank you for not making me look too closely at IDBKeyRange: that looks messy.

Comment 3

[Tom S. (please needinfo tschuster)]

Comment on attachment 806587 [details] [diff] [review]
handlify-date-apis

Review of attachment 806587 [details] [diff] [review]:
-----------------------------------------------------------------

KeyRange is horrible!

::: dom/indexedDB/IDBKeyRange.cpp
@@ +239,5 @@
>  {
>    nsresult rv;
>    nsRefPtr<IDBKeyRange> keyRange;
>  
> +  if (!JSVAL_IS_VOID(aVal) && !JSVAL_IS_NULL(aVal)) {

if (!aVal.isNullOrUndefined()) {

@@ +245,2 @@
>  
> +    JS::Rooted<JSObject*> obj(aCx, aVal.isObject() ? aVal.toObjectOrNull() : NULL);

I think we should use JS::RootedObject as well, when we use HandleObject already. If you already use isObject, then you can also use aVal.toObject()

@@ +245,3 @@
>  
> +    JS::Rooted<JSObject*> obj(aCx, aVal.isObject() ? aVal.toObjectOrNull() : NULL);
> +    if (JSVAL_IS_PRIMITIVE(aVal) ||

aVal.isPrimitive(). Seems like JS_IsArrayObject(aCx, obj) would fit on this line?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 806587 [details] [diff] [review]
handlify-date-apis

The codegen bit should probably use "possibleDateObject" as the variable name, to reduce risk of collisions.

r=me with that on that part.

Someone needs to carefully review the IDB bits, still.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 806587 [details] [diff] [review]
handlify-date-apis

Review of attachment 806587 [details] [diff] [review]:
-----------------------------------------------------------------

Also, r=me on the IDBKeyRange bits, although I don't have anything to add over what Tom found.

Comment 6

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/020f08616536

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/020f08616536