Closed Bug 1285917 Opened 3 years ago Closed 3 years ago

Expose InformalValueTypeName in JSAPI

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, 2 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 change to JS_GetTypeName in f497c1d55fd0 [1] removed that useful functionality from the API. Would it be possible to expose the new InformalValueTypeName function in the JS namespace?

[1] https://hg.mozilla.org/mozilla-central/rev/f497c1d55fd0
Attached patch Expose JS_InformalValueTypeName (obsolete) — Splinter Review
This patch changes the name of the function for consistency with the API, most changes arise from there.
Attachment #8769652 - Flags: review?(jorendorff)
Attached patch JS_InformalValueTypeName.patch (obsolete) — Splinter Review
This is a saner version of the previous patch, after input from arai on #jsapi.
Attachment #8769652 - Attachment is obsolete: true
Attachment #8769652 - Flags: review?(jorendorff)
Attachment #8808110 - Flags: review?(jorendorff)
Comment on attachment 8808110 [details] [diff] [review]
JS_InformalValueTypeName.patch

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

Can you export patch with "hg export" or "git format-patch" or something, to include commit message and author information?
Also, it would be nice to include more context in the diff (8 is suggested)
here's example mercurial configuration:
  [diff]
  git = 1
  showfunc = 1
  unified = 8

Same for bug 1285909.

::: js/src/jsobj.cpp
@@ +89,5 @@
>  
> +JS_PUBLIC_API(const char*)
> +JS_InformalValueTypeName(const Value& v)
> +{
> +	return InformalValueTypeName(v);

please replace HTAB with 4 SPACEs.
Comment on attachment 8808110 [details] [diff] [review]
JS_InformalValueTypeName.patch

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

Please rename this to use the JS namespace: JS::InformalValueTypeName. I think this means you can just have one function instead of two.
Attachment #8808110 - Flags: review?(jorendorff)
Attached patch 1285917.patchSplinter Review
I used the correct tool with the proposed configuration for this patch. It addresses your comments. Thanks for the review!
Attachment #8808110 - Attachment is obsolete: true
Attachment #8808833 - Flags: review?(jorendorff)
Comment on attachment 8808833 [details] [diff] [review]
1285917.patch

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

Looks good!
Attachment #8808833 - Flags: review?(jorendorff) → review+
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71a163471efd
Expose InformalValueType in JSAPI. r=jorendorff
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/71a163471efd
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8808833 [details] [diff] [review]
1285917.patch

Approval Request Comment
[Feature/regressing bug #]: 1285917
[User impact if declined]: Adding this piece of API would allow us in the 0 A.D. development team to not copy entirely this function to get the informal type name (it would then allow us to get updates to that function from Mozilla). 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, this 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]: Very low risk: No new code was added, only function declarations were moved and adapted to expose this function to the API.
[String/UUID change made/needed]: None.
Attachment #8808833 - Flags: approval-mozilla-aurora?
Comment on attachment 8808833 [details] [diff] [review]
1285917.patch

export InformalValueTypeName, take in aurora52

Hopefully this API has some automated test coverage?
Flags: needinfo?(na.itms76)
Attachment #8808833 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This patch adds a new test for InformalValueTypeName, which was not covered as far as I could see. I created a new file because I did not find a suitable place in the existing ones, but if you know one please point it to me.
Flags: needinfo?(na.itms76)
Attachment #8817914 - Flags: review?(jorendorff)
Attachment #8817914 - Flags: review?(jcristau)
Comment on attachment 8817914 [details] [diff] [review]
Add tests for InformalValueTypeName.

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

I'm not the right person to review this.
Attachment #8817914 - Flags: review?(jcristau) → feedback+
Comment on attachment 8817914 [details] [diff] [review]
Add tests for InformalValueTypeName.

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

This still applies fine. We can take it.
Attachment #8817914 - Flags: review?(jorendorff) → review+
You need to log in before you can comment on or make changes to this bug.