Closed
Bug 858482
Opened 8 years ago
Closed 8 years ago
[OS.File] Expose macro DEBUG to OS.Constants
Categories
(Toolkit :: OS.File, defect)
Toolkit
OS.File
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: Yoric, Assigned: j4nu5)
Details
(Whiteboard: [mentor=Yoric][lang=c++])
Attachments
(2 files, 8 obsolete files)
887 bytes,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
3.88 KB,
patch
|
Details | Diff | Splinter Review |
For debugging purposes, it would be useful to expose C macro DEBUG to JavaScript through OS.Constants.
Comment 1•8 years ago
|
||
Hello, I am interested to take up the bug for solving. Can you please give me some more info on it. I'm a newbie to firefox codebase but having knowledge of c,c++,javascript,php etc. Thanks Gaurab Patra
Reporter | ||
Comment 2•8 years ago
|
||
Great :) First thing to remember: don't hesitate to ask questions, preferably over IRC at http://client02.chat.mibbit.com/?server=irc.mozilla.org&channel=%23introduction&nick=GaurabPatra Now, I will assume that you already know how to build Firefox. If this is not the case, you should take a look here: https://developer.mozilla.org/en-US/docs/introduction The idea is to add a field called "DEBUG" to object OS.Constants.Sys. The code that defines OS.Constants.Sys is in the following file: http://dxr.mozilla.org/mozilla-central/dom/system/OSFileConstants.cpp?from=OSFileConstants.cpp, around line 700. You can find the reference of most functions used here: https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference
Comment 3•8 years ago
|
||
I wants to work on this bug. I have prerequisite knowledge of c,c++,javascript, Html and Css. So please assign this to me.
![]() |
||
Updated•8 years ago
|
Assignee: nobody → rahulgandhi38
![]() |
||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•8 years ago
|
||
Rahul is getting warmed up with bug 857077. Until he's done, let's keep this bug unassigned.
Assignee: rahulgandhi38 → nobody
Status: ASSIGNED → NEW
Reporter | ||
Comment 6•8 years ago
|
||
Ah, well, we have a motivated candidate who pinged me over irc :) Assigning the bug. Gaurab, if you want to work on another bug, feel free to ping me.
Assignee: nobody → sinha.kushagra
Flags: needinfo?(gaurabpa1992)
Assignee | ||
Comment 7•8 years ago
|
||
Assignee | ||
Comment 8•8 years ago
|
||
I have written a patch for this bug. Can someone please help me on how to write test-cases and docs? Thanks!
Status: NEW → ASSIGNED
Reporter | ||
Comment 9•8 years ago
|
||
Comment on attachment 739476 [details] [diff] [review] OS.Constants.Sys.DEBUG will be set to 1 if its a build configuration, will be non-existent otherwise Review of attachment 739476 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/OSFileConstants.cpp @@ +712,5 @@ > } > } > > +#if defined(DEBUG) > + if (!SetStringProperty(cx, objSys, "DEBUG", nsAutoString('1'))) { That looks good, but instead of a string, we should put a boolean. Use |JS_SetProperty| instead of the utility function |SetStringProperty|, and |JSVAL_TRUE| for the value. If you need it, you can find the reference of JSAPI here: https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference
Attachment #739476 -
Flags: feedback+
Reporter | ||
Comment 10•8 years ago
|
||
Thanks for the patch :) Now, for tests. You are testing a simple property, but the writing test might be quite tricky. Let me suggest the following thing: - create a new xpcshell test https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests - place this test in a new directory dom/system/tests/xpcshell - in this test, use #ifdef DEBUG as needed - in the Makefile.in, you will need to politely request that the test should be run through the preprocessor, using something along the lines of http://dxr.mozilla.org/mozilla-central/toolkit/components/ctypes/tests/Makefile.in#l49 (lines 49, 50, 51 and 56) Have fun :) Don't hesitate to ask questions over irc.
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #739476 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #739496 -
Flags: feedback+
Reporter | ||
Comment 12•8 years ago
|
||
Ah, someone pointed to me that there is a simpler solution than Makefile.in hacking. So, alternative technique - create a xpcshell test, as above; - in that test, use nsIDebug2::isDebugBuild to determine whether DEBUG is true. See http://dxr.mozilla.org/mozilla-central/toolkit/components/telemetry/TelemetryPing.js#l150 for how to instantiate nsIDebug2.
Assignee | ||
Comment 13•8 years ago
|
||
Can't we write a test-case in dom/system/tests/test_constants.xul ?
Reporter | ||
Comment 14•8 years ago
|
||
Yes, that should work. Generally, whenever possible, we try and write xpcshell tests, because they isolate behaviors more efficiently. But you can start with test_constants.xul, which is a mochitest.
Assignee | ||
Comment 15•8 years ago
|
||
Added chrome mochitest to check the value of OS.Constants.Sys.DEBUG
Assignee | ||
Updated•8 years ago
|
Attachment #739673 -
Flags: review?(sinha.kushagra)
Assignee | ||
Updated•8 years ago
|
Attachment #739673 -
Flags: review?(sinha.kushagra) → review?(dteller)
Assignee | ||
Updated•8 years ago
|
Attachment #739496 -
Flags: review?(dteller)
Reporter | ||
Comment 16•8 years ago
|
||
Comment on attachment 739673 [details] [diff] [review] Testcases for the above patch Review of attachment 739673 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, with the minor change below. ::: dom/system/tests/test_constants.xul @@ +68,5 @@ > + is(true, OS.Constants.Sys.DEBUG, "OS.Constants.Sys.DEBUG is set properly"); > + } > + else { > + is(null, OS.Constants.Sys.DEBUG, "OS.Constants.Sys.DEBUG is set properly"); > + } You could just write is(OS.Constants.Sys.DEBUG, debugService.isDebugBuild, "...");
Attachment #739673 -
Flags: review?(dteller) → review+
Reporter | ||
Comment 17•8 years ago
|
||
Comment on attachment 739496 [details] [diff] [review] OS.Constants.Sys.DEBUG will be set to true if its a build configuration, will be non-existent otherwise Review of attachment 739496 [details] [diff] [review]: ----------------------------------------------------------------- Accepted, with the minor change below. ::: dom/system/OSFileConstants.cpp @@ +712,5 @@ > } > } > > +#if defined(DEBUG) > + JS::Value debug = JSVAL_TRUE; Could you rename this valDebug?
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #739673 -
Attachment is obsolete: true
Attachment #739721 -
Flags: review?(dteller)
Assignee | ||
Comment 19•8 years ago
|
||
Attachment #739496 -
Attachment is obsolete: true
Attachment #739496 -
Flags: review?(dteller)
Attachment #739724 -
Flags: review?
Assignee | ||
Updated•8 years ago
|
Attachment #739724 -
Flags: review? → review?(dteller)
Reporter | ||
Comment 20•8 years ago
|
||
Comment on attachment 739721 [details] [diff] [review] Mochitest testcases Review of attachment 739721 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Please don't forget to add ";r=yoric" at the end of your commit message.
Attachment #739721 -
Flags: review?(dteller) → review+
Reporter | ||
Comment 21•8 years ago
|
||
Comment on attachment 739724 [details] [diff] [review] OS.Constants.Sys.DEBUG will be set to true if its a debug build configuration, will be non-existent otherwise Review of attachment 739724 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.
Attachment #739724 -
Flags: review?(dteller) → review+
Reporter | ||
Comment 22•8 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=9c7c69a4bc84
Reporter | ||
Comment 23•8 years ago
|
||
Oops, forgot to actually launch mochitest. Try run: https://tbpl.mozilla.org/?tree=Try&rev=5da8f1bcf64b
Assignee | ||
Comment 24•8 years ago
|
||
Updated commit message to add r=yoric
Attachment #739724 -
Attachment is obsolete: true
Assignee | ||
Comment 25•8 years ago
|
||
Attachment #739721 -
Attachment is obsolete: true
Reporter | ||
Comment 26•8 years ago
|
||
Comment on attachment 739956 [details] [diff] [review] OS.Constants.Sys.DEBUG will be set to true if its a debug build configuration, will be non-existent otherwise Review of attachment 739956 [details] [diff] [review]: ----------------------------------------------------------------- I realize that I don't have review rights for this module. Passing the baby to khuey.
Attachment #739956 -
Flags: review?(khuey)
Reporter | ||
Comment 27•8 years ago
|
||
Comment on attachment 739957 [details] [diff] [review] Mochitest testcases Review of attachment 739957 [details] [diff] [review]: ----------------------------------------------------------------- As above.
Attachment #739957 -
Flags: review?(khuey)
What is the motivation for not using nsIDebug2?
Flags: needinfo?(dteller)
Attachment #739956 -
Flags: review?(khuey) → review+
Comment on attachment 739957 [details] [diff] [review] Mochitest testcases Review of attachment 739957 [details] [diff] [review]: ----------------------------------------------------------------- You should test the DEBUG property off the main thread, since that's the primary motivation for this patch.
Attachment #739957 -
Flags: review?(khuey) → review-
Assignee | ||
Comment 31•8 years ago
|
||
Changed reviewer from yoric to khuey
Attachment #739956 -
Attachment is obsolete: true
Attachment #740353 -
Flags: review?(khuey)
Assignee | ||
Comment 32•8 years ago
|
||
Added Mochitest testcases for checking DEBUG value in worker thread, in addition to main thread
Attachment #739957 -
Attachment is obsolete: true
Attachment #740356 -
Flags: review?(khuey)
Comment on attachment 740353 [details] [diff] [review] OS.Constants.Sys.DEBUG will be set to true if its a debug build configuration, will be non-existent otherwise Review of attachment 740353 [details] [diff] [review]: ----------------------------------------------------------------- I already reviewed this.
Attachment #740353 -
Flags: review?(khuey) → review+
Comment on attachment 740356 [details] [diff] [review] Mochitest testcases Review of attachment 740356 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/tests/test_constants.xul @@ +74,5 @@ > Components.utils.import("resource://gre/modules/ctypes.jsm"); > test_xul(); > test_libc(); > test_Win(); > + Extra whitespace at EOL. @@ +106,5 @@ > SimpleTest.ok(false, "test_constants.xul: wrong message " + JSON.stringify(msg.data)); > return; > } > }; > + Extra whitespace at EOL.
Attachment #740356 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 35•8 years ago
|
||
Removed whitespaces after EOL
Attachment #740356 -
Attachment is obsolete: true
Reporter | ||
Comment 36•8 years ago
|
||
Try: ttps://tbpl.mozilla.org/?tree=Try&rev=5b603c43d742
Assignee | ||
Comment 37•8 years ago
|
||
^ Tests successful! Check-in please :)
Reporter | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 38•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9fa331e7a0f https://hg.mozilla.org/integration/mozilla-inbound/rev/12700c8a8355
Flags: in-testsuite+
Keywords: checkin-needed
Comment 39•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a9fa331e7a0f https://hg.mozilla.org/mozilla-central/rev/12700c8a8355
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•