Bug 139526 – Web Inspector: REGRESSION Showing debugger sidebar shouldn't change selected JS file

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 243091 [details]
Screencast

Steps to reproduce:

0. Open http://apple.com/ (or any website with JS files)
1. Open Resources sidebar in Inspector
2. Select home.built.js (or any other JS file)
3. Switch to Debugger sidebar

Expected:
Same JS shown in the content area in the middle

Actual:
Content switched to index.html

[Nikita Vasilyev]

Created attachment 243093 [details]
patch

[Nikita Vasilyev]

Created attachment 243094 [details]
Patch

This seems to fix the problem.

(The previous patch was mine, but `webkit-patch upload` sent it from a wrong account again. Sorry.)

[Nikita Vasilyev]

Imagine we’re in the Resources. We select a CSS, an image, or anything else besides JS. Then we switch to Debugger; which file should it show?

I think it should show the previously selected JS file. If none were selected, show the default content view, e.g. index.html.

What do you think?

[Joseph Pecoraro]

Comment on attachment 243094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243094&action=review

> Source/WebInspectorUI/UserInterface/Base/Main.js:-946
> -    if (!selectedSidebarPanel.hasSelectedElement)
> -        selectedSidebarPanel.showDefaultContentView();

Removing this probably breaks first launch of the inspector to the Debugger sidebar.

What happens in this scenario?

1. Inspect any web page
2. Switch to Debugger panel
3. Close inspector
4. Navigate to a page you have never inspected before
5. Open inspector
  => Debugger panel should be active, what is in the Content Browser?

I want to make sure the content browser is not empty.

[Joseph Pecoraro]

(In reply to comment #3)
> Imagine we’re in the Resources. We select a CSS, an image, or anything else
> besides JS. Then we switch to Debugger; which file should it show?
> 
> I think it should show the previously selected JS file. If none were
> selected, show the default content view, e.g. index.html.
> 
> What do you think?

I agree. But I also think you have to handle the case of the content browser not having a content view, and putting something there.

[Nikita Vasilyev]

(In reply to comment #4)
> Comment on attachment 243094 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243094&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:-946
> > -    if (!selectedSidebarPanel.hasSelectedElement)
> > -        selectedSidebarPanel.showDefaultContentView();
> 
> Removing this probably breaks first launch of the inspector to the Debugger
> sidebar.
> 
> What happens in this scenario?
> 
> 1. Inspect any web page
> 2. Switch to Debugger panel
> 3. Close inspector
> 4. Navigate to a page you have never inspected before
> 5. Open inspector
>   => Debugger panel should be active, what is in the Content Browser?
> 
> I want to make sure the content browser is not empty.

Tried it on a few pages – it was always the default content view (e.g. index.html).

[Nikita Vasilyev]

(In reply to comment #5)
> (In reply to comment #3)
> > Imagine we’re in the Resources. We select a CSS, an image, or anything else
> > besides JS. Then we switch to Debugger; which file should it show?
> > 
> > I think it should show the previously selected JS file. If none were
> > selected, show the default content view, e.g. index.html.
> > 
> > What do you think?
> 
> I agree. But I also think you have to handle the case of the content browser
> not having a content view, and putting something there.

How can I go into the state when the content browser doesn’t have a content view? I’ve been there (before I started working on this bug), but I don’t remember how to reproduce it.

[Joseph Pecoraro]

Comment on attachment 243094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243094&action=review

>>> Source/WebInspectorUI/UserInterface/Base/Main.js:-946
>>> -        selectedSidebarPanel.showDefaultContentView();
>> 
>> Removing this probably breaks first launch of the inspector to the Debugger sidebar.
>> 
>> What happens in this scenario?
>> 
>> 1. Inspect any web page
>> 2. Switch to Debugger panel
>> 3. Close inspector
>> 4. Navigate to a page you have never inspected before
>> 5. Open inspector
>>   => Debugger panel should be active, what is in the Content Browser?
>> 
>> I want to make sure the content browser is not empty.
> 
> Tried it on a few pages – it was always the default content view (e.g. index.html).

Hmm, okay. This also affects switching to the Timeline navigation sidebar. In the same scenario:

  6. Switch to Timelines panel
    => Timeline content view should be in the Content Browser.

I'm skeptical that this code existed for no reason!

[Nikita Vasilyev]

(In reply to comment #8)
> Comment on attachment 243094 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243094&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/Base/Main.js:-946
> >>> -        selectedSidebarPanel.showDefaultContentView();
> >> 
> >> Removing this probably breaks first launch of the inspector to the Debugger sidebar.
> >> 
> >> What happens in this scenario?
> >> 
> >> 1. Inspect any web page
> >> 2. Switch to Debugger panel
> >> 3. Close inspector
> >> 4. Navigate to a page you have never inspected before
> >> 5. Open inspector
> >>   => Debugger panel should be active, what is in the Content Browser?
> >> 
> >> I want to make sure the content browser is not empty.
> > 
> > Tried it on a few pages – it was always the default content view (e.g. index.html).
> 
> Hmm, okay. This also affects switching to the Timeline navigation sidebar.
> In the same scenario:
> 
>   6. Switch to Timelines panel
>     => Timeline content view should be in the Content Browser.
> 
> I'm skeptical that this code existed for no reason!

Oh, I see the problem. Switching from Timeline to Debugger keeps the content view of the timeline. Thinking how to fix it.

[Nikita Vasilyev]

(In reply to comment #3)
> Imagine we’re in the Resources. We select a CSS, an image, or anything else
> besides JS. Then we switch to Debugger; which file should it show?
> 
> I think it should show the previously selected JS file. If none were
> selected, show the default content view, e.g. index.html.

To do that, I need to check if the previous content view item is a JS file and do it recursively.

I’ve found these content view item are stored in WebInspector.contentBrowser.contentViewContainer._backForwardList. However, I don’t quite see a clean way to distinguish a JS file item from other types (e.g. Timeline content view). What property should I check for?

[Joseph Pecoraro]

(In reply to comment #10)
> (In reply to comment #3)
> > Imagine we’re in the Resources. We select a CSS, an image, or anything else
> > besides JS. Then we switch to Debugger; which file should it show?
> > 
> > I think it should show the previously selected JS file. If none were
> > selected, show the default content view, e.g. index.html.
> 
> To do that, I need to check if the previous content view item is a JS file
> and do it recursively.
> 
> I’ve found these content view item are stored in
> WebInspector.contentBrowser.contentViewContainer._backForwardList. However,
> I don’t quite see a clean way to distinguish a JS file item from other types
> (e.g. Timeline content view). What property should I check for?

I think what is important, is whether or not the content view allows being shown in the navigation sidebar, that already exists and we use it in other places:

See: WebInspector._updateNavigationSidebarForCurrentContentView

    var allowedNavigationSidebarPanels = currentContentView.allowedNavigationSidebarPanels;
    if (allowedNavigationSidebarPanels.length && !allowedNavigationSidebarPanels.contains(selectedSidebarPanel.identifier))
        ...

[Nikita Vasilyev]

Created attachment 243427 [details]
Patch

Animated GIF of Inspector with the patch applied: https://cloudup.com/cKBoXOp0_AO

[Nikita Vasilyev]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #3)
> > > Imagine we’re in the Resources. We select a CSS, an image, or anything else
> > > besides JS. Then we switch to Debugger; which file should it show?
> > > 
> > > I think it should show the previously selected JS file. If none were
> > > selected, show the default content view, e.g. index.html.
> > 
> > To do that, I need to check if the previous content view item is a JS file
> > and do it recursively.
> > 
> > I’ve found these content view item are stored in
> > WebInspector.contentBrowser.contentViewContainer._backForwardList. However,
> > I don’t quite see a clean way to distinguish a JS file item from other types
> > (e.g. Timeline content view). What property should I check for?
> 
> I think what is important, is whether or not the content view allows being
> shown in the navigation sidebar, that already exists and we use it in other
> places:
> 
> See: WebInspector._updateNavigationSidebarForCurrentContentView
> 
>     var allowedNavigationSidebarPanels =
> currentContentView.allowedNavigationSidebarPanels;
>     if (allowedNavigationSidebarPanels.length &&
> !allowedNavigationSidebarPanels.contains(selectedSidebarPanel.identifier))
>         ...

The problem was in the finding the right content view, showing the right sidebar wasn’t the problem. Maybe I misunderstood what you were suggesting, but I don’t see how it will help me to show the right content view when switching to/from debugger.

Take a look at my latest patch.

[Joseph Pecoraro]

Comment on attachment 243427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243427&action=review

Hmm, I don't like the sound of this. showDefaultContentView should be a fairly straightforward way to show a ContentView, it shouldn't check the global Content Browser's state. I think the person who calls showDefaultContentView (Main.js), should check history and use it if possible. Otherwise, show the default.

Does that make sense?

Something like:

    WebInspector._revealAndSelectRepresentedObjectInNavigationSidebar = function(representedObject)
    {
        ...
        var selectedSidebarPanel = this.navigationSidebar.selectedSidebarPanel;
        if (!selectedSidebarPanel)
            return;
        ...
        if (!selectedSidebarPanel.hasSelectedElement) {
            // Check history for a suitable ContentView.
            var backForwardList = WebInspector.contentBrowser.contentViewContainer.backForwardList;
            var index = backForwardList.length;
            var didShowContentView = false;
            while (index--) {
                var contentView = backForwardList[index].contentView;
                var allowedNavigationSidebarPanels = contentView.allowedNavigationSidebarPanels;
                if (!allowedNavigationSidebarPanels.length || allowedNavigationSidebarPanels.contains(selectedSidebarPanel.identifier)) {
                    // FIXME: Is there a way to ensure the representedItem is selected when we show this content view?
                    WebInspector.contentBrowser.contentViewContainer.showBackForwardEntryForIndex(index);
                    didShowContentView = true;
                    break;
                }
            }

            // No suitable view, show a default content view.
            if (!didShowContentView)
                selectedSidebarPanel.showDefaultContentView();
        }
    };

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:168
> +            var listItem = backForwardList[index].contentView;
> +            var resource = listItem.resource || (listItem.representedObject ? listItem.representedObject.mainResource : null);
> +            if (!resource)
> +                continue;

This is brittle. Ideally you would check the ContentView types. But we already established that each ContentViews knows which panels it can be shown in with `allowedNavigationSidebarPanels`.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:171
> +            if (resource.type === "resource-type-script" ||
> +                resource.type === "resource-type-document" // HTML documents can have inline scripts.

You should never use these strings directly, you should use the constants: WebInspector.Resource.Type.*

[Nikita Vasilyev]

(In reply to comment #14)
> Comment on attachment 243427 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243427&action=review
> 
> Hmm, I don't like the sound of this. showDefaultContentView should be a
> fairly straightforward way to show a ContentView, it shouldn't check the
> global Content Browser's state. I think the person who calls
> showDefaultContentView (Main.js), should check history and use it if
> possible. Otherwise, show the default.
> 
> Does that make sense?
> 
> Something like:
> 
>     WebInspector._revealAndSelectRepresentedObjectInNavigationSidebar =
> function(representedObject)
>     {
>         ...
>         var selectedSidebarPanel =
> this.navigationSidebar.selectedSidebarPanel;
>         if (!selectedSidebarPanel)
>             return;
>         ...
>         if (!selectedSidebarPanel.hasSelectedElement) {
>             // Check history for a suitable ContentView.
>             var backForwardList =
> WebInspector.contentBrowser.contentViewContainer.backForwardList;
>             var index = backForwardList.length;
>             var didShowContentView = false;
>             while (index--) {
>                 var contentView = backForwardList[index].contentView;
>                 var allowedNavigationSidebarPanels =
> contentView.allowedNavigationSidebarPanels;
>                 if (!allowedNavigationSidebarPanels.length ||
> allowedNavigationSidebarPanels.contains(selectedSidebarPanel.identifier)) {
>                     // FIXME: Is there a way to ensure the representedItem
> is selected when we show this content view?
>                    
> WebInspector.contentBrowser.contentViewContainer.
> showBackForwardEntryForIndex(index);
>                     didShowContentView = true;
>                     break;
>                 }
>             }
> 
>             // No suitable view, show a default content view.
>             if (!didShowContentView)
>                 selectedSidebarPanel.showDefaultContentView();
>         }
>     };
> 
> > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:168
> > +            var listItem = backForwardList[index].contentView;
> > +            var resource = listItem.resource || (listItem.representedObject ? listItem.representedObject.mainResource : null);
> > +            if (!resource)
> > +                continue;
> 
> This is brittle. Ideally you would check the ContentView types. But we
> already established that each ContentViews knows which panels it can be
> shown in with `allowedNavigationSidebarPanels`.
> 

allowedNavigationSidebarPanels has only two different states: [] and ["timeline"].


TimelineContentView.js:
    get allowedNavigationSidebarPanels()
    {
        return ["timeline"];
    },


ContentView.js:
    get allowedNavigationSidebarPanels()
    {
        // Allow any navigation sidebar panel.
        return [];
    },


I really don’t see how it’s going to help me not to show in Debugger anything but JS and HTML. E.g. I don’t want to show images in the content view of Debugger.

[Joseph Pecoraro]

> allowedNavigationSidebarPanels has only two different states: [] and
> ["timeline"].
> 
> 
> TimelineContentView.js:
>     get allowedNavigationSidebarPanels()
>     {
>         return ["timeline"];
>     },
> 
> 
> ContentView.js:
>     get allowedNavigationSidebarPanels()
>     {
>         // Allow any navigation sidebar panel.
>         return [];
>     },
> 
> 
> I really don’t see how it’s going to help me not to show in Debugger
> anything but JS and HTML. E.g. I don’t want to show images in the content
> view of Debugger.

Sounds like Images don't make sense in Debugger, so we should fix ImageResourceContentView to only be allowed in the Resources navigation sidebar. Likewise fixing other ContentViews as appropriate.

The default should probably just be Resources, with Text Content Views allowed in the Debugger.

But I still think this approach makes sense.

[Timothy Hatcher]

The premise of the patch shown in the animated gif is correct.

However, the real fix is to have the Debugger sidebar populated with all debuggable Resources and Scripts. This is something I have wanted to do but never had the chance. Then switching from Resources sidebar to Debugger sidebar with base.js would keep base.js selected in the sidebar. If base.js had breakpoints, they would be children of the base.js tree element like we have today when breakpoints are added.

Populating the sidebar with resource, even if the don't have breakpoints, will make the current code in the Inspector "just work".

Here is a video showing the current behavior when the sidebar does have the resource: http://timothy.hatcher.name/demo-139526.mov

Developers have expected to see the debuggable scripts in Debugger for a while now. We should do that and not workaround the issue.

When we do that, we should add an icon to the filter bar to only show resources with breakpoints. Like Xcode has for some sidebars.

[Timothy Hatcher]

Comment on attachment 243427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243427&action=review

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:162
>      showDefaultContentView: function()
>      {
> -        WebInspector.resourceSidebarPanel.showDefaultContentView();
> +        var didSelect = false;
> +        var backForwardList = WebInspector.contentBrowser.contentViewContainer.backForwardList;
> +        var index = backForwardList.length;

I like the premise of looking in the history to find a better view. Perhaps we should do that (in addition to populating the sidebar with debuggable resources). That would likely give you a better default view when switching from the Timeline overview, instead of just selecting the first tree element. However, the searching of history should be generic and live higher than DebuggerSidebarPanel.js, likely in WebInspector._revealAndSelectRepresentedObjectInNavigationSidebar before resorting to calling showDefaultContentView().

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:173
> +                WebInspector.contentBrowser.contentViewContainer.showBackForwardEntryForIndex(index);

I would _not_ call showBackForwardEntryForIndex. I think that would mess with the linear nature of the history and confuse the user. We should just show the represented object / content view again to push it on the end of the history stack.

[Timothy Hatcher]

Here is another demo of current behavior with a Timeline recording: http://timothy.hatcher.name/demo-timelines-139526.mov

Notice how the content view is never forcibly changed, it attempts to stay the same as the sidebar is switched. This keeps the user from being jerked around, and just changes the selection in the sidebar they are switching to to match the content view (if possible). Otherwise the default view will be shown.

By populating the Debugger sidebar, we will get better behavior when switching sidebars and users will have a list of debuggable resources without all the non-debuggable ones — win win.

[Brian Burg]

(In reply to comment #19)
> Here is another demo of current behavior with a Timeline recording:
> http://timothy.hatcher.name/demo-timelines-139526.mov
> 
> Notice how the content view is never forcibly changed, it attempts to stay
> the same as the sidebar is switched. This keeps the user from being jerked
> around, and just changes the selection in the sidebar they are switching to
> to match the content view (if possible). Otherwise the default view will be
> shown.
> 
> By populating the Debugger sidebar, we will get better behavior when
> switching sidebars and users will have a list of debuggable resources
> without all the non-debuggable ones — win win.

I agree- it's frustrating trying to find the main resource vs a JS file, and switching around between different sidebars to find the resource shouldn't be necessary.

Messing around with back-forward list directly is also not advised. That code is very tricky.

[Radar WebKit Bug Importer]

<rdar://problem/19281503>

[Radar WebKit Bug Importer]

<rdar://problem/19281643>

[Nikita Vasilyev]

Thanks for the feedback.
(In reply to comment #19)
> Here is another demo of current behavior with a Timeline recording:
> http://timothy.hatcher.name/demo-timelines-139526.mov
> 
> Notice how the content view is never forcibly changed, it attempts to stay
> the same as the sidebar is switched. This keeps the user from being jerked
> around, and just changes the selection in the sidebar they are switching to
> to match the content view (if possible). Otherwise the default view will be
> shown.

I’m not sure it a good thing. I often switch between CSS editing and JS debugging, so I go back and forth from from Debugger (well, for JS debugging) to Resources (for CSS editing). The irritating thing is that every time I switch back to Resources I need to:
– select the same HTML file again
– change the view from Source Code back to DOM Tree

Using the back button to get to the previous state helps. However, if I had opened 10 JS files I have to press the back button 10 times. I don’t enjoy doing that.


> http://timothy.hatcher.name/demo-139526.mov

By the way, why does it show demo.js on 9th second when the last selected file in the debugger was index.html? Doesn’t look right to me.

[Timothy Hatcher]

(In reply to comment #23)
> Thanks for the feedback.
> (In reply to comment #19)
> > Here is another demo of current behavior with a Timeline recording:
> > http://timothy.hatcher.name/demo-timelines-139526.mov
> > 
> > Notice how the content view is never forcibly changed, it attempts to stay
> > the same as the sidebar is switched. This keeps the user from being jerked
> > around, and just changes the selection in the sidebar they are switching to
> > to match the content view (if possible). Otherwise the default view will be
> > shown.
> 
> I’m not sure it a good thing. I often switch between CSS editing and JS
> debugging, so I go back and forth from from Debugger (well, for JS
> debugging) to Resources (for CSS editing). The irritating thing is that
> every time I switch back to Resources I need to:
> – select the same HTML file again
> – change the view from Source Code back to DOM Tree
> 
> Using the back button to get to the previous state helps. However, if I had
> opened 10 JS files I have to press the back button 10 times. I don’t enjoy
> doing that.

So you advocate for the opposite of what we have today? Each sidebar has a selection, and switching sidebars changes the content view to match the last selection in that sidebar?

> > http://timothy.hatcher.name/demo-139526.mov
> 
> By the way, why does it show demo.js on 9th second when the last selected
> file in the debugger was index.html? Doesn’t look right to me.

Good question. I suspect it is because I switched to Timeline, which had no selection and Resources showed the previous selection it had.

[Timothy Hatcher]

(In reply to comment #24)
> (In reply to comment #23)
> > Thanks for the feedback.
> > (In reply to comment #19)
> > 
> > I’m not sure it a good thing. I often switch between CSS editing and JS
> > debugging, so I go back and forth from from Debugger (well, for JS
> > debugging) to Resources (for CSS editing). The irritating thing is that
> > every time I switch back to Resources I need to:
> > – select the same HTML file again
> > – change the view from Source Code back to DOM Tree
> > 
> > Using the back button to get to the previous state helps. However, if I had
> > opened 10 JS files I have to press the back button 10 times. I don’t enjoy
> > doing that.
> 
> So you advocate for the opposite of what we have today? Each sidebar has a
> selection, and switching sidebars changes the content view to match the last
> selection in that sidebar?

We can experiment with that approach if you want. That might be a good setting, if not the default behavior.

[Nikita Vasilyev]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > Thanks for the feedback.
> > > (In reply to comment #19)
> > > 
> > > I’m not sure it a good thing. I often switch between CSS editing and JS
> > > debugging, so I go back and forth from from Debugger (well, for JS
> > > debugging) to Resources (for CSS editing). The irritating thing is that
> > > every time I switch back to Resources I need to:
> > > – select the same HTML file again
> > > – change the view from Source Code back to DOM Tree
> > > 
> > > Using the back button to get to the previous state helps. However, if I had
> > > opened 10 JS files I have to press the back button 10 times. I don’t enjoy
> > > doing that.
> > 
> > So you advocate for the opposite of what we have today? Each sidebar has a
> > selection, and switching sidebars changes the content view to match the last
> > selection in that sidebar?
> 
> We can experiment with that approach if you want. That might be a good
> setting, if not the default behavior.

It would solve the problem I’ve mentioned so, yes, I’d like to explore this approach.

[Nikita Vasilyev]

(In reply to comment #14)
> Comment on attachment 243427 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243427&action=review
> 
> Hmm, I don't like the sound of this. showDefaultContentView should be a
> fairly straightforward way to show a ContentView, it shouldn't check the
> global Content Browser's state. I think the person who calls
> showDefaultContentView (Main.js), should check history and use it if
> possible. Otherwise, show the default.
> 
> Does that make sense?
> 
> Something like:
> 
>     WebInspector._revealAndSelectRepresentedObjectInNavigationSidebar =
> function(representedObject)
>     {
>         ...
>         var selectedSidebarPanel =
> this.navigationSidebar.selectedSidebarPanel;
>         if (!selectedSidebarPanel)
>             return;
>         ...
>         if (!selectedSidebarPanel.hasSelectedElement) {
>             // Check history for a suitable ContentView.
>             var backForwardList =
> WebInspector.contentBrowser.contentViewContainer.backForwardList;
>             var index = backForwardList.length;
>             var didShowContentView = false;
>             while (index--) {
>                 var contentView = backForwardList[index].contentView;
>                 var allowedNavigationSidebarPanels =
> contentView.allowedNavigationSidebarPanels;
>                 if (!allowedNavigationSidebarPanels.length ||
> allowedNavigationSidebarPanels.contains(selectedSidebarPanel.identifier)) {
>                     // FIXME: Is there a way to ensure the representedItem
> is selected when we show this content view?
>                    
> WebInspector.contentBrowser.contentViewContainer.
> showBackForwardEntryForIndex(index);
>                     didShowContentView = true;
>                     break;
>                 }
>             }
> 
>             // No suitable view, show a default content view.
>             if (!didShowContentView)
>                 selectedSidebarPanel.showDefaultContentView();
>         }
>     };
> 
> > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:168
> > +            var listItem = backForwardList[index].contentView;
> > +            var resource = listItem.resource || (listItem.representedObject ? listItem.representedObject.mainResource : null);
> > +            if (!resource)
> > +                continue;
> 
> This is brittle. Ideally you would check the ContentView types. But we
> already established that each ContentViews knows which panels it can be
> shown in with `allowedNavigationSidebarPanels`.

I see the contentView for both images and scripts being ResourceClusterContentView, so it doesn’t help with distinguishing the two.

[Joseph Pecoraro]

> I see the contentView for both images and scripts being
> ResourceClusterContentView, so it doesn’t help with distinguishing the two.

ResourceClusterContentView should be able to supply a list based on the list of content views in the cluster.

[Nikita Vasilyev]

Created attachment 243538 [details]
Patch

Animated GIF: https://cloudup.com/ct5WzR_Sb4J

[Timothy Hatcher]

Comment on attachment 243538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243538&action=review

Nice!

> Source/WebInspectorUI/UserInterface/Base/Main.js:945
> +    if (!selectedSidebarPanel.hasSelectedElement) {

This could be:

if (selectedSidebarPanel.hasSelectedElement)
    return;

> Source/WebInspectorUI/UserInterface/Base/Main.js:954
> +                // FIXME: Is there a way to ensure the representedItem is selected when we show this content view?

You will likely need to repeat this code from above (maybe add a helper function to do it):

var treeElement = selectedSidebarPanel.treeElementForRepresentedObject(representedObject);
if (treeElement)
    treeElement.revealAndSelect(true, false, false, true);

> Source/WebInspectorUI/UserInterface/Base/Main.js:955
> +                WebInspector.contentBrowser.contentViewContainer.showContentView(contentView);

ContentBrowser has a showContentView. Could just be: WebInspector.contentBrowser.showContentView

[Joseph Pecoraro]

Comment on attachment 243538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243538&action=review

Looks good to me as well. My comments are just nits, take them if you wish.

> Source/WebInspectorUI/UserInterface/Base/Main.js:946
> +        // Check history for a suitable ContentView.

Comment not necessary. It is clear that is what the code is doing.

> Source/WebInspectorUI/UserInterface/Base/Main.js:961
> +        // No suitable view, show a default content view.

Comment not necessary.

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:71
> +            return [
> +                "debugger",
> +                "resource"
> +            ];

Nit: I think the whitespace is unnecessary.
Nit: Maybe we should avoid raw strings. How about this:

    return [WebInspector.debuggerSidebarPanel.identifier, WebInspector.resourceSidebarPanel.identifier]

[Nikita Vasilyev]

Comment on attachment 243538 [details]
Patch

I’ve found a bug: https://cloudup.com/con2_5cg2DA

[Nikita Vasilyev]

(In reply to comment #30)
> Comment on attachment 243538 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243538&action=review
> 
> Nice!
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:945
> > +    if (!selectedSidebarPanel.hasSelectedElement) {
> 
> This could be:
> 
> if (selectedSidebarPanel.hasSelectedElement)
>     return;
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:954
> > +                // FIXME: Is there a way to ensure the representedItem is selected when we show this content view?
> 
> You will likely need to repeat this code from above (maybe add a helper
> function to do it):
> 
> var treeElement =
> selectedSidebarPanel.treeElementForRepresentedObject(representedObject);
> if (treeElement)
>     treeElement.revealAndSelect(true, false, false, true);

I’m not sure why I need to repeat this, it seems to select the right tree element already.

[Nikita Vasilyev]

(In reply to comment #31)
> Comment on attachment 243538 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243538&action=review
> 
> Looks good to me as well. My comments are just nits, take them if you wish.
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:946
> > +        // Check history for a suitable ContentView.
> 
> Comment not necessary. It is clear that is what the code is doing.
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:961
> > +        // No suitable view, show a default content view.
> 
> Comment not necessary.
> 
> > Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:71
> > +            return [
> > +                "debugger",
> > +                "resource"
> > +            ];
> 
> Nit: I think the whitespace is unnecessary.
> Nit: Maybe we should avoid raw strings. How about this:
> 
>     return [WebInspector.debuggerSidebarPanel.identifier,
> WebInspector.resourceSidebarPanel.identifier]

I find it a bit more readable with the whitespace. If you don’t insist, I’ll leave it like this:
[
    WebInspector.debuggerSidebarPanel.identifier,
    WebInspector.resourceSidebarPanel.identifier
]

Agreed on everything else.

[Nikita Vasilyev]

Created attachment 243621 [details]
Patch

Fixed the problem with the Timeline: https://cloudup.com/clTOHM-ebVu

[Timothy Hatcher]

Comment on attachment 243621 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243621&action=review

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:165
> +        return [
> +            WebInspector.resourceSidebarPanel.identifier,
> +            WebInspector.timelineSidebarPanel.identifier
> +        ];

This should move to any subclass that cares. By returning [] it works with any of the three sidebars we have now. Then your FrameContentView.js change can be reverted. You will likely need to return ["resource", "timeline"] from some other subclasses.

> Source/WebInspectorUI/UserInterface/Views/FrameContentView.js:66
> +            WebInspector.resourceSidebarPanel.identifier,
> +            WebInspector.timelineSidebarPanel.identifier,
> +            WebInspector.debuggerSidebarPanel.identifier // Frames are usually HTML documents and they may have inline scripts.

Could return [] to work with any or be removed if you keep ContentView.js as-is.

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:71
> +                WebInspector.resourceSidebarPanel.identifier,
> +                WebInspector.timelineSidebarPanel.identifier,
> +                WebInspector.debuggerSidebarPanel.identifier

This could just return [] to work with any sidebar.

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:72
> +

Extra empty line.

[Nikita Vasilyev]

Comment on attachment 243621 [details]
Patch

This patch has a regression: console content view doesn’t hide when switching to other sidebars.

https://cloudup.com/cXdGOpGwveK

[Nikita Vasilyev]

Created attachment 243922 [details]
Patch

I fixed the problem with the console mentioned in the previous comment (https://cloudup.com/cGwe91uFCwJ), but I broke back/forward navigation in Debugger (https://cloudup.com/ckcB-0zTDIp). If you have a clue what I’m missing, please let me know, I’m a bit stuck on this one.

[Nikita Vasilyev]

*** Bug 138993 has been marked as a duplicate of this bug. ***

[Timothy Hatcher]

Comment on attachment 243922 [details]
Patch

I helped Nikita find the issue in his patch. He will post a new version that addresses all known regressions.

[Nikita Vasilyev]

Created attachment 244319 [details]
Patch

I haven’t found any problems mentioned above in this patch.

[WebKit Commit Bot]

Comment on attachment 244319 [details]
Patch

Clearing flags on attachment: 244319

Committed r178155: <http://trac.webkit.org/changeset/178155>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.