Summary: Object.shallowEqual always fails when comparing array property values. Steps to Reproduce: 1. Inspect the Inspector 2. In the console, evaluate: > Object.shallowEqual({a: []}, {a: []}); > Object.shallowEqual({a: new Array}, {a: new Array}); > Object.shallowEqual({a: [1]}, {a: [1]}); Expected: All true Actual: All false Notes: Object.shallowEqual uses Array.shallowEqual when both objects are arrays, but always uses strict equality when comparing property values.
<rdar://problem/37374639>
Created attachment 333436 [details] Patch
Comment on attachment 333436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333436&action=review > Source/WebInspectorUI/UserInterface/Base/Utilities.js:92 > + let aValue = a[aKey]; > + let bValue = b[aKey]; > + if (Array.isArray(aValue) && Array.isArray(bValue)) { > + // Use Array.shallowEqual, since struct equality fails for similar arrays. > + if (!Array.shallowEqual(aValue, bValue)) > + return false; > + } else { > + // Check that the values are strict equal since this is only > + // a shallow check, not a recursive one. > + if (aValue !== bValue) > + return false; > + } I think you could simplify this to always call `Array.shallowEqual` regardless of whether `aValue` or `bValue` is an array, since it already early returns false if either is not an array. (along these lines, we could also rework the logic earlier in `Object.shallowEqual` that checks if the values are arrays to simply just call `Array.shallowEqual` and return true if it does) if (aValue !== bValue && !Array.shallowEqual(aValue, bValue) return false; I think we still want to do a strict equality comparison even for arrays, as that might help us avoid unnecessary work if it is the case that two different objects have a reference to the same array. let a = []; for (let i = 0; i < 10000; ++i) a.push(i); Object.shallowEquals({a}, {a});
Created attachment 333456 [details] Patch
Comment on attachment 333436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333436&action=review >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:92 >> + } > > I think you could simplify this to always call `Array.shallowEqual` regardless of whether `aValue` or `bValue` is an array, since it already early returns false if either is not an array. (along these lines, we could also rework the logic earlier in `Object.shallowEqual` that checks if the values are arrays to simply just call `Array.shallowEqual` and return true if it does) > > if (aValue !== bValue && !Array.shallowEqual(aValue, bValue) > return false; > > I think we still want to do a strict equality comparison even for arrays, as that might help us avoid unnecessary work if it is the case that two different objects have a reference to the same array. > > let a = []; > for (let i = 0; i < 10000; ++i) > a.push(i); > > Object.shallowEquals({a}, {a}); Nice suggestions, I simplified the patch a bit, and removed redundant comments. We shouldn't need to do an additional check for two property values that refer to the same array. Array.shallowEqual performs this check early on.
Comment on attachment 333456 [details] Patch r=me. Nice fix =D
Comment on attachment 333456 [details] Patch Clearing flags on attachment: 333456 Committed r228336: <https://trac.webkit.org/changeset/228336>
All reviewed patches have been landed. Closing bug.