Bug 59688 - Web Inspector: TextEditor should support editing a range.
Summary: Web Inspector: TextEditor should support editing a range.
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: Pavel Podivilov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-28 04:12 PDT by Pavel Podivilov
Modified: 2011-05-13 11:00 PDT (History)
12 users (show)

See Also:


Attachments
Patch. (19.19 KB, patch)
2011-04-28 04:14 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Patch. (22.34 KB, patch)
2011-04-28 06:30 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Patch. (21.94 KB, patch)
2011-04-29 03:21 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Patch. (21.93 KB, patch)
2011-04-29 03:35 PDT, Pavel Podivilov
yurys: review-
Details | Formatted Diff | Diff
Patch. (6.39 KB, patch)
2011-05-05 08:16 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Patch. (8.26 KB, patch)
2011-05-05 08:23 PDT, Pavel Podivilov
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 Podivilov 2011-04-28 04:12:09 PDT
Web Inspector: TextEditor should support editing a range.

This is needed for editing scripts inlined in html.
Comment 1 Pavel Podivilov 2011-04-28 04:14:47 PDT
Created attachment 91468 [details]
Patch.

Add support for editing a range of lines rather then entire content in TextViewer.
Comment 2 Andrey Adaikin 2011-04-28 04:59:54 PDT
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);
Comment 3 Pavel Podivilov 2011-04-28 06:30:02 PDT
Created attachment 91480 [details]
Patch.
Comment 4 Pavel Podivilov 2011-04-28 06:34:37 PDT
(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.
Comment 5 Andrey Adaikin 2011-04-29 00:16:19 PDT
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?
Comment 6 Pavel Podivilov 2011-04-29 03:21:07 PDT
Created attachment 91659 [details]
Patch.
Comment 7 Andrey Adaikin 2011-04-29 03:28:06 PDT
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)
Comment 8 Pavel Podivilov 2011-04-29 03:35:46 PDT
Created attachment 91661 [details]
Patch.
Comment 9 Pavel Podivilov 2011-04-29 03:36:18 PDT
(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.
Comment 10 Andrey Adaikin 2011-04-29 04:44:15 PDT
Looks good.
Comment 11 Yury Semikhatsky 2011-05-05 06:22:38 PDT
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.
Comment 12 Pavel Podivilov 2011-05-05 08:16:38 PDT
Created attachment 92416 [details]
Patch.

This patch only touches internal TextViewer details.
Comment 13 Pavel Podivilov 2011-05-05 08:18:41 PDT
> > 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.
Comment 14 Pavel Podivilov 2011-05-05 08:23:12 PDT
Created attachment 92417 [details]
Patch.
Comment 15 Yury Semikhatsky 2011-05-13 04:59:44 PDT
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?
Comment 16 Pavel Podivilov 2011-05-13 11:00:50 PDT
Committed r86446: <http://trac.webkit.org/changeset/86446>