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

RESOLVED FIXED in mozilla23

Status

()

Toolkit
OS.File
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Yoric, Assigned: rabbidrabbit)

Tracking

Trunk
mozilla23
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 12 obsolete attachments)

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.

Comment 1

4 years ago
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.
(Assignee)

Comment 5

4 years ago
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)

Comment 7

4 years ago
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.
(Assignee)

Comment 11

4 years ago
Created attachment 730121 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
(Assignee)

Comment 12

4 years ago
Created attachment 730162 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
(Assignee)

Comment 13

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

Updated

4 years ago
Attachment #730162 - Flags: feedback?(dteller)
(Assignee)

Updated

4 years ago
Attachment #730121 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
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)
(Assignee)

Comment 16

4 years ago
Created attachment 730175 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Attachment #730162 - Attachment is obsolete: true
Attachment #730175 - 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+
(Assignee)

Comment 18

4 years ago
Created attachment 730344 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Attachment #730175 - Attachment is obsolete: true
Attachment #730344 - Flags: feedback?
(Assignee)

Updated

4 years ago
Attachment #730344 - Flags: feedback? → feedback?(dteller)
(Assignee)

Comment 19

4 years ago
Created attachment 730368 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Attachment #730344 - Attachment is obsolete: true
Attachment #730344 - Flags: feedback?(dteller)
(Assignee)

Updated

4 years ago
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)
(Assignee)

Comment 21

4 years ago
Created attachment 730883 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Attachment #730368 - Attachment is obsolete: true
Attachment #730883 - 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+
(Assignee)

Comment 23

4 years ago
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.
(Assignee)

Comment 25

4 years ago
Ah yes. Of course.
(Assignee)

Comment 26

4 years ago
Created attachment 731105 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Attachment #730883 - Attachment is obsolete: true
Attachment #731105 - Flags: feedback?
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 28

4 years ago
Created attachment 731108 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Attachment #731105 - Attachment is obsolete: true
Attachment #731108 - Flags: feedback?(dteller)
(Assignee)

Comment 29

4 years ago
Created attachment 731111 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Attachment #731108 - Attachment is obsolete: true
Attachment #731108 - Flags: feedback?(dteller)
Attachment #731111 - Flags: feedback?(dteller)
(Assignee)

Comment 30

4 years ago
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)
(Assignee)

Comment 31

4 years ago
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.
(Assignee)

Comment 33

4 years ago
Created attachment 731136 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString
Attachment #731111 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #731136 - Flags: feedback?(dteller)
(Assignee)

Comment 34

4 years ago
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|.
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+
Try: https://tbpl.mozilla.org/?tree=Try&rev=5d952c18be44
Sorry, wrong try link: https://tbpl.mozilla.org/?tree=Try&rev=09db2217def0
(Assignee)

Comment 38

4 years ago
Created attachment 733779 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

Add commit message.
Attachment #731203 - Attachment is obsolete: true
(Assignee)

Comment 39

4 years ago
Created attachment 733780 [details] [diff] [review]
Modifies stringifyArgs to use the objects toString method if it is not Object.prototype.toString

add r=Yoric
Attachment #733779 - Attachment is obsolete: true
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+
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Attachment #733780 - Flags: checkin?
(Assignee)

Updated

4 years ago
Attachment #733780 - Flags: checkin?
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f6b9b15be23
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8f6b9b15be23
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.