Bug 81758 - Web Inspector: Indenting fully selected line should not indent the line next to it
Summary: Web Inspector: Indenting fully selected line should not indent the line next ...
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: Nikita Vasilyev
URL: http://www.screenr.com/0oB8
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-21 04:10 PDT by Nikita Vasilyev
Modified: 2012-03-25 23:18 PDT (History)
12 users (show)

See Also:


Attachments
First try (2.48 KB, patch)
2012-03-21 04:22 PDT, Nikita Vasilyev
timothy: review-
Details | Formatted Diff | Diff
Add detailed comment to ChangeLog file (3.08 KB, patch)
2012-03-21 14:15 PDT, Nikita Vasilyev
pfeldman: review-
Details | Formatted Diff | Diff
Add a test (5.54 KB, patch)
2012-03-22 03:37 PDT, Nikita Vasilyev
pfeldman: review-
Details | Formatted Diff | Diff
Couple more tests (6.41 KB, patch)
2012-03-24 11:10 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Whoops, forgot to update ChangeLogs (6.31 KB, patch)
2012-03-24 11:16 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Coding style (6.30 KB, patch)
2012-03-24 12:07 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Coding style, again (6.30 KB, patch)
2012-03-24 12:10 PDT, Nikita Vasilyev
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (9.55 MB, application/zip)
2012-03-24 12:39 PDT, WebKit Review Bot
no flags Details
Fix the test (6.30 KB, patch)
2012-03-25 13:34 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2012-03-21 04:10:27 PDT
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.
Comment 1 Nikita Vasilyev 2012-03-21 04:22:28 PDT
Created attachment 133010 [details]
First try

After the patch applied: http://www.screenr.com/LoB8
Comment 2 Timothy Hatcher 2012-03-21 13:00:28 PDT
Comment on attachment 133010 [details]
First try

Please revise the ChangeLog and provide information on what changed and why (overall summary and per function).
Comment 3 Nikita Vasilyev 2012-03-21 14:15:49 PDT
Created attachment 133112 [details]
Add detailed comment to ChangeLog file
Comment 4 Pavel Feldman 2012-03-21 23:07:32 PDT
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)
Comment 5 Nikita Vasilyev 2012-03-22 03:37:47 PDT
Created attachment 133214 [details]
Add a test
Comment 6 Pavel Feldman 2012-03-24 02:06:45 PDT
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).
Comment 7 Nikita Vasilyev 2012-03-24 04:56:23 PDT
(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...
Comment 8 Pavel Feldman 2012-03-24 05:54:39 PDT
(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.
Comment 9 Nikita Vasilyev 2012-03-24 11:10:08 PDT
Created attachment 133642 [details]
Couple more tests
Comment 10 Nikita Vasilyev 2012-03-24 11:16:38 PDT
Created attachment 133644 [details]
Whoops, forgot to update ChangeLogs
Comment 11 Pavel Feldman 2012-03-24 11:48:23 PDT
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.
Comment 12 Nikita Vasilyev 2012-03-24 12:07:04 PDT
Created attachment 133646 [details]
Coding style

Didn’t know it applies to JS.
Comment 13 Nikita Vasilyev 2012-03-24 12:10:02 PDT
Created attachment 133647 [details]
Coding style, again
Comment 14 WebKit Review Bot 2012-03-24 12:39:51 PDT
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
Comment 15 WebKit Review Bot 2012-03-24 12:39:58 PDT
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
Comment 16 Nikita Vasilyev 2012-03-25 13:34:56 PDT
Created attachment 133690 [details]
Fix the test
Comment 17 Pavel Feldman 2012-03-25 22:48:24 PDT
Comment on attachment 133690 [details]
Fix the test

Thanks
Comment 18 WebKit Review Bot 2012-03-25 23:18:03 PDT
Comment on attachment 133690 [details]
Fix the test

Clearing flags on attachment: 133690

Committed r112053: <http://trac.webkit.org/changeset/112053>
Comment 19 WebKit Review Bot 2012-03-25 23:18:23 PDT
All reviewed patches have been landed.  Closing bug.