Bug 176041

Summary: Web Inspector: Optimize View.prototype.removeSubview
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, inspector-bugzilla-changes, joepeck, mattbaker, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Patch
mattbaker: review+
Patch none

Description Nikita Vasilyev 2017-08-28 17:56:25 PDT
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
Comment 1 Nikita Vasilyev 2017-08-28 18:01:06 PDT
Created attachment 319223 [details]
Patch
Comment 2 Matt Baker 2017-08-28 18:39:39 PDT
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 3 Nikita Vasilyev 2017-08-28 18:49:05 PDT
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?
Comment 4 Nikita Vasilyev 2017-08-28 19:11:03 PDT
Created attachment 319228 [details]
Patch
Comment 5 Matt Baker 2017-08-28 20:10:13 PDT
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 6 Build Bot 2017-08-28 20:16:33 PDT
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
Comment 7 Build Bot 2017-08-28 20:16:34 PDT
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 8 Build Bot 2017-08-28 20:21:46 PDT
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
Comment 9 Build Bot 2017-08-28 20:21:48 PDT
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 10 Build Bot 2017-08-28 20:37:32 PDT
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
Comment 11 Build Bot 2017-08-28 20:37:33 PDT
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
Comment 12 Nikita Vasilyev 2017-08-29 11:15:41 PDT
Created attachment 319263 [details]
Patch
Comment 13 Matt Baker 2017-08-29 11:25:14 PDT
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 14 Joseph Pecoraro 2017-08-29 11:28:36 PDT
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.
Comment 15 Nikita Vasilyev 2017-08-29 11:44:08 PDT
Created attachment 319267 [details]
Patch
Comment 16 Nikita Vasilyev 2017-08-29 11:51:09 PDT
(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 17 Nikita Vasilyev 2017-08-29 11:54:09 PDT
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 18 WebKit Commit Bot 2017-08-29 12:26:00 PDT
Comment on attachment 319267 [details]
Patch

Clearing flags on attachment: 319267

Committed r221308: <http://trac.webkit.org/changeset/221308>
Comment 19 WebKit Commit Bot 2017-08-29 12:26:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2017-08-29 12:26:59 PDT
<rdar://problem/34139108>
Comment 21 Nikita Vasilyev 2017-08-29 14:54:43 PDT
(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.