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
Pavel Feldman
2011-04-15 02:04:29 PDT
Created attachment 89757 [details]
Patch
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. Created attachment 89768 [details]
[PATCH] Review comments addressed.
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. Created attachment 89770 [details]
[PATCH] Review comments addressed.
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 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. Committed r83962: <http://trac.webkit.org/changeset/83962> |