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

Status: RESOLVED FIXED

(Toolkit :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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]

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] - still alive but not very active]

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] (please use "needinfo")]

(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] (please use "needinfo")]

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

Hi, I'm interested in working on this bug. Do I just asign myself it?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Jonathan: well, that depends on whether Bablu Kumar is actually working on the bug.

Bablu Kumar, what's your status?

Comment 7

[Bablu Kumar]

I'm not able to devote much time to this bug.
let this bug be assigned to Jonathan Laver.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

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] (please use "needinfo")]

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] - still alive but not very active]

This makes sense. Unregistering from the observe function is generally a good idea.

Comment 11

[Jonathan Laver [:rabbidrabbit]]

Attachment: Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Comment 12

[Jonathan Laver [:rabbidrabbit]]

Attachment: Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Comment 13

[Jonathan Laver [:rabbidrabbit]]

Just realised that I didn't use the result of arg.toString() which I cache.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

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] - still alive but not very active]

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

Attachment: Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Comment 17

[David Teller [:Yoric] - still alive but not very active]

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

Attachment: Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Comment 19

[Jonathan Laver [:rabbidrabbit]]

Attachment: Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Comment 20

[David Teller [:Yoric] - still alive but not very active]

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

Attachment: Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Comment 22

[David Teller [:Yoric] - still alive but not very active]

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

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] - still alive but not very active]

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

Ah yes. Of course.

Comment 26

[Jonathan Laver [:rabbidrabbit]]

Attachment: Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Comment 27

[David Teller [:Yoric] - still alive but not very active]

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

Attachment: Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Comment 29

[Jonathan Laver [:rabbidrabbit]]

Attachment: Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Comment 30

[Jonathan Laver [:rabbidrabbit]]

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

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] (please use "needinfo")]

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

Attachment: Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Comment 34

[Jonathan Laver [:rabbidrabbit]]

Attachment: 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] - still alive but not very active]

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] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=5d952c18be44

Comment 37

[David Teller [:Yoric] - still alive but not very active]

Sorry, wrong try link: https://tbpl.mozilla.org/?tree=Try&rev=09db2217def0

Comment 38

[Jonathan Laver [:rabbidrabbit]]

Attachment: Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Add commit message.

Comment 39

[Jonathan Laver [:rabbidrabbit]]

Attachment: Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

add r=Yoric

Comment 40

[David Teller [:Yoric] - still alive but not very active]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f6b9b15be23

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8f6b9b15be23