Closed Bug 1306538 Opened 4 years ago Closed 4 years ago

Remove js::ReportASCIIErrorWithId

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

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

People

(Reporter: arai, Assigned: pass2pawan, Mentored)

References

Details

Attachments

(1 file, 8 obsolete files)

separated from bug 1289050.

js::ReportASCIIErrorWithId is used only in one file.
the function should be removed.
no, not blocking :P
No longer blocks: 1289050
See Also: → 1289050
i'd like to take this bug , is it a good first bug ?
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.
Mentor: arai.unmht
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 .
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
Attached patch bug-1306538.patch (obsolete) — Splinter Review
It does work after i rebuilt it , and ive exported it as patch here , what do i do next
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
Attached patch bug-1306538.patch (obsolete) — Splinter Review
Attachment #8817367 - Attachment is obsolete: true
Arai is that okay now ?
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?
hg commit gives me the output "created new head" . 
hg status gives me "? bug-1306538.patch".
and i tried to do this , is this right https://pastebin.mozilla.org/8950685
yes, it's now exporting your changeset.
now, apply proper change and try again
What do you mean by proper change ? 
and do i run hg commit again and then export it to a patch ?
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.
https://pastebin.mozilla.org/8950706 
My name was there twice as you sad there would be .
Arai i have folded the changesets can you tell me the commands i need to run after this?
Attached patch bug-1306538.patch (obsolete) — Splinter Review
Attachment #8817394 - Attachment is obsolete: true
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 .
Attached patch bug-1306538.patch (obsolete) — Splinter Review
Attachment #8817775 - Attachment is obsolete: true
This should finally do it , thanks a lot for all your help .
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.
Attachment #8817783 - Flags: feedback+
Attached patch bug-1306538.patch (obsolete) — Splinter Review
Attachment #8817783 - Attachment is obsolete: true
Attachment #8817955 - Flags: review?(arai.unmht)
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
Attachment #8817955 - Flags: review?(arai.unmht) → feedback+
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 .
can you post the current patch and the error you get?
Attached patch bug-1306538.patch (obsolete) — Splinter Review
Attachment #8817955 - Attachment is obsolete: true
Attachment #8818200 - Flags: review?(arai.unmht)
can you also post the compiler output ?
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 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.
Attachment #8818200 - Flags: review?(arai.unmht) → feedback+
Attached patch bug-1306538.patch (obsolete) — Splinter Review
Attachment #8818200 - Attachment is obsolete: true
Attachment #8818215 - Flags: review?(arai.unmht)
Thanks a lot for all your help :)
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.
Attachment #8818215 - Flags: review?(arai.unmht) → feedback+
Attached patch bug-1306538.patch (obsolete) — Splinter Review
Attachment #8818215 - Attachment is obsolete: true
Attachment #8818216 - Flags: review?(arai.unmht)
i havent actually been able to see what you mean by whitespace ?
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)
Attachment #8818216 - Flags: review?(arai.unmht) → feedback+
(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.
Finally :P
Attachment #8818216 - Attachment is obsolete: true
Attachment #8818220 - Flags: review?(arai.unmht)
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
Attachment #8818220 - Flags: review?(arai.unmht) → review+
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
is it the same as in checking the box of checkin-needed like i did with review ?
In "Keywords" field at the top of bug page, you can add it.
ok thank you and will i be assigned this bug at the completion of the entire procedure ?
oops, sorry I forgot to assign it to you.
basically you'll be assigned when you post a patch.
Assignee: nobody → pass2pawan
Status: NEW → ASSIGNED
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 ?
(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.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3561e1497acf
Move js::ReportASCIIErrorWithId to AddonWrapper.cpp. r=arai
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3561e1497acf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
No need to backport this to aurora
You need to log in before you can comment on or make changes to this bug.