Bug 91382 – Web Inspector: Comment/uncomment line(s) by pressing Cmd + /

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

[Nikita Vasilyev]

⌘/ (Ctrl/ on non-Mac) comments/uncomments lines in all code editors I’ve tested (Xcode, IntelliJ IDEA, Sublime Text 2, Textmate).

In Web Inspector it currently pause/continue script execution. I believe, it came from Firebug that doesn’t support code editing. I suggest to change this shorthand to something else. For example, Xcode uses Ctrl + Cmd + Y.

[Pavel Feldman]

Sounds good to me. Do you have a patch?

[Nikita Vasilyev]

Not yet. I’m going to start working on it.

[Mihai Balan]

Given that Nikita  gave no sign since July, I'll give it a try with this bug :)

[Nikita Vasilyev]

Go ahead, Mihai.

Few things to consider:

It should work with both the current editor and CodeMirror.

Sublime Text adds adds "//" at the current indent level:
if (1)
    // foo

Conversely, Xcode and IntelliJ IDEA adds "//" at the beginning of the line, e.g.:

if (1)
//    foo

I’m fine with either of them as long as it doesn’t comment a line twice:

if (1)
//    // foo

Xcode suffers from it.

[Pavel Feldman]

> It should work with both the current editor and CodeMirror.

Correction, CodeMirror is a non-goal for Web Inspector (at least not in its current state). I'll remove the experiment.

[Nikita Vasilyev]

(In reply to comment #5)
> > It should work with both the current editor and CodeMirror.
> 
> Correction, CodeMirror is a non-goal for Web Inspector (at least not in its current state). I'll remove the experiment.

Why so? Any details somewhere?

[Pavel Feldman]

v2 had gutter problems we could not afford (https://github.com/marijnh/CodeMirror/issues/730). When v3 is ready, we will perform another assessment.

[Mihai Balan]

Created attachment 165135 [details]
Proposed bug fix. Replaced Cmd + / from Pause script to Toggle code comment.

[Nikita Vasilyev]

Selection moves 2 characters on the left after commenting out a line. Screencast: http://www.screenr.com/O338.
Also, on 17 second I select one whole line but it comments out the next one too.

[Andrey Adaikin]

Comment on attachment 165135 [details]
Proposed bug fix. Replaced Cmd + / from Pause script to Toggle code comment.

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1594
> +            if (line.match(commentRegEx)) {

this will not work for multiple lines with comments:

var a = 1;
// a comment
var b = 2;

will become:

// var a = 1;
a comment
// var b = 2;

while should be:

// var a = 1;
// // a comment
// var b = 2;

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1606
> +    },

return true;

[Andrey Adaikin]

Also how about adding "// " (with a space at the end) instead of "//"?

[Mihai Balan]

(In reply to comment #11)
> Also how about adding "// " (with a space at the end) instead of "//"?

Sounds doable :) What should happen however when removing the comments from the beginning of the line? Should it remove only "//"? Only "// "? Both?
One of the reasons I opted for "//" is that it preserves the most of the original code when adding/removing (even though there are cases when after a comment/uncomment the source will not be same anymore).

[Mihai Balan]

Comment on attachment 165135 [details]
Proposed bug fix. Replaced Cmd + / from Pause script to Toggle code comment.

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

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1594
>> +            if (line.match(commentRegEx)) {
> 
> this will not work for multiple lines with comments:
> 
> var a = 1;
> // a comment
> var b = 2;
> 
> will become:
> 
> // var a = 1;
> a comment
> // var b = 2;
> 
> while should be:
> 
> // var a = 1;
> // // a comment
> // var b = 2;

I have already discussed this aspect on IRC with apavolov and a new patch is in works, complete with unit tests and a some refactoring of the code :)

[Mihai Balan]

Created attachment 166026 [details]
Proposed bug fix. Replaced Cmd + / from Pause script to Toggle code comment. Missing expected results for tests.

Doesn't have expected results for the test. What's the recommended way of producing the expected results?

[WebKit Review Bot]

Attachment 166026 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1
LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mihai Balan]

Created attachment 166027 [details]
Proposed bug fix. Missing expected results for tests.

[Mihai Balan]

Created attachment 166030 [details]
Proposed bug fix. Missing expected results for tests.

Minor style fixes.

[Build Bot]

Comment on attachment 166030 [details]
Proposed bug fix. Missing expected results for tests.

Attachment 166030 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14059004

New failing tests:
inspector/editor/comment-toggling.html

[WebKit Review Bot]

Comment on attachment 166030 [details]
Proposed bug fix. Missing expected results for tests.

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

New failing tests:
inspector/editor/comment-toggling.html

[Alexander Pavlov (apavlov)]

(In reply to comment #14)
> Created an attachment (id=166026) [details]
> Proposed bug fix. Replaced Cmd + / from Pause script to Toggle code comment. Missing expected results for tests.
> 
> Doesn't have expected results for the test. What's the recommended way of producing the expected results?

When you run a test with missing expectations, DRT will write down the "*-expected.txt" file for you, so you just need to scrutinize it and make sure the results are correct. And don't forget to add it to your patch :)

[Andrey Adaikin]

Comment on attachment 166030 [details]
Proposed bug fix. Missing expected results for tests.

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1581
> +    _commentLines: function(range, commentRegEx)

So, we already have _indentLines and _unindentLines methods, which do almost the same work (just a regexp is different). I'd guess there must be much of the code that can be reused.

Also, I'd move out these fancy IDE-like code transformations from the basic editor functionality into a separate class, and ideally make it work with any TextEditor implementation.

Also, we should look at how Xcode comments the lines. For example this test case:

//line1
// line2
 //line3

  //line4
   //            line5

I don't have an Xcode by my hand, but Sublime for example, would uncomment all lines first, and then comment in the following way:

// line1
// line2
//  line3

//   line4
//               line5

(Note, that empty lines are ignored, unlike your implementation).

Also, we'll probably want Ctrl+Shift+/ to add block comments /* ... */

[Pavel Feldman]

Comment on attachment 166030 [details]
Proposed bug fix. Missing expected results for tests.

r- per Andrey's comments.

[Mihai Balan]

Comment on attachment 166030 [details]
Proposed bug fix. Missing expected results for tests.

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

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1581
>> +    _commentLines: function(range, commentRegEx)
> 
> So, we already have _indentLines and _unindentLines methods, which do almost the same work (just a regexp is different). I'd guess there must be much of the code that can be reused.
> 
> Also, I'd move out these fancy IDE-like code transformations from the basic editor functionality into a separate class, and ideally make it work with any TextEditor implementation.
> 
> Also, we should look at how Xcode comments the lines. For example this test case:
> 
> //line1
> // line2
>  //line3
> 
>   //line4
>    //            line5
> 
> I don't have an Xcode by my hand, but Sublime for example, would uncomment all lines first, and then comment in the following way:
> 
> // line1
> // line2
> //  line3
> 
> //   line4
> //               line5
> 
> (Note, that empty lines are ignored, unlike your implementation).
> 
> Also, we'll probably want Ctrl+Shift+/ to add block comments /* ... */

First of all, is really block commenting in the scope for this bug? Because I think it needs even more specifying than line comments (e.g. should the editor look for block comments outside the selection? Should it look for comments inside the selection but smaller than it? What should happen if the beginning of the selection is on a line that is commented with //? etc.)

Finally, what are the actual r- items (besides the missing test expectations) and what are nice-to-have-s (possibly extractable in a different patch)?

[Andrey Adaikin]

(In reply to comment #23)
> (From update of attachment 166030 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166030&action=review
> 
> >> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1581
> >> +    _commentLines: function(range, commentRegEx)
> > 
> > So, we already have _indentLines and _unindentLines methods, which do almost the same work (just a regexp is different). I'd guess there must be much of the code that can be reused.
> > 
> > Also, I'd move out these fancy IDE-like code transformations from the basic editor functionality into a separate class, and ideally make it work with any TextEditor implementation.
> > 
> > Also, we should look at how Xcode comments the lines. For example this test case:
> > 
> > //line1
> > // line2
> >  //line3
> > 
> >   //line4
> >    //            line5
> > 
> > I don't have an Xcode by my hand, but Sublime for example, would uncomment all lines first, and then comment in the following way:
> > 
> > // line1
> > // line2
> > //  line3
> > 
> > //   line4
> > //               line5
> > 
> > (Note, that empty lines are ignored, unlike your implementation).
> > 
> > Also, we'll probably want Ctrl+Shift+/ to add block comments /* ... */
> 
> First of all, is really block commenting in the scope for this bug? Because I think it needs even more specifying than line comments (e.g. should the editor look for block comments outside the selection? Should it look for comments inside the selection but smaller than it? What should happen if the beginning of the selection is on a line that is commented with //? etc.)
> 
> Finally, what are the actual r- items (besides the missing test expectations) and what are nice-to-have-s (possibly extractable in a different patch)?

As far as I'm concerned, r- is for code duplication.

[Mihai Balan]

> 
> As far as I'm concerned, r- is for code duplication.

OK, I'll fix that :)

[Jan Keromnes]

Sorry for the somewhat unrelated and belated comment.

(In reply to comment #5)
> Correction, CodeMirror is a non-goal for Web Inspector (at least not in its current state). I'll remove the experiment.

(In reply to comment #7)
> v2 had gutter problems we could not afford (https://github.com/marijnh/CodeMirror/issues/730). When v3 is ready, we will perform another assessment.

Pavel please don't delete the experiment just yet, I have an upcoming patch to update to v3. It should be stable soon, will fix the #94118 gutter problem, and should help implementing #94056 and #94062 a great deal.

Also, implementing `Cmd + /` in the experiment shouldn't be a problem.

[Brian Burg]

Closing as invalid, as this bug pertains to the old inspector UI and/or its tests.
Please file a new bug (https://www.webkit.org/new-inspector-bug) if the bug/feature/issue is still relevant to WebKit trunk.