Closed Bug 1285909 Opened 4 years ago Closed 3 years ago

Add JS_IsMapObject and JS_IsSetObject

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: na.itms76, Unassigned)

Details

Attachments

(2 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160623154057

Steps to reproduce:

The JSAPI currently provides JS_IsArrayObject [1]. It would be useful to have the same functionality for Maps and Sets in the API.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_IsArrayObject
Attached patch JS_IsMap-SetObject.patch (obsolete) — Splinter Review
Sorry for messing up with the patch attachment in the first place
Attachment #8769630 - Attachment is obsolete: true
Attachment #8769634 - Flags: review?(jorendorff)
Attachment #8769634 - Attachment is obsolete: true
Attachment #8769634 - Flags: review?(jorendorff)
Attachment #8769638 - Flags: review?(jorendorff)
Comment on attachment 8769638 [details] [diff] [review]
Fixed version after IRC input from jandem

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

Please put these functions in the JS namespace: JS::IsMapObject, JS::IsSetObject, and please factor out a common function so that JS_IsArrayObject and the other two are each one-liners.

With those changes, this can go in. Thanks for the ping this morning.
Attachment #8769638 - Flags: review?(jorendorff)
Attached patch 1285909.patchSplinter Review
Patch with correct style which addresses the comment above. Thanks for the review!
Attachment #8769638 - Attachment is obsolete: true
Attachment #8808839 - Flags: review?(jorendorff)
Attachment #8808839 - Flags: review?(jorendorff) → review+
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7653eef0aae2
Add JS::IsMapObject and JS::IsSetObject. r=jorendorff
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7653eef0aae2
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8808839 [details] [diff] [review]
1285909.patch

Approval Request Comment
[Feature/regressing bug #]: 1285909
[User impact if declined]: Adding this piece of API would allow us in the 0 A.D. development team to not rely on hacks to get the type of standards objects. Moreover, we use bundled versions of SpiderMonkey so we would love to have this in the future SM 52 tarball, instead of applying this patch until SM 59 is out.
[Describe test coverage new/current, TreeHerder]: Automated tests were ran on that patch and it has landed on mozilla-central. Additionally, an adapted version of the patch was tested on my copy of 0 A.D.: it works as expected and went through our own test suite with success.
[Risks and why]: Low risk: the new IsArrayObject relies on code that was not modified, and the new functions rely on the same code.
[String/UUID change made/needed]: None.
Attachment #8808839 - Flags: approval-mozilla-aurora?
Comment on attachment 8808839 [details] [diff] [review]
1285909.patch

export IsMapObject/IsSetObject in aurora52.

It might be nice to have tests for this, I guess?
Flags: needinfo?(na.itms76)
Attachment #8808839 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This adds a new test for the new functions. I placed it in the same file as the test that covers JS_IsArrayObject.
Flags: needinfo?(na.itms76)
Attachment #8817915 - Flags: review?(jorendorff)
Attachment #8817915 - Flags: review?(jcristau)
Comment on attachment 8817915 [details] [diff] [review]
Add test coverage for IsMapObject and IsSetObject.

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

I'm not the right person to review this.
Attachment #8817915 - Flags: review?(jcristau) → feedback+
it might be better filing a new bug for adding tests, since this bug is already fixed and uplift is also performed.
Comment on attachment 8817915 [details] [diff] [review]
Add test coverage for IsMapObject and IsSetObject.

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

Sorry for the ridiculously slow review. We might as well land this. I'll do it.

::: js/src/jsapi-tests/testNewObject.cpp
@@ +119,5 @@
>      return true;
>  }
>  END_TEST(testNewObject_1)
> +
> +BEGIN_TEST(testNewObject_2)

I'll change the name of this test to `testNewObject_IsMapObject`.

@@ +130,5 @@
> +
> +    bool isMap;
> +    JS::RootedObject mapObj(cx, JS_New(cx, Map, JS::HandleValueArray::empty()));
> +    CHECK(mapObj);
> +    JS::RootedValue rtMap(cx, JS::ObjectValue(*mapObj));

rtMap and rtSet are unused.
Attachment #8817915 - Flags: review?(jorendorff) → review+
You need to log in before you can comment on or make changes to this bug.