Closed
Bug 1306538
Opened 8 years ago
Closed 8 years ago
Remove js::ReportASCIIErrorWithId
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: arai, Assigned: pass2pawan, Mentored)
References
Details
Attachments
(1 file, 8 obsolete files)
4.53 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
separated from bug 1289050.
js::ReportASCIIErrorWithId is used only in one file.
the function should be removed.
Reporter | ||
Comment 1•8 years ago
|
||
no, not blocking :P
Assignee | ||
Comment 2•8 years ago
|
||
i'd like to take this bug , is it a good first bug ?
Reporter | ||
Comment 3•8 years ago
|
||
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
Assignee | ||
Comment 4•8 years ago
|
||
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 .
Reporter | ||
Comment 5•8 years ago
|
||
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
Assignee | ||
Comment 6•8 years ago
|
||
Assignee | ||
Comment 7•8 years ago
|
||
It does work after i rebuilt it , and ive exported it as patch here , what do i do next
Reporter | ||
Comment 8•8 years ago
|
||
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
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8817367 -
Attachment is obsolete: true
Assignee | ||
Comment 10•8 years ago
|
||
Arai is that okay now ?
Reporter | ||
Comment 11•8 years ago
|
||
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?
Assignee | ||
Comment 12•8 years ago
|
||
hg commit gives me the output "created new head" .
hg status gives me "? bug-1306538.patch".
Assignee | ||
Comment 13•8 years ago
|
||
and i tried to do this , is this right https://pastebin.mozilla.org/8950685
Reporter | ||
Comment 14•8 years ago
|
||
yes, it's now exporting your changeset.
now, apply proper change and try again
Assignee | ||
Comment 15•8 years ago
|
||
What do you mean by proper change ?
and do i run hg commit again and then export it to a patch ?
Reporter | ||
Comment 16•8 years ago
|
||
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.
Assignee | ||
Comment 17•8 years ago
|
||
https://pastebin.mozilla.org/8950706
My name was there twice as you sad there would be .
Assignee | ||
Comment 18•8 years ago
|
||
Arai i have folded the changesets can you tell me the commands i need to run after this?
Assignee | ||
Comment 19•8 years ago
|
||
Attachment #8817394 -
Attachment is obsolete: true
Reporter | ||
Comment 20•8 years ago
|
||
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 .
Assignee | ||
Comment 21•8 years ago
|
||
Attachment #8817775 -
Attachment is obsolete: true
Assignee | ||
Comment 22•8 years ago
|
||
This should finally do it , thanks a lot for all your help .
Reporter | ||
Comment 23•8 years ago
|
||
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+
Assignee | ||
Comment 24•8 years ago
|
||
Attachment #8817783 -
Attachment is obsolete: true
Attachment #8817955 -
Flags: review?(arai.unmht)
Reporter | ||
Comment 25•8 years ago
|
||
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+
Assignee | ||
Comment 26•8 years ago
|
||
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 .
Reporter | ||
Comment 27•8 years ago
|
||
can you post the current patch and the error you get?
Assignee | ||
Comment 28•8 years ago
|
||
Attachment #8817955 -
Attachment is obsolete: true
Attachment #8818200 -
Flags: review?(arai.unmht)
Reporter | ||
Comment 29•8 years ago
|
||
can you also post the compiler output ?
Assignee | ||
Comment 30•8 years ago
|
||
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
Reporter | ||
Comment 31•8 years ago
|
||
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+
Assignee | ||
Comment 32•8 years ago
|
||
Attachment #8818200 -
Attachment is obsolete: true
Attachment #8818215 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 33•8 years ago
|
||
Thanks a lot for all your help :)
Reporter | ||
Comment 34•8 years ago
|
||
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+
Assignee | ||
Comment 35•8 years ago
|
||
Attachment #8818215 -
Attachment is obsolete: true
Attachment #8818216 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 36•8 years ago
|
||
i havent actually been able to see what you mean by whitespace ?
Reporter | ||
Comment 37•8 years ago
|
||
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+
Reporter | ||
Comment 38•8 years ago
|
||
(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.
Assignee | ||
Comment 39•8 years ago
|
||
Finally :P
Attachment #8818216 -
Attachment is obsolete: true
Attachment #8818220 -
Flags: review?(arai.unmht)
Reporter | ||
Comment 40•8 years ago
|
||
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+
Reporter | ||
Comment 41•8 years ago
|
||
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
Assignee | ||
Comment 42•8 years ago
|
||
is it the same as in checking the box of checkin-needed like i did with review ?
Reporter | ||
Comment 43•8 years ago
|
||
In "Keywords" field at the top of bug page, you can add it.
Assignee | ||
Comment 44•8 years ago
|
||
ok thank you and will i be assigned this bug at the completion of the entire procedure ?
Reporter | ||
Comment 45•8 years ago
|
||
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
Assignee | ||
Comment 46•8 years ago
|
||
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 ?
Reporter | ||
Comment 47•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 48•8 years ago
|
||
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
Comment 49•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 50•8 years ago
|
||
No need to backport this to aurora
You need to log in
before you can comment on or make changes to this bug.
Description
•