Bug 81758 – Web Inspector: Indenting fully selected line should not indent the line next to it

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

[Nikita Vasilyev]

Screencast: http://www.screenr.com/0oB8

I’ve never seen this behavior in the others code editors such as TextMate, IntelliJ IDEA, Espresso, CodeMirror and Ace.

[Nikita Vasilyev]

Created attachment 133010 [details]
First try

After the patch applied: http://www.screenr.com/LoB8

[Timothy Hatcher]

Comment on attachment 133010 [details]
First try

Please revise the ChangeLog and provide information on what changed and why (overall summary and per function).

[Nikita Vasilyev]

Created attachment 133112 [details]
Add detailed comment to ChangeLog file

[Pavel Feldman]

Comment on attachment 133112 [details]
Add detailed comment to ChangeLog file

This logic covers way more edge cases, it'll be really easy to regress it. Could you add a test for it (http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/LayoutTests/inspector/editor/&exact_package=chromium&q=file:LayoutTests/inspector%20TextEditor)

[Nikita Vasilyev]

Created attachment 133214 [details]
Add a test

[Pavel Feldman]

Comment on attachment 133214 [details]
Add a test

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

The change looks good. Could you please add more test scenarios and check for the post-indent / unindent selection as well?

> LayoutTests/inspector/editor/indenting.html:6
> +function dumpTextModel(model) {

Please remove this.

> LayoutTests/inspector/editor/indenting.html:14
> +        "*/";

You might want to indent this one char so that it looked like properly formatted comment and dump the initial state.

> LayoutTests/inspector/editor/indenting.html:21
> +    function dumpTextModel(msg) {

nit: { on the next line.

> LayoutTests/inspector/editor/indenting.html:39
> +This test checks code indentation.

Please provide more verbose description. I would call the test "indentation.html" as well.

> Source/WebCore/inspector/front-end/TextViewer.js:1109
> +        if (range.startColumn !== 0)

Placing a comment that we distinguish between full and partial line selection won't hurt. This could actually be tested as well. I.e. query selection after the indentation in both cases (full line select and partial line select).

[Nikita Vasilyev]

(In reply to comment #6)
> (From update of attachment 133214 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133214&action=review
> 
> The change looks good. Could you please add more test scenarios and check for the post-indent / unindent selection as well?

Okay. I’m in the process of figuring out how to get selection. window.getSelection() doesn’t return anything since an editor element is neither attached to the document nor visible...

[Pavel Feldman]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 133214 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=133214&action=review
> > 
> > The change looks good. Could you please add more test scenarios and check for the post-indent / unindent selection as well?
> 
> Okay. I’m in the process of figuring out how to get selection. window.getSelection() doesn’t return anything since an editor element is neither attached to the document nor visible...

You could just dump the range that is being returned as a start.

[Nikita Vasilyev]

Created attachment 133642 [details]
Couple more tests

[Nikita Vasilyev]

Created attachment 133644 [details]
Whoops, forgot to update ChangeLogs

[Pavel Feldman]

Comment on attachment 133644 [details]
Whoops, forgot to update ChangeLogs

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

> Source/WebCore/inspector/front-end/TextViewer.js:1111
> +        if (range.startColumn !== 0)

Looks great! One last thing: WebKit prohibits comparison to 0. Should be "if (range.startColumn)". See http://www.webkit.org/coding/coding-style.html#zero. Sorry for pointing it out so late in the review cycle.

[Nikita Vasilyev]

Created attachment 133646 [details]
Coding style

Didn’t know it applies to JS.

[Nikita Vasilyev]

Created attachment 133647 [details]
Coding style, again

[WebKit Review Bot]

Comment on attachment 133647 [details]
Coding style, again

Attachment 133647 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12127228

New failing tests:
inspector/editor/indentation.html

[WebKit Review Bot]

Created attachment 133648 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikita Vasilyev]

Created attachment 133690 [details]
Fix the test

[Pavel Feldman]

Comment on attachment 133690 [details]
Fix the test

Thanks

[WebKit Review Bot]

Comment on attachment 133690 [details]
Fix the test

Clearing flags on attachment: 133690

Committed r112053: <http://trac.webkit.org/changeset/112053>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.