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
Created attachment 243093 [details] patch
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.)
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?
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.
(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.
(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).
(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.
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!
(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.
(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?
(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)) ...
Created attachment 243427 [details] Patch Animated GIF of Inspector with the patch applied: https://cloudup.com/cKBoXOp0_AO
(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.
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.*
(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.
> 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.
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.
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.
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.
(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.
<rdar://problem/19281503>
<rdar://problem/19281643>
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.
(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.
(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.
(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.
(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.
> 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.
Created attachment 243538 [details] Patch Animated GIF: https://cloudup.com/ct5WzR_Sb4J
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
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]
Comment on attachment 243538 [details] Patch I’ve found a bug: https://cloudup.com/con2_5cg2DA
(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.
(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.
Created attachment 243621 [details] Patch Fixed the problem with the Timeline: https://cloudup.com/clTOHM-ebVu
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.
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
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.
*** Bug 138993 has been marked as a duplicate of this bug. ***
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.
Created attachment 244319 [details] Patch I haven’t found any problems mentioned above in this patch.
Comment on attachment 244319 [details] Patch Clearing flags on attachment: 244319 Committed r178155: <http://trac.webkit.org/changeset/178155>
All reviewed patches have been landed. Closing bug.