72483 2011-11-16 02:21:30 -0800 Add .dir-locals.el file for Emacs 2011-11-24 05:01:06 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 wingo webkit-unassigned jsbell mrobinson tommyw webkit.review.bot oldest_to_newest 503725 0 wingo 2011-11-16 02:21:30 -0800 Emacs 23 and later support directory-local variables: http://www.gnu.org/s/libtool/manual/emacs/Directory-Variables.html I occasionally see patches fail style checks because of indentation. Adding a .dir-locals file to the repo would help avoid these, and as it's invisible to a normal `ls', it shouldn't be a burden on anyone. Patch to be attached. 503728 1 115353 wingo 2011-11-16 02:33:47 -0800 Created attachment 115353 Patch 503845 2 115374 wingo 2011-11-16 07:06:26 -0800 Created attachment 115374 Patch 503848 3 wingo 2011-11-16 07:07:13 -0800 Updated with explicit blocks for c-mode and c++-mode, so that we really do override a user's default settings. 503921 4 115353 mrobinson 2011-11-16 08:40:51 -0800 Comment on attachment 115353 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115353&action=review > .dir-locals.el:6 > + (fill-column . 80) The most common range I've heard is 120-180, but definitely not 80: https://lists.webkit.org/pipermail/webkit-dev/2009-September/009761.html > .dir-locals.el:7 > + (tab-width . 8) Why not 4 here? 503935 5 wingo 2011-11-16 08:51:37 -0800 I must admit that I do not understand WebKit's predilection for long lines. I assumed that people producing long lines were not using Emacs. But I am fine putting in some other value. Regarding tab widths, the docs say: Documentation: *Distance between tab stops (for display of tab characters), in columns. That is the amount that TAB characters are interpreted as. TAB characters are not usually present in WK, but where they are, as in Makefiles, they should probably be represented as 8 characters wide. It is the default though, so it can probably be elided. Do you have concrete recommendations? 503940 6 mrobinson 2011-11-16 08:55:44 -0800 (In reply to comment #5) > I must admit that I do not understand WebKit's predilection for long lines. I assumed that people producing long lines were not using Emacs. But I am fine putting in some other value. I would say either 180 or having no maximum. At least in vim it can sometimes be annoying when the editor folds a pre-existing line when you modify it. Perosonally, I'm not a fan of long lines either, but the source contains a lot of code with very long lines. > That is the amount that TAB characters are interpreted as. TAB characters are not usually present in WK, but where they are, as in Makefiles, they should probably be represented as 8 characters wide. It is the default though, so it can probably be elided. You can probably just omit it. The only place we have tab characters is in our automake files. I think it'd be best if an emacs user gives the final r+. :) 503965 7 115374 xan.lopez 2011-11-16 09:31:42 -0800 Comment on attachment 115374 Patch I think it's best to just omit the maximum for fill-column, WebKit has lots of really crazy-long lines. Other than that, looks great! Emacs is the default GNU editor. 503979 8 115396 wingo 2011-11-16 09:42:37 -0800 Created attachment 115396 Patch 503981 9 115396 wingo 2011-11-16 09:43:18 -0800 Comment on attachment 115396 Patch Removed the fill-column and tab-width bits. Xan, cq? 506840 10 115396 xan.lopez 2011-11-21 03:37:20 -0800 Comment on attachment 115396 Patch Bam. 506884 11 115396 webkit.review.bot 2011-11-21 04:48:30 -0800 Comment on attachment 115396 Patch Clearing flags on attachment: 115396 Committed r100902: <http://trac.webkit.org/changeset/100902> 506885 12 webkit.review.bot 2011-11-21 04:48:34 -0800 All reviewed patches have been landed. Closing bug. 508087 13 tommyw 2011-11-23 05:44:36 -0800 I'm not particularly happy with this since it interferes my my existing .dir-local.el that have something similar but also modifications to the compile command :/ But if it does something useful I guess I can live with it. 508146 14 mrobinson 2011-11-23 08:06:02 -0800 Perhaps there can be a .dir.locals.locals.el or something similar to allow extending the WebKit defaults. 508252 15 jsbell 2011-11-23 11:23:22 -0800 (In reply to comment #14) > Perhaps there can be a .dir.locals.locals.el or something similar to allow extending the WebKit defaults. http://code.google.com/p/chromium/wiki/Emacs ... recommends having .dir-locals.el specify (c-file-style . "WebKit") for c-mode/c++-mode so that additional customization can be done in your .emacs - this would at least be a standard way to allow users to make additional style changes. While I'm skeptical that editor-specific style options should be baked into the tree like this, I'd personally be satisfied (for now) if the c-file-style entries were added. 508721 16 wingo 2011-11-24 04:59:49 -0800 (In reply to comment #15) > http://code.google.com/p/chromium/wiki/Emacs > > ... recommends having .dir-locals.el specify (c-file-style . "WebKit") That's an interesting page, thanks for the link. There are definitely things you can do to improve webkit editing if you are willing to edit your .emacs, but my goal with this was to make things better for people that don't have .emacs customizations. If you are editing your .emacs, something like: (defun webkit-c++-mode () "C++ mode with adjusted defaults for use with WebKit." (interactive) (c++-mode) (c-set-style "WebKit") (setq indent-tabs-mode t) (setq c-basic-offset 4)) (setq auto-mode-alist (cons '("/WebKit/.*\\.[ch]*$" . webkit-c++-mode) auto-mode-alist)) 508722 17 wingo 2011-11-24 05:01:06 -0800 Properly indented ;) (setq auto-mode-alist (cons '("/WebKit/.*\\.[ch]*$" . webkit-c++-mode) auto-mode-alist)) 115353 2011-11-16 02:33:47 -0800 2011-11-16 08:40:51 -0800 Patch bug-72483-20111116113345.patch text/plain 1184 wingo U3VidmVyc2lvbiBSZXZpc2lvbjogMTAwNDI0CmRpZmYgLS1naXQgYS8uZGlyLWxvY2Fscy5lbCBi Ly5kaXItbG9jYWxzLmVsCm5ldyBmaWxlIG1vZGUgMTAwNjQ0CmluZGV4IDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAuLjYxMDQ5NjMxY2M0NTlmMWIyZTk0ZTdiNjZjM2E4 NmZiYWMwOGI0MDAKLS0tIC9kZXYvbnVsbAorKysgYi8uZGlyLWxvY2Fscy5lbApAQCAtMCwwICsx LDExIEBACis7OyBQZXItZGlyZWN0b3J5IGxvY2FsIHZhcmlhYmxlcyBmb3IgR05VIEVtYWNzIDIz IGFuZCBsYXRlci4KKworOzsgU3ludGF4OiAoKE1PREUgKFZBUiAuIFZBTCkgLi4uKSAuLi4pCis7 OyBNT0RFIGlzIGEgc3ltYm9sIGxpa2UgYGMtbW9kZScsIG9yIGBuaWwnIGZvciBhbGwgbW9kZXMu CisoKG5pbAorICAoZmlsbC1jb2x1bW4gLiA4MCkKKyAgKHRhYi13aWR0aCAuIDgpCisgIChpbmRl bnQtdGFicy1tb2RlIC4gbmlsKQorICAoYy1iYXNpYy1vZmZzZXQgLiA0KSkKKyAoY2hhbmdlLWxv Zy1tb2RlCisgIChpbmRlbnQtdGFicy1tb2RlIC4gbmlsKSkpCmRpZmYgLS1naXQgYS9DaGFuZ2VM b2cgYi9DaGFuZ2VMb2cKaW5kZXggYmZhNWRmYjZhZGQ3ZjA2N2RhMzgyOTBlZTVmYTlmYjZhZjYy MDdmNi4uZmRjYzRlMzRhNzg4NjIyYTc2ODAzMzQ4N2RkNjc1MjM5MTg5NzY3ZSAxMDA2NDQKLS0t IGEvQ2hhbmdlTG9nCisrKyBiL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEyIEBACisyMDExLTExLTE2 ICBBbmR5IFdpbmdvICA8d2luZ29AaWdhbGlhLmNvbT4KKworICAgICAgICBBZGQgLmRpci1sb2Nh bHMuZWwgZmlsZSBmb3IgYmV0dGVyIEVtYWNzIGRlZmF1bHRzCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03MjQ4MworCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogLmRpci1sb2NhbHMuZWw6IFNldCBhcHByb3By aWF0ZSBkaXJlY3RvcnktbG9jYWwgdmFyaWFibGVzIGZvciBFbWFjcy4KKwogMjAxMS0xMS0xNSAg U2ltb24gSGF1c21hbm4gIDxzaW1vbi5oYXVzbWFubkBub2tpYS5jb20+CiAKICAgICAgICAgW1F0 XSBDZW50cmFsaXplIGhpZGVfc3ltYm9scyBhbmQgZW5zdXJlIGFsbCBsaWJzIGFyZSBidWlsdCB3 aXRoIHN5bWJvbCB2aXNpYmlsaXR5ICYgYnN5bWJvbGljX2Z1bmN0aW9ucwo= 115374 2011-11-16 07:06:26 -0800 2011-11-16 09:42:28 -0800 Patch bug-72483-20111116160624.patch text/plain 1394 wingo U3VidmVyc2lvbiBSZXZpc2lvbjogMTAwNDI0CmRpZmYgLS1naXQgYS8uZGlyLWxvY2Fscy5lbCBi Ly5kaXItbG9jYWxzLmVsCm5ldyBmaWxlIG1vZGUgMTAwNjQ0CmluZGV4IDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAuLmZkMjQxODA3ZjczOGE1N2VhYmIxODhlMzNkZTY3 ZDBmNDYzMGJmMTcKLS0tIC9kZXYvbnVsbAorKysgYi8uZGlyLWxvY2Fscy5lbApAQCAtMCwwICsx LDIxIEBACis7OyBQZXItZGlyZWN0b3J5IGxvY2FsIHZhcmlhYmxlcyBmb3IgR05VIEVtYWNzIDIz IGFuZCBsYXRlci4KKworOzsgU3ludGF4OiAoKE1PREUgKFZBUiAuIFZBTCkgLi4uKSAuLi4pCis7 OyBNT0RFIGlzIGEgc3ltYm9sIGxpa2UgYGMtbW9kZScsIG9yIGBuaWwnIGZvciBhbGwgbW9kZXMu CisoKG5pbAorICAoZmlsbC1jb2x1bW4gLiA4MCkKKyAgKHRhYi13aWR0aCAuIDgpCisgIChpbmRl bnQtdGFicy1tb2RlIC4gbmlsKQorICAoYy1iYXNpYy1vZmZzZXQgLiA0KSkKKyAoYy1tb2RlCisg IChmaWxsLWNvbHVtbiAuIDgwKQorICAodGFiLXdpZHRoIC4gOCkKKyAgKGluZGVudC10YWJzLW1v ZGUgLiBuaWwpCisgIChjLWJhc2ljLW9mZnNldCAuIDQpKQorIChjKystbW9kZQorICAoZmlsbC1j b2x1bW4gLiA4MCkKKyAgKHRhYi13aWR0aCAuIDgpCisgIChpbmRlbnQtdGFicy1tb2RlIC4gbmls KQorICAoYy1iYXNpYy1vZmZzZXQgLiA0KSkKKyAoY2hhbmdlLWxvZy1tb2RlCisgIChpbmRlbnQt dGFicy1tb2RlIC4gbmlsKSkpCmRpZmYgLS1naXQgYS9DaGFuZ2VMb2cgYi9DaGFuZ2VMb2cKaW5k ZXggYmZhNWRmYjZhZGQ3ZjA2N2RhMzgyOTBlZTVmYTlmYjZhZjYyMDdmNi4uZmRjYzRlMzRhNzg4 NjIyYTc2ODAzMzQ4N2RkNjc1MjM5MTg5NzY3ZSAxMDA2NDQKLS0tIGEvQ2hhbmdlTG9nCisrKyBi L0NoYW5nZUxvZwpAQCAtMSwzICsxLDEyIEBACisyMDExLTExLTE2ICBBbmR5IFdpbmdvICA8d2lu Z29AaWdhbGlhLmNvbT4KKworICAgICAgICBBZGQgLmRpci1sb2NhbHMuZWwgZmlsZSBmb3IgYmV0 dGVyIEVtYWNzIGRlZmF1bHRzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD03MjQ4MworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgICogLmRpci1sb2NhbHMuZWw6IFNldCBhcHByb3ByaWF0ZSBkaXJlY3RvcnktbG9j YWwgdmFyaWFibGVzIGZvciBFbWFjcy4KKwogMjAxMS0xMS0xNSAgU2ltb24gSGF1c21hbm4gIDxz aW1vbi5oYXVzbWFubkBub2tpYS5jb20+CiAKICAgICAgICAgW1F0XSBDZW50cmFsaXplIGhpZGVf c3ltYm9scyBhbmQgZW5zdXJlIGFsbCBsaWJzIGFyZSBidWlsdCB3aXRoIHN5bWJvbCB2aXNpYmls aXR5ICYgYnN5bWJvbGljX2Z1bmN0aW9ucwo= 115396 2011-11-16 09:42:37 -0800 2011-11-21 04:48:30 -0800 Patch bug-72483-20111116184235.patch text/plain 1299 wingo U3VidmVyc2lvbiBSZXZpc2lvbjogMTAwNDI0CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAx MWZjMThmY2Q4YjI3YWNiOGU0NzFjM2UxOTRmMzhlMzg0OTFmNGNjLi40NzdhYjk2YWEzYzA2YzM2 OTAwYWYyNTgwZGQ0N2QwNjExNGEwNjA2IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxMiBAQAorMjAxMS0xMS0xNiAgQW5keSBXaW5nbyAgPHdpbmdvQGlnYWxpYS5jb20+CisK KyAgICAgICAgQWRkIC5kaXItbG9jYWxzLmVsIGZpbGUgZm9yIGJldHRlciBFbWFjcyBkZWZhdWx0 cworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzI0ODMK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIC5kaXIt bG9jYWxzLmVsOiBTZXQgYXBwcm9wcmlhdGUgZGlyZWN0b3J5LWxvY2FsIHZhcmlhYmxlcyBmb3Ig RW1hY3MuCisKIDIwMTEtMTEtMTYgIERhcmluIEFkbGVyICA8ZGFyaW5AYXBwbGUuY29tPgogCiAg ICAgICAgIEFkZCBhICJwYXNzIHR5cGUiIGFuZCAicGVlayB0eXBlIiBjb25jZXB0IHRvIEhhc2hU cmFpdHMKZGlmZiAtLWdpdCBhLy5kaXItbG9jYWxzLmVsIGIvLmRpci1sb2NhbHMuZWwKbmV3IGZp bGUgbW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAw MDAwMC4uMjI5YThhMmJiZjMwMmQ4MTgyZmE5OTc1NTIyZWMxMDdiOGI0MjQwMQotLS0gL2Rldi9u dWxsCisrKyBiLy5kaXItbG9jYWxzLmVsCkBAIC0wLDAgKzEsMTUgQEAKKzs7IFBlci1kaXJlY3Rv cnkgbG9jYWwgdmFyaWFibGVzIGZvciBHTlUgRW1hY3MgMjMgYW5kIGxhdGVyLgorCis7OyBTeW50 YXg6ICgoTU9ERSAoVkFSIC4gVkFMKSAuLi4pIC4uLikKKzs7IE1PREUgaXMgYSBzeW1ib2wgbGlr ZSBgYy1tb2RlJywgb3IgYG5pbCcgZm9yIGFsbCBtb2Rlcy4KKygobmlsCisgIChpbmRlbnQtdGFi cy1tb2RlIC4gbmlsKQorICAoYy1iYXNpYy1vZmZzZXQgLiA0KSkKKyAoYy1tb2RlCisgIChpbmRl bnQtdGFicy1tb2RlIC4gbmlsKQorICAoYy1iYXNpYy1vZmZzZXQgLiA0KSkKKyAoYysrLW1vZGUK KyAgKGluZGVudC10YWJzLW1vZGUgLiBuaWwpCisgIChjLWJhc2ljLW9mZnNldCAuIDQpKQorIChj aGFuZ2UtbG9nLW1vZGUKKyAgKGluZGVudC10YWJzLW1vZGUgLiBuaWwpKSkK