* SUMMARY REGRESSION(Tabs) Issues reloading a resource with breakpoints * STEPS TO REPRODUCE 1. Inspect <http://bogojoker.com/shell/> 2. Set a breakpoint on easySlider.min.js:56:21 ("if (autoplay) {") 3. Cmd+R to reload => Duplicate breakpoints, with 1 inactive and one on the pre-pretty-printed line.
<rdar://problem/20829494>
Tracing SourceCodeTextEditor.prototype.close calls I see ResourceSidebarPanel closes its views when the main resource changes: Views/SourceCodeTextEditor.js:130:20: CONSOLE LOG close called Views/SourceCodeTextEditor.js:131:22: CONSOLE TRACE 0: close(Views/SourceCodeTextEditor.js:131:22) 1: closed(Views/TextResourceContentView.js:120:31) 2: _disassociateFromContentView(Views/ContentViewContainer.js:459:27) 3: closeAllContentViews(Views/ContentViewContainer.js:384:46) 4: closed(Views/ClusterContentView.js:85:56) 5: closed(Views/ResourceClusterContentView.js:135:62) 6: _disassociateFromContentView(Views/ContentViewContainer.js:459:27) 7: closeAllContentViews(Views/ContentViewContainer.js:384:46) 8: _mainFrameMainResourceDidChange(Views/ResourceSidebarPanel.js:170:70) 9: _mainResourceDidChange(Views/ResourceSidebarPanel.js:160:45) 10: dispatch(Base/Object.js:124:55) 11: dispatchEventToListeners(Base/Object.js:139:21) 12: _dispatchMainResourceDidChangeEvent(Models/Frame.js:474:38) 13: commitProvisionalLoad(Models/Frame.js:138:53) 14: frameDidNavigate(Controllers/FrameResourceManager.js:105:40) 15: frameNavigated(Protocol/PageObserver.js:42:59) 16: dispatchEvent(Protocol/InspectorBackend.js:353:42) 17: _dispatchEvent(Protocol/InspectorBackend.js:233:32) 18: dispatch(Protocol/InspectorBackend.js:88:32) 19: dispatchNextQueuedMessageFromBackend(Protocol/MessageDispatcher.js:42:34) Other panels, like the DebuggerSidebarPanel should probably be doing the same. In this case, what I'm seeing is the old ContentView without a sidebar tree element, and selecting the _new_ easySlider.min.js tree element gives me what I expect.
I count a jumble of issues investigating this: 1. DebuggerSidebarPanel needs to clear content views and tree elements on main resource changes. - closeAllContentViews - breakpointsContentTreeOutline, remove non-global folder tree elements 2. BreakpointTreeElement restoration doesn't work - display source code for a WebInspector.Breakpoint represented object 3. Cookie saving is cumulative instead of replacing - TabContentView adds new cookie state onto its cookie instead of replacing the cookie 4. TabContentView.restoreStateFromCookie(causedByReload) parameter is never set - sounds like a missed opportunity, but it is always unused 5. Order of operations on WebInspector.Frame.Event.MainResourceDidChange is problematic - Main.js performs state restoration on MainResourceDidChange - NavigationSidebarPanels (Resources, Debugger, etc) perform clean up MainResourceDidChange - Typically, Main.js will do its work first, which may be very problematic in restoration! So, I'm going to hesitantly attach a patch, but Tim, feel free to take over this.
Created attachment 252441 [details] [PATCH] Progress I noticed that DOM Node restoration is failing. I believe it was failing before my changes as well.
> 5. Order of operations on WebInspector.Frame.Event.MainResourceDidChange is > problematic > - Main.js performs state restoration on MainResourceDidChange > - NavigationSidebarPanels (Resources, Debugger, etc) perform clean up > MainResourceDidChange > - Typically, Main.js will do its work first, which may be very problematic > in restoration! The reason this is problematic was because of this scenario: (1) DebuggerSidebarPanel wants to remove its ResourceTreeElements for the old Frame and add ResourceTreeElements for the new Frame (2) State restoration wants to find a ResourceTreeElement basically just via URL On reload, in cases where (2) happens before (1), then the ResourceTreeElement found during restoration was the OLD ResourceTreeElement, that immediately gets removed.
Comment on attachment 252441 [details] [PATCH] Progress View in context: https://bugs.webkit.org/attachment.cgi?id=252441&action=review > Source/WebInspectorUI/UserInterface/Views/ContentView.js:124 > + console.error("Unknown ContentView", representedObject); > + > throw "Can't make a ContentView for an unknown representedObject."; I've been considering removing the throw here and just doing a console.error. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:393 > + for (var i = this._breakpointsContentTreeOutline.children.length - 1; i >= 0; --i) { > + var treeElement = this._breakpointsContentTreeOutline.children[i]; > + if (treeElement !== this._globalBreakpointsFolderTreeElement) > + this._breakpointsContentTreeOutline.removeChildAtIndex(i, true, true); > + } You should not need to do this. DebuggerSidebarPanel opts-in to NavigationSidebarPanel's autoPruneOldTopLevelResourceTreeElements. That will remove ResourceTreeElements that go away on MainResourceDidChange, ChildFrameWasRemoved, and ResourceWasRemoved events. The closeAllContentViews is all we should need. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:429 > + // FIXME: Is this needed, now that we clear everything on main resource changes? Yes, since NavigationSidebarPanel does not auto-prune these. > Source/WebInspectorUI/UserInterface/Views/TabContentView.js:127 > - var cookie = this._cookieSetting.value || {}; > + var cookie = {}; Ah, yes. I see how this was wrong now.
I'll take this over from Joe since he is out on vacation.
Created attachment 252894 [details] Patch
Comment on attachment 252894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252894&action=review r=me, with some speling mistakes. Naming suggestions are sort of advisory, since this patch didn't establish the naming convention. > Source/WebInspectorUI/ChangeLog:5 > + I really appreciate the big list of issues here :) > Source/WebInspectorUI/UserInterface/Base/Main.js:1147 > +WebInspector._restoreCookieForOpenTabs = function(causedByNaviation) "navigation" > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:379 > + var oldCurrentContentView = this.currentContentView; visibleContentView? shownContentView? oldCurrent is awkward. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:390 > + // prune normally happen on a delay. "happens" on an later event loop turn. > Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:71 > + this._autoPruneOldTopLevelResourceTreeElements = autoPruneOldTopLevelResourceTreeElements || false; Why not !!autoPruneOldTopLevelResourceElements? As for the autoPruneOldXXX, I think a naming convention like autoPruneStaleXXX better conveys why it's being pruned. And for a flag, this._shouldAutoPruneTopLevelResourceTreeElements makes it clear what purpose it serves (a policy decision). > Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:420 > + pruneOldResourceTreeElements() See naming note above. > Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:424 > + this._checkForOldResourcesTimeoutIdentifier = undefined; Why not delete? > Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:-620 > - this._checkForOldResourcesTimeoutIdentifier = setTimeout(delayedWork.bind(this), 0); (We really should make a helper method to do this coalescing, I go cross-eyed looking for mistakes every time it's reimplemented.) > Source/WebInspectorUI/UserInterface/Views/TabContentView.js:110 > + restoreStateFromCookie: function(causedByNaviation) "navigation" > Source/WebInspectorUI/UserInterface/Views/TabContentView.js:115 > var matchTypeOnlyDelayForReload = 2000; Might as well fix this variable name to be consistent.
Comment on attachment 252894 [details] Patch https://trac.webkit.org/r184130