Closed Bug 858482 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: OS.File, defect)

Product:
Toolkit
Component:
OS.File
Type:
defect
Priority:
Not set

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla23

People

(Reporter: Yoric, Assigned: j4nu5)

Details

(Whiteboard: [mentor=Yoric][lang=c++])

Attachments

(2 files, 8 obsolete files)

OS.Constants.Sys.DEBUG will be set to true if its a debug build configuration, will be non-existent otherwise
887 bytes, patch
khuey
: review+
Details | Diff | Splinter Review
Mochitest testcases
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.
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.
Assignee: nobody → rahulgandhi38
Status: NEW → ASSIGNED
Rahul is getting warmed up with bug 857077. Until he's done, let's keep this bug unassigned.
Assignee: rahulgandhi38 → nobody
Status: ASSIGNED → NEW
Any news, Gaurab?
Flags: needinfo?(gaurabpa1992)
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)
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
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+
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.
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.
Attached patch Testcases for the above patch (obsolete) — — Splinter Review 
Added chrome mochitest to check the value of OS.Constants.Sys.DEBUG
Attachment #739673 - Flags: review?(sinha.kushagra)
Attachment #739673 - Flags: review?(sinha.kushagra) → review?(dteller)
Attachment #739496 - Flags: review?(dteller)
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+
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?
Attached patch Mochitest testcases (obsolete) — — Splinter Review
Attachment #739673 - Attachment is obsolete: true
Attachment #739721 - Flags: review?(dteller)
Attachment #739724 - Flags: review? → review?(dteller)
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+
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+
Oops, forgot to actually launch mochitest.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=5da8f1bcf64b
Updated commit message to add r=yoric
Attachment #739724 - Attachment is obsolete: true
Attached patch Mochitest testcases (obsolete) — — Splinter Review
Attachment #739721 - Attachment is obsolete: true
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)
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)
As usual, making things work OMT.
Flags: needinfo?(dteller)
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-
Changed reviewer from yoric to khuey
Attachment #739956 - Attachment is obsolete: true
Attachment #740353 - Flags: review?(khuey)
Attached patch Mochitest testcases (obsolete) — — Splinter Review 
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+
Removed whitespaces after EOL
Attachment #740356 - Attachment is obsolete: true
Try: ttps://tbpl.mozilla.org/?tree=Try&rev=5b603c43d742
^ Tests successful! Check-in please :)
https://hg.mozilla.org/mozilla-central/rev/a9fa331e7a0f
https://hg.mozilla.org/mozilla-central/rev/12700c8a8355
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.