For debugging purposes, it would be useful to expose C macro DEBUG to JavaScript through OS.Constants.
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
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
I wants to work on this bug. I have prerequisite knowledge of c,c++,javascript, Html and Css. So please assign this to me.
Rahul is getting warmed up with bug 857077. Until he's done, let's keep this bug unassigned.
Any news, Gaurab?
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.
Created attachment 739476 [details] [diff] [review] OS.Constants.Sys.DEBUG will be set to 1 if its a build configuration, will be non-existent otherwise
I have written a patch for this bug. Can someone please help me on how to write test-cases and docs? Thanks!
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
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.
Created attachment 739496 [details] [diff] [review] OS.Constants.Sys.DEBUG will be set to true if its a build configuration, will be non-existent otherwise
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.
Can't we write a test-case in dom/system/tests/test_constants.xul ?
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.
Created attachment 739673 [details] [diff] [review] Testcases for the above patch Added chrome mochitest to check the value of OS.Constants.Sys.DEBUG
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, "...");
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?
Created attachment 739721 [details] [diff] [review] Mochitest testcases
Created 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
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.
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.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=9c7c69a4bc84
Oops, forgot to actually launch mochitest. Try run: https://tbpl.mozilla.org/?tree=Try&rev=5da8f1bcf64b
Created 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 Updated commit message to add r=yoric
Created attachment 739957 [details] [diff] [review] Mochitest testcases
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.
Comment on attachment 739957 [details] [diff] [review] Mochitest testcases Review of attachment 739957 [details] [diff] [review]: ----------------------------------------------------------------- As above.
What is the motivation for not using nsIDebug2?
As usual, making things work OMT.
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.
Created 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 Changed reviewer from yoric to khuey
Created attachment 740356 [details] [diff] [review] Mochitest testcases Added Mochitest testcases for checking DEBUG value in worker thread, in addition to main thread
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.
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.
Created attachment 740901 [details] [diff] [review] Mochitest testcases Removed whitespaces after EOL
Try: ttps://tbpl.mozilla.org/?tree=Try&rev=5b603c43d742
^ Tests successful! Check-in please :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9fa331e7a0f https://hg.mozilla.org/integration/mozilla-inbound/rev/12700c8a8355
https://hg.mozilla.org/mozilla-central/rev/a9fa331e7a0f https://hg.mozilla.org/mozilla-central/rev/12700c8a8355
or
