[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

Brian Burg <bburg at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #271128|review?                     |review-
              Flags|                            |

--- Comment #9 from Brian Burg <bburg at apple.com> ---
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

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)