webkitbot rollout ChangeLogs should be nicer: 1. There should be a title of rolled out patch, so that one could easily tell what was rolled out. 2. List of changed files and functions should be omitted - there are never per-function comments in an automatically prepared rollout, and searching ChangeLogs for function names doesn't need to show rollouts.
<rdar://problem/15208418>
Created attachment 226314 [details] Patch
Comment on attachment 226314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226314&action=review Great work, I really like this new rollout changelog format. The patch looks good to me in general, but I have some minor comments. > Tools/ChangeLog:8 > + Added bug urls and descriptions of rolled out patches to the rollout changelog if they are present in the original changelog. Additionally removed the list of changed files and functions. This line is too long, wrap it please. > Tools/ChangeLog:11 > + (ChangeLog.update_with_unreviewed_message): Cut off the list of modified files It is a sentence, please close it with "." (not only here) > Tools/Scripts/webkitpy/tool/steps/preparechangelogforrevert.py:48 > + if index < len(description_list) and index < len(bug_url_list): Do we really need this check? I think the length of revision_list, description_list and bug_url_list must be same always. If they are same, we don't need this check. If they aren't same, it is a bug which should be fixed. > Tools/Scripts/webkitpy/tool/steps/preparechangelogforrevert.py:64 > bug_url = self._tool.bugs.bug_url_for_bug_id(state["bug_id"]) if state["bug_id"] else None > - message = self._message_for_revert(state["revision_list"], state["reason"], bug_url) > + for bug_id in state["bug_id_list"]: > + bug_url_list.append(self._tool.bugs.bug_url_for_bug_id(bug_id)) > + message = self._message_for_revert(state["revision_list"], state["reason"], state["description_list"], bug_url_list) You missed to pass bug_url to _message_for_revert. Additionally these names are a little bit confusing: bug_url and bug_url_list I managed to understand that bug_url is the URL of the rollout bug (if there is) and bug_url_list contains the urls for rolled out patches. But it would be great to have better names, for example rollout_bug_url instead of bug_url and reverted_bug_url_list instead of bug_url_list.
Created attachment 226327 [details] Patch
Comment on attachment 226327 [details] Patch r=me
Comment on attachment 226327 [details] Patch Clearing flags on attachment: 226327 Committed r165447: <http://trac.webkit.org/changeset/165447>
All reviewed patches have been landed. Closing bug.
(In reply to comment #6) > (From update of attachment 226327 [details]) > Clearing flags on attachment: 226327 > > Committed r165447: <http://trac.webkit.org/changeset/165447> Please send a heads-up mail to webkit-dev too.