Bug 30212 - Each JS execution in console adds extra item into "scripts" combo
: Each JS execution in console adds extra item into "scripts" combo
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: PC Windows XP
: P2 Minor
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-10-08 08:21 PST by
Modified: 2009-11-02 14:54 PST (History)


Attachments
Screenshot (63.89 KB, image/jpeg)
2009-10-08 08:21 PST, Vadim Ridosh
no flags Details
proposed patch 2009/10/29 - a (4.74 KB, patch)
2009-10-29 13:51 PST, Patrick Mueller
pfeldman: review-
Review Patch | Details | Formatted Diff | Diff
proposed patch 2009/11/02 - a (6.12 KB, patch)
2009-11-02 14:41 PST, Patrick Mueller
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-10-08 08:21:21 PST
Created an attachment (id=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.
------- Comment #1 From 2009-10-08 09:54:44 PST -------
This is a duplicate of Bug 9895.
------- Comment #2 From 2009-10-29 07:42:01 PST -------
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
------- Comment #3 From 2009-10-29 08:18:35 PST -------
I like the opt-in approch. And we would show the eval if it is stepped into or an exception is hit.
------- Comment #4 From 2009-10-29 12:29:25 PST -------
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.
------- Comment #5 From 2009-10-29 13:51:55 PST -------
Created an attachment (id=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.
------- Comment #6 From 2009-10-29 14:21:58 PST -------
(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.
------- Comment #7 From 2009-10-29 14:34:20 PST -------
(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.
------- Comment #8 From 2009-10-30 09:14:47 PST -------
(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.
------- Comment #9 From 2009-10-30 09:22:14 PST -------
(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.
------- Comment #10 From 2009-10-30 09:23:34 PST -------
(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.
------- Comment #11 From 2009-11-02 11:25:42 PST -------
(In reply to comment #6)
> (From update of attachment 42137 [details] [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?
------- Comment #12 From 2009-11-02 14:41:06 PST -------
Created an attachment (id=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.
------- Comment #13 From 2009-11-02 14:53:55 PST -------
(From update of attachment 42344 [details])
Clearing flags on attachment: 42344

Committed r50431: <http://trac.webkit.org/changeset/50431>
------- Comment #14 From 2009-11-02 14:54:01 PST -------
All reviewed patches have been landed.  Closing bug.