Bug 193859 - Web Inspector: Changes: style attribute changes aren't being tracked
Summary: Web Inspector: Changes: style attribute changes aren't being tracked
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on: 193803
Blocks:
  Show dependency treegraph
 
Reported: 2019-01-25 18:08 PST by Nikita Vasilyev
Modified: 2019-03-16 01:31 PDT (History)
6 users (show)

See Also:


Attachments
Patch (12.58 KB, patch)
2019-03-15 17:11 PDT, Nikita Vasilyev
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
[Image] With patch applied (64.64 KB, image/png)
2019-03-15 17:13 PDT, Nikita Vasilyev
no flags Details
Archive of layout-test-results from ews101 for mac-highsierra (3.19 MB, application/zip)
2019-03-15 18:02 PDT, EWS Watchlist
no flags Details
Patch (12.98 KB, patch)
2019-03-15 18:13 PDT, Nikita Vasilyev
drousso: review+
drousso: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (13.05 KB, patch)
2019-03-16 00:52 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2019-01-25 18:08:55 PST
The experimental Changes panel doesn't show any CSS edits in style="...".
Comment 1 Radar WebKit Bug Importer 2019-01-25 18:09:27 PST
<rdar://problem/47568977>
Comment 2 Nikita Vasilyev 2019-03-15 17:11:22 PDT Comment hidden (obsolete)
Comment 3 Nikita Vasilyev 2019-03-15 17:13:20 PDT
Created attachment 364887 [details]
[Image] With patch applied
Comment 4 EWS Watchlist 2019-03-15 18:02:55 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-03-15 18:02:56 PDT Comment hidden (obsolete)
Comment 6 Nikita Vasilyev 2019-03-15 18:13:26 PDT
Created attachment 364894 [details]
Patch
Comment 7 Devin Rousso 2019-03-15 23:31:19 PDT
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()`.
Comment 8 Nikita Vasilyev 2019-03-16 00:48:10 PDT
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.
Comment 9 Nikita Vasilyev 2019-03-16 00:52:56 PDT
Created attachment 364922 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2019-03-16 01:31:21 PDT
Comment on attachment 364922 [details]
Patch for landing

Clearing flags on attachment: 364922

Committed r243038: <https://trac.webkit.org/changeset/243038>
Comment 11 WebKit Commit Bot 2019-03-16 01:31:23 PDT
All reviewed patches have been landed.  Closing bug.