Bug 31248 - Tab width for javascript source is 8, should be 4
: Tab width for javascript source is 8, should be 4
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-11-08 19:11 PST by
Modified: 2010-02-05 04:47 PST (History)


Attachments
Patch to convert tabs to 4 spaces (659 bytes, patch)
2010-02-04 15:26 PST, aparajita@aparajita.com
pfeldman: review-
Review Patch | Details | Formatted Diff | Diff
Proposed patch (1.24 KB, patch)
2010-02-04 18:03 PST, aparajita@aparajita.com
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (1.24 KB, patch)
2010-02-04 18:04 PST, aparajita@aparajita.com
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] Proposed change. (3.10 KB, patch)
2010-02-05 00:01 PST, Pavel Feldman
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-11-08 19:11:52 PST
Currently the tab width for the javascript source display is 8 spaces. These days most people use 4 spaces in their editors, so perhaps it should be 4 in the source display.
------- Comment #1 From 2009-12-04 09:37:37 PST -------
I agree
------- Comment #2 From 2009-12-04 10:41:54 PST -------
I briefly looked into this at one point.  

I didn't see that we supported any kind of tab expansion setting in WebKit's CSS.  

There were a couple of proposals to in CSS spec-land to set tab expansion values, but they all seemed kind of dead-ish when I remember looking at them.  

I suppose we could expand the tabs ourselves.  No fun, but not very hard either.

It would be nice to do something, as tabs == 8 spaces is so 1970's ...
------- Comment #3 From 2010-02-03 08:17:48 PST -------
I should be noted that FireBug uses 4 spaces for tabs.
------- Comment #4 From 2010-02-04 15:26:39 PST -------
Created an attachment (id=48176) [details]
Patch to convert tabs to 4 spaces
------- Comment #5 From 2010-02-04 15:27:27 PST -------
It was an easy fix, patch is attached. Hope this makes it into the source.
------- Comment #6 From 2010-02-04 15:57:09 PST -------
You should make a ChangeLog entry so we can land this. Looks good though!

http://webkit.org/coding/contributing.html
------- Comment #7 From 2010-02-04 16:05:27 PST -------
(From update of attachment 48176 [details])
Text editor does not support tabs yet. I'd like to keep it (and its model) operating real characters with no tab substitutions for now.
There are good reasons for your change though, so if you are to make this replace, please do it in the SourceFrame.js (setContent method).

Please also follow the patch submit guidelines (your change is missing ChangeLog entry).
------- Comment #8 From 2010-02-04 18:03:29 PST -------
Created an attachment (id=48187) [details]
Proposed patch
------- Comment #9 From 2010-02-04 18:04:30 PST -------
Created an attachment (id=48188) [details]
Proposed patch
------- Comment #10 From 2010-02-04 18:06:08 PST -------
Okay, here is a complete patch with ChangeLog, and the mod was moved to SourceFrame#setContent
------- Comment #11 From 2010-02-04 18:08:42 PST -------
Attachment 48188 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #12 From 2010-02-04 22:19:57 PST -------
The patch is doing a blind replacement of \t with 4 space characters; but that's not how tabs actually work; they add a variable # of spaces to get the following characters to align on the tab-width boundaries.

For example, if the original text looks is this, where - is the tab character

----------------
-a-b-c
 -d -e -f
----------------

then the text will be displayed like this:

----------------
    a   b   c
    d   e   f
----------------

The following original text, again, with - as the tab character, yields the same display as above; note there is a space before each tab character:

----------------
-a-b-c
 -d -e -f
----------------

However, if we use the "\t == 4 spaces" replacement, the expanded text becomes this:

----------------
    a    b    c
     d     e     f
----------------

In practice, if people consistently use tabs just at the beginnings of lines, and never accidently inject spaces, then the expansion will work fine.  In my practice, when I used to use tabs in source files, I found that I frequently accidently mixed spaces with tabs; everything LOOKED ok in the editor.  But using the "\t == 4" spaces replacement would have ended up with the unaligned output as above.
------- Comment #13 From 2010-02-05 00:01:18 PST -------
Created an attachment (id=48208) [details]
[PATCH] Proposed change.
------- Comment #14 From 2010-02-05 00:04:43 PST -------
(In reply to comment #10)
> Okay, here is a complete patch with ChangeLog, and the mod was moved to
> SourceFrame#setContent

I added a setting into the editor model and made a correct tab substitution based on the character offset instead.

@aparajita sorry for stealing your bug / patch. We've got lots of stuff to do though, so if you have spare cycles / are interested in doing something, jump into #webkit-inspector IRC channel and talk to us!
------- Comment #15 From 2010-02-05 02:50:48 PST -------
(From update of attachment 48208 [details])
Clearing flags on attachment: 48208

Committed r54415: <http://trac.webkit.org/changeset/54415>
------- Comment #16 From 2010-02-05 02:50:59 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #17 From 2010-02-05 04:47:25 PST -------
@pavel don't be sorry, I'm happy I pushed you guys to fix this. ;-)