Bug 32592

Created attachment 45227 [details] Proposed patch Let me know if you want me to split this into two patches. It looks longer than it really is because of some large deletes. In any case, I just want to get it out there so you can see it in one piece. Thanks.

Created attachment 45290 [details] Proposed patch 2 Brought up to date.

Comment on attachment 45290 [details] Proposed patch 2 > Let me know if you want me to split this into two patches. > > It looks longer than it really is because of some large deletes. Yeah, I think it would be better to split this patch. I'd suggest moving code chunk first, then refactoring. > @@ -360,15 +310,15 @@ class _CppStyleState(object): > """Maintains module-wide state..""" > > def __init__(self): > - self.verbose_level = _DEFAULT_VERBOSITY # global setting. > + self.verbose_level = 1 # global setting. > self.error_count = 0 # global count of reported errors > # filters to apply when emitting error messages > - self.filters = _DEFAULT_FILTER_RULES[:] > + self.filters = [] > > # output format: > # "emacs" - format that emacs can parse (default) > # "vs7" - format that Microsoft Visual Studio 7 can parse > - self.output_format = _DEFAULT_OUTPUT_FORMAT > + self.output_format = 'emacs' > > def set_output_format(self, output_format): > """Sets the output format for errors.""" I'm not sure, but the default value of verbose_level, filters, and output_format must be always overwritten? If so, None would be better as the initial value? If you are planning to remove _CppStyleState, it's OK as is.

(In reply to comment #3) > Yeah, I think it would be better to split this patch. I'd suggest moving code > chunk first, then refactoring. Thanks for the comments, Shinichiro. Sure, I'll go ahead and do that. > > @@ -360,15 +310,15 @@ class _CppStyleState(object): > > - self.filters = _DEFAULT_FILTER_RULES[:] > > + self.filters = [] > I'm not sure, but the default value of verbose_level, filters, and > output_format must be always overwritten? If so, None would be better as the > initial value? If you are planning to remove _CppStyleState, it's OK as is. Yes, I was planning to remove those properties in a future patch. I just didn't want it to look like the global variables were playing a role there.

Created attachment 45313 [details] Proposed patch 3 (dividing earlier patch into two) Dividing into two patches as Shinichiro requested. This patch has all of the moves with the minimum changes needed for things to work.

Created attachment 45317 [details] Proposed patch 4 I noticed that the defaults were not getting set after the code move, so I changed two lines of style.parse_arguments() to this: verbosity = _DEFAULT_VERBOSITY output_format = _DEFAULT_OUTPUT_FORMAT I'm not sure how much this matters since it will be refactored away momentarily.

Comment on attachment 45313 [details] Proposed patch 3 (dividing earlier patch into two) Thanks for splitting the patch! Looks good assuming this patch is basically moving code. > @@ -48,117 +47,9 @@ import sys > import unicodedata > > > -# This is set by check-webkit-style. > _USAGE = '' We don't need this in cpp_style anymore?

(In reply to comment #7) > (From update of attachment 45313 [details]) > Thanks for splitting the patch! Looks good assuming this patch is basically > moving code. Yeah, pretty much. The only changes within moves were selectively changing some "cpp_style." prefixes to "style." or "". > > _USAGE = '' > We don't need this in cpp_style anymore? You're right -- we don't. I can get rid of it in part two if that's okay.

Comment on attachment 45313 [details] Proposed patch 3 (dividing earlier patch into two) It appears that patch 4 replaces this one.

(In reply to comment #9) > (From update of attachment 45313 [details]) > It appears that patch 4 replaces this one. Thanks, David. Does Bugzilla allow us to "obsolete" a patch without attaching a new one? I couldn't find where to do this in the UI after forgetting to check the obsolete box.

You can click on the "details" for a patch to obsolete it. If you have lots of patches to obsolete, then "bugzilla-tool obsolete-attachments BUGNUM" will obsolete all attachments on a bug for you.

Eric said it. I should finish one this sometime today.

Comment on attachment 45317 [details] Proposed patch 4 > Index: ChangeLog WebKitTools/ChangeLog.... This isn't going to land easily (using cq+) since the patch isn't rooted at WebKit. > Index: Scripts/modules/cpp_style.py > -# This is set by check-webkit-style. > _USAGE = '' Is this _USAGE still needed?

(In reply to comment #13) Thanks for the review, David. > > Index: Scripts/modules/cpp_style.py > > -# This is set by check-webkit-style. > > _USAGE = '' > > Is this _USAGE still needed? No, I will remove it in the second patch.

Committed r52541: <http://trac.webkit.org/changeset/52541>

Comment on attachment 45317 [details] Proposed patch 4 It seems the ChangeLog had two issues: - It didn't have "Reviewed by NOBODY (OOPS!)". Please consider using prepare-ChangeLog to add a ChangeLog entry. - It had only two lines of context and patch command was confused. Normally, it should have 3 lines in the context. This also happened when I landed another patch you posted. I'm not sure why this happened.

(In reply to comment #16) > (From update of attachment 45317 [details]) > It seems the ChangeLog had two issues: Thank you for landing this patch, Shinichiro, and for correcting and reporting on the ChangeLog issues. I do use prepare-Changelog. I must have overwritten the "Reviewed by" line by accident. I will be more attentive to that in the future. I don't know why the context has only two lines at the end of that portion. I tried re-running svn-create-patch and verified that it has only two lines. In contrast, running "svn diff" does generate three lines of context at the end. I will investigate further for a possible cause and, if necessary, file a bug report. Thanks.

(In reply to comment #17) > I will investigate further for a possible cause and, if necessary, file a bug > report. Thanks. I created a bug report for this issue here: https://bugs.webkit.org/show_bug.cgi?id=32919

(In reply to comment #15) > Committed r52541: <http://trac.webkit.org/changeset/52541> It looks like the commit did not include the following new file in the patch: Scripts/modules/style_unittest.py. Also, the run-webkit-unittests scripts can't execute without this file.

Added missing file style_unittest.py and committed in <http://trac.webkit.org/changeset/52583>. Without this file, run-webkit-unittests died with the following error: Traceback (most recent call last): File "WebKitTools/Scripts/run-webkit-unittests", line 49, in <module> from modules.style_unittest import * ImportError: No module named style_unittest (In reply to comment #19) > (In reply to comment #15) > > Committed r52541: <http://trac.webkit.org/changeset/52541> > > It looks like the commit did not include the following new file in the patch: > Scripts/modules/style_unittest.py. Also, the run-webkit-unittests scripts > can't execute without this file.

Moving part 2 to here: https://bugs.webkit.org/show_bug.cgi?id=32966