Last Comment Bug 851044 - [OS.File] Logging should use |toString| when available
: [OS.File] Logging should use |toString| when available
Status: RESOLVED FIXED
[mentor=Yoric][lang=js]
:
Product: Toolkit
Classification: Components
Component: OS.File (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla23
Assigned To: Jonathan Laver [:rabbidrabbit]
:
: David Teller [:Yoric] (please use "needinfo")
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-14 04:45 PDT by David Teller [:Yoric] (please use "needinfo")
Modified: 2013-04-05 13:30 PDT (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString (4.30 KB, patch)
2013-03-27 06:42 PDT, Jonathan Laver [:rabbidrabbit]
no flags Details | Diff | Splinter Review
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString (4.30 KB, patch)
2013-03-27 07:58 PDT, Jonathan Laver [:rabbidrabbit]
dteller: feedback+
Details | Diff | Splinter Review
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString (4.29 KB, patch)
2013-03-27 08:31 PDT, Jonathan Laver [:rabbidrabbit]
dteller: feedback+
Details | Diff | Splinter Review
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString (4.61 KB, patch)
2013-03-27 14:04 PDT, Jonathan Laver [:rabbidrabbit]
no flags Details | Diff | Splinter Review
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString (4.49 KB, patch)
2013-03-27 14:48 PDT, Jonathan Laver [:rabbidrabbit]
no flags Details | Diff | Splinter Review
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString (4.58 KB, patch)
2013-03-28 14:22 PDT, Jonathan Laver [:rabbidrabbit]
dteller: feedback+
Details | Diff | Splinter Review
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString (4.10 KB, patch)
2013-03-29 03:35 PDT, Jonathan Laver [:rabbidrabbit]
dteller: feedback+
Details | Diff | Splinter Review
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString (4.04 KB, patch)
2013-03-29 03:45 PDT, Jonathan Laver [:rabbidrabbit]
no flags Details | Diff | Splinter Review
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString (4.03 KB, patch)
2013-03-29 03:49 PDT, Jonathan Laver [:rabbidrabbit]
no flags Details | Diff | Splinter Review
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString (4.03 KB, patch)
2013-03-29 05:57 PDT, Jonathan Laver [:rabbidrabbit]
no flags Details | Diff | Splinter Review
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString (4.02 KB, patch)
2013-03-29 09:55 PDT, Jonathan Laver [:rabbidrabbit]
dteller: feedback+
Details | Diff | Splinter Review
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString (4.33 KB, patch)
2013-04-05 02:32 PDT, Jonathan Laver [:rabbidrabbit]
no flags Details | Diff | Splinter Review
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString (4.34 KB, patch)
2013-04-05 02:36 PDT, Jonathan Laver [:rabbidrabbit]
dteller: review+
Details | Diff | Splinter Review

Description David Teller [:Yoric] (please use "needinfo") 2013-03-14 04:45:18 PDT
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.
Comment 1 Bablu Kumar 2013-03-14 13:33:12 PDT
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.
Comment 2 David Teller [:Yoric] (please use "needinfo") 2013-03-14 14:45:04 PDT
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?
Comment 3 Yura Zenevich [:yzen] 2013-03-14 14:50:58 PDT
(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!
Comment 4 Yura Zenevich [:yzen] 2013-03-18 21:00:34 PDT
Hi Bablu Kumar,

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

Also, feel free to assign it to yourself.
Comment 5 Jonathan Laver [:rabbidrabbit] 2013-03-24 16:38:05 PDT
Hi, I'm interested in working on this bug. Do I just asign myself it?
Comment 6 David Teller [:Yoric] (please use "needinfo") 2013-03-25 01:58:57 PDT
Jonathan: well, that depends on whether Bablu Kumar is actually working on the bug.

Bablu Kumar, what's your status?
Comment 7 Bablu Kumar 2013-03-25 07:36:27 PDT
I'm not able to devote much time to this bug.
let this bug be assigned to Jonathan Laver.
Comment 8 David Teller [:Yoric] (please use "needinfo") 2013-03-25 07:43:28 PDT
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.
Comment 9 Yura Zenevich [:yzen] 2013-03-26 20:30:48 PDT
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.
Comment 10 David Teller [:Yoric] (please use "needinfo") 2013-03-27 03:57:01 PDT
This makes sense. Unregistering from the observe function is generally a good idea.
Comment 11 Jonathan Laver [:rabbidrabbit] 2013-03-27 06:42:46 PDT
Created attachment 730121 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Comment 12 Jonathan Laver [:rabbidrabbit] 2013-03-27 07:58:06 PDT
Created attachment 730162 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Comment 13 Jonathan Laver [:rabbidrabbit] 2013-03-27 07:59:37 PDT
Just realised that I didn't use the result of arg.toString() which I cache.
Comment 14 David Teller [:Yoric] (please use "needinfo") 2013-03-27 08:14:12 PDT
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?
Comment 15 David Teller [:Yoric] (please use "needinfo") 2013-03-27 08:17:59 PDT
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.
Comment 16 Jonathan Laver [:rabbidrabbit] 2013-03-27 08:31:35 PDT
Created attachment 730175 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Comment 17 David Teller [:Yoric] (please use "needinfo") 2013-03-27 09:14:22 PDT
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.
Comment 18 Jonathan Laver [:rabbidrabbit] 2013-03-27 14:04:18 PDT
Created attachment 730344 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Comment 19 Jonathan Laver [:rabbidrabbit] 2013-03-27 14:48:45 PDT
Created attachment 730368 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Comment 20 David Teller [:Yoric] (please use "needinfo") 2013-03-28 08:04:50 PDT
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.
Comment 21 Jonathan Laver [:rabbidrabbit] 2013-03-28 14:22:19 PDT
Created attachment 730883 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Comment 22 David Teller [:Yoric] (please use "needinfo") 2013-03-29 02:29:35 PDT
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(...)|.
Comment 23 Jonathan Laver [:rabbidrabbit] 2013-03-29 02:38:24 PDT
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.
Comment 24 David Teller [:Yoric] (please use "needinfo") 2013-03-29 02:39:22 PDT
Actually, you should leave the deferred.resolve() in the timeout. I just wanted to not put the |test.is| in it.
Comment 25 Jonathan Laver [:rabbidrabbit] 2013-03-29 02:43:20 PDT
Ah yes. Of course.
Comment 26 Jonathan Laver [:rabbidrabbit] 2013-03-29 03:35:58 PDT
Created attachment 731105 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Comment 27 David Teller [:Yoric] (please use "needinfo") 2013-03-29 03:40:19 PDT
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.
Comment 28 Jonathan Laver [:rabbidrabbit] 2013-03-29 03:45:53 PDT
Created attachment 731108 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Comment 29 Jonathan Laver [:rabbidrabbit] 2013-03-29 03:49:41 PDT
Created attachment 731111 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Comment 30 Jonathan Laver [:rabbidrabbit] 2013-03-29 05:19:04 PDT
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.
Comment 31 Jonathan Laver [:rabbidrabbit] 2013-03-29 05:46:11 PDT
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.
Comment 32 Yura Zenevich [:yzen] 2013-03-29 05:51:47 PDT
(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.
Comment 33 Jonathan Laver [:rabbidrabbit] 2013-03-29 05:57:51 PDT
Created attachment 731136 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Comment 34 Jonathan Laver [:rabbidrabbit] 2013-03-29 09:55:46 PDT
Created attachment 731203 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Just noticed |do_execute_soon()| in the xpcshell docs. Looks like I want to use that instead of |do_timeout|.
Comment 35 David Teller [:Yoric] (please use "needinfo") 2013-04-02 02:52:37 PDT
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.
Comment 36 David Teller [:Yoric] (please use "needinfo") 2013-04-02 02:54:57 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=5d952c18be44
Comment 37 David Teller [:Yoric] (please use "needinfo") 2013-04-03 01:45:32 PDT
Sorry, wrong try link: https://tbpl.mozilla.org/?tree=Try&rev=09db2217def0
Comment 38 Jonathan Laver [:rabbidrabbit] 2013-04-05 02:32:03 PDT
Created attachment 733779 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Add commit message.
Comment 39 Jonathan Laver [:rabbidrabbit] 2013-04-05 02:36:15 PDT
Created attachment 733780 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

add r=Yoric
Comment 40 David Teller [:Yoric] (please use "needinfo") 2013-04-05 02:37:24 PDT
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!
Comment 41 Ryan VanderMeulen [:RyanVM] 2013-04-05 05:52:20 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f6b9b15be23
Comment 42 Ryan VanderMeulen [:RyanVM] 2013-04-05 13:30:38 PDT
https://hg.mozilla.org/mozilla-central/rev/8f6b9b15be23

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