removeSubview looks up a value in an array twice: 1. this._subviews.includes(view) 2. this._subviews.remove(view, true), which loops over an array https://github.com/WebKit/webkit/blob/0b21e4ce918800f832d96011b4ac695024c7882e/Source/WebInspectorUI/UserInterface/Views/View.js#L118
Created attachment 319223 [details] Patch
Comment on attachment 319223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319223&action=review > Source/WebInspectorUI/ChangeLog:12 > + Since we loop backwards, the argument should be onlyLast. Wow, pretty big oversight! I think the places where onlyFirst/onlyLast was set to true are all arrays of unique items, so it probably doesn't matter. In fact the only place where we actually depend on it being false is in ContentViewContainer.prototype._clearTombstonesForContentView. In any case, whether it is the first or last item that is removed is an implementation detail. This function has a confusing API, and should probably be split into remove and removeAll. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:497 > + value: function(value, onlyLast) One downside to naming parameters passed to functions is that they need to be fixed up when making changes like this: .../OpenSource/Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js: 875 const onlyFirst = true; 876 for (let invalidAction of invalidActions) 877: options.actions.remove(invalidAction, onlyFirst); 878 879 return options; .../OpenSource/Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js: 387 let tombstoneContentViewContainers = this._tombstoneContentViewContainersForContentView(contentView); 388 const onlyFirst = false; 389: tombstoneContentViewContainers.remove(this, onlyFirst); 390 391 for (let entry of this._backForwardList) { ... 405 let tombstoneContentViewContainers = this._tombstoneContentViewContainersForContentView(contentView); 406 const onlyFirst = true; 407: tombstoneContentViewContainers.remove(this, onlyFirst); 408 return; 409 } > Source/WebInspectorUI/UserInterface/Views/View.js:124 > + Style: remove extra newline.
Comment on attachment 319223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319223&action=review >> Source/WebInspectorUI/ChangeLog:12 >> + Since we loop backwards, the argument should be onlyLast. > > Wow, pretty big oversight! I think the places where onlyFirst/onlyLast was set to true are all arrays of unique items, so it probably doesn't matter. In fact the only place where we actually depend on it being false is in ContentViewContainer.prototype._clearTombstonesForContentView. In any case, whether it is the first or last item that is removed is an implementation detail. This function has a confusing API, and should probably be split into remove and removeAll. If we split it into remove and removeAll, what would remove do? Remove onlyLast?
Created attachment 319228 [details] Patch
Comment on attachment 319228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319228&action=review > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:388 > + tombstoneContentViewContainers.remove(this); `onlyFirst` was false in this case, so it should be `removeAll`.
Comment on attachment 319228 [details] Patch Attachment 319228 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4400803 New failing tests: inspector/unit-tests/array-utilities.html
Created attachment 319234 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 319228 [details] Patch Attachment 319228 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4400811 New failing tests: inspector/unit-tests/array-utilities.html
Created attachment 319235 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 319228 [details] Patch Attachment 319228 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4400815 New failing tests: inspector/unit-tests/array-utilities.html
Created attachment 319237 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 319263 [details] Patch
Comment on attachment 319263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319263&action=review r=me, with a nit. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:499 > + for (let i = this.length - 1; i >= 0; --i) { Iterating from the end of the array feels weird now that it's no longer necessary. Plus the test output currently states that "remove should only remove the first matching value".
Comment on attachment 319263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319263&action=review > LayoutTests/inspector/unit-tests/array-utilities.html:133 > + InspectorTest.expectShallowEqual(arr1, [1, 2, 3], "remove should only remove the first matching value."); As Matt points out, this says it should remove the first but we see that the last was removed. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:497 > - value: function(value, onlyFirst) > + value: function(value) Nit: You can use the shorthand syntax here `value(value)` instead of `value: function(value)` > Source/WebInspectorUI/UserInterface/Base/Utilities.js:510 > + value: function(value) Nit: You can use the shorthand syntax here `value(value)` instead of `value: function(value)` > Source/WebInspectorUI/UserInterface/Views/View.js:129 > - this._subviews.remove(view, true); > + this._subviews.splice(index, 1); We could add an Array.prototype.removeAtIndex. Since I never know what splice does, removeAtIndex ends up reading clearer to me.
Created attachment 319267 [details] Patch
(In reply to Joseph Pecoraro from comment #14) > Comment on attachment 319263 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=319263&action=review > > > LayoutTests/inspector/unit-tests/array-utilities.html:133 > > + InspectorTest.expectShallowEqual(arr1, [1, 2, 3], "remove should only remove the first matching value."); > > As Matt points out, this says it should remove the first but we see that the > last was removed. Makes sense, thanks! > > Source/WebInspectorUI/UserInterface/Views/View.js:129 > > - this._subviews.remove(view, true); > > + this._subviews.splice(index, 1); > > We could add an Array.prototype.removeAtIndex. Since I never know what > splice does, removeAtIndex ends up reading clearer to me. I don't know if I like adding a new method for this. I'd need to know of it existence to use it and I personally remember what splice does already.
Comment on attachment 319263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319263&action=review >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:510 >> + value: function(value) > > Nit: You can use the shorthand syntax here `value(value)` instead of `value: function(value)` Most methods in this file still use the old syntax. I'm going to convert all of them to the shorthand one.
Comment on attachment 319267 [details] Patch Clearing flags on attachment: 319267 Committed r221308: <http://trac.webkit.org/changeset/221308>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34139108>
(In reply to Nikita Vasilyev from comment #16) > > > Source/WebInspectorUI/UserInterface/Views/View.js:129 > > > - this._subviews.remove(view, true); > > > + this._subviews.splice(index, 1); > > > > We could add an Array.prototype.removeAtIndex. Since I never know what > > splice does, removeAtIndex ends up reading clearer to me. > > I don't know if I like adding a new method for this. I'd need to know of it > existence to use it and I personally remember what splice does already. On the other hand, we have Array.prototyte.insertAtIndex already. I don't mind if somebody adds Array.prototype.removeAtIndex.