1306538 - Remove js::ReportASCIIErrorWithId

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

separated from bug 1289050.

js::ReportASCIIErrorWithId is used only in one file.
the function should be removed.

Comment 1

[Tooru Fujisawa [:arai]]

no, not blocking :P

Comment 2

[pass2pawan]

i'd like to take this bug , is it a good first bug ?

Comment 3

[Tooru Fujisawa [:arai]]

yes, I'm happy to mentor.
I'll post the details shortly.
If you have any question in advance, feel free to ask in comment.

Comment 4

[pass2pawan]

This will be my first bug , although i'll fix the bug , please do help me through the process of patching and sending it for review .

Comment 5

[Tooru Fujisawa [:arai]]

js::ReportASCIIErrorWithId is declared in js/src/jsfriendapi.h

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/js/src/jsfriendapi.h#2773-2774
> JS_FRIEND_API(void)
> ReportASCIIErrorWithId(JSContext* cx, const char* msg, JS::HandleId id);


and defined in js/src/jsfriendapi.cpp

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/js/src/jsfriendapi.cpp#1344-1357
> JS_FRIEND_API(void)
> js::ReportASCIIErrorWithId(JSContext* cx, const char* msg, HandleId id)
> {
>     RootedValue idv(cx);
>     if (!JS_IdToValue(cx, id, &idv))
>         return;
>     RootedString idstr(cx, JS::ToString(cx, idv));
>     if (!idstr)
>         return;
>     JSAutoByteString bytes;
>     if (!bytes.encodeUtf8(cx, idstr))
>         return;
>     JS_ReportErrorUTF8(cx, msg, bytes.ptr());
> }


but it's used only from 1 file, js/xpconnect/wrappers/AddonWrapper.cpp

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/js/xpconnect/wrappers/AddonWrapper.cpp#234
> template<typename Base>
> bool
> AddonWrapper<Base>::defineProperty(JSContext* cx, HandleObject wrapper, HandleId id,
>                                    Handle<PropertyDescriptor> desc,
>                                    ObjectOpResult& result) const
> {
> ...
>     js::ReportASCIIErrorWithId(cx, "unable to modify interposed property %s", id);
>     return false;
> }

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/js/xpconnect/wrappers/AddonWrapper.cpp#250
> template<typename Base>
> bool
> AddonWrapper<Base>::delete_(JSContext* cx, HandleObject wrapper, HandleId id,
>                             ObjectOpResult& result) const
> {
> ...
>     js::ReportASCIIErrorWithId(cx, "unable to delete interposed property %s", id);
>     return false;
> }

So, it's not necessarily be in jsfriendapi, but it should be the file static function.

Required code change:
  * Move js::ReportASCIIErrorWithId to js/xpconnect/wrappers/AddonWrapper.cpp and change it to file static function
  * Remove declaration of ReportASCIIErrorWithId from js/src/jsfriendapi.h

I assume you've already built firefox once, so, you need to apply the change, and then re-build to see it works.

About process, the following documentation will help:
  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
  https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html

so, basically, push your changeset to review repository, or export it as a patch file and attach here

Comment 6

[pass2pawan]

Attachment: bug-1306538.patch

Comment 7

[pass2pawan]

It does work after i rebuilt it , and ive exported it as patch here , what do i do next

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8817367 [details] [diff] [review]
bug-1306538.patch

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

Thank you, but unfortunately this is not correct changeset to export.
please commit your change before exporting.
  hg commit -m 'Bug 1306538 - Move js::ReportASCIIErrorWithId to AddonWrapper.cpp.'
  hg export -r . > ../bug-1306538.patch

Comment 9

[pass2pawan]

Attachment: bug-1306538.patch

Comment 10

[pass2pawan]

Arai is that okay now ?

Comment 11

[Tooru Fujisawa [:arai]]

no
the patch file is a text file that contains the diff of your change, so you can check if it contains correct change, by opening it with text editor.

then, it still contains the same thing as you posted first, so I think hg commit failed for some reason.
can you check the output of hg commit?
also, what does "hg status" say?

Comment 12

[pass2pawan]

hg commit gives me the output "created new head" . 
hg status gives me "? bug-1306538.patch".

Comment 13

[pass2pawan]

and i tried to do this , is this right https://pastebin.mozilla.org/8950685

Comment 14

[Tooru Fujisawa [:arai]]

yes, it's now exporting your changeset.
now, apply proper change and try again

Comment 15

[pass2pawan]

What do you mean by proper change ? 
and do i run hg commit again and then export it to a patch ?

Comment 16

[Tooru Fujisawa [:arai]]

Do you see the content of the patch you posted?
it doesn't contain the change mentioned in comment #5.

If you've already applied some other changes than the patch above, maybe it's in other changeset than the one in above pastebin.
If so, can you check the output of "hg log -f -l10" ?
it will print the information of topmost 10 changesets
If you see your name twice, it will mean that you've committed twice, and the expected change may be in other one,
in that case, you may need to fold them into one, so, please post the output of hg log here, or in IRC and ask the way to fold them properly.

if you haven't changed, please apply the change and then do "hg commit --amend -m ..." to update the topmost changeset, and then export it again.

Comment 17

[pass2pawan]

https://pastebin.mozilla.org/8950706 
My name was there twice as you sad there would be .

Comment 18

[pass2pawan]

Arai i have folded the changesets can you tell me the commands i need to run after this?

Comment 19

[pass2pawan]

Attachment: bug-1306538.patch

Comment 20

[Tooru Fujisawa [:arai]]

looks like you've folded wrong changesets and also exported patch file, as you see totally unrelated files in the patch (next time, try opening the patch file in text editor and check if it contains correct changes)
it would be better remove the changeset and create from clean tree again
you can remove the topmost changeset by
  hg strip -r .

Comment 21

[pass2pawan]

Attachment: bug-1306538.patch

Comment 22

[pass2pawan]

This should finally do it , thanks a lot for all your help .

Comment 23

[Tooru Fujisawa [:arai]]

Comment on attachment 8817783 [details] [diff] [review]
bug-1306538.patch

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

Thank you for update :)

It's almost there.  Can you update the patch to address the following comments, and post it again and ask review?
(you can ask review in attachment form, by choosing "?" in "review" field, and fill the text area next to it with email of me)

::: js/src/jsfriendapi.h
@@ +2785,1 @@
>  ReportASCIIErrorWithId(JSContext* cx, const char* msg, JS::HandleId id);

please remove all those 3 lines.
it's no more exported from jsfriendapi

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +18,5 @@
>  
>  #include "nsID.h"
>  
> +/* The Function ReportASCIIErrorWithId is being added to this file , although this belong to jsfriendspi.h it is being added here because 
> +this is the only file that uses the function*/

This comment is not necessary.
please remove them.

@@ +19,5 @@
>  #include "nsID.h"
>  
> +/* The Function ReportASCIIErrorWithId is being added to this file , although this belong to jsfriendspi.h it is being added here because 
> +this is the only file that uses the function*/
> +static inline JS_FRIEND_API(void)

JS_FRIEND_API macro is used only in jsfriendapi
so, it's not necessary here.
please change it to just "static inline void"

also, this definition should be inside namespace xpc below,
also after "using namespace js" and "using namespace JS".

@@ +20,5 @@
>  
> +/* The Function ReportASCIIErrorWithId is being added to this file , although this belong to jsfriendspi.h it is being added here because 
> +this is the only file that uses the function*/
> +static inline JS_FRIEND_API(void)
> +js::ReportASCIIErrorWithId(JSContext* cx, const char* msg, HandleId id)

This shouldn't be in js namespace.
please remove "js::"

@@ +22,5 @@
> +this is the only file that uses the function*/
> +static inline JS_FRIEND_API(void)
> +js::ReportASCIIErrorWithId(JSContext* cx, const char* msg, HandleId id)
> +{
> +	RootedValue idv(cx);

Please use 4 spaces indent, instead of tab.

Comment 24

[pass2pawan]

Attachment: bug-1306538.patch

Comment 25

[Tooru Fujisawa [:arai]]

Comment on attachment 8817955 [details] [diff] [review]
bug-1306538.patch

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

Thanks again.
can you update the patch to follow the review comments?

::: js/src/jsfriendapi.cpp
@@ +1339,5 @@
>  {
>      return ReportIsNotFunction(cx, v, -1);
>  }
>  
> +

please remove these 2 empty lines

::: js/src/jsfriendapi.h
@@ +2785,1 @@
>  ReportASCIIErrorWithId(JSContext* cx, const char* msg, JS::HandleId id);

Please remove this declaration.
we don't need prototype here.
since this is now file static function in AddonWrapper.cpp.

(also please remove empty line above, so that there's only 1 empty line between 2 declarations)

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +20,5 @@
>  
>  using namespace js;
>  using namespace JS;
> +static inline void
> +js::ReportASCIIErrorWithId(JSContext* cx, const char* msg,HandleId id)

1. move this under "namespace xpc {" line
2. remove "js::" prefix, since this should be file static.
3. add space after "msg," that was there before you move this line

Comment 26

[pass2pawan]

when i move it under the xpc namespace and remove js:: prefix i get a compilation error , because the compiler cannot distinguish between xpc:: and js:: namespace .

Comment 27

[Tooru Fujisawa [:arai]]

can you post the current patch and the error you get?

Comment 28

[pass2pawan]

Attachment: bug-1306538.patch

Comment 29

[Tooru Fujisawa [:arai]]

can you also post the compiler output ?

Comment 30

[pass2pawan]

it worked this time , i was getting an error before because there was a function declaration in jsfriendapi.h , so the final compiler output was build successful

Comment 31

[Tooru Fujisawa [:arai]]

Comment on attachment 8818200 [details] [diff] [review]
bug-1306538.patch

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

Okay, it's almost there.
only cosmetic changes.

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +20,5 @@
>  
>  using namespace js;
>  using namespace JS;
>  
> +

please remove this empty line

@@ +25,2 @@
>  namespace xpc {
> +	

please remove the trailing white spaces.

Comment 32

[pass2pawan]

Attachment: bug-1306538.patch

Comment 33

[pass2pawan]

Thanks a lot for all your help :)

Comment 34

[Tooru Fujisawa [:arai]]

Comment on attachment 8818215 [details] [diff] [review]
bug-1306538.patch

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

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +20,5 @@
>  
>  using namespace js;
>  using namespace JS;
>  
> +namespace xpc {	

there's still trailing whitespaces (actually a tab).
it would be nice to configure your text editor to highlight the trailing whitespaces, so that you can notice it before creating a patch.

also, please insert one empty line before "static inline void" below.

Comment 35

[pass2pawan]

Attachment: bug-1306538.patch

Comment 36

[pass2pawan]

i havent actually been able to see what you mean by whitespace ?

Comment 37

[Tooru Fujisawa [:arai]]

Comment on attachment 8818216 [details] [diff] [review]
bug-1306538.patch

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

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +20,5 @@
>  
>  using namespace js;
>  using namespace JS;
>  
> +namespace xpc {	

uh, it's still there :P
you can check the highlighted tab in review page
(click [review] link of the patch, and click "AddonWrapper.cpp" filename)

Comment 38

[Tooru Fujisawa [:arai]]

(In reply to pass2pawan from comment #36)
> i havent actually been able to see what you mean by whitespace ?

you've added a tab (HTAB) after "namespace xpc {" line.
the change should be reverted.

Comment 39

[pass2pawan]

Attachment: bug-1306538.patch

Finally :P

Comment 40

[Tooru Fujisawa [:arai]]

Comment on attachment 8818220 [details] [diff] [review]
bug-1306538.patch

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

Great :D
Thank you again for your patch!

Okay, here's remaining things to do:
  1. push this patch to try server (I'll do this)
  2. check the try server result (after 2-3 hours)
  3. add "checkin-needed" keyword if the try is green (means no failure)
  4. wait for the patch to be landed to mozilla-inbound repository
  5. wait for the patch to be merged to mozilla-central

Comment 41

[Tooru Fujisawa [:arai]]

here's try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9e88962070a6461d41962bdf04a79620e396838

when it shows 100% and there's no failure (or only known intermittent failure), you can add "checkin-needed" keyword

Comment 42

[pass2pawan]

is it the same as in checking the box of checkin-needed like i did with review ?

Comment 43

[Tooru Fujisawa [:arai]]

In "Keywords" field at the top of bug page, you can add it.

Comment 44

[pass2pawan]

ok thank you and will i be assigned this bug at the completion of the entire procedure ?

Comment 45

[Tooru Fujisawa [:arai]]

oops, sorry I forgot to assign it to you.
basically you'll be assigned when you post a patch.

Comment 46

[pass2pawan]

arai , i dont know how to evaluate the try tests  there are 7 unclassified errors , 6 of them intermittent one of them said the build failed but when i saw the log all of them were a success . Can i put in checkin-needed or is there something wrong ?

Comment 47

[Tooru Fujisawa [:arai]]

(In reply to pass2pawan from comment #46)
> arai , i dont know how to evaluate the try tests  there are 7 unclassified
> errors , 6 of them intermittent one of them said the build failed but when i
> saw the log all of them were a success . Can i put in checkin-needed or is
> there something wrong ?

Yes, they're all intermittent.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3561e1497acf
Move js::ReportASCIIErrorWithId to AddonWrapper.cpp. r=arai

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3561e1497acf

Comment 50

[Hannes Verschore [:h4writer]]

No need to backport this to aurora