1285917 - Expose InformalValueTypeName in JSAPI

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas Auvray]

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

Comment 1

[Nicolas Auvray]

Attachment: Expose JS_InformalValueTypeName

This patch changes the name of the function for consistency with the API, most changes arise from there.

Comment 2

[Nicolas Auvray]

Attachment: JS_InformalValueTypeName.patch

This is a saner version of the previous patch, after input from arai on #jsapi.

Comment 3

[Tooru Fujisawa [:arai]]

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 4

[Jason Orendorff [:jorendorff]]

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.

Comment 5

[Nicolas Auvray]

Attachment: 1285917.patch

I used the correct tool with the proposed configuration for this patch. It addresses your comments. Thanks for the review!

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8808833 [details] [diff] [review]
1285917.patch

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

Looks good!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71a163471efd
Expose InformalValueType in JSAPI. r=jorendorff

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71a163471efd

Comment 9

[Nicolas Auvray]

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.

Comment 10

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8808833 [details] [diff] [review]
1285917.patch

export InformalValueTypeName, take in aurora52

Hopefully this API has some automated test coverage?

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/64c2b28ed882

Comment 12

[Nicolas Auvray]

Attachment: Add tests for InformalValueTypeName.

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.

Comment 13

[Julien Cristau [:jcristau] (back April 22)]

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.

Comment 14

[Jason Orendorff [:jorendorff]]

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.

Comment 15

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bc347a8bb31fa7fdc54ac94efb5bcbe039b64c0
Bug 1285917 - Add tests for JS::InformalValueTypeName. r=jorendorff

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0bc347a8bb31