[Webkit-unassigned] [Bug 144717] Web Inspector: Tabs: Conflicts with multiple Formatters per SourceCode

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=144717

--- Comment #11 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 271128
  --> https://bugs.webkit.org/attachment.cgi?id=271128
[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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160212/bccd3e26/attachment.html>