<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Tabs: Conflicts with multiple Formatters per SourceCode"
href="https://bugs.webkit.org/show_bug.cgi?id=144717#c6">Comment # 6</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Tabs: Conflicts with multiple Formatters per SourceCode"
href="https://bugs.webkit.org/show_bug.cgi?id=144717">bug 144717</a>
from <span class="vcard"><a class="email" href="mailto:dcrousso+webkit@gmail.com" title="Devin Rousso <dcrousso+webkit@gmail.com>"> <span class="fn">Devin Rousso</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=271128&action=diff" name="attach_271128" title="[PATCH] Proposed Fix">attachment 271128</a> <a href="attachment.cgi?id=271128&action=edit" title="[PATCH] Proposed Fix">[details]</a></span>
[PATCH] Proposed Fix
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=271128&action=review">https://bugs.webkit.org/attachment.cgi?id=271128&action=review</a>
<span class="quote">> Source/WebInspectorUI/UserInterface/Models/BackForwardEntry.js:133
> + var position = {scrollTop: element.scrollTop, scrollLeft: element.scrollLeft};</span >
Might as well make this "let" too
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContentView.js:44
> + static existingContentViewForRepresentedObject(representedObject)</span >
NIT: Shouldn't this go under a "// Static" section?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContentView.js:47
> + return finalRepresentedObject[WebInspector.ContentView.ContentViewForRepresentedObjectSymbol];</span >
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.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContentView.js:50
> + static closedContentViewForRepresentedObject(representedObject)</span >
Should this be "closeContentViewForRepresentedObject" instead of a past-tense version?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContentView.js:82
> + if (representedObject.sourceCode instanceof WebInspector.Resource)</span >
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...
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContentView.js:91
> + static _createFromRepresentedObject(representedObject, extraArguments)</span >
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.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:70
> + var contentView = WebInspector.ContentView.existingContentViewForRepresentedObject(representedObject);</span >
NIT: let instead of var
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:407
> + console.assert(contentView.parentContainer !== this);</span >
In the case that this is true, I think you should early return to prevent a bunch of extra work from happening.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:414
> + contentView._parentContainer = this;</span >
Shouldn't we make _parentContainer into a setter on ContentView?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:486
> + if (otherInterestedContentViews[0] !== this)</span >
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.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:519
> + // If this was a tombstone, it should already have been hidden.</span >
Should you add an assert here then?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>