Changeset 196508 in webkit


Ignore:
Timestamp:
Feb 12, 2016 2:15:14 PM (8 years ago)
Author:
commit-queue@webkit.org
Message:

Web Inspector: Tabs: Conflicts with multiple Formatters per SourceCode
https://bugs.webkit.org/show_bug.cgi?id=144717
<rdar://problem/20845163>

Patch by Joseph Pecoraro <Joseph Pecoraro> on 2016-02-12
Reviewed by Timothy Hatcher.

The underlying issue here is that each tab may create its own ContentView,
and therefore SourceCodeTextEditor, per-SourceCode. Each SourceCodeTextEditor
was mutating the SourceCode's state without listening for or expecting
updates from the other. This causes a bunch of different issues:

  • editing in one tab does not get reflected in another tab for the same resource. This is common when using the Search tab to find and make an edit, then debug in another tab.
  • one tab may auto format (pretty print) a resource and set the formatter on the SourceCode to make SourceCodeLocations know about formatted locations. However, a jump to location that opens a new ContentView for the same Resource will start out un-formatted, and misunderstand the location. This often results in an unexpected jump to 0:0.

The solution taken by this change is to have a single ContentView
per represented object. When that ContentView gets shown in a new
ContentViewContainer it gets transferred, leaving a tombstone in the
previous ContentViewContainer that can be revived later. This keeps
back foward lists with expected values. It also means there is a
single ContentView that doesn't need to worry about having the
state of its represented object getting overrun.

Currently this makes the assumption that we won't ever show multiple
ContentViews for the same represented object at the same time. This
may need to change if we were to support split pane editor or
something like that.

This also makes the assumption that ContentViewContainer's showEntry
and hideEntry do not modify the back forward list. That has not been
the case, and I think it is safe to assume it will never be the case.

The contracts this patch maintains:

  • a ContentView is always owned by one ViewContainer. This ViewContainer is the one showing the ContentView.
  • when another ViewContainer wants to share the ContentView ownership is transferred. Creating tombstones in the old ViewContainer and Reviving tombstones in the new ViewContainer.
  • ViewContainer's have a tombstone per-BackForwardEntry that references the ContentView.
  • In order to ensure a ContentView always gets closed, when the owning ViewContainer would close the ContentView it checks if it should instead transfer ownership of the ContentView to another interested ViewContainer.

This also maintains the contract that a ContentView should only be
closed once. When the ContentView is transferred between two
ContentViewContainers it should hide/show from the old to the new.
The last ContentViewContainer to reference a ContentView should
be the one to close the ContentView.

  • UserInterface/Models/BackForwardEntry.js:

(WebInspector.BackForwardEntry):
(WebInspector.BackForwardEntry.prototype.get tombstone):
(WebInspector.BackForwardEntry.prototype.set tombstone):
(WebInspector.BackForwardEntry.prototype.prepareToShow):
(WebInspector.BackForwardEntry.prototype.prepareToHide):
Tombstone state and assertions that we don't show/hide tombstones,
that should all be done before a back forward entry has become a tombstone.

  • UserInterface/Views/ContentView.js:

(WebInspector.ContentView.contentViewForRepresentedObject):
(WebInspector.ContentView.closedContentViewForRepresentedObject):
(WebInspector.ContentView.resolvedRepresentedObjectForRepresentedObject):
Helpers for getting / creating / clearing the single ContentView that
is associated with a represented object.

  • UserInterface/Views/ContentViewContainer.js:

(WebInspector.ContentViewContainer.prototype.contentViewForRepresentedObject):
(WebInspector.ContentViewContainer.prototype.showContentView):
Eliminate code that dealt with multiple content views per represented object.
That is replaced by multiple ContentViewContainers per ContentView.

(WebInspector.ContentViewContainer.prototype.replaceContentView):
This is called in special places where we don't need to worry about a tombstone.
It is an in replace of a content view.

(WebInspector.ContentViewContainer.closeAllContentViewsOfPrototype):
(WebInspector.ContentViewContainer.prototype.closeContentView):
(WebInspector.ContentViewContainer.prototype.closeAllContentViews):
(WebInspector.ContentViewContainer.prototype._disassociateFromContentView):
Deal with closing BackForwardEntrys that are tombstones.

(WebInspector.ContentViewContainer.prototype._takeOwnershipOfContentView):
(WebInspector.ContentViewContainer.prototype._placeTombstonesForContentView):
(WebInspector.ContentViewContainer.prototype._clearTombstonesForContentView):
Helpers for transfering ownership of a ContentView to a ContentViewContainer.
There is always one owner of the ContentView. Non-owners have tombstone
BackForward entries.

(WebInspector.ContentViewContainer.prototype._showEntry):
If we are showing a tombstone, gain ownership.

(WebInspector.ContentViewContainer.prototype._hideEntry):
This may happen in closing, for simplicity we bail here instead of include
messy logic at the call site. We would have already hidden this entry
when making it a tombstone.

Location:
trunk/Source/WebInspectorUI
Files:
4 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebInspectorUI/ChangeLog

    r196464 r196508  
     12016-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
    11122016-02-11  Joseph Pecoraro  <pecoraro@apple.com>
    2113
  • trunk/Source/WebInspectorUI/UserInterface/Models/BackForwardEntry.js

    r192612 r196508  
    3333        this._contentView = contentView;
    3434
     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
    3540        // Cookies are compared with Object.shallowEqual, so should not store objects or arrays.
    3641        this._cookie = cookie || {};
     
    5358    }
    5459
     60    get tombstone()
     61    {
     62        return this._tombstone;
     63    }
     64
     65    set tombstone(tombstone)
     66    {
     67        this._tombstone = tombstone;
     68    }
     69
    5570    prepareToShow(shouldCallShown)
    5671    {
     72        console.assert(!this._tombstone, "Should not be calling shown on a tombstone");
     73
    5774        this._restoreFromCookie();
    5875
     
    6582    prepareToHide()
    6683    {
     84        console.assert(!this._tombstone, "Should not be calling hidden on a tombstone");
     85
    6786        this.contentView.visible = false;
    6887        this.contentView.hidden();
     
    112131                continue;
    113132
    114             var position = { scrollTop: element.scrollTop, scrollLeft: element.scrollLeft };
     133            let position = {scrollTop: element.scrollTop, scrollLeft: element.scrollLeft};
    115134            if (this.contentView.shouldKeepElementsScrolledToBottom)
    116135                position.isScrolledToBottom = element.isScrolledToBottom();
  • trunk/Source/WebInspectorUI/UserInterface/Views/ContentView.js

    r195999 r196508  
    4040    }
    4141
    42     // Public
     42    // Static
    4343
    4444    static createFromRepresentedObject(representedObject, extraArguments)
     
    141141    }
    142142
     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
    143191    static isViewable(representedObject)
    144192    {
     
    346394    NavigationItemsDidChange: "content-view-navigation-items-did-change"
    347395};
     396
     397WebInspector.ContentView.ContentViewForRepresentedObjectSymbol = Symbol("content-view-for-represented-object");
  • trunk/Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js

    r192049 r196508  
    6464    contentViewForRepresentedObject(representedObject, onlyExisting, extraArguments)
    6565    {
    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);
    10767    }
    10868
     
    12484            return null;
    12585
    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);
    13190
    13291        var currentEntry = this.currentBackForwardEntry;
     
    159118
    160119            if (shouldDissociateContentView)
    161                 this._disassociateFromContentView(removedEntries[i].contentView);
     120                this._disassociateFromContentView(removedEntries[i].contentView, removedEntries[i].tombstone);
    162121        }
    163122
     
    219178
    220179        // Disassociate with the old content view.
    221         this._disassociateFromContentView(oldContentView);
     180        this._disassociateFromContentView(oldContentView, false);
    222181
    223182        // Associate with the new content view.
     
    227186        for (var i = 0; i < this._backForwardList.length; ++i) {
    228187            if (this._backForwardList[i].contentView === oldContentView) {
     188                console.assert(!this._backForwardList[i].tombstone);
    229189                let currentCookie = this._backForwardList[i].cookie;
    230190                this._backForwardList[i] = new WebInspector.BackForwardEntry(newContentView, currentCookie);
     
    279239            }
    280240
    281             this._disassociateFromContentView(entry.contentView);
     241            this._disassociateFromContentView(entry.contentView, entry.tombstone);
    282242
    283243            // Remove the item from the back/forward list.
     
    335295            }
    336296
    337             this._disassociateFromContentView(entry.contentView);
     297            this._disassociateFromContentView(entry.contentView, entry.tombstone);
    338298
    339299            // Remove the item from the back/forward list.
     
    365325            if (entry.contentView === visibleContentView)
    366326                this._hideEntry(entry);
    367             this._disassociateFromContentView(entry.contentView);
     327            this._disassociateFromContentView(entry.contentView, entry.tombstone);
    368328        }
    369329
     
    418378    // Private
    419379
    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
    422448        console.assert(!contentView.visible);
    423449
     
    427453        contentView._parentContainer = null;
    428454
    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        }
    432463
    433464        contentView.closed();
     465
     466        if (contentView.representedObject)
     467            WebInspector.ContentView.closedContentViewForRepresentedObject(contentView.representedObject);
    434468    }
    435469
     
    437471    {
    438472        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        }
    439481
    440482        if (!this.subviews.includes(entry.contentView))
     
    447489    {
    448490        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;
    449496
    450497        entry.prepareToHide();
     
    452499            this.removeSubview(entry.contentView)
    453500    }
     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    }
    454509};
    455510
     
    457512    CurrentContentViewDidChange: "content-view-container-current-content-view-did-change"
    458513};
     514
     515WebInspector.ContentViewContainer.TombstoneContentViewContainersSymbol = Symbol("content-view-container-tombstone-content-view-containers");
Note: See TracChangeset for help on using the changeset viewer.