No subject
Mon Sep 28 12:00:37 PDT 2015
> 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?
--
You are receiving this mail because:
You are the assignee for the bug.
--1455301448.d2708.30515
Date: Fri, 12 Feb 2016 10:24:08 -0800
MIME-Version: 1.0
Content-Type: text/html
<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:bburg@apple.com" title="Brian Burg <bburg@apple.com>"> <span class="fn">Brian Burg</span></a>
</span> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #271128 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<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#c9">Comment # 9</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:bburg@apple.com" title="Brian Burg <bburg@apple.com>"> <span class="fn">Brian Burg</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>
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.
<span class="quote">> Source/WebInspectorUI/ChangeLog:89
> + It is an in replace of a content view.</span >
Nit: 'is an in replace' ?
<span class="quote">> Source/WebInspectorUI/ChangeLog:95
> + Deal with closing backforward entries that are tombstones.</span >
Nit: back-forward
<span class="quote">>> Source/WebInspectorUI/UserInterface/Views/ContentView.js:44
>> + static existingContentViewForRepresentedObject(representedObject)
>
> NIT: Shouldn't this go under a "// Static" section?</span >
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.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContentView.js:68
> + static _finalRepresentedObjectForRepresentedObject(representedObject)</span >
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.
<span class="quote">> 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</span >
If we maintain the same API above, then this code will call the same method twice if !onlyExisting.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:89
> representedObject = contentView.representedObject;</span >
The result of this assignment is not used.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:112
> + // not owned by us, it may need to be transferred to us.</span >
s/us/this container/
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:405
> + _transferContentView(contentView)</span >
More information about the webkit-unassigned
mailing list