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
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-08 19:11 PST by aparajita
Modified: 2010-02-05 04:47 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description aparajita 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 Timothy Hatcher 2009-12-04 09:37:37 PST
I agree
Comment 2 Patrick Mueller 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 aparajita 2010-02-03 08:17:48 PST
I should be noted that FireBug uses 4 spaces for tabs.
Comment 4 aparajita 2010-02-04 15:26:39 PST
Created attachment 48176 [details]
Patch to convert tabs to 4 spaces
Comment 5 aparajita 2010-02-04 15:27:27 PST
It was an easy fix, patch is attached. Hope this makes it into the source.
Comment 6 Timothy Hatcher 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 Pavel Feldman 2010-02-04 16:05:27 PST
Comment on attachment 48176 [details]
Patch to convert tabs to 4 spaces

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 aparajita 2010-02-04 18:03:29 PST
Created attachment 48187 [details]
Proposed patch
Comment 9 aparajita 2010-02-04 18:04:30 PST
Created attachment 48188 [details]
Proposed patch
Comment 10 aparajita 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 WebKit Review Bot 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 Patrick Mueller 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 Pavel Feldman 2010-02-05 00:01:18 PST
Created attachment 48208 [details]
[PATCH] Proposed change.
Comment 14 Pavel Feldman 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 WebKit Commit Bot 2010-02-05 02:50:48 PST
Comment on attachment 48208 [details]
[PATCH] Proposed change.

Clearing flags on attachment: 48208

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