Web Inspector: Changes: style attribute changes aren't being tracked

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

The experimental Changes panel doesn't show any CSS edits in style="...".

[Radar WebKit Bug Importer]

<rdar://problem/47568977>

[Nikita Vasilyev]

Created attachment 364884 [details]
Patch

[Nikita Vasilyev]

Created attachment 364887 [details]
[Image] With patch applied

[EWS Watchlist]

Comment on attachment 364884 [details]
Patch

Attachment 364884 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11525121

New failing tests:
inspector/debugger/tail-deleted-frames.html
inspector/debugger/probe-manager-add-remove-actions.html
inspector/debugger/tail-recursion.html
inspector/debugger/reload-paused.html
inspector/debugger/breakpoint-columns.html
http/tests/inspector/network/set-resource-caching-disabled-memory-cache.html
inspector/debugger/tail-deleted-frames-this-value.html
http/tests/inspector/network/resource-response-source-memory-cache.html
inspector/page/overrideUserAgent.html
inspector/debugger/tail-deleted-frames-from-vm-entry.html
inspector/debugger/search-scripts.html
inspector/debugger/breakpoint-scope.html
http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html
inspector/console/messagesCleared.html
http/tests/inspector/network/resource-response-source-memory-cache-revalidate-expired-only.html
inspector/debugger/debugger-stack-overflow.html

[EWS Watchlist]

Created attachment 364893 [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

[Nikita Vasilyev]

Created attachment 364894 [details]
Patch

[Devin Rousso]

Comment on attachment 364894 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:48
> +        this._modifiedStyleDeclarations = new Map;

I'd drop the `Declaration`, as we almost always refer to them as a `style`, not a `styleDeclaration`.

    this._modifiedStyles = new Map;

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:424
> +    get modifiedStyleDeclarations()

Ditto (>48).

    get modifiedStyles()

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:426
> +        return Array.from(this._modifiedStyleDeclarations.values());

Ditto (>48).

    return Array.from(this._modifiedStyles.values());

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:429
> +    addModifiedStyleDeclaration(declaration)

Ditto (>48).

    addModifiedStyle(style)

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:431
> +        this._modifiedStyleDeclarations.set(declaration.stringId, declaration);

Ditto (>48).

   this._modifiedStyles.set(style.stringId, style);

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:532
> +        this._modifiedStyleDeclarations.clear();

Ditto (>48).

    this._modifiedStyles.clear();

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:68
> +        if (this._id)
> +            return this._id.styleSheetId + "/" + this._id.ordinal;

We should always have a return for any function that can ever has a return value, so either make the `this._id` check an assert, or add a default `return`.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:383
> +        WI.cssManager.addModifiedStyleDeclaration(this);

Ditto (>CSSManager.js48).

   WI.cssManager.addModifiedStyle(this);

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:84
> +        let modifiedStyles = WI.cssManager.modifiedStyleDeclarations;

Ditto (>CSSManager.js48).

   let modifiedStyles = WI.cssManager.modifiedStyles;

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:94
> +                        return stylesForNode.matchedRules.some((matchedRule) => style.ownerRule.isEqualTo(matchedRule))

We should always have a return for any function that can ever has a return value, so please add a base case `return false;`.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:106
> +        let declarationsForStylesheet = new Map();

Style: unnecessary parentheses.
NIT: the "s" of "sheet" should be capitalized, like `declarationsForStyleSheet`.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:113
> +            let styleDeclarations = declarationsForStylesheet.get(style.ownerStyleSheet);
> +            if (!styleDeclarations) {
> +                styleDeclarations = [];
> +                declarationsForStylesheet.set(style.ownerStyleSheet, styleDeclarations);
>              }
> -            cssRules.push(cssRule);
> +            styleDeclarations.push(style);

You could use a `MultiMap` to do this work for you.  If you do, you'd need to introduce a new iteration method, mabye something called `MultiMap.prototype.sets`:

    sets()
    {
        return this._map[Symbol.iterator]();
    }

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:116
> +        for (let [styleSheet, styles] of declarationsForStylesheet) {

which you could then call here, like `declarationsForStylesheet.sets()`.  The current `MultiMap.prototype.[Symbol.iterator]()` iterates as `[key, value]`, not `[key, valuesForKey]` as you'd like here.

Alternatively, there are no callers of the current `MutliMap.prototype.[Symbol.iterator]()` so you could change it if you feel that it would make more sense as `[key, valuesForKey]`.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:123
> +            let resourceHeaderContent;

Style: please initialize this to some value (e.g. `null`).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:124
> +            if (styleSheet.isInlineStyleAttributeStyleSheet() && styles[0])

It should not be possible for `styles[0]` to be falsy.  You can get rid of that check.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:129
> +            resourceHeader.append(resourceHeaderContent);

I'd inline this, as it's simple/small/short enough that the code de-duplication isn't that beneficial (if at all) considering the "length" of `resourceHeaderContent` (I'm specifically referring to the variable name).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:132
> +            for (let style of styles)
> +                resourceSection.append(this._createRuleElement(style));

This is more of a stylistic preference/opinion, but I think we should add some sort of border between each rule element, as otherwise they look quite cramped.  I'm not sure how to best do that, but I think something should be done.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:145
> +        selectorLineElement.className = "selector-line";
> +        let selectorElement = selectorLineElement.appendChild(document.createElement("span"));

Style: missing newline.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:190
> +        if (styleSheet.isInlineStyleAttributeStyleSheet())
> +            return WI.UIString("Style Attribute");

This is unnecessary, as you only call this in an else of `styleSheet.isInlineStyleAttributeStyleSheet()`.

[Nikita Vasilyev]

Comment on attachment 364894 [details]
Patch

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

Thanks for reviewing!

>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:116
>> +        for (let [styleSheet, styles] of declarationsForStylesheet) {
> 
> which you could then call here, like `declarationsForStylesheet.sets()`.  The current `MultiMap.prototype.[Symbol.iterator]()` iterates as `[key, value]`, not `[key, valuesForKey]` as you'd like here.
> 
> Alternatively, there are no callers of the current `MutliMap.prototype.[Symbol.iterator]()` so you could change it if you feel that it would make more sense as `[key, valuesForKey]`.

I think I'll just keep what I have now. Another layer of abstraction seems to add little benefit here.

>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:132
>> +                resourceSection.append(this._createRuleElement(style));
> 
> This is more of a stylistic preference/opinion, but I think we should add some sort of border between each rule element, as otherwise they look quite cramped.  I'm not sure how to best do that, but I think something should be done.

Yeah, I agree. I'll use the same 0.5px border we use in the Styles panel.

[Nikita Vasilyev]

Created attachment 364922 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 364922 [details]
Patch for landing

Clearing flags on attachment: 364922

Committed r243038: <https://trac.webkit.org/changeset/243038>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.