Bug 144717 – Web Inspector: Tabs: Conflicts with multiple Formatters per SourceCode

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Tabs: Conflicts with multiple Formatters per SourceCode,

With the new Tab design, there may be multiple SourceCodeTextEditors per-SourceCode. Previously this was a 1-to-1 relationship and we asserted as such. This can lead to subtle issues, and we may want to reconsider how this works.

* SCENARIO
1. Open editor for SourceCode X in Resources tab
  => pretty printing creates formatter A for X
2. Open editor for SourceCode X in Debugger tab
  => pretty printing creates formatter B for X
3. Modify the resource in the Debugger tab
  => view for X differs between Resources and Debugger tabs
  => if you re-pretty-print, a new formatter C is created which is different from the others
  => if you un-pretty-print you can wipe out a formatter for another tab

* NOTES
How do other editors handle the same file open in multiple tabs? Is there just one "editor" per resource, but that view can show up in multiple places? Similar to how we handle the LogContentView for the Console Tab and Split Console.

[Radar WebKit Bug Importer]

<rdar://problem/20845163>

[Joseph Pecoraro]

This causes issues when there are multiple TextEditors with some features applied (pretty printing, Type Profiler, searching, editing). We should really reduce this duplication.

[Joseph Pecoraro]

Also bad, this affects SourceCodeLocation links and jump to location when there are multiple editors with different formatted/unformatted states. E.g. Timeline showing a content view for a resource that a background tab already had formatted => jumps to 0:0 because it didn't understand the incoming location was already formatted.

[Joseph Pecoraro]

Created attachment 271128 [details]
[PATCH] Proposed Fix

Technically an issue with multiple Formatters can still exist:

    1. Open Resources tab
    2. Open jquery.js
    3. Close Resources tab
    4. Open / Jump to Location for jquery.js
       => possible problem

But this change is a big leap forward in the common case. Closing tabs presents all kinds of other problems we perform poorly at anyways.

[Joseph Pecoraro]

Comment on attachment 271128 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:87
> -        // contentViews on the real representedObject and not the one originally supplied.
> +        // contentView on the real representedObject may not be one originally supplied.

Oops.

[Devin Rousso]

Comment on attachment 271128 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Models/BackForwardEntry.js:133
> +            var position = {scrollTop: element.scrollTop, scrollLeft: element.scrollLeft};

Might as well make this "let" too

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:44
> +    static existingContentViewForRepresentedObject(representedObject)

NIT: Shouldn't this go under a "// Static" section?

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:47
> +        return finalRepresentedObject[WebInspector.ContentView.ContentViewForRepresentedObjectSymbol];

It seems like everywhere you call this function asserts that the returned object is truthy (has value).  Maybe move some/all of the asserts above this line and say something along the lines of "Represented Object should have content view" if it is falsy.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:50
> +    static closedContentViewForRepresentedObject(representedObject)

Should this be "closeContentViewForRepresentedObject" instead of a past-tense version?

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:82
> +            if (representedObject.sourceCode instanceof WebInspector.Resource)

NIT: Are you not combining the "else if" condition with this because it's too long, or is there a different reason?  Since they both return the same thing, it looks a bit weird...

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:91
> +    static _createFromRepresentedObject(representedObject, extraArguments)

NIT: Instead of making this a "private" static function, could you just place it inside another function in createFromRepresentedObject?  I personally think that having a "public" and "private" function with almost the same name is a bit confusing.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:70
> +        var contentView = WebInspector.ContentView.existingContentViewForRepresentedObject(representedObject);

NIT: let instead of var

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:407
> +        console.assert(contentView.parentContainer !== this);

In the case that this is true, I think you should early return to prevent a bunch of extra work from happening.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:414
> +        contentView._parentContainer = this;

Shouldn't we make _parentContainer into a setter on ContentView?

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:486
> +            if (otherInterestedContentViews[0] !== this)

So, if I am following this right, I think the reason why you're using [0] here is because you're only interested in passing the contentView to the most recent interested view if one exists and is not the current one.  If so, could you add a brief statement explaining something to that effect (about the [0]).  If not, definitely add a comment.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:519
> +        // If this was a tombstone, it should already have been hidden.

Should you add an assert here then?

[Build Bot]

Comment on attachment 271128 [details]
[PATCH] Proposed Fix

Attachment 271128 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/817620

New failing tests:
http/tests/xmlhttprequest/workers/referer.html

[Build Bot]

Created attachment 271134 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Blaze Burg]

Comment on attachment 271128 [details]
[PATCH] Proposed Fix

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

The approach looks good overall, but would like some questions answered and I want to review this again with fresh eyes this afternoon or weekend. It would be good if you lived on this for a few days to make sure nothing is obviously broken. Maybe you can write up some manual test cases that you did along the way to make sure everything is working right.

> Source/WebInspectorUI/ChangeLog:89
> +        It is an in replace of a content view.

Nit: 'is an in replace' ?

> Source/WebInspectorUI/ChangeLog:95
> +        Deal with closing backforward entries that are tombstones.

Nit: back-forward

>> Source/WebInspectorUI/UserInterface/Views/ContentView.js:44
>> +    static existingContentViewForRepresentedObject(representedObject)
> 
> NIT: Shouldn't this go under a "// Static" section?

I would prefer that our API match the old one: ContentView.contentViewForRepresentedObject(object, existingOnly). With this patch it's split between existingContentViewForRepresentedObject, createFromRepresentedObject, and two more helpers. This makes it harder to follow, in my opinion.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:68
> +    static _finalRepresentedObjectForRepresentedObject(representedObject)

The '_final' terminology is confusing to me. Can we call it 'resolved' or 'displayable' or something like that? When reading it as 'finalRepresentedObject' i had the impression that it was being mutated or something.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:-70
> -        // Iterate over all the known content views for the representedObject (if any) and find one that doesn't

If we maintain the same API above, then this code will call the same method twice if !onlyExisting.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:89
>          representedObject = contentView.representedObject;

The result of this assignment is not used.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:112
> +        // not owned by us, it may need to be transferred to us.

s/us/this container/

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:405
> +    _transferContentView(contentView)

From reading the call sites its not always clear whether this method transfer the content view to or from |this|. Can we hint at that with the name? Maybe, becomeParentOfContentView or takeContentView.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:420
> +    _placeTombstonesForContentView(contentView)

s/place/add/

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:431
> +        for (let entry of this._backForwardList) {

This sort of assumes that backForwardList won't be mutated by _hideEntry(). This may be true now, but taking a copy for iterating may be safer.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:447
> +    _reviveTombstonesForContentView(contentView)

s/revive/clear/

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:456
> +        for (let entry of this._backForwardList) {

Ditto.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:465
> +    _disassociateFromContentView(contentView, tombstone)

What is the intended type of the 'tombstone' argument? It's used as a boolean,  but it's not named like one.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:472
> +            const onlyFirst = true;

Thanks for using a named argument! But, could there be multiple tombstones for the same content view (ie., when switching between two content views several times, then another container transfers it away)?

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:484
> +        let otherInterestedContentViews = contentView.__tombstoneContentViewContainers;

This variable's name doesn't match its type. It's a content view container. Also the 'interested' terminology is weird, how about:

let associatedContainers = ...

>> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:486
>> +            if (otherInterestedContentViews[0] !== this)
> 
> So, if I am following this right, I think the reason why you're using [0] here is because you're only interested in passing the contentView to the most recent interested view if one exists and is not the current one.  If so, could you add a brief statement explaining something to that effect (about the [0]).  If not, definitely add a comment.

There seem to be two cases here when dissociating the content view:

- the container is the parent, or
- the container has a tombstone

In the second case, this code would transfer the content view to some other container. But I would have expected |this| to never be an interested container at this point since we removed it from the tombstone list in the branch above (modulo my comment). So why is this if check necessary?

[Timothy Hatcher]

Comment on attachment 271128 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/ContentView.js:68
>> +    static _finalRepresentedObjectForRepresentedObject(representedObject)
> 
> The '_final' terminology is confusing to me. Can we call it 'resolved' or 'displayable' or something like that? When reading it as 'finalRepresentedObject' i had the impression that it was being mutated or something.

I like resolved.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:84
> +            else if (representedObject.sourceCode instanceof WebInspector.Script)

No else.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:423
> +        console.assert(!contentView.__tombstoneContentViewContainers || !contentView.__tombstoneContentViewContainers.includes(this));

Why not a symbol?

[Joseph Pecoraro]

Comment on attachment 271128 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/ContentView.js:50
>> +    static closedContentViewForRepresentedObject(representedObject)
> 
> Should this be "closeContentViewForRepresentedObject" instead of a past-tense version?

I prefer the past tense here, because we call this _after_ we call closed() so that the cached ContentView can be removed. I would have just put this in the base classes' ContentView.prototype.closed but it seems some superclasses do not call super.closed() and I didn't want to clean up all the ContentViews just yet.

>> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:414
>> +        contentView._parentContainer = this;
> 
> Shouldn't we make _parentContainer into a setter on ContentView?

This is a rare oddity in our code. I suppose we could, but the idea is ContentViewContainer is the only one to set it, so we avoided adding a public setter that someone else might use. Weird, I know.

>> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:431
>> +        for (let entry of this._backForwardList) {
> 
> This sort of assumes that backForwardList won't be mutated by _hideEntry(). This may be true now, but taking a copy for iterating may be safer.

This may be too paranoid. I don't think this will ever be the case, but I'll add something to the ChangeLog.

>>> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:486
>>> +            if (otherInterestedContentViews[0] !== this)
>> 
>> So, if I am following this right, I think the reason why you're using [0] here is because you're only interested in passing the contentView to the most recent interested view if one exists and is not the current one.  If so, could you add a brief statement explaining something to that effect (about the [0]).  If not, definitely add a comment.
> 
> There seem to be two cases here when dissociating the content view:
> 
> - the container is the parent, or
> - the container has a tombstone
> 
> In the second case, this code would transfer the content view to some other container. But I would have expected |this| to never be an interested container at this point since we removed it from the tombstone list in the branch above (modulo my comment). So why is this if check necessary?

In this case "this" is the owning content view container and should not be in the list, so the if check can be dropped.

I was using [0] not because it would be the most recent, I was just passing it on to any other interested content view container. I'll improve the comment.

>> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:519
>> +        // If this was a tombstone, it should already have been hidden.
> 
> Should you add an assert here then?

This was explained briefly in the ChangeLog. Closing content views will call hide, and we stick this bail here instead of in closing methods to simplify the logic in closing.

We can't necessarily assert that if this is a tombstone the view is hidden, because whoever else is owning the content view might be showing it! We just have to trust that when we made this a tombstone that we hid it if it was the visible content view. Which is the case in placeTombstones.

[Joseph Pecoraro]

Created attachment 271201 [details]
[PATCH] Proposed Fix

Addressed review comments.

[Timothy Hatcher]

Comment on attachment 271201 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:178
> +        if (representedObject instanceof WebInspector.Breakpoint) {
> +            if (representedObject.sourceCodeLocation)

Could be one line.

[Joseph Pecoraro]

(In reply to comment #13)
> Comment on attachment 271201 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271201&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/ContentView.js:178
> > +        if (representedObject instanceof WebInspector.Breakpoint) {
> > +            if (representedObject.sourceCodeLocation)
> 
> Could be one line.

Its matching the normal create and isViewable style, so I left it

[WebKit Commit Bot]

Comment on attachment 271201 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 271201

Committed r196508: <http://trac.webkit.org/changeset/196508>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.