Bug 34574

Created attachment 48286 [details] Proposed patch

Created attachment 48287 [details] Proposed patch 2 Minor fixes to ChangeLog and a code comment.

Adding Gustavo in CC list. My comment about GTK+ isn't related to this bug. Please feel free to land this patch as is (or you may want to modify the list-comprehension), and I'll open another bug to decide path-rules for GTK+. > @@ -98,6 +106,15 @@ _PATH_RULES_SPECIFIER = [ > "WebKit/qt/QGVLauncher/"], > ("-build/include", > "-readability/streams")), > + ([# The GTK+ APIs use GTK+ naming style, which includes > + # lower-cased, underscore-separated values. > + "WebKit/gtk/webkit/", > + # There is no clean way to avoid "xxx_data" methods inside > + # Qt's autotests since they are called automatically by the > + # QtTest module. > + "WebKit/qt/tests/", > + "JavaScriptCore/qt/tests"], > + ("-readability/naming",)), > # These are test file patterns. > (["_test.cpp", > "_unittest.cpp", Per discussion in Bug 34572, we need to add WebKitTools/DumpRenderTree/gtk. I guess gtk code doesn't care build/include_order (#includes should be alphabetically sorted) and readability/null (don't use NULL, use 0 instead) as well. So, I'd propose ([# The GTK+ APIs use GTK+ naming style, which includes # lower-cased, underscore-separated values. # GTK+ code uses NULL and unsorted #includes, too. "WebKit/gtk/webkit/", "WebKitTools/DumpRenderTree/gtk",], ("-build/include_order", "-readability/naming", "-readability/null")), ([# There is no clean way to avoid "xxx_data" methods inside # Qt's autotests since they are called automatically by the # QtTest module. "WebKit/qt/tests/", "JavaScriptCore/qt/tests"], ("-readability/naming",)), or something like this. Gustavo, could you check my guess is correct? > + def _get_path_specific_lower(self): > + """Return a copy of self._path_specific with the paths lower-cased.""" > + if self._path_specific_lower is None: > + self._path_specific_lower = [] > + for (sub_paths, path_rules) in self._path_specific: > + sub_paths = [sub_path.lower() for sub_path in sub_paths] Though this is fine as is, I think there's a bit simpler way: sub_paths = map(str.lower, sub_paths)

Probably, for gtk, we may want to disable whitespace/declaration as well? See the review bot's comment in Bug 32170.

Comment on attachment 48287 [details] Proposed patch 2 cq- to use webkit-patch

Manually committed (via "git svn dcommit"): http://trac.webkit.org/changeset/54482

any particular reason you removed - self.assert_lint('void this_is_a_gtk_style_name(int var1, int var2)', '', 'WebKit/gtk/webkit/foo.cpp') ? Seems like a good test to have...

(In reply to comment #7) > any particular reason you removed > > - self.assert_lint('void this_is_a_gtk_style_name(int var1, int var2)', '', > 'WebKit/gtk/webkit/foo.cpp') > > ? Seems like a good test to have... Yes. I removed it because cpp.py is no longer responsible for checking that file path. All of that is now encoded by the _PATH_RULES_SPECIFIER configuration variable and taken care of by the FilterConfiguration class. I had a similar discussion with Shinichiro at the end of this comment https://bugs.webkit.org/show_bug.cgi?id=33684#c19 after removing similar hard-coded directory names from cpp.py and their corresponding unit tests. Testing like the above can now go in the GlobalVariablesTest.test_path_rules_specifier() unit test method. That method is responsible for checking that the _PATH_RULES_SPECIFIER variable is functioning as expected. I suppose I could have added another assert to that method for a gtk file path, but there is already an assert in there for a qt file path that tests the same exemption: > self.assertFalse(config.should_check("readability/naming", > "WebKit/qt/tests/qwebelement/tst_qwebelement.cpp")) The exemption is encoded here: > ([# The GTK+ APIs use GTK+ naming style, which includes > # lower-cased, underscore-separated values. > "WebKit/gtk/webkit/", > # There is no clean way to avoid "xxx_data" methods inside > # Qt's autotests since they are called automatically by the > # QtTest module. > "WebKit/qt/tests/", > "JavaScriptCore/qt/tests"], > ["-readability/naming"]),

(In reply to comment #8) > I suppose I could have added another assert to that method for a gtk file path, > but there is already an assert in there for a qt file path that tests the same > exemption: > > > self.assertFalse(config.should_check("readability/naming", > > "WebKit/qt/tests/qwebelement/tst_qwebelement.cpp")) I do think it makes sense to have at least one assert per string that appears in _PATH_RULES_SPECIFIER (to have complete code coverage and avoid typos, etc). I will do that the next time I modify this area of code.

(In reply to comment #7) > any particular reason you removed > > - self.assert_lint('void this_is_a_gtk_style_name(int var1, int var2)', '', > 'WebKit/gtk/webkit/foo.cpp') > > ? Seems like a good test to have... FYI, as I mentioned in comment 9 of this report, I restored the unit test for a "WebKit/gtk/webkit/" path in the proposed patches of this report: https://bugs.webkit.org/show_bug.cgi?id=34932 I also added a unit test for every other path that appears in the _PATH_RULES_SPECIFIER configuration variable.

It seems that the problem is still valid. Please look at bug 40911.

This bug has been waiting to land for 6 months? Or did it get landed already?

Sadly, cjerdonek seems to have moved on to geener pastures.

I think there are no reason we cannot land this patch. I guess Chris put cq- just because this requires Bug 33684 closed. As the patch for Bug 33684 seems to be landed already, I think we should be able to land this patch. I'll try.

Comment on attachment 48287 [details] Proposed patch 2 (In reply to comment #14) > I think there are no reason we cannot land this patch. I guess Chris put cq- just because this requires Bug 33684 closed. As the patch for Bug 33684 seems to be landed already, I think we should be able to land this patch. I'll try. Actually, this patch has already been landed: http://trac.webkit.org/changeset/54482 The false-positives in Bug 40911 happened because we don't have qt/benchmark in the exemption list.

Filed Bug 51467.