[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 #6 from Devin Rousso <dcrousso+webkit at gmail.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

> 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?

-- 
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/0e856341/attachment-0001.html>