Closed
Bug 1285909
Opened 8 years ago
Closed 7 years ago
Add JS_IsMapObject and JS_IsSetObject
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: na.itms76, Unassigned)
Details
Attachments
(2 files, 3 obsolete files)
3.60 KB,
patch
|
jorendorff
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.52 KB,
patch
|
jorendorff
:
review+
jcristau
:
feedback+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•8 years ago
|
||
Sorry for messing up with the patch attachment in the first place
Attachment #8769630 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8769634 -
Flags: review?(jorendorff)
Reporter | ||
Comment 2•8 years ago
|
||
Attachment #8769634 -
Attachment is obsolete: true
Attachment #8769634 -
Flags: review?(jorendorff)
Attachment #8769638 -
Flags: review?(jorendorff)
Comment 3•8 years ago
|
||
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)
Reporter | ||
Comment 4•8 years ago
|
||
Patch with correct style which addresses the comment above. Thanks for the review!
Attachment #8769638 -
Attachment is obsolete: true
Attachment #8808839 -
Flags: review?(jorendorff)
Updated•7 years ago
|
Attachment #8808839 -
Flags: review?(jorendorff) → review+
Updated•7 years ago
|
Keywords: checkin-needed
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
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7653eef0aae2
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Reporter | ||
Comment 7•7 years ago
|
||
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 8•7 years ago
|
||
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+
Comment 9•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/06e5a06d89b3
status-firefox52:
--- → fixed
Reporter | ||
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
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+
Comment 12•7 years ago
|
||
it might be better filing a new bug for adding tests, since this bug is already fixed and uplift is also performed.
Comment 13•6 years ago
|
||
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+
Comment 14•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e3dce97e85597982416ffb9dc8f81ac9c56bc2 Bug 1285909 - Part 2: Add test coverage for IsMapObject and IsSetObject. r=jorendorff.
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a1e3dce97e85
You need to log in
before you can comment on or make changes to this bug.
Description
•