| Summary: | Web Inspector: CSS Changes: modifications aren't shared for rules that match multiple elements | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||||||||||||||||||||||||||||||
| Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, nvasilyev, rniwa, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||
|
Description
Devin Rousso
2019-03-03 20:46:41 PST
Thank you for reporting this! I wish I'd foresee this scenario sooner. `body, div` rule has different WI.CSSStyleDeclaration instances when inspecting <div> and <body>. However, both of these instances have the same `id`! Perhaps, the best solution here would be to avoid storing `_initialState` on the model directly, and maintain a map (CSSStyleDeclaration.id -> CSSStyleDeclaration) of modified properties on CSSManager. Created attachment 366548 [details]
Patch
This is a sizable patch. I'd like to know if the overall direction is correct.
Created attachment 366552 [details]
Patch
Rebaselined.
Comment on attachment 366552 [details] Patch Attachment 366552 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11745327 New failing tests: inspector/css/add-css-property.html inspector/css/overridden-property.html inspector/css/modify-css-property.html inspector/css/modify-inline-style.html inspector/css/modify-css-property-race.html inspector/css/css-property.html Created attachment 366558 [details]
Archive of layout-test-results from ews101 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 366561 [details]
Patch
Comment on attachment 366561 [details] Patch Attachment 366561 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11745976 New failing tests: inspector/css/modify-inline-style.html Created attachment 366564 [details]
Archive of layout-test-results from ews100 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 366561 [details] Patch Attachment 366561 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11746170 New failing tests: inspector/css/modify-inline-style.html Created attachment 366568 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 366561 [details] Patch Attachment 366561 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11746128 New failing tests: inspector/css/modify-inline-style.html Created attachment 366569 [details]
Archive of layout-test-results from ews117 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 366672 [details]
Patch
Comment on attachment 366672 [details] Patch Attachment 366672 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11757907 New failing tests: inspector/css/add-css-property.html inspector/css/overridden-property.html inspector/css/modify-css-property.html inspector/css/modify-inline-style.html inspector/css/modify-css-property-race.html inspector/css/css-property.html Created attachment 366678 [details]
Archive of layout-test-results from ews101 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 366683 [details]
Patch
Comment on attachment 366683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366683&action=review r-, I'm still confused as to why we "need" jsdiff? It seems like the majority of that library focuses on text diffing, which we aren't using at all. Is there a reason why we can't modify `Array.diffArrays` to support a custom comparator? If there's "more advanced" functionality provided by jsdiff, is it something which we could try to duplicate using our style/logic/methodology in `Array.diffArrays`? If we do end up using jsdiff, you'll need to modify WebInspectorUI/Scripts/copy-user-interface-resources.pl to make sure it's included in minified builds (you should probably also change WebKit/InspectorGResources.cmake). > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:508 > + ModifiedStatusChanged: "css-property-modified-status-changed", NIT: I realize that there's already a `*StatusChanged` event for this object, but I'd prefer `*StateChanged` (more common), or even better just `*Changed` (the most common). It avoids unnecessary characters and conveys the same message. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:59 > + set initialState(state) { this._initialState = state || null; } When is this used? Is it necessary to have the fallback `|| null`? > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:434 > + if (item.added) { Is the reason that we don't need to check for `item.removed` as well because the removed value won't be visible in the Rules panel (e.g. it'll have been removed)? (In reply to Devin Rousso from comment #18) > Comment on attachment 366683 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=366683&action=review > > r-, I'm still confused as to why we "need" jsdiff? It seems like the > majority of that library focuses on text diffing, which we aren't using at > all. Is there a reason why we can't modify `Array.diffArrays` to support a > custom comparator? Yes, there's a reason. Array.diffArrays can't take a custom comparator. Internally, diffArrays uses sets: Object.defineProperty(Array, "diffArrays", { value(initialArray, currentArray, onEach) { let initialSet = new Set(initialArray); let currentSet = new Set(currentArray); Array.diffArrays requires each item in the array to be unique. It's possible that several CSS properties in the same CSS rule are identical (even though I can't provide of a case when it would be intentional). jsdiff uses an algorithm based on "An O(ND) Difference Algorithm and Its Variations" by Eugene W. Myers. http://blog.robertelder.org/diff-algorithm/ It's a more complex algorithm that I wrote for Array.diffArrays. Yes, >80% of jsdiff code is currently unused. I can reimplement Myers' algorithm. If it isn't too time-consuming, maybe we can avoid using a 3rd party library. (In reply to Nikita Vasilyev from comment #19) > Array.diffArrays can't take a custom comparator. Internally, diffArrays uses sets: Can we change it so that it doesn't use sets? We don't need this algorithm to be the *most* efficient it can be, since we're basically only using this for CSS diffing right now and each rule is likely to have a very small (e.g. <100) number of properties. Couldn't we do a "naive" approach by checking all future items in `currentArray` against the current item of `initialArray` (e.g. `initialArray[indexInitial]` compared to all items from `indexCurrent` to `currentArray.length - 1` in `currentArray`)? I think it should work. Diff algorithm improvements could be a separate patch. (In reply to Nikita Vasilyev from comment #21) > I think it should work. Diff algorithm improvements could be a separate > patch. The naive algorithm produced a very suboptimal diff. It explained well in this article: https://epxx.co/artigos/diff_en.html I'm trying something different to avoid using Myers' algorithm. Comment on attachment 366683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366683&action=review >> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:508 >> + ModifiedStatusChanged: "css-property-modified-status-changed", > > NIT: I realize that there's already a `*StatusChanged` event for this object, but I'd prefer `*StateChanged` (more common), or even better just `*Changed` (the most common). It avoids unnecessary characters and conveys the same message. I like `*Changed`. >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:434 >> + if (item.added) { > > Is the reason that we don't need to check for `item.removed` as well because the removed value won't be visible in the Rules panel (e.g. it'll have been removed)? Correct. (I may add red markers to show removed properties in the future but not in this patch.) Created attachment 368625 [details]
Patch
My latest patch doesn't use any 3rd party libraries. I modified the diff method to take a hash function. It allowed me to continue using Sets. To allow repeating items in the arrays, I replaced Sets with Maps to store usage counts. Comment on attachment 368625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368625&action=review r=me, please address the `Array.diffArrays` question before landing. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:504 > + value(initialArray, currentArray, hashFunction, onEach) NIT: I'd make `hashFunction` an "optional" and have a fallback implementation, which is just an ident function. This matches other uses of this pattern, like `comparator`. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:523 > + function decrementUsageCount(map, key) { Style: put this below the other function so that they're all nearby. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:-531 > - ++deltaCurrent; Why did this move? > Source/WebInspectorUI/UserInterface/Base/Utilities.js:578 > + onEach(initialArray[i], -1); I think you should always be doing this, not just after the final decrement. If I had three items left in this array, I'd expect to see three removals. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:589 > + onEach(current, 1); Ditto (>578). > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:408 > + let cssProperty = new WI.CSSProperty( > + this._index, > + this._text, > + this._name, > + this._rawValue, > + this._priority, > + this._enabled, > + this._overridden, > + this._implicit, > + this._anonymous, > + this._valid, > + this._styleSheetTextRange); I hate this syntax T.T > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:59 > + set initialState(state) { this._initialState = state; } Why is this needed? > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:166 > + propertyLineElement.classList.add("css-property-line", className); > + let stylePropertyView = new WI.SpreadsheetStyleProperty(null, cssProperty, {readOnly: true}); Style: missing newline. NIT: you should add a `const delegate = null;` so it's clearer. Comment on attachment 368625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368625&action=review >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:-531 >> - ++deltaCurrent; > > Why did this move? This shouldn't move. I'm adding more tests now I found this doesn't work correctly with repeating items. >> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:408 >> + this._styleSheetTextRange); > > I hate this syntax T.T What are you proposing? >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:59 >> + set initialState(state) { this._initialState = state; } > > Why is this needed? It isn't! Created attachment 369076 [details]
Patch
Resetting to r? since I made changes to Array.diffArrays.
Created attachment 369077 [details]
Patch
Comment on attachment 369077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369077&action=review > LayoutTests/inspector/unit-tests/array-utilities-expected.txt:92 > +["b","a","b"], ["a","b","a"] => [["a",0],["b",0],["b",-1],["a",1]] This seems wrong. I'd expect to see a `["b",-1]` at the very beginning, not the end. ["b","a","b"], ["a","b","a"] => [["b",-1],["a",0],["b",0],["a",1]] > LayoutTests/inspector/unit-tests/array-utilities-expected.txt:95 > +["a","b","b","c"], ["c","b","b","b","a"] => [["c",0],["b",0],["b",0],["b",1],["c",-1],["a",1]] > +["a","b","b","b","c"], ["c","b","b","a"] => [["c",0],["b",0],["b",0],["b",-1],["a",0]] > +["a","a","b","b","c","c"], ["b","b","c","c","a","a"] => [["b",0],["b",0],["c",0],["c",0],["a",0],["a",0]] Ditto (>92). What happened to the starting "a"s? ["a","b","b","c"], ["c","b","b","b","a"] => [["a",-1],["c",1],["b",0],["b",0],["b",1],["c",-1],["a",1]] ["a","b","b","b","c"], ["c","b","b","a"] => [["a",-1],["c",1],["b",0],["b",0],["b",-1],["c",-1],["a",1]] ["a","a","b","b","c","c"], ["b","b","c","c","a","a"] => [["a",-1],["a",-1],["b",0],["b",0],["c",0],["c",0],["a",1],["a",1]] Especially in the last case, it seems like `Array.diffArrays` considers an item that moved as a non-delete-then-add, which I'd disagree with. A move involves a delete at one place and an add at another. The only time we should see a `0` action is when the item is in the same relative positioning as it was initially (like the "bbcc" in the last example). Moves should be a zero-sum of `-1` and `+1` (or in the opposite order). Comment on attachment 369077 [details]
Patch
I was contemplating even adding tests for repeated items because I don't see why would anyone intentionally add CSS properties that are exactly the same. In fact, we already show "Duplicate property" warning in the Styles panel.
This is the case that I had in mind:
font-size: 12px;
[10 other properties]
Removing `font-size: 12px` and adding it as the last property would not show any changes, which I understand is a bit unconventional. The goal was to make the output a little less noisy since it's likely to be used for copy/pasting new values.
I can go either way but ultimatelly I don't think any developers would even notice it in their workflows.
Created attachment 369201 [details]
[Image] Firefox Changes panel
To bolster my case, this what removing the 1st CSS property ("font-size: 4.8rem" in this case) and appending it as the last looks in Firefox.
Since they don't display unchanged properties, the diff output becomes confusing.
Also, if we want an optimal diff output and at the same time don't want to eliminate common (i.e. moved) items, we are back to square one. The solution is Myers' algorithm. Comment on attachment 369077 [details]
Patch
Greg pointed out offline that order does matter:
font-size: 12px;
font-size: .8rem;
If I swap the properties, the diff should indeed display the change.
Created attachment 369999 [details]
Patch
(In reply to Devin Rousso from comment #30) > Comment on attachment 369077 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=369077&action=review > > > LayoutTests/inspector/unit-tests/array-utilities-expected.txt:92 > > +["b","a","b"], ["a","b","a"] => [["a",0],["b",0],["b",-1],["a",1]] > > This seems wrong. I'd expect to see a `["b",-1]` at the very beginning, not > the end. > > ["b","a","b"], ["a","b","a"] => [["b",-1],["a",0],["b",0],["a",1]] With my latest patch, it's: [["a",1],["b",0],["a",0],["b",-1]] I agree that what you suggested is better. This could be improved by using "patience diff": https://blog.jcoglan.com/2017/09/19/the-patience-diff-algorithm/ I'd prefer to do it later. (In reply to Nikita Vasilyev from comment #33) > Also, if we want an optimal diff output and at the same time don't want to > eliminate common (i.e. moved) items, we are back to square one. The solution > is Myers' algorithm. My latest patch does NOT eliminate common (e.e. moved) items. I didn't use Myers' algorithm. I used a naive algorithm with an added common suffix and prefix eliminations. It's plenty fast for my case and the output is acceptable, in my opinion. Created attachment 370198 [details]
Patch
Removed an assert that no longer makes sense ("CSSProperty was modified without saving initial state").
The rest is the same as the previous patch.
Comment on attachment 370198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370198&action=review > LayoutTests/inspector/unit-tests/array-utilities-expected.txt:92 > +["a"], ["a","a"] => [["a",1],["a",0]] > +["a","a"], ["a"] => [["a",-1],["a",0]] > +["a","a"], ["a","a"] => [["a",0],["a",0]] > +["b","a","b"], ["a","b","a"] => [["a",1],["b",0],["a",0],["b",-1]] NIT: is there any way for you to reverse the ordering of all these? [["a",1],["a",0]] vs [["a",0],["a",1]] [["a",-1],["a",0]] vs [["a",0],["a",-1]] [["a",1],["b",0],["a",0],["b",-1]] vs [["b",-1],["a",0],["b",0],["a",1]] > LayoutTests/inspector/unit-tests/array-utilities-expected.txt:95 > +["a","a","b","b","c","c"], ["b","b","c","c","a","a"] => [["a",-1],["a",-1],["b",0],["b",0],["c",0],["c",0],["a",1],["a",1]] and yet somehow, this one is exactly as I'd expect :) > Source/WebInspectorUI/UserInterface/Base/Utilities.js:512 > + value(initialArray, currentArray, onEach, comparator = (initial, current) => initial === current) Our usual style of default comparators is to do a check in the function itself (see any of the other `Array.prototype` functions). ``` function defaultComparator(initial, current) { return initial === current; } comparator = comparator || defaultComparator; ``` > Source/WebInspectorUI/UserInterface/Base/Utilities.js:514 > + function findShortestEdit(initialArray, currentArray) { Do you actually need these arguments, since this is called with variables of the same name? Please avoid shadowing wherever possible. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:519 > + for (let i = 0; i < initialArray.length; ++i) { > + for (let j = 0; j < currentArray.length; ++j) { If the arrays are [11, 12, 13, 14, 15] and [21, 12, 13, 14, 25], would this think that the entire array was a giant edit? I'd expect two smaller sub-edit "sections" at the beginning (11 and 21) and end (15 and 25). Regardless, this NEEDS a comment or some sort of explanation as to what it's expected to do. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:536 > + function commonPrefixSize(listA, listB) { NIT: use `length` instead of `size` since this is an array (matches `array.length`). > Source/WebInspectorUI/UserInterface/Base/Utilities.js:537 > + let shortListLength = Math.min(listA.length, listB.length); NIT: `shorterListLength` or `commonLength` > Source/WebInspectorUI/UserInterface/Base/Utilities.js:541 > + for (; i < shortListLength; ++i) { > + if (!comparator(listA[i], listB[i])) > + break; Use a while loop. ``` while (i < shortListLength && comparator(listA[i], listB[i])) ++i; ``` > Source/WebInspectorUI/UserInterface/Base/Utilities.js:546 > + function commonSuffixSize(listA, listB) { Ditto (>536). > Source/WebInspectorUI/UserInterface/Base/Utilities.js:548 > + let aLength = listA.length; > + let bLength = listB.length; It's unnecessary to save these to variables. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:549 > + let shortListLength = Math.min(aLength, bLength); Ditto (>537). > Source/WebInspectorUI/UserInterface/Base/Utilities.js:551 > + for (; i < shortListLength; ++i) { Ditto (>541). > Source/WebInspectorUI/UserInterface/Base/Utilities.js:553 > + let a = listA[aLength - i - 1]; > + let b = listB[bLength - i - 1]; Ditto (>548). > Source/WebInspectorUI/UserInterface/Base/Utilities.js:560 > + function callOnEach(count, action, array) { NIT: this name makes me think it's going to invoke `action` (which is a function) on the first `count` items of `array`. I'd use a different name either for the function itself, or just the `action` argument. Alternatively, you could just inline this at each callsite. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:567 > + let suffixLength = commonSuffixSize(initialArray, currentArray); > + let suffix = []; NIT: I'd reverse the order of these so that `suffixLength` lines up with the `if`. (In reply to Devin Rousso from comment #39) > Comment on attachment 370198 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370198&action=review > > > LayoutTests/inspector/unit-tests/array-utilities-expected.txt:92 > > +["a"], ["a","a"] => [["a",1],["a",0]] > > +["a","a"], ["a"] => [["a",-1],["a",0]] > > +["a","a"], ["a","a"] => [["a",0],["a",0]] > > +["b","a","b"], ["a","b","a"] => [["a",1],["b",0],["a",0],["b",-1]] > > NIT: is there any way for you to reverse the ordering of all these? > [["a",1],["a",0]] vs [["a",0],["a",1]] > [["a",-1],["a",0]] vs [["a",0],["a",-1]] > [["a",1],["b",0],["a",0],["b",-1]] vs [["b",-1],["a",0],["b",0],["a",1]] As I said, I'd need to use patience diff/preprocessing algorithm. I agree that what you suggested is better. This could be improved by using "patience diff": https://blog.jcoglan.com/2017/09/19/the-patience-diff-algorithm/ I'd prefer to do it later. Comment on attachment 370198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370198&action=review >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:519 >> + for (let j = 0; j < currentArray.length; ++j) { > > If the arrays are [11, 12, 13, 14, 15] and [21, 12, 13, 14, 25], would this think that the entire array was a giant edit? I'd expect two smaller sub-edit "sections" at the beginning (11 and 21) and end (15 and 25). > > Regardless, this NEEDS a comment or some sort of explanation as to what it's expected to do. No, it wouldn't. It does what you expect: - 11 + 21 12 13 14 - 15 + 25 because... > Source/WebInspectorUI/UserInterface/Base/Utilities.js:529 > + if (comparator(initialArray[i], currentArray[j])) { > + if (newEditCount < editCount) { > + editCount = newEditCount; > + deletionCount = i; > + additionCount = j; > + } > + break; ...it exits here. I don't know about the comment. At a high level, findShortestEdit does what it says. I couldn't find a good middle ground between function name and a very long comment. Comment on attachment 370198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370198&action=review >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:529 >> + break; > > ...it exits here. > > I don't know about the comment. At a high level, findShortestEdit does what it says. I couldn't find a good middle ground between function name and a very long comment. This only breaks out of the inner loop though. It will continue iterating the outer loop. The only reason this "ends" is because of the other `break` after `if (newEditCount > editCount)`. The very fact that I didn't understand _how_ it worked means that it needs a comment. That would've made it much easier to understand. Comment on attachment 370198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370198&action=review >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:541 >> + break; > > Use a while loop. > ``` > while (i < shortListLength && comparator(listA[i], listB[i])) > ++i; > ``` This looks too dense for me. Especially... >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:551 >> + for (; i < shortListLength; ++i) { > > Ditto (>541). ...here: while (i < shortListLength && comparator(listA[listA.length - i - 1], listB[listB.length - i - 1])) Comment on attachment 370198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370198&action=review >>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:529 >>> + break; >> >> ...it exits here. >> >> I don't know about the comment. At a high level, findShortestEdit does what it says. I couldn't find a good middle ground between function name and a very long comment. > > This only breaks out of the inner loop though. It will continue iterating the outer loop. The only reason this "ends" is because of the other `break` after `if (newEditCount > editCount)`. > > The very fact that I didn't understand _how_ it worked means that it needs a comment. That would've made it much easier to understand. This got me thinking, that this function could be simplified. This function should find the first longest edit. The current name is misleading. It could break out of the outer loop instead, it seems 🤔 (In reply to Nikita Vasilyev from comment #44) > Comment on attachment 370198 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370198&action=review > > >>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:529 > >>> + break; > >> > >> ...it exits here. > >> > >> I don't know about the comment. At a high level, findShortestEdit does what it says. I couldn't find a good middle ground between function name and a very long comment. > > > > This only breaks out of the inner loop though. It will continue iterating the outer loop. The only reason this "ends" is because of the other `break` after `if (newEditCount > editCount)`. > > > > The very fact that I didn't understand _how_ it worked means that it needs a comment. That would've made it much easier to understand. > > This got me thinking, that this function could be simplified. > This function should find the first longest edit. The current name is > misleading. > It could break out of the outer loop instead, it seems 🤔 Actually, I don't think it can be simplified. Consider this case: abbc cbbba Expected result: - a + c b b - c + b + a If I break out of the outside loop, it would result in: + c + b + b + b a - b - b - c It would exit on the first iteration of the outer loop after finding matching "a": abbc cbbba ---- +++ Which is incorrect. When it continues iterating, it finds the shortest edit correctly: abbc cbbba - + Created attachment 370376 [details]
Patch
Comment on attachment 370376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370376&action=review > Source/WebInspectorUI/UserInterface/Base/Utilities.js:584 > + let suffixLength = commonSuffixLength(initialArray, currentArray); What exactly is the point of determining the common suffix? Couldn't you just add a `break` inside the `while` below that checks that `!initialArray.length && !currentArray.length`? Since you are `slice`ing each array in each iteration, eventually the common suffix will be the common prefix (e.g. everything before it would have been `slice`d). Comment on attachment 370376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370376&action=review >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:584 >> + let suffixLength = commonSuffixLength(initialArray, currentArray); > > What exactly is the point of determining the common suffix? Couldn't you just add a `break` inside the `while` below that checks that `!initialArray.length && !currentArray.length`? Since you are `slice`ing each array in each iteration, eventually the common suffix will be the common prefix (e.g. everything before it would have been `slice`d). This is a performance optimization. It's faster to remove the suffix once and don't do extra work in the `while` below. Also, commonSuffixLength has linear time complexity, when findShortestEdit is O(nm). I'd rather keep it. Comment on attachment 370376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370376&action=review > Source/WebInspectorUI/UserInterface/Base/Utilities.js:521 > + // Find the first shortest edit. In the another words, find the prefix of the first longest common sequence. This doesn't really explain what it's doing as far as I understand it. It's really more about finding the first index in each array where the items at those indexes are equal such that the total length of both non-equal prefixes is minimized. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:534 > + if (newEditCount > editCount) { As a potential optimization, it may be worth adding another `break` outside the inner `for` inside an `if (i > editCount)`. This should help avoid some unnecessary looping if the shortest edit is indeed very short :P >>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:584 >>> + let suffixLength = commonSuffixLength(initialArray, currentArray); >> >> What exactly is the point of determining the common suffix? Couldn't you just add a `break` inside the `while` below that checks that `!initialArray.length && !currentArray.length`? Since you are `slice`ing each array in each iteration, eventually the common suffix will be the common prefix (e.g. everything before it would have been `slice`d). > > This is a performance optimization. It's faster to remove the suffix once and don't do extra work in the `while` below. Also, commonSuffixLength has linear time complexity, when findShortestEdit is O(nm). > > I'd rather keep it. I was saying to add the `if (!initialArray.length && !currentArray.length)` after the `if (prefixLength) { ... }`, not after the `findShortestEdit()` call. I don't think what you have right now is more performant, as it has to do the check of finding a common suffix where there might not even be one (e.g. it "front loads" the work). If I had a "sequence" that followed <common prefix> <edit> <common suffix>, then removing the <common suffix> is unnecessary, since the fundamental logic for `commonPrefixLength` and `commonSuffixLength` are the same. The first iteration of the `while` will remove <common prefix> and <edit>, meaning that all that's left is the <common suffix>, which can be handled by the next iteration's `commonPrefixLength` (a prefix is the same as a suffix if the entire string _is_ the prefix). Then, adding the early `break` I suggested would break out of the `while` before it calls `findShortestEdit()` for the second time. In this example, the overall amount of work done is the same (albeit in a different ordering), but if the "sequence" was instead just <common prefix> <edit>, then you'd have tried to find a `commonSuffixLength` for nothing and wasted some "effort" as a result. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:437 > + }, (a, b) => a.equals(b)); Style: I don't usually like having additional arguments after an inline function argument, so I'd prefer if you moved both of these functions to be non-inline instead. ``` function onEach(cssProperty, action) { ... } function comparator(a, b) { return a.equals(b); } Array.diffArrays(initialCSSProperties, cssProperties, onEach, comparator); ``` > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:170 > + }, (a, b) => a.equals(b)); Ditto (>CSSStyleDeclaration.js:437). Comment on attachment 370376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370376&action=review >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:521 >> + // Find the first shortest edit. In the another words, find the prefix of the first longest common sequence. > > This doesn't really explain what it's doing as far as I understand it. It's really more about finding the first index in each array where the items at those indexes are equal such that the total length of both non-equal prefixes is minimized. You're correct. Can we simplify your 2nd sentence? How about: "Find the shortest prefix of matching items in both arrays." >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:534 >> + if (newEditCount > editCount) { > > As a potential optimization, it may be worth adding another `break` outside the inner `for` inside an `if (i > editCount)`. This should help avoid some unnecessary looping if the shortest edit is indeed very short :P I like it! Created attachment 370902 [details]
Patch
Comment on attachment 370902 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370902&action=review r=me, nice work! > LayoutTests/inspector/unit-tests/array-utilities-expected.txt:90 > +["a"], ["a","a"] => [["a",0],["a",1]] > +["a","a"], ["a"] => [["a",0],["a",-1]] I love how these are now in the "right" order :) > Source/WebInspectorUI/UserInterface/Base/Utilities.js:532 > + if (i > editCount) This could have a comment like >537. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:534 > + break; > + for (let j = 0; j < currentArray.length; ++j) { Style: missing newline > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:429 > + let initialCSSProperties = this._initialState.visibleProperties; > + let cssProperties = this.visibleProperties; NIT: these can be inlined. > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:153 > + let initialCSSProperties = style.initialState.visibleProperties; > + let cssProperties = style.visibleProperties; NIT: these can be inlined. Created attachment 371081 [details]
Patch
Comment on attachment 371081 [details] Patch Clearing flags on attachment: 371081 Committed r245991: <https://trac.webkit.org/changeset/245991> All reviewed patches have been landed. Closing bug. *** Bug 194607 has been marked as a duplicate of this bug. *** |