Last Comment Bug 858482 - [OS.File] Expose macro DEBUG to OS.Constants
: [OS.File] Expose macro DEBUG to OS.Constants
Status: RESOLVED FIXED
[mentor=Yoric][lang=c++]
:
Product: Toolkit
Classification: Components
Component: OS.File (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla23
Assigned To: Kushagra Sinha [:j4nu5]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-05 03:40 PDT by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2013-04-24 15:02 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
OS.Constants.Sys.DEBUG will be set to 1 if its a build configuration, will be non-existent otherwise (853 bytes, patch)
2013-04-19 02:22 PDT, Kushagra Sinha [:j4nu5]
dteller: feedback+
Details | Diff | Splinter Review
OS.Constants.Sys.DEBUG will be set to true if its a build configuration, will be non-existent otherwise (872 bytes, patch)
2013-04-19 02:55 PDT, Kushagra Sinha [:j4nu5]
dteller: feedback+
Details | Diff | Splinter Review
Testcases for the above patch (1.83 KB, patch)
2013-04-19 10:51 PDT, Kushagra Sinha [:j4nu5]
dteller: review+
Details | Diff | Splinter Review
Mochitest testcases (1.70 KB, patch)
2013-04-19 12:05 PDT, Kushagra Sinha [:j4nu5]
dteller: review+
Details | Diff | Splinter Review
OS.Constants.Sys.DEBUG will be set to true if its a debug build configuration, will be non-existent otherwise (878 bytes, patch)
2013-04-19 12:11 PDT, Kushagra Sinha [:j4nu5]
dteller: review+
Details | Diff | Splinter Review
OS.Constants.Sys.DEBUG will be set to true if its a debug build configuration, will be non-existent otherwise (887 bytes, patch)
2013-04-20 05:09 PDT, Kushagra Sinha [:j4nu5]
khuey: review+
Details | Diff | Splinter Review
Mochitest testcases (1.71 KB, patch)
2013-04-20 05:10 PDT, Kushagra Sinha [:j4nu5]
khuey: review-
Details | Diff | Splinter Review
OS.Constants.Sys.DEBUG will be set to true if its a debug build configuration, will be non-existent otherwise (887 bytes, patch)
2013-04-22 10:20 PDT, Kushagra Sinha [:j4nu5]
khuey: review+
Details | Diff | Splinter Review
Mochitest testcases (3.88 KB, patch)
2013-04-22 10:23 PDT, Kushagra Sinha [:j4nu5]
khuey: review+
Details | Diff | Splinter Review
Mochitest testcases (3.88 KB, patch)
2013-04-23 10:14 PDT, Kushagra Sinha [:j4nu5]
no flags Details | Diff | Splinter Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-05 03:40:39 PDT
For debugging purposes, it would be useful to expose C macro DEBUG to JavaScript through OS.Constants.
Comment 1 Gaurab Patra 2013-04-06 05:35:51 PDT
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
Comment 2 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-06 06:35:50 PDT
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 Rahul Gandhi 2013-04-07 11:55:33 PDT
I wants to work on this bug. I have prerequisite knowledge of  c,c++,javascript, Html and Css.
So please assign this to me.
Comment 4 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-15 01:46:10 PDT
Rahul is getting warmed up with bug 857077. Until he's done, let's keep this bug unassigned.
Comment 5 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-17 04:59:18 PDT
Any news, Gaurab?
Comment 6 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-18 13:22:51 PDT
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.
Comment 7 Kushagra Sinha [:j4nu5] 2013-04-19 02:22:50 PDT
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
Comment 8 Kushagra Sinha [:j4nu5] 2013-04-19 02:24:45 PDT
I have written a patch for this bug. Can someone please help me on how to write test-cases and docs? Thanks!
Comment 9 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-19 02:28:32 PDT
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
Comment 10 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-19 02:40:56 PDT
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.
Comment 11 Kushagra Sinha [:j4nu5] 2013-04-19 02:55:39 PDT
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
Comment 12 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-19 06:30:28 PDT
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.
Comment 13 Kushagra Sinha [:j4nu5] 2013-04-19 09:14:20 PDT
Can't we write a test-case in dom/system/tests/test_constants.xul ?
Comment 14 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-19 09:39:02 PDT
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.
Comment 15 Kushagra Sinha [:j4nu5] 2013-04-19 10:51:12 PDT
Created attachment 739673 [details] [diff] [review]
Testcases for the above patch

Added chrome mochitest to check the value of OS.Constants.Sys.DEBUG
Comment 16 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-19 11:56:09 PDT
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 17 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-19 11:57:15 PDT
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?
Comment 18 Kushagra Sinha [:j4nu5] 2013-04-19 12:05:29 PDT
Created attachment 739721 [details] [diff] [review]
Mochitest testcases
Comment 19 Kushagra Sinha [:j4nu5] 2013-04-19 12:11:38 PDT
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 20 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-19 12:13:33 PDT
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 21 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-19 12:15:40 PDT
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.
Comment 22 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-19 12:26:07 PDT
Try run: https://tbpl.mozilla.org/?tree=Try&rev=9c7c69a4bc84
Comment 23 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-20 01:11:07 PDT
Oops, forgot to actually launch mochitest.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=5da8f1bcf64b
Comment 24 Kushagra Sinha [:j4nu5] 2013-04-20 05:09:17 PDT
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
Comment 25 Kushagra Sinha [:j4nu5] 2013-04-20 05:10:43 PDT
Created attachment 739957 [details] [diff] [review]
Mochitest testcases
Comment 26 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-20 05:38:13 PDT
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 27 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-20 05:38:45 PDT
Comment on attachment 739957 [details] [diff] [review]
Mochitest testcases

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

As above.
Comment 28 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-04-20 07:22:47 PDT
What is the motivation for not using nsIDebug2?
Comment 29 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-20 09:21:59 PDT
As usual, making things work OMT.
Comment 30 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-04-22 09:24:43 PDT
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.
Comment 31 Kushagra Sinha [:j4nu5] 2013-04-22 10:20:53 PDT
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
Comment 32 Kushagra Sinha [:j4nu5] 2013-04-22 10:23:24 PDT
Created attachment 740356 [details] [diff] [review]
Mochitest testcases

Added Mochitest testcases for checking DEBUG value in worker thread, in addition to main thread
Comment 33 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-04-23 09:43:27 PDT
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 34 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-04-23 09:45:32 PDT
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.
Comment 35 Kushagra Sinha [:j4nu5] 2013-04-23 10:14:55 PDT
Created attachment 740901 [details] [diff] [review]
Mochitest testcases

Removed whitespaces after EOL
Comment 36 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-23 11:30:35 PDT
Try: ttps://tbpl.mozilla.org/?tree=Try&rev=5b603c43d742
Comment 37 Kushagra Sinha [:j4nu5] 2013-04-23 22:42:12 PDT
^ Tests successful! Check-in please :)

Note You need to log in before you can comment on or make changes to this bug.