Bug 27552 – remember last script displayed in Scripts panel

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

[Patrick Mueller]

The following states should be saved across invocations in the Scripts panel:

- current source file being viewed
- expand/collapse state of:
   - Call Stack
   - Scope Variables
   
None of these seem to be saved; first source displayed seems to be random, and I think Call Stack and Scope Variables expand/collapse is hardcoded as expanded.

There may be other things on the panel, like Pause on Exception (not sure).

For current source file, I suggest we attempt to open the last open file if it's in the source list, otherwise resort to some other fixed behavior, like the first file on the list.  Anything seems better than random.

Another potential saveable thing would be the list of source files open, but that's dicer; even the current source file is a bit dicey.    I suspect this may work in practice, probably most developers are working on the same web pages with the same resources all day long.

[Patrick Mueller]

I've changed the scope of the bug to limit to the last script viewed.  The other items I listed - expand/collapse state of Call Stack and Scope Variables - probably make sense to leave expanded on open all the time (current behavior).

Not remembering the most recently viewed script turns out to be incredibly annoying for some of my debugging scenarios.  Namely, those with a number of scripts in the list.  Saves having to bring up the list of scripts, scroll to the one you want, and then selecting it.

[Patrick Mueller]

Created attachment 34506 [details]
proposed patch

Couple of issues with this patch:

- the behavior of always showing the first file added to the scripts menu still happens.  So if the "last script" file is also found in the list, two files will be added.  You can tell this has happened as the "left arrow" in the script traversal buttons is enabled.

- using the left and right arrows to traverse the "open" script files does not change the last viewed script

The behavior of the first point probably can't be easily fixed, as there probably is no good way to tell when scripts are being done loaded into the list - scripts may be added after the document has been loaded by more scripts.  

The second item can probably be fixed.

I suspect the existing behavior works in almost all cases though - I don't think I've ever used the left/right arrows to traverse open scripts files, on purpose.

[Timothy Hatcher]

Comment on attachment 34506 [details]
proposed patch

I think this is a good idea. But this assumes you only debug one site, and have only one Web Inspecgor open. This might be fine, but it means it wont always do the right thing. Maybe we need a "LastViewedScriptFile-domain.com" kind of thing?

Needs a ChangeLog.

> if (url && (url == lastURL)) {
>     this._showScriptOrResource(option.representedObject);
> }

No need for braces.

CC me on bus so I know to look at them. Otherwise I miss them until I go through the whole Web Inspector bug list every couple months.

[Patrick Mueller]

(In reply to comment #3)
> I think this is a good idea. But this assumes you only debug one site, and have
> only one Web Inspecgor open. This might be fine, but it means it wont always do
> the right thing. Maybe we need a "LastViewedScriptFile-domain.com" kind of
> thing?

I suspect for most developers, it actually is the case that they are debugging one site at a time - or even if they aren't, they may well be debugging the same file across multiple pages.  Let's see how it works out in practice - if we need to add a domain to the key we're going to be potentially generating a lot of entries in the preferences.

[Timothy Hatcher]

(In reply to comment #4)
> (In reply to comment #3)
> > I think this is a good idea. But this assumes you only debug one site, and have
> > only one Web Inspecgor open. This might be fine, but it means it wont always do
> > the right thing. Maybe we need a "LastViewedScriptFile-domain.com" kind of
> > thing?
> 
> I suspect for most developers, it actually is the case that they are debugging
> one site at a time - or even if they aren't, they may well be debugging the
> same file across multiple pages.  Let's see how it works out in practice - if
> we need to add a domain to the key we're going to be potentially generating a
> lot of entries in the preferences.

Yeah I think you might be right.

[Patrick Mueller]

Created attachment 43379 [details]
proposed patch 2009/11/17 - a

Similar to previous patch, but catches more cases of the "current script" being selected.

The last script file viewed is saved in the setting "LastViewedScriptFile".  This is set in ScriptsPanel._showScriptOrResource(), which is the common function used to cause a script to be displayed.  The parameters for this function have been refactored, since it would have otherwise meant adding a fourth parameter needed by only one call.  The "option" parameters are now collected as properties in a single "option" parameter, and the call sites have been changed to pass in literal objects as appropriate.

[Timothy Hatcher]

Comment on attachment 43379 [details]
proposed patch 2009/11/17 - a

> +
> +        // if not first item, check to see if this was the last viewed
> +        else {

Put the comment inside the else block and remove the empty line.

> +            if (url && (url == lastURL))

Remove the inner parentheses and use ===. 

Otherwise r+. Nice cleanup with the options object.

[Patrick Mueller]

Created attachment 43433 [details]
proposed patch 2009/11/18 - a

addressed review issues from comment 7

[Pavel Feldman]

Comment on attachment 43433 [details]
proposed patch 2009/11/18 - a

Looks good.
One comment though - you are using InspectorController.setting() in a sync manner, whereas we are going away from that. We now think that there is agent (with inspector backend) and a client (frontend). Interaction between those is supposed to be serialized and asynchronous (imagine that remote client attaches to the agent).

So we should either convert setting to async call with a callback or extract such methods into another interface like InspectorHost. First seems to be simpler.

[Timothy Hatcher]

Comment on attachment 43433 [details]
proposed patch 2009/11/18 - a

Making InspectorController.setting async makes sense, but that will require many other changes first before it can be done here too.

[WebKit Commit Bot]

Comment on attachment 43433 [details]
proposed patch 2009/11/18 - a

Clearing flags on attachment: 43433

Committed r51156: <http://trac.webkit.org/changeset/51156>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.