858482 - [OS.File] Expose macro DEBUG to OS.Constants

Status: RESOLVED FIXED

Description

[David Teller [:Yoric] (please use "needinfo")]

For debugging purposes, it would be useful to expose C macro DEBUG to JavaScript through OS.Constants.

Comment 1

[Gaurab Patra]

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 Teller [:Yoric] (please use "needinfo")]

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]

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 Teller [:Yoric] (please use "needinfo")]

Rahul is getting warmed up with bug 857077. Until he's done, let's keep this bug unassigned.

Comment 5

[David Teller [:Yoric] (please use "needinfo")]

Any news, Gaurab?

Comment 6

[David Teller [:Yoric] (please use "needinfo")]

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]]

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]]

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 Teller [:Yoric] (please use "needinfo")]

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 Teller [:Yoric] (please use "needinfo")]

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]]

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 Teller [:Yoric] (please use "needinfo")]

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]]

Can't we write a test-case in dom/system/tests/test_constants.xul ?

Comment 14

[David Teller [:Yoric] (please use "needinfo")]

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]]

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 Teller [:Yoric] (please use "needinfo")]

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 Teller [:Yoric] (please use "needinfo")]

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]]

Created attachment 739721 [details] [diff] [review]
Mochitest testcases

Comment 19

[Kushagra Sinha [:j4nu5]]

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 Teller [:Yoric] (please use "needinfo")]

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 Teller [:Yoric] (please use "needinfo")]

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 Teller [:Yoric] (please use "needinfo")]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=9c7c69a4bc84

Comment 23

[David Teller [:Yoric] (please use "needinfo")]

Oops, forgot to actually launch mochitest.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=5da8f1bcf64b

Comment 24

[Kushagra Sinha [:j4nu5]]

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]]

Created attachment 739957 [details] [diff] [review]
Mochitest testcases

Comment 26

[David Teller [:Yoric] (please use "needinfo")]

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 Teller [:Yoric] (please use "needinfo")]

Comment on attachment 739957 [details] [diff] [review]
Mochitest testcases

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

As above.

Comment 28

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

What is the motivation for not using nsIDebug2?

Comment 29

[David Teller [:Yoric] (please use "needinfo")]

As usual, making things work OMT.

Comment 30

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

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]]

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]]

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] (Exited; not receiving bugmail, email if necessary)]

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] (Exited; not receiving bugmail, email if necessary)]

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]]

Created attachment 740901 [details] [diff] [review]
Mochitest testcases

Removed whitespaces after EOL

Comment 36

[David Teller [:Yoric] (please use "needinfo")]

Try: ttps://tbpl.mozilla.org/?tree=Try&rev=5b603c43d742

Comment 37

[Kushagra Sinha [:j4nu5]]

^ Tests successful! Check-in please :)

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9fa331e7a0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/12700c8a8355

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a9fa331e7a0f
https://hg.mozilla.org/mozilla-central/rev/12700c8a8355