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.
Created attachment 133010 [details] First try After the patch applied: http://www.screenr.com/LoB8
Comment on attachment 133010 [details] First try Please revise the ChangeLog and provide information on what changed and why (overall summary and per function).
Created attachment 133112 [details] Add detailed comment to ChangeLog file
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)
Created attachment 133214 [details] Add a test
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).
(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...
(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.
Created attachment 133642 [details] Couple more tests
Created attachment 133644 [details] Whoops, forgot to update ChangeLogs
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.
Created attachment 133646 [details] Coding style Didn’t know it applies to JS.
Created attachment 133647 [details] Coding style, again
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
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
Created attachment 133690 [details] Fix the test
Comment on attachment 133690 [details] Fix the test Thanks
Comment on attachment 133690 [details] Fix the test Clearing flags on attachment: 133690 Committed r112053: <http://trac.webkit.org/changeset/112053>
All reviewed patches have been landed. Closing bug.