Bug 81787 – Web Inspector: make CSS and JavaScript files editable by default.

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

[Pavel Feldman]

This change is finalizing the migration to the experiment, fixing many things along the way.

[Pavel Feldman]

Created attachment 133049 [details]
Patch

[Pavel Feldman]

Created attachment 133055 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 133055 [details]
Patch

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

> Source/WebCore/inspector/front-end/JavaScriptSourceFrame.js:72
> +        function mycallback(mimeType, content)

myCallback?

> Source/WebCore/inspector/front-end/JavaScriptSourceFrame.js:90
>      editContent: function(newContent, callback)

consider renaming to saveContent.

> Source/WebCore/inspector/front-end/JavaScriptSourceFrame.js:92
> +        this._originalContent = newContent;

Please move to didEditContent

> Source/WebCore/inspector/front-end/SourceFrame.js:97
>              this._wasHiddenWhileEditing = true;

Please remove these 2 lines.

> Source/WebCore/inspector/front-end/SourceFrame.js:530
>      doubleClick: function(lineNumber)

Consider removing this method.

> Source/WebCore/inspector/front-end/TextViewer.js:1016
> +            this.revealLine(lineNumber);

revealLine() should be called in both modes.

[Timothy Hatcher]

Please stop disguising random fixes under a bug that implies a simple switch is being flipped. Ideally these fixes would have been done in separate, logically grouped changes before the switch was flipped.

[Pavel Feldman]

(In reply to comment #4)
> Please stop disguising random fixes under a bug that implies a simple switch is being flipped. Ideally these fixes would have been done in separate, logically grouped changes before the switch was flipped.

I am sorry, I don't get it. Could you be more specific?

[Timothy Hatcher]

The bug title implies this should be a simple 1-10 line change. Instead the change is a collection of misc fixes (not described in the ChangeLog either.)

These fixes should be broken up into smaller patches, with descriptions of what was fixed in the ChangeLog for each function. Then a final change to move the feature out of experiment status.

No comments in the ChangeLog is the main issue here. Breaking things up into smaller changes would be nice too.

[Pavel Feldman]

(In reply to comment #6)
> The bug title implies this should be a simple 1-10 line change. Instead the change is a collection of misc fixes (not described in the ChangeLog either.)
> 
> These fixes should be broken up into smaller patches, with descriptions of what was fixed in the ChangeLog for each function. Then a final change to move the feature out of experiment status.
> 

This change is not 'flipping the switch', all these changes are required for editing to be enabled by default. Changes under the experiment were only used to assess the user experience. I had no intention to complete it as an experiment and then flip the switch - too much work.

> No comments in the ChangeLog is the main issue here. Breaking things up into smaller changes would be nice too.

This change removes cancelEditing and setReadOnly. It establishes proper content dispatching. All three are inter-dependent, need to be landed simultaneously. Popover change removes the readOnly guard, also necessary. I'll update the log.

Two drive-by fixes I can think of are: hiding popover upon Esc and a fix with the no-op change in the text model. I'll extract them into separate patches.

[Timothy Hatcher]

(In reply to comment #7)
> (In reply to comment #6)
> > The bug title implies this should be a simple 1-10 line change. Instead the change is a collection of misc fixes (not described in the ChangeLog either.)
> > 
> > These fixes should be broken up into smaller patches, with descriptions of what was fixed in the ChangeLog for each function. Then a final change to move the feature out of experiment status.
> > 
> 
> This change is not 'flipping the switch', all these changes are required for editing to be enabled by default. Changes under the experiment were only used to assess the user experience. I had no intention to complete it as an experiment and then flip the switch - too much work.

Okay, the bug title (and lack of details) made it sound like a switch flip.

> > No comments in the ChangeLog is the main issue here. Breaking things up into smaller changes would be nice too.
> 
> This change removes cancelEditing and setReadOnly. It establishes proper content dispatching. All three are inter-dependent, need to be landed simultaneously. Popover change removes the readOnly guard, also necessary. I'll update the log.
> 
> Two drive-by fixes I can think of are: hiding popover upon Esc and a fix with the no-op change in the text model. I'll extract them into separate patches.

And I wouldn't know this without asking you or trying to infer this from the code. That is great info to add to the bug and the ChangeLog. Please revise this patch and add that info (ideally per changed function).

I filed bug 81828 about making the style bot complain about sparse ChangeLogs.

[Timothy Hatcher]

Comment on attachment 133055 [details]
Patch

Please revise the ChangeLog.

[Pavel Feldman]

(In reply to comment #9)
> (From update of attachment 133055 [details])
> Please revise the ChangeLog.

Now that you r- ed it, I hope you will be available to review revised ChangeLog tomorrow at 4am PST :)

[Timothy Hatcher]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 133055 [details] [details])
> > Please revise the ChangeLog.
> 
> Now that you r- ed it, I hope you will be available to review revised ChangeLog tomorrow at 4am PST :)

It is the responsibility of all reviewers to make sure the ChangeLog has decent info. If not, maybe they shouldn't be reviewers.

http://www.webkit.org/coding/contributing.html#changelogs

[Pavel Feldman]

Created attachment 133241 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 133241 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        I removes dblclick handler as well since one does not need to enter editing mode.

I -> It

> Source/WebCore/inspector/front-end/TextViewer.js:1173
> +        this._restoreSelection(newRange.collapseToEnd(), true);

Please mention this change in ChangeLog

[Pavel Feldman]

Committed r111682: <http://trac.webkit.org/changeset/111682>