See summary.
Created attachment 77192 [details] Patch
Comment on attachment 77192 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77192&action=review > Tools/Scripts/webkitpy/style/checkers/cpp.py:371 > + # to ensure only the parameters remain without the surrounding paranthesis. typo: paranthesis > Tools/Scripts/webkitpy/style/checkers/cpp.py:1322 > + extra blank line
Comment on attachment 77192 [details] Patch Making some improvements.
Created attachment 77282 [details] Patch
Created attachment 77283 [details] Patch
Attachment 77282 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/style/checkers/cpp.py', u'Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py']" exit_code: 1 Tools/Scripts/webkitpy/style/checkers/cpp.py:391: whitespace before ')' [pep8/E202] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 77308 [details] Patch
Comment on attachment 77308 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77308&action=review Should we have a separate helper class for this? Seems like a lot of logic. Or at least we could break thsi huge new function down into smaller parts. > Tools/Scripts/webkitpy/style/checkers/cpp.py:352 > + def get_parameter_list(self): Do we really use get in names? This function is also huge :((
Comment on attachment 77308 [details] Patch Need to address comments.
Created attachment 77391 [details] Patch
Attachment 77391 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/style/checkers/cpp.py', u'Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py']" exit_code: 1 Tools/Scripts/webkitpy/style/checkers/cpp.py:310: expected 2 blank lines, found 1 [pep8/E302] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 77394 [details] Patch
(In reply to comment #11) > Attachment 77391 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/style/checkers/cpp.py', u'Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py']" exit_code: 1 > Tools/Scripts/webkitpy/style/checkers/cpp.py:310: expected 2 blank lines, found 1 [pep8/E302] [5] > Total errors found: 1 in 3 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. I should change the webkit-patch upload to warn you if you have local changes that aren't in the change or something like that :( (The style step warned me. I fixed it and then uploaded without committing first.)
(In reply to comment #8) > (From update of attachment 77308 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77308&action=review > > Should we have a separate helper class for this? Seems like a lot of logic. Or at least we could break thsi huge new function down into smaller parts. > > > Tools/Scripts/webkitpy/style/checkers/cpp.py:352 > > + def get_parameter_list(self): > > Do we really use get in names? Oops. fixed. > This function is also huge :(( If you think things are still too huge or not understand, let me know and I'll work to make it better. At least, it looks a lot better than before to me.
Comment on attachment 77394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77394&action=review I don't fully understand this, but I'm also willing to rs=me. This is definitely a very cool addition to our style-checker. You might consider making it a generator instead. It would simplify the code ever-so-slightly due to not needing current or current_parameter. I guess the main part I don't undestand is PameterIterator.__init__ Tentative r+, but I'd also happily go another round if you have it in you. :) > Tools/Scripts/webkitpy/style/checkers/cpp.py:322 > + def __init__(self, clean_lines, parameters_start, parameters_end): I assume start/end are offsets in a string? > Tools/Scripts/webkitpy/style/checkers/cpp.py:327 > + # Form a single string with all parameters. Trimming the first and last lines > + # to ensure only the parameters remain without the surrounding parenthesis. > + parameters_lines = clean_lines.elided[self._starting_line:parameters_end[0] + 1] > + parameters_lines[-1] = parameters_lines[-1][:parameters_end[1] - 1] I'm confused as to what this does? parameters_lines[-1] = parameters_lines[-1]? > Tools/Scripts/webkitpy/style/checkers/cpp.py:329 > + # Form a single string with all parameters. Trimming the first and last lines > + # to ensure only the parameters remain without the surrounding parenthesis. > + parameters_lines = clean_lines.elided[self._starting_line:parameters_end[0] + 1] > + parameters_lines[-1] = parameters_lines[-1][:parameters_end[1] - 1] > > > + parameters_lines[0] = parameters_lines[0][parameters_start[1] + 1:] > + self._all_parameters = ' '.join(parameters_lines) This could be a separate function and unit tested. > Tools/Scripts/webkitpy/style/checkers/cpp.py:402 > + self._skeleton_parameters = skeleton_parameters Cleaner woudl be to just return the skeleton parameters here and initialize the self. in teh caller. Then this is also much easier to unit test. Would be nice to see some unit tests for this function. :) > Tools/Scripts/webkitpy/style/checkers/cpp.py:449 > + parameter_parser = ParameterIterator(self._clean_lines, self.parameter_start, self.parameter_end) > + while parameter_parser.next(): The python way would be a generator instead of an explicit current function. :) http://docs.python.org/tutorial/classes.html#generators But this is also fine as-is. :) Then you would just do parameter_list(self._clean_lines, ...) and that would be it. You could wrap it in tuple() if you wanted to convert it all at once.
Comment on attachment 77394 [details] Patch I'll address feedback and do another round.
Comment on attachment 77394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77394&action=review A few more comments in addition to Eric's. > Tools/Scripts/webkitpy/style/checkers/cpp.py:313 > + def __init__(self, parameter, type_name_index, line_number): Is the type_name_index good name? I guess parameter_name_index would be better. > Tools/Scripts/webkitpy/style/checkers/cpp.py:316 > + self.name = sub(r'=.*', '', parameter[type_name_index:]).strip() Do we need this line? It looks like line 386 is doing this job already. > Tools/Scripts/webkitpy/style/checkers/cpp.py:399 > + if skeleton_parameters.strip(): I couldn't understand the intention of this if-clause at first. We might want a comment for this.
Created attachment 77451 [details] Patch
Created attachment 77456 [details] Patch
Comment on attachment 77456 [details] Patch I believe that all feedback has been addressed.
(In reply to comment #17) > (From update of attachment 77394 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77394&action=review > > Tools/Scripts/webkitpy/style/checkers/cpp.py:316 > > + self.name = sub(r'=.*', '', parameter[type_name_index:]).strip() > > Do we need this line? It looks like line 386 is doing this job already. True. Line 386 did the same thing but that was for the purpose of creating the skeleton parameters. This is operating on the original parameters. At this point, the code operates on the original parameters in order to extra the names without the modifications done during the process of creating skeleton parameters. (Of course this part of the change was still useful.)
Comment on attachment 77456 [details] Patch OK.
Comment on attachment 77456 [details] Patch Clearing flags on attachment: 77456 Committed r74670: <http://trac.webkit.org/changeset/74670>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/74670 might have broken GTK Linux 32-bit Release The following tests are not passing: media/controls-strict.html