Bug 25475 – Show the filename and first line for "(program)" in the Profiler/Debugger

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Timothy Hatcher]

We could likely show the file name the function was parsed/created in when showing anonymous function names. This would give a little more context with no effort on the developers part.

[Maciej Stachowiak]

For global code, it might make sense to show just the file name (and maybe initial line number) instead of (global).

[Timothy Hatcher]

Since we already show the file and line number for anonymous functions, this is about global/program code.

[Patrick Mueller]

Firefox has been doing this for a while, but I was thinking of something else as well.

Check the first so many characters of the source for a line of the form:

//@eval-source-name NameGoesHere

(or similar)

and then use NamesGoesHere for 'name' of the compilation unit.

Should probably collaborate with FireBug folks to agree on the syntax.

[Patrick Mueller]

I discovered that FireBug already supports a feature like I suggested in comment #3.  They look for a line of the format

   //@ sourceURL=<url>

in eval()'d scripts, and use that as the source url for the script.  The dojo framework has this baked into their code; at development time, they tend to eval() a lot of the JavaScript code.  

Mention of this feature here:

   http://fbug.googlecode.com/svn/branches/firebug1.1/docs/ReleaseNotes_1.1.txt

I did a quick prototype of this, by adding the following code at the end of the WebInspector.Script constructor in WebCore/Inspector/front-end/Script.js:

    // if no URL, look for "//@ sourceURL=" decorator     
    if (!sourceURL) {
        var pattern = /.*\s*\/\/\s*@\s*sourceURL\s*=\s*(\S+)\s*/;
        var match = pattern.exec(source);
        
        if (match) {
            this.sourceURL = "eval:" + match[1];
        }
    }

Seems to work ok.  Although dojo supports this directly, they sniff the browser and only add this comment when you're running in Firefox.  I "fixed" that code and all of a sudden 20-30 "(Program)" entries in the scripts pull-down became useful names.

I'll post some before/after screenshots as attachments.

Questions:

- would this processing be unconditional, for eval()'d scripts?  Looks like FireBug initially enabled this only via user setting, but I don't see a setting for it anymore.  I don't think it would cause much harm to do it unconditionally.

- this code actually sets the sourceURL to the specified value - prefixed with "eval:".  I figured that we want these in a different "space" than the other scripts, and it turns out they sort well this way also.  I'm tempted to prefix with " eval:" or "#eval:" to get them all to sort to the top.

- wondering if having these fake sourceURLs will cause a problem for any other processing; I'll poke around.

- the regex needs some additional work - it's failing in a graceful way on some incorrectly specified lines (also need to figure out where to put tests for this).

[Patrick Mueller]

Created attachment 30852 [details]
script list for dojo flashcard sample before patch

[Patrick Mueller]

Created attachment 30853 [details]
script list for dojo flashcard sample after patch

[Patrick Mueller]

I poked around a bit yesterday, looking for an appropriate place and method to do testing for this functionality.  Looking at the profiler tests, I realized that sourceURLs show up in the output for profile output, so that seemed like a possibility.  However, as I tried this out, it turns out that profiling does NOT pick up the overridden sourceURL specified using the //@sourceURL= technique.  I suspect the JS VM is actually providing this information directly to the profile code, as opposed to going through Script.js.

This in itself may be a problem - SHOULD the overridden sourceURL show up in profiles?  I did a little profiling work, and found that while it would be nice, it's not clear to me that this is a stopper sort of issue.  Something to look into later perhaps.  

I also realized that console.trace() can show sourceURLs, but it also did not not show the overridden version.  Again, doesn't seem to me to be a stopper issue.

At this point, I don't think there is any way to add verifiable tests, but we should add some manual tests.  There are some files in LayoutTests/fast/inspector which appear to be manual tests, or could be; ie, there are .html files with no corresponding -expected.txt files with them.  Would it be appropriate to add manual tests by adding an additional .html file with no -expected.txt file, in that directory?

[Timothy Hatcher]

(In reply to comment #7)
> I poked around a bit yesterday, looking for an appropriate place and method to
> do testing for this functionality.  Looking at the profiler tests, I realized
> that sourceURLs show up in the output for profile output, so that seemed like a
> possibility.  However, as I tried this out, it turns out that profiling does
> NOT pick up the overridden sourceURL specified using the //@sourceURL=
> technique.  I suspect the JS VM is actually providing this information directly
> to the profile code, as opposed to going through Script.js.
> 
> This in itself may be a problem - SHOULD the overridden sourceURL show up in
> profiles?  I did a little profiling work, and found that while it would be
> nice, it's not clear to me that this is a stopper sort of issue.  Something to
> look into later perhaps.  
> 
> I also realized that console.trace() can show sourceURLs, but it also did not
> not show the overridden version.  Again, doesn't seem to me to be a stopper
> issue.

Making these work will be tricky and require the change to be in JavaScriptCore directly. It can wait, but a bug should be filed about them.

> At this point, I don't think there is any way to add verifiable tests, but we
> should add some manual tests.  There are some files in
> LayoutTests/fast/inspector which appear to be manual tests, or could be; ie,
> there are .html files with no corresponding -expected.txt files with them. 
> Would it be appropriate to add manual tests by adding an additional .html file
> with no -expected.txt file, in that directory?

Nothing in LayoutTests is manual, they all run with the run-webkit-tests script. Th expected results are in the LayoutTests/platform dir.

Manual tests for the Inspector can be found in WebCore/manual-tests/inspector.

I will reply later about your other comments, to busy this week and next with WWDC. Just didn't want you to think I was ignoring you here.

[Patrick Mueller]

Created attachment 30924 [details]
patch with changes described in the bug up through 6/3

I've made the regex a bit more precise; was matching across lines previously, if the comment wasn't formatted correctly.  Also changed the prefix on the sourceURL to "(eval):", so these now sort with (program) and are also themselves grouped together.  Added a manual testcase.

[Patrick Mueller]

It was just pointed out to me that the //@ syntax may negatively interact with IE's conditional comments capability.  Some info on that here:

http://msdn.microsoft.com/en-us/library/6s6fab9k(VS.85).aspx

Also look at the @if, @set, and @cc_on "statements" in the same TOC for that entry.

Argh.

Using a different character would maybe be better, though we certainly should work with FireBug devs to make sure we use the same syntax.

[johnjbarton]

(In reply to comment #10)
> It was just pointed out to me that the //@ syntax may negatively interact with
> IE's conditional comments capability.  
...
> Using a different character would maybe be better, though we certainly should
> work with FireBug devs to make sure we use the same syntax.

As you have noted, the Firebug syntax is already embedded in dojo consequently in many dojo-based projects. For that reason I would be very reluctant to change the syntax without requests from the users who rely on the function. If IE wants to use the syntax as well, that's fine by me.

[Patrick Mueller]

OK, so the IE thing was a false alarm, it appears.

The way I understood this, as it was presented to me, was that using the @sourceURL comment actually BROKE apps in IE.  I took my dojo and Cappuccino demos, that I hacked to add the @sourceURL comment, and ran them on IE 7, and they ran fine.  I guess it's more of a conceptual issue.  On the other hand, if IE were to adopt this, would kind of fit in with their current conditional comment thing.

[Adam Peller]

At one point in time, it did break IE, at least IE6.  See http://bugs.dojotoolkit.org/ticket/5901  I'm unable to reproduce this.  Perhaps it happened in conjunction with a @cc_on directive which we no longer have?  Given that IE is stuck supporting conditional compilation, I have to wonder if they even could support this syntax, but it might not be worth changing things for that hypothetical reason!

One other thought -- it would be great to have eval() grok this comment syntax even when not in the debugger, such that uncaught exceptions might provide better information to the user.  That probably wouldn't come without significant development cost and a runtime hit.

[Timothy Hatcher]

Comment on attachment 30924 [details]
patch with changes described in the bug up through 6/3

> +    // if no URL, look for "//@ sourceURL=" decorator     

This comment should mention that this matches what Firebug looks for and not somthing we made up. Also remove the trailing whitespace.

> +        // use of [ \t] rather than /s is to prevent \n from matching

This comment references /s which whould be \s.

> +        if (match) {
> +            this.sourceURL = "(eval):" + match[1];
> +        }

No need for the braces around the if body. Remove them per our style guidlines.

Also I think this should say "(program): ". We don't use "eval" since it can be code from setTimeout strings, new Function() or eval.

This should also be a localized string, and use WebInspector.UIString().

So WebInspector.UIString("(program): %s", match[1]).

Then add "(program): %s" to WebCore/English.lproj/localizedStrings.js.

[Timothy Hatcher]

Thanks for adding this. I will have more time to review patches now that WWDC is over.

[Patrick Mueller]

Catching up from vacation; agree with all the items from the review; should have a new patch available by tomorrow.

While I have it open, thought I'd paste the URL to the firebug code that has the sourceURL comment check, since it's otherwise hard to find (it's near the top - search for "reURIinComment"), and I don't believe documented anywhere.

   http://fbug.googlecode.com/svn/trunk/content/firebug/debugger.js

Seeing it reminds me that I made the regex for Web Inspector more flexible than FireBug's - mine allows for additional whitespace in places that FireBug's doesn't.  The two regexes should probably more closely match each other than they currently do - meaning Web Inspectors should probably be made less flexible.  Thoughts?

[Timothy Hatcher]

I like the felxability you added. I hate imposing whitespace requirements like that.

[Patrick Mueller]

Created attachment 31699 [details]
patch with changes described in the bug up through 6/22

new version of patch with changes based on review of previous patch (attachment 30924 [details])

[David Levin]

Assign to levin for landing.

[David Levin]

Committed as http://trac.webkit.org/changeset/45186.