[webkit-reviews] review denied: [Bug 195264] Web Inspector: CSS Changes: modifications aren't shared for rules that match multiple elements : [Attachment 366683] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 22 10:37:45 PDT 2019


Devin Rousso <drousso at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 195264: Web Inspector: CSS Changes: modifications aren't shared for rules
that match multiple elements
https://bugs.webkit.org/show_bug.cgi?id=195264

Attachment 366683: Patch

https://bugs.webkit.org/attachment.cgi?id=366683&action=review




--- Comment #18 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 366683
  --> https://bugs.webkit.org/attachment.cgi?id=366683
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)?


More information about the webkit-reviews mailing list