46099 2010-09-20 09:21:41 -0700 check-webkit-style and the coding style guidelines page are inconsistent 2010-09-22 02:05:57 -0700 1 1 1 Unclassified WebKit WebKit Website 528+ (Nightly build) PC All RESOLVED FIXED P2 Normal --- 1 kbalazs webkit-unassigned darin eric levin ossy oldest_to_newest 281598 0 kbalazs 2010-09-20 09:21:41 -0700 The code style guideline page says: "Other #include statements should be in sorted order (case sensitive, as done by the command-line sort tool or the Xcode sort selection command)". According to that an ordering is ok if sort do not reorders it. This one is ordered as sort would do: ------------------------ #include "Chrome.h" #include "ChromeClientQt.h" #include <IntSize.h> #include "NotImplemented.h" #include <Page.h> ------------------------ check-webkit-style does not accept this. It accepts the following: ------------------------ #include "Chrome.h" #include "ChromeClientQt.h" #include "NotImplemented.h" #include <IntSize.h> #include <Page.h> ------------------------ I agree that we should separate the system style includes from the normal ones but we should make it clear in the style guidelines. 281611 1 kbalazs 2010-09-20 09:46:18 -0700 I have realized that check-webkit-style does not expect grouping. It accepts the following: ------------------------ #include "Chrome.h" #include "ChromeClientQt.h" #include "NotImplemented.h" #include <IntSize.h> #include <Page.h> ------------------------ So the rule is that system style includes should not come before normal style ones. 281614 2 68098 kbalazs 2010-09-20 09:49:32 -0700 Created attachment 68098 proposed patch 282122 3 68098 ossy 2010-09-20 23:46:57 -0700 Comment on attachment 68098 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=68098&action=review I have never seen "system style" and "normal style" includes in C terminology, but system headers. See: http://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html#Include-Syntax Otherwise LGTM, making clear this style issue is necessary. > WebKitSite/ChangeLog:9 > + * coding/coding-style.html: Making clear that system style includes > + should not come before normal ones. I prefer like this: Making clear that includes of system headers must come after includes of other headers. > WebKitSite/coding/coding-style.html:759 > +<li>System syle includes should not come before normal ones. I prefer like this: Includes of system headers must come after includes of other headers. 282194 4 kbalazs 2010-09-21 03:58:01 -0700 What a terminology guru you are! :) I agree with the changes you mentioned, those will be adopted in the next patch. 282198 5 68213 kbalazs 2010-09-21 04:01:18 -0700 Created attachment 68213 proposed patch v2 282202 6 ossy 2010-09-21 04:07:20 -0700 (In reply to comment #5) > Created an attachment (id=68213) [details] > proposed patch v2 LGTM. I cc-ed Darin, he is the best person for reviewing website modification. 282607 7 levin 2010-09-21 16:17:48 -0700 (In reply to comment #6) > (In reply to comment #5) > > Created an attachment (id=68213) [details] [details] > > proposed patch v2 > > LGTM. > > I cc-ed Darin, he is the best person for reviewing website modification. I'd suggest an email to webkit-dev about this change. If people are fine with it, then r+'ing this will be easy. I think this practice is already widely done and recommended, just not in the styleguide. 282702 8 68213 darin 2010-09-21 17:45:24 -0700 Comment on attachment 68213 proposed patch v2 I think it’s fine to make this clearer in this fashion. Ordered as “sort” would do was intended to be ordered as sort would sort the entire source lines, not just the filenames, and so that’s why there’s no separate rule about this today. If it was me, I would simply add some system header to the existing example instead of making this a separate rule, but I’m OK with it like this too. I don’t think this really requires webkit-dev discussion. This rule has not changed. 282852 9 kbalazs 2010-09-22 02:05:29 -0700 Landed in http://trac.webkit.org/changeset/68030. Closing bug 68098 2010-09-20 09:49:32 -0700 2010-09-21 04:01:18 -0700 proposed patch land_csg.diff text/plain 1621 kbalazs ZGlmZiAtLWdpdCBhL1dlYktpdFNpdGUvQ2hhbmdlTG9nIGIvV2ViS2l0U2l0ZS9DaGFuZ2VMb2cK aW5kZXggNjI5ZDJhNC4uYzQzOGRmMiAxMDA2NDQKLS0tIGEvV2ViS2l0U2l0ZS9DaGFuZ2VMb2cK KysrIGIvV2ViS2l0U2l0ZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxMyBAQAorMjAxMC0wOS0yMCAg QmFsYXpzIEtlbGVtZW4gIDxrYkBpbmYudS1zemVnZWQuaHU+CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgY2hlY2std2Via2l0LXN0eWxlIGFuZCB0aGUg Y29kaW5nIHN0eWxlIGd1aWRlbGluZXMgcGFnZSBhcmUgaW5jb25zaXN0ZW50CisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00NjA5OQorCisgICAgICAgICog Y29kaW5nL2NvZGluZy1zdHlsZS5odG1sOiBNYWtpbmcgY2xlYXIgdGhhdCBzeXN0ZW0gc3R5bGUg aW5jbHVkZXMKKyAgICAgICAgc2hvdWxkIG5vdCBjb21lIGJlZm9yZSBub3JtYWwgb25lcy4KKwog MjAxMC0wOS0xNiAgVG9ueSBHZW50aWxjb3JlICA8dG9ueWdAY2hyb21pdW0ub3JnPgogCiAgICAg ICAgIFJldmlld2VkIGJ5IEFkYW0gQmFydGguCmRpZmYgLS1naXQgYS9XZWJLaXRTaXRlL2NvZGlu Zy9jb2Rpbmctc3R5bGUuaHRtbCBiL1dlYktpdFNpdGUvY29kaW5nL2NvZGluZy1zdHlsZS5odG1s CmluZGV4IDEwNTI3ZjIuLjVlOGRkNTkgMTAwNjQ0Ci0tLSBhL1dlYktpdFNpdGUvY29kaW5nL2Nv ZGluZy1zdHlsZS5odG1sCisrKyBiL1dlYktpdFNpdGUvY29kaW5nL2NvZGluZy1zdHlsZS5odG1s CkBAIC03NTUsNiArNzU1LDMzIEBAIERvbid0IGJvdGhlciB0byBvcmdhbml6ZSB0aGVtIGluIGEg bG9naWNhbCBvcmRlci4KICNpbmNsdWRlICJRdWFsaWZpZWROYW1lLmgiCiAjaW5jbHVkZSAiQXR0 cmlidXRlLmgiCiA8L3ByZT4KKworPGxpPlN5c3RlbSBzeWxlIGluY2x1ZGVzIHNob3VsZCBub3Qg Y29tZSBiZWZvcmUgbm9ybWFsIG9uZXMuCisKKzxoNCBjbGFzcz0icmlnaHQiPlJpZ2h0OjwvaDQ+ Cis8cHJlIGNsYXNzPSJjb2RlIj4KKy8vIENvbm5lY3Rpb25RdC5jcHAKKyNpbmNsdWRlICJBcmd1 bWVudEVuY29kZXIuaCIKKyNpbmNsdWRlICJQcm9jZXNzTGF1bmNoZXIuaCIKKyNpbmNsdWRlICJX ZWJQYWdlUHJveHlNZXNzYWdlS2luZHMuaCIKKyNpbmNsdWRlICJXb3JrSXRlbS5oIgorI2luY2x1 ZGUgJmx0UUFwcGxpY2F0aW9uJmd0CisjaW5jbHVkZSAmbHRRTG9jYWxTZXJ2ZXImZ3QKKyNpbmNs dWRlICZsdFFMb2NhbFNvY2tldCZndAorPC9wcmU+CisKKzxoNCBjbGFzcz0id3JvbmciPldyb25n OjwvaDQ+Cis8cHJlIGNsYXNzPSJjb2RlIj4KKy8vIENvbm5lY3Rpb25RdC5jcHAKKyNpbmNsdWRl ICJBcmd1bWVudEVuY29kZXIuaCIKKyNpbmNsdWRlICJQcm9jZXNzTGF1bmNoZXIuaCIKKyNpbmNs dWRlICZsdFFBcHBsaWNhdGlvbiZndAorI2luY2x1ZGUgJmx0UUxvY2FsU2VydmVyJmd0CisjaW5j bHVkZSAmbHRRTG9jYWxTb2NrZXQmZ3QKKyNpbmNsdWRlICJXZWJQYWdlUHJveHlNZXNzYWdlS2lu ZHMuaCIKKyNpbmNsdWRlICJXb3JrSXRlbS5oIgorPC9wcmU+Cis8L2xpPgogPC9vbD4KIAogPGgz PiJ1c2luZyIgU3RhdGVtZW50czwvaDM+Cg== 68213 2010-09-21 04:01:18 -0700 2010-09-22 02:05:57 -0700 proposed patch v2 land_csg.diff text/plain 1646 kbalazs ZGlmZiAtLWdpdCBhL1dlYktpdFNpdGUvQ2hhbmdlTG9nIGIvV2ViS2l0U2l0ZS9DaGFuZ2VMb2cK aW5kZXggNjI5ZDJhNC4uYzQzOGRmMiAxMDA2NDQKLS0tIGEvV2ViS2l0U2l0ZS9DaGFuZ2VMb2cK KysrIGIvV2ViS2l0U2l0ZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxMyBAQAorMjAxMC0wOS0yMCAg QmFsYXpzIEtlbGVtZW4gIDxrYkBpbmYudS1zemVnZWQuaHU+CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgY2hlY2std2Via2l0LXN0eWxlIGFuZCB0aGUg Y29kaW5nIHN0eWxlIGd1aWRlbGluZXMgcGFnZSBhcmUgaW5jb25zaXN0ZW50CisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00NjA5OQorCisgICAgICAgICog Y29kaW5nL2NvZGluZy1zdHlsZS5odG1sOgorICAgICAgICBNYWtpbmcgY2xlYXIgdGhhdCBpbmNs dWRlcyBvZiBzeXN0ZW0gaGVhZGVycyBtdXN0IGNvbWUgYWZ0ZXIgaW5jbHVkZXMgb2Ygb3RoZXIg aGVhZGVycy4KKwogMjAxMC0wOS0xNiAgVG9ueSBHZW50aWxjb3JlICA8dG9ueWdAY2hyb21pdW0u b3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEFkYW0gQmFydGguCmRpZmYgLS1naXQgYS9XZWJL aXRTaXRlL2NvZGluZy9jb2Rpbmctc3R5bGUuaHRtbCBiL1dlYktpdFNpdGUvY29kaW5nL2NvZGlu Zy1zdHlsZS5odG1sCmluZGV4IDEwNTI3ZjIuLjVlOGRkNTkgMTAwNjQ0Ci0tLSBhL1dlYktpdFNp dGUvY29kaW5nL2NvZGluZy1zdHlsZS5odG1sCisrKyBiL1dlYktpdFNpdGUvY29kaW5nL2NvZGlu Zy1zdHlsZS5odG1sCkBAIC03NTUsNiArNzU1LDMzIEBAIERvbid0IGJvdGhlciB0byBvcmdhbml6 ZSB0aGVtIGluIGEgbG9naWNhbCBvcmRlci4KICNpbmNsdWRlICJRdWFsaWZpZWROYW1lLmgiCiAj aW5jbHVkZSAiQXR0cmlidXRlLmgiCiA8L3ByZT4KKworPGxpPkluY2x1ZGVzIG9mIHN5c3RlbSBo ZWFkZXJzIG11c3QgY29tZSBhZnRlciBpbmNsdWRlcyBvZiBvdGhlciBoZWFkZXJzLgorCis8aDQg Y2xhc3M9InJpZ2h0Ij5SaWdodDo8L2g0PgorPHByZSBjbGFzcz0iY29kZSI+CisvLyBDb25uZWN0 aW9uUXQuY3BwCisjaW5jbHVkZSAiQXJndW1lbnRFbmNvZGVyLmgiCisjaW5jbHVkZSAiUHJvY2Vz c0xhdW5jaGVyLmgiCisjaW5jbHVkZSAiV2ViUGFnZVByb3h5TWVzc2FnZUtpbmRzLmgiCisjaW5j bHVkZSAiV29ya0l0ZW0uaCIKKyNpbmNsdWRlICZsdFFBcHBsaWNhdGlvbiZndAorI2luY2x1ZGUg Jmx0UUxvY2FsU2VydmVyJmd0CisjaW5jbHVkZSAmbHRRTG9jYWxTb2NrZXQmZ3QKKzwvcHJlPgor Cis8aDQgY2xhc3M9Indyb25nIj5Xcm9uZzo8L2g0PgorPHByZSBjbGFzcz0iY29kZSI+CisvLyBD b25uZWN0aW9uUXQuY3BwCisjaW5jbHVkZSAiQXJndW1lbnRFbmNvZGVyLmgiCisjaW5jbHVkZSAi UHJvY2Vzc0xhdW5jaGVyLmgiCisjaW5jbHVkZSAmbHRRQXBwbGljYXRpb24mZ3QKKyNpbmNsdWRl ICZsdFFMb2NhbFNlcnZlciZndAorI2luY2x1ZGUgJmx0UUxvY2FsU29ja2V0Jmd0CisjaW5jbHVk ZSAiV2ViUGFnZVByb3h5TWVzc2FnZUtpbmRzLmgiCisjaW5jbHVkZSAiV29ya0l0ZW0uaCIKKzwv cHJlPgorPC9saT4KIDwvb2w+CiAKIDxoMz4idXNpbmciIFN0YXRlbWVudHM8L2gzPgo=