Bug 58649 – Web Inspector: refactor resource setContent / revisions infrastructure to get rid of onRevert callback.

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

[Pavel Feldman]

Instead, we will have DomainModel/Resource binding responsible for changing underlying model upon resource changes and vice versa.

[Pavel Feldman]

Created attachment 89757 [details]
Patch

[Pavel Podivilov]

Comment on attachment 89757 [details]
Patch

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

> Source/WebCore/inspector/front-end/CSSStyleModel.js:633
> +    _innerSetContent: function(url, content, majorChange, userCallback, error)

Do we really need the "error" argument here? It looks confusing.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:150
> +                this._updateBreakpointsAfterLiveEdit(sourceFile.id, oldSource, newSource);

This should only be called when content is changed via "revert to this revision". When editing script in scripts panel breakpoints are moved accurately by editor, and we don't want to move them again here.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:155
> +                sourceFile.reload();

Why this changed? Errors during live edit should not lead to source frame reload.

[Pavel Feldman]

Created attachment 89768 [details]
[PATCH] Review comments addressed.

[Pavel Podivilov]

Comment on attachment 89768 [details]
[PATCH] Review comments addressed.

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

LGTM with nit.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:635
> +        if (error)

Return.

[Pavel Feldman]

Created attachment 89770 [details]
[PATCH] Review comments addressed.

[Alexander Pavlov (apavlov)]

Comment on attachment 89770 [details]
[PATCH] Review comments addressed.

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

> Source/WebCore/inspector/front-end/CSSStyleModel.js:179
> +            this.dispatchEventToListeners(WebInspector.CSSStyleModel.Events.StyleSheetChanged, {styleSheetId:styleSheetId, content:content, majorChange:majorChange});

A common object notation across Web Inspector is: { prop1: prop1Val, prop2: prop2Val }

> Source/WebCore/inspector/front-end/CSSStyleModel.js:643
> +                userCallback("No stylesheet found");

A good idea is to include the url into the message. We also can refine the message saying that the stylesheet is not accessible from DOM (since this stylesheet can be linked as "stylesheet alternate" and still be non-accessible).

[Yury Semikhatsky]

Comment on attachment 89770 [details]
[PATCH] Review comments addressed.

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

> LayoutTests/inspector/styles/get-set-stylesheet-text.html:45
> +                InspectorTest.addResult("Failed to set stylesheet text");

Can you add the error string to the result as well.

[Pavel Feldman]

Committed r83962: <http://trac.webkit.org/changeset/83962>