Bug 30212 – Each JS execution in console adds extra item into "scripts" combo

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

[Vadim Ridosh]

Created attachment 40871 [details]
Screenshot

Steps to reproduce:

Open "Scripts" panel, open console, print f.e. "document" and press Enter. This will create new "(program)" item in scripts combobox.
Repeat this several times - each time new "(program)" items will be created.

[Patrick Mueller]

This is a duplicate of Bug 9895.

[Patrick Mueller]

Over IRC yesterday, pfeldman suggest the current thinking was to just remove all unnamed (program) entries from the Scripts drop-down, although allow any "real" evals from code to be able to stepped into.

The downside of this approach is for someone who actually depends on seeing eval()'d code in the drop-downs, they won't see it anymore.  As a work-around, they can add

   \n//@sourceURL=some-name-here\n

to their script, and we'll "name" the eval string in the Scripts drop-down.

Think of this as the opt-in approach.

Another story would be the opt-out approach.  Support another directive comment like the @sourceURL one, but one indicates that this eval should NOT be listed in the drop-down.  

The idea would then be for the evals that we actually do today (command-line, watches, etc), we augment the strings we eval with said directive, and nothing will get added to the Scripts drop-down.  Users would still see their own evals, as they won't be adding the directive (unless they really want to).

Here are some kinda rando suggestions for directives.

//@do-not-list-script
//@not-enumerable
//@invisible-script

[Timothy Hatcher]

I like the opt-in approch. And we would show the eval if it is stepped into or an exception is hit.

[Patrick Mueller]

I have a very simple fix for this I'm testing now.

The only little odd thing is that if you step into a function which is not in the list, the Scripts combo box shows the name of the last script/resource you were in.  In a perfect world, we might want to change the value in the combo to "(program)", I suppose, but I'm not sure it's worth the effort.  Especially since it seems like this is a path few people will exercise.

[Patrick Mueller]

Created attachment 42137 [details]
proposed patch 2009/10/29 - a

proposed patch - simple two line code addition to just not add sourceURL-free scripts to the scripts drop down.  With a testcase.  Still exhibits the small problem described in comment 4.

[Pavel Feldman]

Comment on attachment 42137 [details]
proposed patch 2009/10/29 - a

I think filtering out is reasonable at least for now. Time to do some automated tests though:
- You should do a layout test page with eval in it (the one alike LayoutTests/inspector/console*
- Replace console.log with some eval there
- Inject two methods into frontend: 
    switchToPanel (you can make it universal in the utility.js that is imported)
    dumpScriptCombo

Then you should be able to call them one by one and validate result returned form dumpScriptCombo.

[Timothy Hatcher]

(In reply to comment #4)
> I have a very simple fix for this I'm testing now.
> 
> The only little odd thing is that if you step into a function which is not in
> the list, the Scripts combo box shows the name of the last script/resource you
> were in.  In a perfect world, we might want to change the value in the combo to
> "(program)", I suppose, but I'm not sure it's worth the effort.  Especially
> since it seems like this is a path few people will exercise.

I think we need to show "(program)", otherwise the developer can be confused.

It might happen more than you think. If you eval and it calls a function in a real file and you break in that file. Clicking through the call stack to switch resources, you will not see the popup change but the source will, and that is confusing.

[Patrick Mueller]

(In reply to comment #7)
> (In reply to comment #4)
> I think we need to show "(program)", otherwise the developer can be confused.

To do this will mean, presumably, programmatically managing  a "(program)" <option> in the <select>.  I think this means every time the user displays the source for an unnamed eval, we add the option and select it so they see it.  And when the user displays the source for something that's really in the <select> list, we want to programmatically remove the "(program)" <option>.  The <option> is really just being used to set the text content of the unexpanded <select>. 

Which is also a bit confusing.  An entry that comes and goes.  

Another option would be to add a "(program)" <option> for each eval that gets displayed, and then just leave it in there.  For someone who steps through a bunch of evals, their <select> will end up getting littered with "(program)" <option>'s, but that's the behavior today anyway.  This option seems easier to implement as well.

[Pavel Feldman]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #4)
> > I think we need to show "(program)", otherwise the developer can be confused.
> 
> To do this will mean, presumably, programmatically managing  a "(program)"
> <option> in the <select>.  I think this means every time the user displays the
> source for an unnamed eval, we add the option and select it so they see it. 
> And when the user displays the source for something that's really in the
> <select> list, we want to programmatically remove the "(program)" <option>. 
> The <option> is really just being used to set the text content of the
> unexpanded <select>. 
> 
> Which is also a bit confusing.  An entry that comes and goes.  
> 
> Another option would be to add a "(program)" <option> for each eval that gets
> displayed, and then just leave it in there.  For someone who steps through a
> bunch of evals, their <select> will end up getting littered with "(program)"
> <option>'s, but that's the behavior today anyway.  This option seems easier to
> implement as well.

I like the second option (which is "leave it there"). It is simpler and is more matches expectations: things user has seen do not disappear.

[Timothy Hatcher]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #4)
> > > I think we need to show "(program)", otherwise the developer can be confused.
> > 
> > To do this will mean, presumably, programmatically managing  a "(program)"
> > <option> in the <select>.  I think this means every time the user displays the
> > source for an unnamed eval, we add the option and select it so they see it. 
> > And when the user displays the source for something that's really in the
> > <select> list, we want to programmatically remove the "(program)" <option>. 
> > The <option> is really just being used to set the text content of the
> > unexpanded <select>. 
> > 
> > Which is also a bit confusing.  An entry that comes and goes.  
> > 
> > Another option would be to add a "(program)" <option> for each eval that gets
> > displayed, and then just leave it in there.  For someone who steps through a
> > bunch of evals, their <select> will end up getting littered with "(program)"
> > <option>'s, but that's the behavior today anyway.  This option seems easier to
> > implement as well.
> 
> I like the second option (which is "leave it there"). It is simpler and is more
> matches expectations: things user has seen do not disappear.

I agree.

Maybe later we can have a clean up stage if it becomes an issue for someone, but this will be much better than we have it today.

[Patrick Mueller]

(In reply to comment #6)
> (From update of attachment 42137 [details])
> I think filtering out is reasonable at least for now. Time to do some automated
> tests though:
> - You should do a layout test page with eval in it (the one alike
> LayoutTests/inspector/console*
> - Replace console.log with some eval there
> - Inject two methods into frontend: 
>     switchToPanel (you can make it universal in the utility.js that is
> imported)
>     dumpScriptCombo
> 
> Then you should be able to call them one by one and validate result returned
> form dumpScriptCombo.

I'm going to need some help with "switchToPanel".  I've tried the obvious thing, of running 

   WebInspector.showScriptsPanel();

within a evaluateInWebInspector().  I then wait for the panel to switch, via the hacky:

    for (var panelName in WebInspector.panels)
        if (WebInspector.panels[panelName] == WebInspector.currentPanel)
            return panelName;

    return "unknown";

which is run in a evaluateInWebInspector() which I run on an interval.  I've waited up to a total of 10 seconds, and "elements" is always the current panel.  

Somehow I need to "kick" WebInspector or something.  

Is there some way to actually debug what's going on in the layout tests?

[Patrick Mueller]

Created attachment 42344 [details]
proposed patch 2009/11/02 - a

accomodated Tim's request to have the "(program)" entries added as you step through them.  Unable to get the automated test suite to stop in the scripts panel correctly, so no automated test; a manual one instead.

[WebKit Commit Bot]

Comment on attachment 42344 [details]
proposed patch 2009/11/02 - a

Clearing flags on attachment: 42344

Committed r50431: <http://trac.webkit.org/changeset/50431>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.