Bug 58649

Summary: Web Inspector: refactor resource setContent / revisions infrastructure to get rid of onRevert callback.
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
[PATCH] Review comments addressed.
none
[PATCH] Review comments addressed. yurys: review+

Description Pavel Feldman 2011-04-15 02:04:29 PDT
Instead, we will have DomainModel/Resource binding responsible for changing underlying model upon resource changes and vice versa.
Comment 1 Pavel Feldman 2011-04-15 02:11:31 PDT
Created attachment 89757 [details]
Patch
Comment 2 Pavel Podivilov 2011-04-15 03:39:40 PDT
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.
Comment 3 Pavel Feldman 2011-04-15 04:20:28 PDT
Created attachment 89768 [details]
[PATCH] Review comments addressed.
Comment 4 Pavel Podivilov 2011-04-15 05:02:29 PDT
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.
Comment 5 Pavel Feldman 2011-04-15 05:04:23 PDT
Created attachment 89770 [details]
[PATCH] Review comments addressed.
Comment 6 Alexander Pavlov (apavlov) 2011-04-15 05:24:41 PDT
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).
Comment 7 Yury Semikhatsky 2011-04-15 05:31:51 PDT
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.
Comment 8 Pavel Feldman 2011-04-15 05:39:54 PDT
Committed r83962: <http://trac.webkit.org/changeset/83962>