Closed Bug 851044 Opened 9 years ago Closed 9 years ago

[OS.File] Logging should use |toString| when available

Categories

(Toolkit :: OS.File, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: Yoric, Assigned: rabbidrabbit)

Details

(Whiteboard: [mentor=Yoric][lang=js])

Attachments

(1 file, 12 obsolete files)

4.34 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
In OS.File, function LOG automatically stringifies all objects. We should be a little smarter: when the object has a method |toString| that is not |Object.prototype.toString|, we should use that method.
I want to work on this bug.I am well acquainted with the knowledge of c/c++.I think it this first bug would be a great opportunity for me.
Hi Bablu Kumar.
That's great :) Unfortunately, I will be mostly unreachable until March 26th, so I will not be able to follow this work closely.

Note that this is JavaScript work.

Yura, since this will touch one of your contributions, would you be available to give a hand?
Flags: needinfo?(yura.zenevich)
(In reply to David Rajchenbach Teller [:Yoric] <on workweek, will not follow bugzilla until March 26th> from comment #2)
> Hi Bablu Kumar.
> That's great :) Unfortunately, I will be mostly unreachable until March
> 26th, so I will not be able to follow this work closely.
> 
> Note that this is JavaScript work.
> 
> Yura, since this will touch one of your contributions, would you be
> available to give a hand?

Yes, definitely, I'll be around to help!
Flags: needinfo?(yura.zenevich)
Hi Bablu Kumar,

Let me know if you have any questions about this bug.

Also, feel free to assign it to yourself.
Hi, I'm interested in working on this bug. Do I just asign myself it?
Jonathan: well, that depends on whether Bablu Kumar is actually working on the bug.

Bablu Kumar, what's your status?
Flags: needinfo?(bk.spartann)
I'm not able to devote much time to this bug.
let this bug be assigned to Jonathan Laver.
Flags: needinfo?(bk.spartann)
In that case, Jonathan, have fun.
If you have any question, do not hesitate to ask, preferably over irc (server irc.mozilla.org, channel #introduction). Both yzen and myself will be glad to help you.
Assignee: nobody → jonathan.laver
I played around with the test a little bit and it looks like the observe method is applied too late (after the test debugging is already disabled and the test is finished).

I have, however, successfully ran the test where I unregistered a listener inside the observe function itself, right after the actual asserts of the log messages.
This makes sense. Unregistering from the observe function is generally a good idea.
Just realised that I didn't use the result of arg.toString() which I cache.
Attachment #730162 - Flags: feedback?(dteller)
Attachment #730121 - Attachment is obsolete: true
Attachment #730162 - Flags: review?(dteller)
Comment on attachment 730162 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

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

Thanks, good patch.
Could you please proceed with the minor changes detailed below?

::: toolkit/components/osfile/osfile_shared_allthreads.jsm
@@ +102,5 @@
>         };
>       }
>  
>       /**
> +      * Returns arg.toString if avialable, otherwise

Nit: "available".
Also: |arg.toString()|, with the parenthese (and preferably the pipes, although they were previously absent).

@@ +103,5 @@
>       }
>  
>       /**
> +      * Returns arg.toString if avialable, otherwise
> +      * applys JSON.Stringify.

Nit: "applies"
Also: "stringify" (lower case)

@@ +118,5 @@
> +
> +         // As every module has its own implimentation of
> +         // Object a comparison between arg.toString and
> +         // Object.prototype.toString cannot be made.
> +         // This hack works around the issue.

I would write something along the lines of "The only way to detect whether this object has a non-default implementation of |toString| is to check whether it returns '[object Object]'. Unfortunately, we cannot simply compare |arg.toString| and |Object.prototype.toString| as |arg| typically comes from distinct compartments."

@@ +119,5 @@
> +         // As every module has its own implimentation of
> +         // Object a comparison between arg.toString and
> +         // Object.prototype.toString cannot be made.
> +         // This hack works around the issue.
> +         if(argToString === "[object Object]") {

Nit: By convention, "if (" (whitespace).

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +892,5 @@
>    });
> +});
> +
> +/**
> + * Tests logging by passing OS.Shared.LOG both an object with its own 

Nit: Please remove the whitespace at the end of the line.

@@ +935,5 @@
> +    // Save original DEBUG value.
> +    let originalPref = OS.Shared.DEBUG;
> +    toggleDebugTest(true);
> +    let objectDefault = {name: "test"};
> +    let customToString = function() {

By convention, we generally name constructors with an uppercase.

@@ +946,5 @@
> +
> +    OS.Shared.LOG(objectDefault);
> +    OS.Shared.LOG(objectCustom);
> +    // Restore DEBUG to its original.
> +    OS.Shared.DEBUG = originalPref;

That line doesn't look necessary, does it?
Attachment #730162 - Flags: feedback?(dteller) → feedback+
Comment on attachment 730162 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

You should first proceed to the minor changes indicated above, before we proceed to review.
Attachment #730162 - Flags: review?(dteller)
Comment on attachment 730175 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

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

::: toolkit/components/osfile/osfile_shared_allthreads.jsm
@@ +119,5 @@
> +         /**
> +           * The only way to detect whether this object has a non-default
> +           * implementation of |toString| is to check whether it returns
> +           * '[object Object]'. Unfortunately, we cannot simply compare |arg.toString|
> +           * and |Object.prototype.toString| as |arg| typically comes from distinct

My bad: "from another compartment"

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +930,5 @@
> +        consoleListener);
> +    }
> +
> +    OS.Shared.DEBUG = true;
> +    OS.Shared.TEST = true;

I read the code too quickly. Could you please move these two lines inside |toggleConsoleListener|? The objective is to reset |DEBUG| and |TEST| to |false| once |toggleConsoleListener| is called for the second time.

You will also need to add a comment at the end of this test to mention that DEBUG and TEST are reset to false once the message is observed.
Attachment #730175 - Flags: review?(dteller) → feedback+
Attachment #730344 - Flags: feedback? → feedback?(dteller)
Attachment #730368 - Flags: feedback?(dteller)
Comment on attachment 730368 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

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

I would like to be sure that we understand the DEBUG/TEST problem before I review your patch.
Attachment #730368 - Flags: feedback?(dteller)
Comment on attachment 730883 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

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

That looks good.
Now, ideally, this test should be turned into an independent xpcshell test, to make it faster and simpler to use. Do you feel up to the task of doing this now or would you prefer leaving this as a followup bug?

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +925,5 @@
> +            test.is(aMessage.message, "TEST OS name is test\n",
> +              "Message stringified using toString");
> +            toggleConsoleListener(false);
> +            deferred.resolve();
> +          }, 0);

Please add a comment explaining why you have to add this setTimeout.
Also, you should only need the setTimeout after |test.is(...)|.
Attachment #730883 - Flags: feedback?(dteller) → feedback+
Setting |DEBUG| prints to the console, though you're right in that I do not need |deferred.resolve()| in the setTimout.

I'm happy to convert the test into an xpcshell test.
Actually, you should leave the deferred.resolve() in the timeout. I just wanted to not put the |test.is| in it.
Ah yes. Of course.
Attachment #731105 - Flags: feedback? → feedback?(dteller)
Comment on attachment 731105 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

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

Please don't put the test in test_osfile_async.js. You should create a new test file.
Attachment #731105 - Flags: feedback?(dteller) → feedback+
Attachment #731108 - Attachment is obsolete: true
Attachment #731108 - Flags: feedback?(dteller)
Attachment #731111 - Flags: feedback?(dteller)
Comment on attachment 731111 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Just noticed that I appear to get an error at the end of my test.
Attachment #731111 - Flags: feedback?(dteller)
Hmm. changing the delay of do_timeout from 0 to 40 fixes the error. do_timeout must work differently to setTimeout. Don't like the magic number though.
(In reply to Jonathan Laver [:rabbidrabbit] from comment #31)
> Hmm. changing the delay of do_timeout from 0 to 40 fixes the error.
> do_timeout must work differently to setTimeout. Don't like the magic number
> though.

This is I believe is due to the fact that the actual minimum delay when you call setTimeout is 4ms (see https://developer.mozilla.org/en/docs/DOM/window.setTimeout#Minimum_delay_and_timeout_nesting). do_timeout probably handles the actual value.
Attachment #731136 - Flags: feedback?(dteller)
Just noticed |do_execute_soon()| in the xpcshell docs. Looks like I want to use that instead of |do_timeout|.
Attachment #731136 - Attachment is obsolete: true
Attachment #731136 - Flags: feedback?(dteller)
Attachment #731203 - Flags: feedback?(dteller)
Comment on attachment 731203 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

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

Looks good to me. And yes, do_execute_soon is the right choice.
Attachment #731203 - Flags: feedback?(dteller) → feedback+
Comment on attachment 733780 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

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

Thanks. Ready to go!
Attachment #733780 - Flags: review+
Attachment #733780 - Flags: checkin?
Attachment #733780 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/8f6b9b15be23
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.