Web Inspector: TextEditor should support editing a range.

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

[Pavel Podivilov]

Web Inspector: TextEditor should support editing a range.

This is needed for editing scripts inlined in html.

[Pavel Podivilov]

Created attachment 91468 [details]
Patch.

Add support for editing a range of lines rather then entire content in TextViewer.

[Andrey Adaikin]

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

> Source/WebCore/inspector/front-end/SourceFile.js:251
> +                endLine += 1;

++endLine;

> Source/WebCore/inspector/front-end/SourceFrame.js:885
> +        this._textViewer.setReadOnly(false, range);

Maybe use this._setReadOnly(true|false, range) to enforce the invariant that this._textViewer.setReadOnly() && WebInspector.markBeingEdited() should be called together.

> Source/WebCore/inspector/front-end/SourceFrame.js:893
> +            this._setReadOnly(true);

Otherwise, this method does not accept any arguments right now. Also check elsewhere.

> Source/WebCore/inspector/front-end/TextViewer.js:72
>          if (this._mainPanel.readOnly === readOnly)

Now this check will not work when after a call setReadOnly(false, null) you would call setReadOnly(false, new WebInspector.TextRange(...))

> Source/WebCore/inspector/front-end/TextViewer.js:246
> +        this._delegate.doubleClick(lineRow.lineNumber);

IMO we should rename doubleClick to smth like triggerEditing or whatever. Or even better I suggest moving the double-click logic outside of this class all together.

> Source/WebCore/inspector/front-end/TextViewer.js:461
>              var prefixChunk = this._createNewChunk(oldChunk.startLine, lineNumber);

Optional: maybe introduce a "readOnly" argument to the method this._createNewChunk()

> Source/WebCore/inspector/front-end/TextViewer.js:917
> +            this._splitChunkOnALine(startLine, firstChunkNumber);

Being strict, this is not exactly the right method to use here, as it splits a chunk into 3 chunks. You need splitting into two.

If you decide to go this way, at least, make the this._splitChunkOnALine() return the new chunk, so that you should not call:
    firstChunkNumber = this._chunkNumberForLine(startLine);

> Source/WebCore/inspector/front-end/TextViewer.js:1937
> +            this._updateElementReadOnlyState(this.element);

You should always execute this statement, no matter of the this._expandedLineRows variable.

> Source/WebCore/inspector/front-end/TextViewer.js:1975
> +            lineRow.addStyleClass("text-editor-read-only");

The lineRows from the cache may already have the className set, so you should also call lineRow.removeStyleClass() in the else{...} branch. Just call:
    this._updateElementReadOnlyState(lineRow);

[Pavel Podivilov]

Created attachment 91480 [details]
Patch.

[Pavel Podivilov]

(In reply to comment #2)
> View in context: https://bugs.webkit.org/attachment.cgi?id=91468&action=review
> 
> > Source/WebCore/inspector/front-end/SourceFile.js:251
> > +                endLine += 1;
> 
> ++endLine;
> 
> > Source/WebCore/inspector/front-end/SourceFrame.js:885
> > +        this._textViewer.setReadOnly(false, range);
> 
> Maybe use this._setReadOnly(true|false, range) to enforce the invariant that this._textViewer.setReadOnly() && WebInspector.markBeingEdited() should be called together.
> 
> > Source/WebCore/inspector/front-end/SourceFrame.js:893
> > +            this._setReadOnly(true);
> 
> Otherwise, this method does not accept any arguments right now. Also check elsewhere.

Done.

> 
> > Source/WebCore/inspector/front-end/TextViewer.js:72
> >          if (this._mainPanel.readOnly === readOnly)
> 
> Now this check will not work when after a call setReadOnly(false, null) you would call setReadOnly(false, new WebInspector.TextRange(...))

This was intentional. Editable range is not supposed to be changed on the fly. You should commit editing and start again.

> 
> > Source/WebCore/inspector/front-end/TextViewer.js:246
> > +        this._delegate.doubleClick(lineRow.lineNumber);
> 
> IMO we should rename doubleClick to smth like triggerEditing or whatever. Or even better I suggest moving the double-click logic outside of this class all together.

Renamed delegate's method to startEditing (existing startEditing renamed to beforeTextChanged).

> 
> > Source/WebCore/inspector/front-end/TextViewer.js:461
> >              var prefixChunk = this._createNewChunk(oldChunk.startLine, lineNumber);
> 
> Optional: maybe introduce a "readOnly" argument to the method this._createNewChunk()
> 
> > Source/WebCore/inspector/front-end/TextViewer.js:917
> > +            this._splitChunkOnALine(startLine, firstChunkNumber);
> 
> Being strict, this is not exactly the right method to use here, as it splits a chunk into 3 chunks. You need splitting into two.
> 
> If you decide to go this way, at least, make the this._splitChunkOnALine() return the new chunk, so that you should not call:
>     firstChunkNumber = this._chunkNumberForLine(startLine);

Done.

> 
> > Source/WebCore/inspector/front-end/TextViewer.js:1937
> > +            this._updateElementReadOnlyState(this.element);
> 
> You should always execute this statement, no matter of the this._expandedLineRows variable.
> 
> > Source/WebCore/inspector/front-end/TextViewer.js:1975
> > +            lineRow.addStyleClass("text-editor-read-only");
> 
> The lineRows from the cache may already have the className set, so you should also call lineRow.removeStyleClass() in the else{...} branch. Just call:
>     this._updateElementReadOnlyState(lineRow);

Done.

[Andrey Adaikin]

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

> Source/WebCore/inspector/front-end/SourceFrame.js:945
> +            this._delegate.setScriptSourceIsBeingEdited(false);

Don't you need to execute it for readOnly===false also?

> Source/WebCore/inspector/front-end/TextViewer.js:475
> +        if (createSuffixChunk && oldChunk.startLine + oldChunk.linesCount > lineNumber + 1) {

Replace (lineNumber + 1) with endLine here and below, and remove the createSuffixChunk flag check.

> Source/WebCore/inspector/front-end/TextViewer.js:919
> +            firstChunkNumber += 1;

++firstChunkNumber;

> Source/WebCore/inspector/front-end/TextViewer.js:928
> +                lastChunkNumber += 1;

Replace all "+= 1" with "++" to be consistent.

> Source/WebCore/inspector/front-end/TextViewer.js:1736
> +            newChunk.readOnly = false;

Can we drop this line?

[Pavel Podivilov]

Created attachment 91659 [details]
Patch.

[Andrey Adaikin]

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

> Source/WebCore/inspector/front-end/SourceFrame.js:943
> +            this._delegate.setScriptSourceIsBeingEdited(false);

Don't you need to execute it for readOnly===false also? (x2)

> Source/WebCore/inspector/front-end/TextViewer.js:477
> +            suffixChunk.readOnly = suffixChunk.readOnly;

suffixChunk.readOnly = oldChunk.readOnly;

> Source/WebCore/inspector/front-end/TextViewer.js:928
> +                lastChunkNumber += 1;

Replace all "+= 1" with "++" to be consistent. (x2)

[Pavel Podivilov]

Created attachment 91661 [details]
Patch.

[Pavel Podivilov]

(In reply to comment #7)
> View in context: https://bugs.webkit.org/attachment.cgi?id=91659&action=review
> 
> > Source/WebCore/inspector/front-end/SourceFrame.js:943
> > +            this._delegate.setScriptSourceIsBeingEdited(false);
> 
> Don't you need to execute it for readOnly===false also? (x2)

No, setScriptSourceIsBeingEdited is called only when the text is actually changed.

> 
> > Source/WebCore/inspector/front-end/TextViewer.js:477
> > +            suffixChunk.readOnly = suffixChunk.readOnly;
> 
> suffixChunk.readOnly = oldChunk.readOnly;

Done.

> 
> > Source/WebCore/inspector/front-end/TextViewer.js:928
> > +                lastChunkNumber += 1;
> 
> Replace all "+= 1" with "++" to be consistent. (x2)

I prefer to use "+= x" form for "increase variable by x" even when x == 1, because I find it more readable. "++" usually means increment a counter.

[Andrey Adaikin]

Looks good.

[Yury Semikhatsky]

Comment on attachment 91661 [details]
Patch.

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

> Source/WebCore/inspector/front-end/ResourceView.js:141
> +        if (!this._resource.isEditable())

This looks weird. This method should not be called for read-only resources.

> Source/WebCore/inspector/front-end/SourceFile.js:241
> +        if (!WebInspector.debugMode) {

Remove this?

> Source/WebCore/inspector/front-end/SourceFile.js:247
> +            script = this._scripts[i];

script -> var script, r- for this.

> Source/WebCore/inspector/front-end/SourceFrame.js:992
> +        // Should be implemented by subclasses.

Add return null; ?

> Source/WebCore/inspector/front-end/textViewer.css:47
> +    -webkit-user-modify: read-only;

This will turn background of almost all resources into grey when content editing is disabled in Preferences, not sure this is what we want to achieve.

[Pavel Podivilov]

Created attachment 92416 [details]
Patch.

This patch only touches internal TextViewer details.

[Pavel Podivilov]

> > Source/WebCore/inspector/front-end/textViewer.css:47
> > +    -webkit-user-modify: read-only;
> 
> This will turn background of almost all resources into grey when content editing is disabled in Preferences, not sure this is what we want to achieve.

Lines can only have "text-editor-read-only" style class when editing html (thus impossible in Safari). Read-only state of the viewer is still controlled by "text-editor-editable" style class of the lines container.

[Pavel Podivilov]

Created attachment 92417 [details]
Patch.

[Yury Semikhatsky]

Comment on attachment 92417 [details]
Patch.

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

> Source/WebCore/ChangeLog:5
> +        Web Inspector: TextEditor should support editing a range.

Is there a usage for this or a test for the functionality?

[Pavel Podivilov]

Committed r86446: <http://trac.webkit.org/changeset/86446>