Bug 58649 - Web Inspector: refactor resource setContent / revisions infrastructure to get rid of onRevert callback.
Summary: Web Inspector: refactor resource setContent / revisions infrastructure to get...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-15 02:04 PDT by Pavel Feldman
Modified: 2011-04-15 05:39 PDT (History)
10 users (show)

See Also:


Attachments
Patch (40.77 KB, patch)
2011-04-15 02:11 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
[PATCH] Review comments addressed. (41.23 KB, patch)
2011-04-15 04:20 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
[PATCH] Review comments addressed. (41.26 KB, patch)
2011-04-15 05:04 PDT, Pavel Feldman
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>