Bug 161867 – Web Inspector: Use Array.shallowEqual instead of Object.shallowEqual in more places

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Summary:
Use Array.shallowEqual instead of Object.shallowEqual in more places. Crude perf testing shows the Array version to be ~30x faster. Array.shallowEqual should also check the type of its arguments before checking for equality. The following currently return true:

Array.shallowEqual(null, null)
Array.shallowEqual("abc", "abc")
Array.shallowEqual(1.23, 1.23)
...

[Radar WebKit Bug Importer]

<rdar://problem/28261328>

[Matt Baker]

Created attachment 288579 [details]
[Patch] Proposed Fix

[Build Bot]

Comment on attachment 288579 [details]
[Patch] Proposed Fix

Attachment 288579 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2060313

New failing tests:
inspector/unit-tests/array-utilities.html

[Build Bot]

Created attachment 288590 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288579 [details]
[Patch] Proposed Fix

Attachment 288579 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2060311

New failing tests:
inspector/unit-tests/array-utilities.html

[Build Bot]

Created attachment 288592 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288579 [details]
[Patch] Proposed Fix

Attachment 288579 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2060363

New failing tests:
inspector/unit-tests/array-utilities.html

[Build Bot]

Created attachment 288593 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Matt Baker]

Created attachment 288596 [details]
[Patch] Proposed Fix

[Devin Rousso]

Comment on attachment 288596 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=288596&action=review

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:439
> +        if (!(a instanceof Array) || !(b instanceof Array))

Why not just use Array.isArray()?

[Matt Baker]

(In reply to comment #10)
> Comment on attachment 288596 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288596&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/Utilities.js:439
> > +        if (!(a instanceof Array) || !(b instanceof Array))
> 
> Why not just use Array.isArray()?

`instanceof Array` is far more prevalent in the inspector, but apparently has some shortcomings addressed by `Array.isArray()`: http://web.mit.edu/jwalden/www/isArray.html.

I think it has nicer syntax too. Will change.

[Matt Baker]

Created attachment 288606 [details]
[Patch] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 288606 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=288606&action=review

r=me

> LayoutTests/inspector/unit-tests/array-utilities.html:102
> +            InspectorTest.expectThat(Array.shallowEqual(new Array(...arr1), new Array(...arr1)), "shallowEqual of equal arrays should be true.");
> +            InspectorTest.expectThat(Array.shallowEqual(new Array(...arr1), arr1), "shallowEqual of equal arrays should be true.");

Would it be clearer writing these as [...arr1]? I don't thin we ever use the Array constructor, and I did a double take.

[Matt Baker]

Comment on attachment 288606 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=288606&action=review

>> LayoutTests/inspector/unit-tests/array-utilities.html:102
>> +            InspectorTest.expectThat(Array.shallowEqual(new Array(...arr1), arr1), "shallowEqual of equal arrays should be true.");
> 
> Would it be clearer writing these as [...arr1]? I don't thin we ever use the Array constructor, and I did a double take.

I'll remove or rewrite the tests that use the Array constructor.

[Matt Baker]

I'm also going to replace this function used by the Array.prototype.partition test:

let quickEqual = (a, b) => JSON.stringify(a) === JSON.stringify(b);

with:

Array.shallowEqual, which is faster.

[Matt Baker]

Created attachment 288718 [details]
[Patch] For Landing

[WebKit Commit Bot]

Comment on attachment 288718 [details]
[Patch] For Landing

Clearing flags on attachment: 288718

Committed r205872: <http://trac.webkit.org/changeset/205872>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.