Changeset 196508 in webkit
- Timestamp:
- Feb 12, 2016 2:15:14 PM (8 years ago)
- Location:
- trunk/Source/WebInspectorUI
- Files:
-
- 4 edited
Legend:
- Unmodified
- Added
- Removed
-
trunk/Source/WebInspectorUI/ChangeLog
r196464 r196508 1 2016-02-12 Joseph Pecoraro <pecoraro@apple.com> 2 3 Web Inspector: Tabs: Conflicts with multiple Formatters per SourceCode 4 https://bugs.webkit.org/show_bug.cgi?id=144717 5 <rdar://problem/20845163> 6 7 Reviewed by Timothy Hatcher. 8 9 The underlying issue here is that each tab may create its own ContentView, 10 and therefore SourceCodeTextEditor, per-SourceCode. Each SourceCodeTextEditor 11 was mutating the SourceCode's state without listening for or expecting 12 updates from the other. This causes a bunch of different issues: 13 14 - editing in one tab does not get reflected in another tab for 15 the same resource. This is common when using the Search tab 16 to find and make an edit, then debug in another tab. 17 18 - one tab may auto format (pretty print) a resource and set 19 the formatter on the SourceCode to make SourceCodeLocations 20 know about formatted locations. However, a jump to location 21 that opens a new ContentView for the same Resource will 22 start out un-formatted, and misunderstand the location. 23 This often results in an unexpected jump to 0:0. 24 25 The solution taken by this change is to have a single ContentView 26 per represented object. When that ContentView gets shown in a new 27 ContentViewContainer it gets transferred, leaving a tombstone in the 28 previous ContentViewContainer that can be revived later. This keeps 29 back foward lists with expected values. It also means there is a 30 single ContentView that doesn't need to worry about having the 31 state of its represented object getting overrun. 32 33 Currently this makes the assumption that we won't ever show multiple 34 ContentViews for the same represented object at the same time. This 35 may need to change if we were to support split pane editor or 36 something like that. 37 38 This also makes the assumption that ContentViewContainer's showEntry 39 and hideEntry do not modify the back forward list. That has not been 40 the case, and I think it is safe to assume it will never be the case. 41 42 The contracts this patch maintains: 43 44 - a ContentView is always owned by one ViewContainer. 45 This ViewContainer is the one showing the ContentView. 46 47 - when another ViewContainer wants to share the ContentView 48 ownership is transferred. Creating tombstones in the old 49 ViewContainer and Reviving tombstones in the new ViewContainer. 50 51 - ViewContainer's have a tombstone per-BackForwardEntry that 52 references the ContentView. 53 54 - In order to ensure a ContentView always gets closed, when 55 the owning ViewContainer would close the ContentView it 56 checks if it should instead transfer ownership of the 57 ContentView to another interested ViewContainer. 58 59 This also maintains the contract that a ContentView should only be 60 closed once. When the ContentView is transferred between two 61 ContentViewContainers it should hide/show from the old to the new. 62 The last ContentViewContainer to reference a ContentView should 63 be the one to close the ContentView. 64 65 * UserInterface/Models/BackForwardEntry.js: 66 (WebInspector.BackForwardEntry): 67 (WebInspector.BackForwardEntry.prototype.get tombstone): 68 (WebInspector.BackForwardEntry.prototype.set tombstone): 69 (WebInspector.BackForwardEntry.prototype.prepareToShow): 70 (WebInspector.BackForwardEntry.prototype.prepareToHide): 71 Tombstone state and assertions that we don't show/hide tombstones, 72 that should all be done before a back forward entry has become a tombstone. 73 74 * UserInterface/Views/ContentView.js: 75 (WebInspector.ContentView.contentViewForRepresentedObject): 76 (WebInspector.ContentView.closedContentViewForRepresentedObject): 77 (WebInspector.ContentView.resolvedRepresentedObjectForRepresentedObject): 78 Helpers for getting / creating / clearing the single ContentView that 79 is associated with a represented object. 80 81 * UserInterface/Views/ContentViewContainer.js: 82 (WebInspector.ContentViewContainer.prototype.contentViewForRepresentedObject): 83 (WebInspector.ContentViewContainer.prototype.showContentView): 84 Eliminate code that dealt with multiple content views per represented object. 85 That is replaced by multiple ContentViewContainers per ContentView. 86 87 (WebInspector.ContentViewContainer.prototype.replaceContentView): 88 This is called in special places where we don't need to worry about a tombstone. 89 It is an in replace of a content view. 90 91 (WebInspector.ContentViewContainer.closeAllContentViewsOfPrototype): 92 (WebInspector.ContentViewContainer.prototype.closeContentView): 93 (WebInspector.ContentViewContainer.prototype.closeAllContentViews): 94 (WebInspector.ContentViewContainer.prototype._disassociateFromContentView): 95 Deal with closing BackForwardEntrys that are tombstones. 96 97 (WebInspector.ContentViewContainer.prototype._takeOwnershipOfContentView): 98 (WebInspector.ContentViewContainer.prototype._placeTombstonesForContentView): 99 (WebInspector.ContentViewContainer.prototype._clearTombstonesForContentView): 100 Helpers for transfering ownership of a ContentView to a ContentViewContainer. 101 There is always one owner of the ContentView. Non-owners have tombstone 102 BackForward entries. 103 104 (WebInspector.ContentViewContainer.prototype._showEntry): 105 If we are showing a tombstone, gain ownership. 106 107 (WebInspector.ContentViewContainer.prototype._hideEntry): 108 This may happen in closing, for simplicity we bail here instead of include 109 messy logic at the call site. We would have already hidden this entry 110 when making it a tombstone. 111 1 112 2016-02-11 Joseph Pecoraro <pecoraro@apple.com> 2 113 -
trunk/Source/WebInspectorUI/UserInterface/Models/BackForwardEntry.js
r192612 r196508 33 33 this._contentView = contentView; 34 34 35 // ContentViews may be shared across multiple ContentViewContainers. 36 // A BackForwardEntry containing a tombstone doesn't actually have 37 // the real ContentView, and so should not close it. 38 this._tombstone = false; 39 35 40 // Cookies are compared with Object.shallowEqual, so should not store objects or arrays. 36 41 this._cookie = cookie || {}; … … 53 58 } 54 59 60 get tombstone() 61 { 62 return this._tombstone; 63 } 64 65 set tombstone(tombstone) 66 { 67 this._tombstone = tombstone; 68 } 69 55 70 prepareToShow(shouldCallShown) 56 71 { 72 console.assert(!this._tombstone, "Should not be calling shown on a tombstone"); 73 57 74 this._restoreFromCookie(); 58 75 … … 65 82 prepareToHide() 66 83 { 84 console.assert(!this._tombstone, "Should not be calling hidden on a tombstone"); 85 67 86 this.contentView.visible = false; 68 87 this.contentView.hidden(); … … 112 131 continue; 113 132 114 var position = { scrollTop: element.scrollTop, scrollLeft: element.scrollLeft};133 let position = {scrollTop: element.scrollTop, scrollLeft: element.scrollLeft}; 115 134 if (this.contentView.shouldKeepElementsScrolledToBottom) 116 135 position.isScrolledToBottom = element.isScrolledToBottom(); -
trunk/Source/WebInspectorUI/UserInterface/Views/ContentView.js
r195999 r196508 40 40 } 41 41 42 // Public42 // Static 43 43 44 44 static createFromRepresentedObject(representedObject, extraArguments) … … 141 141 } 142 142 143 static contentViewForRepresentedObject(representedObject, onlyExisting, extraArguments) 144 { 145 console.assert(representedObject); 146 147 let resolvedRepresentedObject = WebInspector.ContentView.resolvedRepresentedObjectForRepresentedObject(representedObject); 148 let existingContentView = resolvedRepresentedObject[WebInspector.ContentView.ContentViewForRepresentedObjectSymbol]; 149 console.assert(!existingContentView || existingContentView instanceof WebInspector.ContentView); 150 if (existingContentView) 151 return existingContentView; 152 153 if (onlyExisting) 154 return null; 155 156 let newContentView = WebInspector.ContentView.createFromRepresentedObject(representedObject, extraArguments); 157 console.assert(newContentView instanceof WebInspector.ContentView); 158 if (!newContentView) 159 return null; 160 161 console.assert(newContentView.representedObject === resolvedRepresentedObject, "createFromRepresentedObject and resolvedRepresentedObjectForRepresentedObject are out of sync for type", representedObject.constructor.name); 162 newContentView.representedObject[WebInspector.ContentView.ContentViewForRepresentedObjectSymbol] = newContentView; 163 return newContentView; 164 } 165 166 static closedContentViewForRepresentedObject(representedObject) 167 { 168 let resolvedRepresentedObject = WebInspector.ContentView.resolvedRepresentedObjectForRepresentedObject(representedObject); 169 resolvedRepresentedObject[WebInspector.ContentView.ContentViewForRepresentedObjectSymbol] = null; 170 } 171 172 static resolvedRepresentedObjectForRepresentedObject(representedObject) 173 { 174 if (representedObject instanceof WebInspector.Frame) 175 return representedObject.mainResource; 176 177 if (representedObject instanceof WebInspector.Breakpoint) { 178 if (representedObject.sourceCodeLocation) 179 return representedObject.sourceCodeLocation.displaySourceCode; 180 } 181 182 if (representedObject instanceof WebInspector.DOMSearchMatchObject) 183 return WebInspector.frameResourceManager.mainFrame.domTree; 184 185 if (representedObject instanceof WebInspector.SourceCodeSearchMatchObject) 186 return representedObject.sourceCode; 187 188 return representedObject; 189 } 190 143 191 static isViewable(representedObject) 144 192 { … … 346 394 NavigationItemsDidChange: "content-view-navigation-items-did-change" 347 395 }; 396 397 WebInspector.ContentView.ContentViewForRepresentedObjectSymbol = Symbol("content-view-for-represented-object"); -
trunk/Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js
r192049 r196508 64 64 contentViewForRepresentedObject(representedObject, onlyExisting, extraArguments) 65 65 { 66 console.assert(representedObject); 67 if (!representedObject) 68 return null; 69 70 // Iterate over all the known content views for the representedObject (if any) and find one that doesn't 71 // have a parent container or has this container as its parent. 72 var contentView = null; 73 for (var i = 0; representedObject.__contentViews && i < representedObject.__contentViews.length; ++i) { 74 var currentContentView = representedObject.__contentViews[i]; 75 if (!currentContentView._parentContainer || currentContentView._parentContainer === this) { 76 contentView = currentContentView; 77 break; 78 } 79 } 80 81 console.assert(!contentView || contentView instanceof WebInspector.ContentView); 82 if (contentView instanceof WebInspector.ContentView) 83 return contentView; 84 85 // Return early to avoid creating a new content view when onlyExisting is true. 86 if (onlyExisting) 87 return null; 88 89 // No existing content view found, make a new one. 90 contentView = WebInspector.ContentView.createFromRepresentedObject(representedObject, extraArguments); 91 92 console.assert(contentView, "Unknown representedObject", representedObject); 93 if (!contentView) 94 return null; 95 96 // The representedObject can change in the constructor for ContentView. Remember the 97 // contentViews on the real representedObject and not the one originally supplied. 98 // The main case for this is a Frame being passed in and the main Resource being used. 99 representedObject = contentView.representedObject; 100 101 // Remember this content view for future calls. 102 if (!representedObject.__contentViews) 103 representedObject.__contentViews = []; 104 representedObject.__contentViews.push(contentView); 105 106 return contentView; 66 return WebInspector.ContentView.contentViewForRepresentedObject(representedObject, onlyExisting, extraArguments); 107 67 } 108 68 … … 124 84 return null; 125 85 126 // Don't allow showing a content view that is already associated with another container. 127 // Showing a content view that is already associated with this container is allowed. 128 console.assert(!contentView.parentContainer || contentView.parentContainer === this); 129 if (contentView.parentContainer && contentView.parentContainer !== this) 130 return null; 86 // ContentViews can be shared between containers. If this content view is 87 // not owned by us, it may need to be transferred to this container. 88 if (contentView.parentContainer !== this) 89 this._takeOwnershipOfContentView(contentView); 131 90 132 91 var currentEntry = this.currentBackForwardEntry; … … 159 118 160 119 if (shouldDissociateContentView) 161 this._disassociateFromContentView(removedEntries[i].contentView );120 this._disassociateFromContentView(removedEntries[i].contentView, removedEntries[i].tombstone); 162 121 } 163 122 … … 219 178 220 179 // Disassociate with the old content view. 221 this._disassociateFromContentView(oldContentView );180 this._disassociateFromContentView(oldContentView, false); 222 181 223 182 // Associate with the new content view. … … 227 186 for (var i = 0; i < this._backForwardList.length; ++i) { 228 187 if (this._backForwardList[i].contentView === oldContentView) { 188 console.assert(!this._backForwardList[i].tombstone); 229 189 let currentCookie = this._backForwardList[i].cookie; 230 190 this._backForwardList[i] = new WebInspector.BackForwardEntry(newContentView, currentCookie); … … 279 239 } 280 240 281 this._disassociateFromContentView(entry.contentView );241 this._disassociateFromContentView(entry.contentView, entry.tombstone); 282 242 283 243 // Remove the item from the back/forward list. … … 335 295 } 336 296 337 this._disassociateFromContentView(entry.contentView );297 this._disassociateFromContentView(entry.contentView, entry.tombstone); 338 298 339 299 // Remove the item from the back/forward list. … … 365 325 if (entry.contentView === visibleContentView) 366 326 this._hideEntry(entry); 367 this._disassociateFromContentView(entry.contentView );327 this._disassociateFromContentView(entry.contentView, entry.tombstone); 368 328 } 369 329 … … 418 378 // Private 419 379 420 _disassociateFromContentView(contentView) 421 { 380 _takeOwnershipOfContentView(contentView) 381 { 382 console.assert(contentView.parentContainer !== this, "We already have ownership of the ContentView"); 383 if (contentView.parentContainer === this) 384 return; 385 386 if (contentView.parentContainer) 387 contentView.parentContainer._placeTombstonesForContentView(contentView); 388 389 contentView._parentContainer = this; 390 391 this._clearTombstonesForContentView(contentView); 392 } 393 394 _placeTombstonesForContentView(contentView) 395 { 396 console.assert(contentView.parentContainer === this); 397 398 // Ensure another ContentViewContainer doesn't close this ContentView while we still have it. 399 let tombstoneContentViewContainers = this._tombstoneContentViewContainersForContentView(contentView); 400 console.assert(!tombstoneContentViewContainers.includes(this)); 401 402 let visibleContentView = this.currentContentView; 403 404 for (let entry of this._backForwardList) { 405 if (entry.contentView !== contentView) 406 continue; 407 408 if (entry.contentView === visibleContentView) { 409 this._hideEntry(entry); 410 visibleContentView = null; 411 } 412 413 console.assert(!entry.tombstone); 414 entry.tombstone = true; 415 416 tombstoneContentViewContainers.push(this); 417 } 418 } 419 420 _clearTombstonesForContentView(contentView) 421 { 422 console.assert(contentView.parentContainer === this); 423 424 let tombstoneContentViewContainers = this._tombstoneContentViewContainersForContentView(contentView); 425 const onlyFirst = false; 426 tombstoneContentViewContainers.remove(this, onlyFirst); 427 428 for (let entry of this._backForwardList) { 429 if (entry.contentView !== contentView) 430 continue; 431 432 console.assert(entry.tombstone); 433 entry.tombstone = false; 434 } 435 } 436 437 _disassociateFromContentView(contentView, isTombstone) 438 { 439 // Just remove one of our tombstone back references. 440 // There may be other back/forward entries that need a reference. 441 if (isTombstone) { 442 let tombstoneContentViewContainers = this._tombstoneContentViewContainersForContentView(contentView); 443 const onlyFirst = true; 444 tombstoneContentViewContainers.remove(this, onlyFirst); 445 return; 446 } 447 422 448 console.assert(!contentView.visible); 423 449 … … 427 453 contentView._parentContainer = null; 428 454 429 var representedObject = contentView.representedObject; 430 if (representedObject && representedObject.__contentViews) 431 representedObject.__contentViews.remove(contentView); 455 // If another ContentViewContainer has tombstones for this, just transfer 456 // ownership to that ContentViewContainer and avoid closing the ContentView. 457 // We don't care who we transfer this to, so just use the first. 458 let tombstoneContentViewContainers = this._tombstoneContentViewContainersForContentView(contentView); 459 if (tombstoneContentViewContainers && tombstoneContentViewContainers.length) { 460 tombstoneContentViewContainers[0]._takeOwnershipOfContentView(contentView); 461 return; 462 } 432 463 433 464 contentView.closed(); 465 466 if (contentView.representedObject) 467 WebInspector.ContentView.closedContentViewForRepresentedObject(contentView.representedObject); 434 468 } 435 469 … … 437 471 { 438 472 console.assert(entry instanceof WebInspector.BackForwardEntry); 473 474 // We may be showing a tombstone from a BackForward list or when re-showing a container 475 // that had previously had the content view transferred away from it. 476 // Take over the ContentView. 477 if (entry.tombstone) { 478 this._takeOwnershipOfContentView(entry.contentView); 479 console.assert(!entry.tombstone); 480 } 439 481 440 482 if (!this.subviews.includes(entry.contentView)) … … 447 489 { 448 490 console.assert(entry instanceof WebInspector.BackForwardEntry); 491 492 // If this was a tombstone, the content view should already have been 493 // hidden when we placed the tombstone. 494 if (entry.tombstone) 495 return; 449 496 450 497 entry.prepareToHide(); … … 452 499 this.removeSubview(entry.contentView) 453 500 } 501 502 _tombstoneContentViewContainersForContentView(contentView) 503 { 504 let tombstoneContentViewContainers = contentView[WebInspector.ContentViewContainer.TombstoneContentViewContainersSymbol]; 505 if (!tombstoneContentViewContainers) 506 tombstoneContentViewContainers = contentView[WebInspector.ContentViewContainer.TombstoneContentViewContainersSymbol] = []; 507 return tombstoneContentViewContainers; 508 } 454 509 }; 455 510 … … 457 512 CurrentContentViewDidChange: "content-view-container-current-content-view-did-change" 458 513 }; 514 515 WebInspector.ContentViewContainer.TombstoneContentViewContainersSymbol = Symbol("content-view-container-tombstone-content-view-containers");
Note: See TracChangeset
for help on using the changeset viewer.