341767 - Should be able to view profile data immediately

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, enhancement)

Description

[:Gijs (he/him)]

Steps:

1. Start profiling
2. run code
3. Pause code
4. Stop profiling
5. Save profile.
6. Attempt to use your browser to look at the profile you just saved.

Result: failure, you're browser is stopped by the debugger. You don't always want to let go of said stopped state, so...

Expected Result: the debugger should provide a facility for the user to look at the data immediately (preferably in something human-readable, so not CSV).

Comment 1

[:Gijs (he/him)]

Attachment: Patch

Patch stealing stuff from save profile data and instead of notifying you, opening a window with the file. It also just saves to a temp file using the directory service.

r? = silver

Comment 2

[James Ross]

Comment on attachment 226432 [details] [diff] [review]
Patch

This is not a review, I'm a bit tired for that. I think all the comments should be fixed, though, so here are some.

>+function cmdShowProfile(e)
>+{
>+    function onComplete (i)
>+    {
>+        w = window.open(getURLSpecFromFile(file.localFile));
>+        file.close();
>+    };

Can we tell if it failed? Does it tell us?

Might also want a comment to indicate that we *must* load the profile in a new window - neither the current window or a new tab will do, because they could well be the window we've got disabled for debugging (if we're paused).

>+    // Get to the temp dir on this machine, for this user:
>+    const DIRSVC_CID = "@mozilla.org/file/directory_service;1";
>+    const nsIProperties = Components.interfaces.nsIProperties;
>+    var dirsvc = Components.classes[DIRSVC_CID].getService(nsIProperties);
>+
>+    tmpFile = dirsvc.get("TmpD", Components.interfaces.nsIFile);

var tmpFile = getSpecialDirectory("TmpD");

>+    // Preferred name for a profile:
>+    tmpFile.append("venkman-profile.tmp");
>+    // Don't overwrite files:
>+    tmpFile.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0664);

0600? Is there any reason at all for allowing more access to a file with potentially sensitive data in?

>+
>+    var file = fopen(tmpFile.path, ">");
>+
>+    var templateFile = console.prefs["profile.template.html"];
>+    var reportTemplate = console.profiler.loadTemplate(templateFile);
>+

How much of the following code is copied from the normal profile gen code? Not critical, but it'd be nice to share what is common if any actually is.

>+    var scriptInstanceList = new Array();

I'm also wondering if there's any chance we can clean up the temp files at some point. I'd have just overwritten the same one for each show command, but I can see the benefit of multiple ones being kept at least temporarily. Maybe save the list of names that were used and kill any that exist on exit?

Comment 3

[:Gijs (he/him)]

(In reply to comment #2)
> Can we tell if it failed? Does it tell us?

No: http://lxr.mozilla.org/seamonkey/source/extensions/venkman/resources/content/venkman-profiler.js#353

> 0600? Is there any reason at all for allowing more access to a file with
> potentially sensitive data in?
Point. 

> How much of the following code is copied from the normal profile gen code? Not
> critical, but it'd be nice to share what is common if any actually is.
As much as possible, so that's everything below that line I believe (give or take a few bits). I'll see what I can do.
 
> >+    var scriptInstanceList = new Array();
> 
> I'm also wondering if there's any chance we can clean up the temp files at some
> point. I'd have just overwritten the same one for each show command, but I can
> see the benefit of multiple ones being kept at least temporarily. Maybe save
> the list of names that were used and kill any that exist on exit?
I sort of assumed that either the app or the OS would take care of that, but I can sort it myself too.

Thanks for the 'review', I'll try to get round to do a newer patch today/tomorrow.

Comment 4

[James Ross]

(In reply to comment #3)
> I sort of assumed that either the app or the OS would take care of that, but I
> can sort it myself too.

You never told the OS that you wanted it to take care of it. To be fair, I don't think Mozilla provides any way to do that, which means you need to play the game yourself.

Comment 5

[Mike Shaver (:shaver emeritus)]

Stop me if this is a dumb question, but why do we need to generate a file at all?  Can we refactor a bit to just pass in an nsIOutputStream, and then give it a string stream or some such, to slurp it on out again?

Comment 6

[James Ross]

Probably nothing stopping that, though we have no neat abstraction for doing it (unlike with LocalFile/fopen). I also don't like long data URIs because a bunch of stuff tends to get rather upset.

Comment 7

[:Gijs (he/him)]

Attachment: Patch without temp files

Better patch, or so I hope :-)

Comment 8

[James Ross]

Comment on attachment 226534 [details] [diff] [review]
Patch without temp files

I'm going to point out style nits in the copied code as well as new; might as well fix them now rather than later.

>Index: mozilla/extensions/venkman/resources/content/venkman-commands.js

>+function cmdShowProfile(e)
>+{
>+    function onComplete (i)

Style nit: space.

>+    var templatePfx = "profile.template.";
>+    var templateName;
>+    templateName = templatePfx + type;

Might as well put those last two lines together. :)

>+    var i, j;
>+    for (i = 0; i < e.urlList.length; ++i)
>+    {
>+        var url = e.urlList[i];
>+        if (!ASSERT (url in console.scriptManagers, "url not loaded"))

Style nit: space.

>+            continue;
>+        var manager = console.scriptManagers[url];
>+        for (j in manager.instances)
>+            scriptInstanceList.push (manager.instances[j]);

Style nit: space.

>+    var profileReport = new ProfileReport (reportTemplate, file, rangeList,
>+                                           scriptInstanceList);

Style nit: space.


>Index: mozilla/extensions/venkman/resources/locale/en-US/venkman.properties

>+cmd.show-profile.label = &Display Profile Data
>+cmd.show-profile.params = [<url> [<...>]]
>+cmd.show-profile.help = Displays the profile data collected for one or more source files specified by <url>. If <url> is not provided, all profile data is saved.

Line up the equals?

r=silver with those nits fixed.

Comment 9

[:Gijs (he/him)]

I just checked this in, butI actually forgot to adjust the equals signs, sorry! Silver said I could leave them be, so marking this FIXED.