Bug 176041 – Web Inspector: Optimize View.prototype.removeSubview

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

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

[Nikita Vasilyev]

Created attachment 319223 [details]
Patch

[Matt Baker]

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.

[Nikita Vasilyev]

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?

[Nikita Vasilyev]

Created attachment 319228 [details]
Patch

[Matt Baker]

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`.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Nikita Vasilyev]

Created attachment 319263 [details]
Patch

[Matt Baker]

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".

[Joseph Pecoraro]

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.

[Nikita Vasilyev]

Created attachment 319267 [details]
Patch

[Nikita Vasilyev]

(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.

[Nikita Vasilyev]

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.

[WebKit Commit Bot]

Comment on attachment 319267 [details]
Patch

Clearing flags on attachment: 319267

Committed r221308: <http://trac.webkit.org/changeset/221308>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34139108>

[Nikita Vasilyev]

(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.