Bug 50227 – Web Inspector: Enable switching between revisions of stylesheets

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Alexander Pavlov (apavlov)]

We currently track revisions of stylesheets edited by the user. We should also be able to apply any revision as the "current" one, to revert the stylesheet state to some point in the past.

[Alexander Pavlov (apavlov)]

Created attachment 75131 [details]
[PATCH] Suggested solution

[Yury Semikhatsky]

Comment on attachment 75131 [details]
[PATCH] Suggested solution

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

> WebCore/inspector/front-end/CSSStyleModel.js:168
> +        function callback(styleSheetId, href, content)

Just use styleSheetId from the outer scope without passing it as additional explicit parameter, this way you would avoid callback.bind call below.

> WebCore/inspector/front-end/CSSStyleModel.js:221
> +    return payload ? new WebInspector.CSSStyleDeclaration(payload) : null;

The contract should be that the caller checks for null payload because otherwise we have two null checks instead of one, first one in the parse method and the second one at the call site where we check for null parsed value.

> WebCore/inspector/front-end/CSSStyleModel.js:369
> +    return payload ? new WebInspector.CSSRule(payload) : null;

ditto

> WebCore/inspector/front-end/CSSStyleModel.js:410
> +    if (!payload)

ditto

> WebCore/inspector/front-end/CSSStyleModel.js:530
> +    return payload ? new WebInspector.CSSStyleSheet(payload) : null;

ditto

[Alexander Pavlov (apavlov)]

Created attachment 75135 [details]
[PATCH] Comment addressed

[Yury Semikhatsky]

Comment on attachment 75135 [details]
[PATCH] Comment addressed

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

> WebCore/inspector/front-end/CSSStyleModel.js:-538
> -        function callback(styleSheetPayload)

Please revert this.

[Alexander Pavlov (apavlov)]

(In reply to comment #4)
> (From update of attachment 75135 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75135&action=review
> 
> > WebCore/inspector/front-end/CSSStyleModel.js:-538
> > -        function callback(styleSheetPayload)
> 
> Please revert this.

The code in question was wrong. setStyleSheetText2() does not return a stylesheet payload. Instead, it returns the stylesheet "href" and the new textual contents. Yet, I'm adding back the missing userCallback invocation.

[Alexander Pavlov (apavlov)]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 75135 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=75135&action=review
> > 
> > > WebCore/inspector/front-end/CSSStyleModel.js:-538
> > > -        function callback(styleSheetPayload)
> > 
> > Please revert this.
> 
> The code in question was wrong. setStyleSheetText2() does not return a stylesheet payload. Instead, it returns the stylesheet "href" and the new textual contents. Yet, I'm adding back the missing userCallback invocation.

Sorry for the confusion. setStyleSheetText2() does not return anything. It's getStyleSheetText2() that returns the href and stylesheet contents.

[Alexander Pavlov (apavlov)]

Created attachment 75140 [details]
[PATCH] Reverted userCallback invocation

[Yury Semikhatsky]

Comment on attachment 75140 [details]
[PATCH] Reverted userCallback invocation

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

> WebCore/inspector/front-end/CSSStyleModel.js:173
> +            if (userCallback)

Let's get rid of the unused callbacks. We can add them later once we need them.

> WebCore/inspector/front-end/CSSStyleModel.js:549
>      setText: function(newText, userCallback)

This method is not used, consider removing it completely.

[Alexander Pavlov (apavlov)]

Created attachment 75144 [details]
[PATCH] Removed unused method and user callback

[Alexander Pavlov (apavlov)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       WebCore/ChangeLog
        M       WebCore/English.lproj/localizedStrings.js
        M       WebCore/inspector/InspectorStyleSheet.cpp
        M       WebCore/inspector/front-end/CSSStyleModel.js
        M       WebCore/inspector/front-end/ResourcesPanel.js
Committed r72914

[Pavel Feldman]

Comment on attachment 75144 [details]
[PATCH] Removed unused method and user callback

I have a much better change that is using universal rollback callback and works both for styles and javascript. It does not introduce dependency from resource panel to style sheet model + does not introduce weird .styleSheetId property. Anyways, I did not land it since setStyleSeetText was not working. Is it fixed now?

[Pavel Feldman]

Created attachment 75147 [details]
[PATCH] My version of the patch.

[Alexander Pavlov (apavlov)]

Comment on attachment 75147 [details]
[PATCH] My version of the patch.

You need to add:
m_pageStyleSheet->styleSheetChanged();
after
m_pageStyleSheet->parseString(text, m_pageStyleSheet->useStrictParsing());
in
InspectorStyleSheet::reparseStyleSheet(const String& text).