668303 - convert mozillafilelogger to specialpowers

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

in keeping with our theme of converting everything to specialpowers, I have a patch which will allow for logging via special powers.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: convert mozillafilelogger to specialpowers (1.0)

this should be ready to go.  I tried to make this generic enough so other tests needing nsILocalFile and nsIFileOutputStream could share these utilities.

Comment 2

[(not currently active) Ted Mielczarek]

I'm not really wild about the API you've exposed here. I think I'd prefer to split the logging and the general file I/O cases to make it cleaner. Then we could just have a few APIs like:
setLogFile(filename)
log(data)
writeFile(filename, data)
readFile(filename)

I think that would cover all of our use cases without having to keep a bunch of file handles laying around in a map, which seems like it'd be more prone to problems.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

that is doable!

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 542902 [details] [diff] [review]
convert mozillafilelogger to specialpowers (1.0)

Clearing this request for now.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

we also have mozillafilelogger.js in reftest (for remote testing), so we need to fix it there or copy an original there.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: convert mozillafilelogger to specialpowers (2.0)

implemented :ted's review feedback on previous patch.  Tested against try server.  This patch also fixes reftest's requirement for MozillaFileLogger since we don't have specialpowers for reftest.

Comment 7

[cmtalbert]

Comment on attachment 545655 [details] [diff] [review]
convert mozillafilelogger to specialpowers (2.0)

So, this is good, but I have some nit-picky reservations here that I'd like to see revised in another draft of the patch.

1. Reftest doesn't need the logcallback at all.  It only uses the direct "log" API and so that's all that's required if we're duplicating the file.
2. I don't really like duplicating the file. I worry that it's going to be hard to keep the APIs in sync between the two implementations of the logging.
2a. If we keep the two files - we should rename the reftest one to "ChromeFileLogger.js" (and rename the underlying objects appropriately).  But the API in it should somehow reference (perhaps in a comment) the other file so that we remember to keep these APIs in sync.

Ideally, I'd prefer to have just one file that presents a unified logging API and that does the right thing based on whether a chrome logger or a specialpowers logger is needed.

Something like:
class MozillaFileLogger {
   setLogFile(path) {  }
   log(msg) { }
   logmsgformatter(SomeCallbackFunctionForFormatting) {}
   close()

then mochitest could do something like:
class SpecialPowersLogger : public MozillaFileLogger
    setLogFile(path) { specialpowers.setLogFile(path) }
    log(msg) { specialpowers.log(msg) }
    ....

and reftest could do something like:
class ChromeLogger : public MozillaFileLogger
    setLogFile(path) { <insert all the chrome calls needed to open the file>}
    log(msg) { <insert chrome stuff to log a msg>}
    ... etc

And also, this is actually why I feel like I ought to minus it - I think the patch is wrong because calling this way from mochitest will write the message directly to the log, whereas the old way that mochitest would call this would perform that specialized formatting (because the original mochitest logger depended on msg being an object).  If this passed on try server, then we probably don't need to depend on mochitest's odd message formatting behaviors, which would be good because we could reduce complexity there.

If we need a "preprocessing" function to format the log message before we write it, then we should add that function to the API and allow the user of the API to define a function callback they would like to use for their formatting.  I've added this to the proposal above.

But, I think in the interest of getting this into a solid API we should really think about whether our interests are best served by duplicating this file.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: convert mozillafilelogger to specialpowers (3.0)

ok, I have a mozillalogger.js file which is used in specialpowers, mochitest, reftest, etc...

This should be the framework for a js logging system, although it seems a bit clunky to me.  Passes all test types (local/remote) for me, currently running on try.

Comment 9

[cmtalbert]

Comment on attachment 546168 [details] [diff] [review]
convert mozillafilelogger to specialpowers (3.0)

It looks like the layout/tools/reftest/mozillafilelogger.js file from the earlier patches made it into this one.  Please remove that for checkin.

This does look a little more clunky but I think it is more versatile
than duplicating the file and it will be easier to maintain with the
logging API all in one place.

Thanks, r+

Comment 10

[cmtalbert]

Attachment: Cleaning up the javascript syntax and js inheritance in mozillalogger.js

Joel,

I went ahead and finished the syntax changes in mozillalogger.js since it was open on my box.

Give this a run around the block and see if it flies.  I also removed the extra MozillaFileLogger.js file from the layout/tools/reftest/ directory.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: convert mozillafilelogger to specialpowers (3.14)

full patch tested on try and with remote testing locally.  This includes all the changes in the javascript cleanup patch as well as some fixes found while testing.

Comment 12

[cmtalbert]

Comment on attachment 546329 [details] [diff] [review]
convert mozillafilelogger to specialpowers (3.14)

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

This looks great.  I have only one nit, and given that this passed on try, it probably means that the code isn't called but we should fix it anyway.  See below.  r+ with this fix.

::: testing/mochitest/tests/SimpleTest/MozillaFileLogger.js
@@ +52,5 @@
> +  },
> +
> +  log : function (msg) {
> +    if (this._foStream)
> +      this._foStream.write(msg, msg.length);

This is SpecialPowersLogger, and as such there is no this._foStream member.  So, we need to change the body of this function to call SpecialPowers.log(msg); and remove this if statement and the foStream.write statement as well.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

good catch, this needs to be SpecialPowers.write(msg) instead.  This log function is just there for completeness, not for anything that is using it right now.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/e437a34e2d6b