Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2439153002:
Script for exporting WPT (Closed)
DescriptionThis CL adds a script, sync-w3c-tests, that will eventually be a script that syncs our local WPT with the upstream GitHub repo in one atomic operation. This CL adds the export side of that script.
BUG=657117
Committed: https://crrev.com/826ed2a741422fa8007dce9bd006da6652cad69e
Cr-Commit-Position: refs/heads/master@{#430425}
Patch Set 1 #Patch Set 2 : Add logic for going back in WPT commits to get last export #Patch Set 3 : Add two unit tests for sync script #Patch Set 4 : Add more cases to check for in local/remote WPT branches #
Total comments: 16
Patch Set 5 : Refactor into classes TestExporter and LocalWPT #Patch Set 6 : Refactor and add create GH PR method #Patch Set 7 : Remove hard-coded WPT stop point now that an export commit is in WPT #Patch Set 8 : Add more unit tests #Patch Set 9 : Linting #
Total comments: 1
Patch Set 10 : Fold TestExporter into sync, improve messaging #Patch Set 11 : Clean up finding Cr-Commit-Pos in WPT with one neat trick #
Total comments: 62
Patch Set 12 : Address most of CL feedback #
Total comments: 11
Patch Set 13 : Address feedback, move from printing to logging #Patch Set 14 : Add configure_logging from test_importer #Patch Set 15 : Filter out WPT import commits #
Total comments: 14
Patch Set 16 : Address CL feedback #Patch Set 17 : . #Patch Set 18 : Oops, add newlines to multi line strings #Patch Set 19 : Fix spacing issues #Messages
Total messages: 45 (13 generated)
foolip@chromium.org changed reviewers: + foolip@chromium.org
https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/imported/wpt/html/semantics/forms/the-input-element/checkbox.html (right): https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/imported/wpt/html/semantics/forms/the-input-element/checkbox.html:17: // Test change for Chromium exporter If you need a large supply of real changes that can be exported, try stripping whitespace: $ find . -name '*.html' | while read f; do git stripspace < $f > tmp; mv tmp $f; done Hundreds of files modified. In some it strips double blank lines that are probably intentional, but just stripping trailing whitespace should give you enough to work with. https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py (right): https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:1: # TODO: Do we need a license at the top of the file? Yep, and the other one too. Use the short one with "The Chromium Authors". https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:17: # - I feel like it would be cleaner to create the local WPT checkout outside of the local Chromium working tree. e.g. /tmp/wpt Is this now doing the same thing as the importer, or what does self.finder.path_from_webkit_base(WPT_DEST_NAME) do? https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:33: if not self.options['no_fetch']: I can't see where this is hooked up, but I guess this and a lot is placeholders still? https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:50: return any([CR_WPT_DIR in f and '-expected' not in f for f in files]) Maybe more strictly require the path to begin with CR_WPT_DIR https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:61: patch = self.host.executive.run_command([ Being able to get this patch on stdout or into a file will be useful at some point probably, when this scripts fails to apply it. https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:85: patch = patch.replace(CR_WPT_DIR, '') Could also use git am -p<n> where n is len(CR_WPT_DIR.split(/')) or something not off-by-one. https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:109: # The diff for the entire history of WPT is 3MB, which works fine for now Works for now. Later, maybe git rev-list OLDEST_POSSIBLE_WPT_COMMIT..HEAD and call git show -s --format=%B $commit for every commit until you find one that matches. https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:112: 'git', 'log', '1d81fe1feb8f9dc43bc101a4cf9d755fdf4df0e7..HEAD' Could use OLDEST_POSSIBLE_WPT_COMMIT here too? https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:134: 'git', 'rev-list', '{}..HEAD'.format(cr_commit)]) Will this not need --reverse to test them in the right order?
https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/imported/wpt/html/semantics/forms/the-input-element/checkbox.html (right): https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/imported/wpt/html/semantics/forms/the-input-element/checkbox.html:17: // Test change for Chromium exporter On 2016/10/25 at 19:31:40, foolip wrote: > If you need a large supply of real changes that can be exported, try stripping whitespace: > $ find . -name '*.html' | while read f; do git stripspace < $f > tmp; mv tmp $f; done > > Hundreds of files modified. In some it strips double blank lines that are probably intentional, but just stripping trailing whitespace should give you enough to work with. Great, thanks! https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py (right): https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:17: # - I feel like it would be cleaner to create the local WPT checkout outside of the local Chromium working tree. e.g. /tmp/wpt On 2016/10/25 at 19:31:41, foolip wrote: > Is this now doing the same thing as the importer, or what does self.finder.path_from_webkit_base(WPT_DEST_NAME) do? It's currently putting the tests in third_party/WebKit/wpt (I'm not super familiar with what the importer does) https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:33: if not self.options['no_fetch']: On 2016/10/25 at 19:31:41, foolip wrote: > I can't see where this is hooked up, but I guess this and a lot is placeholders still? Down on line 141 of this file there's an arg parser for it. https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:134: 'git', 'rev-list', '{}..HEAD'.format(cr_commit)]) On 2016/10/25 at 19:31:41, foolip wrote: > Will this not need --reverse to test them in the right order? Ah thanks for catching that.
The CQ bit was checked by jeffcarp@chromium.org
The CQ bit was unchecked by jeffcarp@chromium.org
jeffcarp@chromium.org changed reviewers: + qyearsley@chromium.org
jeffcarp@chromium.org changed reviewers: - qyearsley@chromium.org
https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py (right): https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:109: # The diff for the entire history of WPT is 3MB, which works fine for now On 2016/10/25 at 19:31:41, foolip wrote: > Works for now. Later, maybe git rev-list OLDEST_POSSIBLE_WPT_COMMIT..HEAD and call git show -s --format=%B $commit for every commit until you find one that matches. I had a possibly even simpler idea: why not just call: git show -s --format=%B HEAD~0 git show -s --format=%B HEAD~1 git show -s --format=%B HEAD~2 git show -s --format=%B HEAD~3 .. and so on?
https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py (right): https://codereview.chromium.org/2439153002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:109: # The diff for the entire history of WPT is 3MB, which works fine for now On 2016/10/26 22:06:12, jeffcarp wrote: > On 2016/10/25 at 19:31:41, foolip wrote: > > Works for now. Later, maybe git rev-list OLDEST_POSSIBLE_WPT_COMMIT..HEAD and > call git show -s --format=%B $commit for every commit until you find one that > matches. > > I had a possibly even simpler idea: why not just call: > git show -s --format=%B HEAD~0 > git show -s --format=%B HEAD~1 > git show -s --format=%B HEAD~2 > git show -s --format=%B HEAD~3 > .. and so on? That'd require you to parse out the commit to terminate, but if we just export a single whitespace change this script can just assume something will be found and drop OLDEST_POSSIBLE_WPT_COMMIT. One problem is that WPT's history is not linear, and the ~N syntax takes the first parent of each commit, anything that came via a merge will be missed. I think we normally won't merge, but it's probably just a question of time before someone does it when resolving a conflict manually. It'd even be preferable for non-trivial conflicts, to create a branch where it applies cleanly, and then resolve conflicts in the merge commit. Merges might also require some finesse in determining what the latest commit is. I don't think --topo-order will do it, but maybe --author-date-order if we're not doing anything weird.
Description was changed from ========== wip - start on export script BUG=657117 ========== to ========== Script for exporting WPT This Cl adds a script, sync-w3c-tests, that will eventually be a script that syncs our local WPT with the upstream GitHub repo in one atomic operation. This CL adds the export side of that script. BUG=657117 ==========
Description was changed from ========== Script for exporting WPT This Cl adds a script, sync-w3c-tests, that will eventually be a script that syncs our local WPT with the upstream GitHub repo in one atomic operation. This CL adds the export side of that script. BUG=657117 ========== to ========== This CL adds a script, sync-w3c-tests, that will eventually be a script that syncs our local WPT with the upstream GitHub repo in one atomic operation. This CL adds the export side of that script. BUG=657117 ==========
I addressed (hopefully) all of the comments @foolip had, and also refactored things and added more testing. This week I will be trying to finish this CL. At this point I think it's ready for a closer look if you have the chance.
https://codereview.chromium.org/2439153002/diff/150001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right): https://codereview.chromium.org/2439153002/diff/150001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:61: Notes on this file: I'm thinking of removing the TestExporter class and turning it into a method in sync.py. Also the two GitHub methods below are here simply out of convenience. I'm thinking of moving them into their own helper class.
Many comments, but this looks great, I'm excited! Don't worry about addressing everything, where you deem it appropriate just add TODOs and follow up in future CLs. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/html/semantics/scripting-1/the-template-element/template-element/node-document-changes.html (left): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/html/semantics/scripting-1/the-template-element/template-element/node-document-changes.html:17: Good changes to test this with, but do you intend to land them together with the script? In order to experiment with the process, I would rather think that you'd first land these scripts, then do a separate CL with whitespace changes and do a dry run to get the bots green. Then, when in the office, land it and run the export scripts as soon as possible after landing. Make sense? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/sync-w3c-tests (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/sync-w3c-tests:7: from webkitpy.w3c import sync Total nit, but maybe sync-wpt is a more accurate name given that csswg-tests are getting merged into web-platform-tests and this script will thus probably never learn about anything other than web-platform-tests. Also, wpt has specs from other orgs than W3C, like WHATWG :) https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:10: class ChromiumWPT(object): A sentence or two of documentation describing the responsibilities of this class would be great. And the "(object)" probably isn't needed? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:14: self.absolute_chromium_wpt_dir = os.path.abspath(os.path.join('..', '..', 'LayoutTests', 'imported', 'wpt')) Does this assume that the script is being run from inside Tools/Scripts? Some other scripts seem to use os.path.abspath(__file__), maybe some variation of that could be used to make this current-working-dir-independent? (If you check and no other scripts seem to actually work when run from elsewhere, ignore this.) https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:28: '--name-only', '-r', '{}'.format(sha) If sha is already a string, which I think it is, then just sha, otherwise str(sha) would be a more explicit stringifier method. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:31: files = [f.strip() for f in filter(bool, diff_files.split('\n'))] Using splitlines() also handled \r\n if it happens to be in there. With that, could you drop the strip bit? Using splitlines() throughout probably makes sense if so. After testing manually I understand that filter(bool, x) is to strip out empty string, but that also seems to be solved by using splitlines(). https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:32: return any([f.startswith(CR_WPT_DIR) and '-expected' not in f for f in files]) In deps_updater.py there's an is_baseline. Using a shared definition would be great. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:38: return filter(bool, commits.split('\n')) Hopefully just return self....().splitlines() will work here. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:42: 'git', 'show', sha, '--format=%s', '--no-patch' Nit: put the sha last consistently, assuming that in fact works. If guess the returned string also includes a newline. If it doesn't matter, fine, otherwise rstrip()? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:45: def message(self, sha): Document that this actually includes both subject and body? I only knew when I looked up the documentation for git show. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:50: def commit_position_str(self, sha): This is the same as the above, intentional? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:55: def wpt_diff_patch(self, sha): When I saw it at the call site I wasn't sure what it was, maybe format_patch, export_patch, or similar? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:59: '{}'.format(sha), self.absolute_chromium_wpt_dir I think this will actually need to use some of the code in has_changes_in_wpt to enumerate affected files. Once there's an exportable commit, using that for a unit test would be great too. A TODO to prepend "Chromium: " if you think that's a reasonable format too. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:19: something/something.html I think the actual output wouldn't have a blank line at the start. You could maybe add asserts to the implementation and not test it. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:50: something/third_party/WebKit/LayoutTests/imported/wpt/something-expected.html Drop the something prefix here to make sure you're testing the right thing? And add an -expected.txt to make sure that's also ignored. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:21: self.host.print_('## Stop trying to make fetch happen, it\'s not going to happen') When will this message be seen? :) https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:31: self.host.print_('## Need to set up remote "github"!') Does this need to be done manually? If so, should the script exit at this point? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:39: return filter(bool, [s.strip() for s in result.split('\n')]) Maybe splitlines() here too, won't comment elsewhere. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:50: print 'yerah man' :) https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:52: match = re.search(r'Cr-Commit-Position: (?P<cr_commit>.+)$', message, re.MULTILINE) Throw in a ^ so that it only matches at the beginning of the line? Revert commits say things like this before the real Cr-Commit-Position: > Cr-Commit-Position: refs/heads/master@{#428769} Also, should this extract out just the number, or does that turn out to not be necessary yet? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:56: self.run(['git', 'reset', '--hard', 'HEAD']) Perhaps also git clean -fdx? If this is supposed to be robust against local changes better go all the way. Or maybe exit with an error if there are local modifications. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:73: self.run(['git', 'push', 'github', ':%s' % branch_name]) This might mess things up if there's an open pull request for it. When should we expect this will happen? In the beginning, maybe err on the side of failing and requiring manual intervention when unexpected things happen? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:79: patch = patch.replace(CR_WPT_DIR, '') Maybe this would also be the place to add "Chromium: ". Although I think having a way to export a match that can be manually be applied using git am will be needed for conflicts, so having a method that does it all is probably best. Could be layered on top of a non-modifying one, of course. (No action required now.) https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:81: # foolip: Could also use git am -p<n> where n is len(CR_WPT_DIR.split(/')) Yep. Can you make this a TODO? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py:24: self.assertEqual(host.executive.calls[0][1], 'checkout') Neat test! https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py:90: self.assertEqual(len(host.executive.calls), 8) Should this assert anything about what the calls are? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:65: host.print_('## Upload failed with error:\n{}'.format(exp)) Should it rethrow the exception, or what will happen if it tries the next commit?
foolip@chromium.org changed reviewers: + qyearsley@chromium.org
qyearsley@, can you also review this? Because of our timezones, I think it'd be good for you to review each other's changes, and also of course because this all has to work together as a whole.
https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/html/semantics/scripting-1/the-template-element/template-element/node-document-changes.html (left): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/html/semantics/scripting-1/the-template-element/template-element/node-document-changes.html:17: On 2016/11/01 at 22:07:44, foolip wrote: > Good changes to test this with, but do you intend to land them together with the script? In order to experiment with the process, I would rather think that you'd first land these scripts, then do a separate CL with whitespace changes and do a dry run to get the bots green. Then, when in the office, land it and run the export scripts as soon as possible after landing. Make sense? This whitespace change made it into WPT master. Next time the WPT are imported into Chromium this diff should go away.
I'm still not really sure whether it will be nicer to have a separate importer and exporter running separately, or to have both generally done together. To keep both possibilities open, what do you think about making sync-w3c-tests and w3c/sync.py explicitly just concerned with exporting, and renaming them to reflect that? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:10: class ChromiumWPT(object): On 2016/11/01 at 22:07:44, foolip wrote: > A sentence or two of documentation describing the responsibilities of this class would be great. And the "(object)" probably isn't needed? It'd work without (object), but explicitly making classes inherit from object is considered good practice I think: https://wiki.python.org/moin/NewClassVsClassicClass https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:14: self.absolute_chromium_wpt_dir = os.path.abspath(os.path.join('..', '..', 'LayoutTests', 'imported', 'wpt')) On 2016/11/01 at 22:07:44, foolip wrote: > Does this assume that the script is being run from inside Tools/Scripts? Some other scripts seem to use os.path.abspath(__file__), maybe some variation of that could be used to make this current-working-dir-independent? (If you check and no other scripts seem to actually work when run from elsewhere, ignore this.) In general code in this codebase avoids using os directly, and instead accesses the filesystem through host.filesystem, which makes it more easily testable. Also, you could avoid adding a member attribute here by making this a method. The way this might be done in this codebase is: from webkitpy.common.memoized import memoized from webkitpy.common.webkit_finder import WebKitFinder ... @memoized def absolute_chromium_wpt_dir(self): finder = WebKitFinder(self.host.filesystem) return finder.path_from_webkit_base('LayoutTests', 'imported', 'wpt') https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:22: LayoutTests/imported/wpt unless they're expectations This docstring could be clarified. A good docstring usually has a really short one-line summary (e.g. Checks whether there are new exportable commits in Chromium) followed by a blank line and then more details. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:5: import unittest Nit: Blank line separating import sections https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:7: from webkitpy.common.host_mock import MockHost Nit: sort imports https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:24: host.filesystem = MockFileSystem() No need to do this I think, since the constructor for MockHost sets filesystem to be a new MockFileSystem by default. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:18: self.run_command = self.host.executive.run_command Here, self.fs and self.run_command are shortcuts, but I'm not sure whether it's worth it; these two attributes could probably be removed (replacing the usages below with self.host.filesystem and self.host.executive.run_command) without making the code much more verbose. If you think it's more readable with these shortcuts, then what do you think about marking them as protected with an underscore prefix? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:24: self.host.print_('## WPT checkout exists at %s, fetching latest' % (self.path)) Note, I think that rather than printing messages like this (like what deps_updater.py does right now), it's actually much nicer to use logs. Most modules in webkitpy have a global logger _log = logging.getLogger(__name__), and then printing of messages is done with log methods (debug, info, warning, error). Logging config can be set in a main method. This makes it easy to add a verbose mode, or to change how messages are printed. I'd like to change deps_updater.py to do this in the future. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:27: self.run(['git', 'merge', '--ff-only', 'origin/master']) Possible optional change: We could also make the constructor more of a no-op, and actually run these git commands in a separate setup method. What do you think? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py:16: host.executive = MockExecutive2() If nothing special is done here, then I think the default executive that comes with MockHost may work.
https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:10: class ChromiumWPT(object): On 2016/11/01 at 22:07:44, foolip wrote: > A sentence or two of documentation describing the responsibilities of this class would be great. And the "(object)" probably isn't needed? The linter yells at me if classes don't inherit from object. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:28: '--name-only', '-r', '{}'.format(sha) On 2016/11/01 at 22:07:44, foolip wrote: > If sha is already a string, which I think it is, then just sha, otherwise str(sha) would be a more explicit stringifier method. I don't think sha could ever be anything other than a string, so I think it's ok here? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:31: files = [f.strip() for f in filter(bool, diff_files.split('\n'))] On 2016/11/01 at 22:07:44, foolip wrote: > Using splitlines() also handled \r\n if it happens to be in there. With that, could you drop the strip bit? Using splitlines() throughout probably makes sense if so. > > After testing manually I understand that filter(bool, x) is to strip out empty string, but that also seems to be solved by using splitlines(). Ah thank you, that's much cleaner. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:59: '{}'.format(sha), self.absolute_chromium_wpt_dir On 2016/11/01 at 22:07:44, foolip wrote: > I think this will actually need to use some of the code in has_changes_in_wpt to enumerate affected files. Once there's an exportable commit, using that for a unit test would be great too. A TODO to prepend "Chromium: " if you think that's a reasonable format too. Passing self.absolute_chromium_wpt_dir limits the format-patch output to only files in imported/wpt (if I understand you correctly). Re: prepending Chromium: I had assumed we'd just re-use the same footer (Cr-Commit-Position: refs/heads/master@{#428681}) in the WPT commits. Does that work? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:21: self.host.print_('## Stop trying to make fetch happen, it\'s not going to happen') On 2016/11/01 at 22:07:44, foolip wrote: > When will this message be seen? :) I was using this flag to avoid fetching the repo every time I tested the script. I'll change the message to something a little less flippant. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:31: self.host.print_('## Need to set up remote "github"!') On 2016/11/01 at 22:07:45, foolip wrote: > Does this need to be done manually? If so, should the script exit at this point? Yes but I'm not entirely sure how that's going to work at the moment. Right now I'm using my own personal fork at jeffcarp/web-platform-tests. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:52: match = re.search(r'Cr-Commit-Position: (?P<cr_commit>.+)$', message, re.MULTILINE) On 2016/11/01 at 22:07:44, foolip wrote: > Throw in a ^ so that it only matches at the beginning of the line? Revert commits say things like this before the real Cr-Commit-Position: > > Cr-Commit-Position: refs/heads/master@{#428769} > > Also, should this extract out just the number, or does that turn out to not be necessary yet? Even better, I found `git footers --position [sha]` will give you the value of Cr-Commit-Position. https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/d... I don't think we need to parse out the number. In my testing, git accepts revs of the format `refs/heads/master@{#428769}`. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py:90: self.assertEqual(len(host.executive.calls), 8) On 2016/11/01 at 22:07:45, foolip wrote: > Should this assert anything about what the calls are? The method was still in flux so I didn't write any assertions that were too specific. I'll add a note to add those. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:65: host.print_('## Upload failed with error:\n{}'.format(exp)) On 2016/11/01 at 22:07:45, foolip wrote: > Should it rethrow the exception, or what will happen if it tries the next commit? I was imagining that even if one patch couldn't apply cleanly, others would still be able to. However, maybe for the sake of not creating complex error states it should just bail out immediately.
https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/html/semantics/scripting-1/the-template-element/template-element/node-document-changes.html (left): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/html/semantics/scripting-1/the-template-element/template-element/node-document-changes.html:17: On 2016/11/01 22:24:42, jeffcarp wrote: > On 2016/11/01 at 22:07:44, foolip wrote: > > Good changes to test this with, but do you intend to land them together with > the script? In order to experiment with the process, I would rather think that > you'd first land these scripts, then do a separate CL with whitespace changes > and do a dry run to get the bots green. Then, when in the office, land it and > run the export scripts as soon as possible after landing. Make sense? > > This whitespace change made it into WPT master. Next time the WPT are imported > into Chromium this diff should go away. Oh: https://github.com/w3c/web-platform-tests/commit/96f559b2ee6a6c7133d1de6d5715... That has a Cr-Commit-Position that doesn't have the corresponding change, does the script notice this and exit requiring human intervention? This was obviously deliberate, but this seems like just the thing that might happen by mistake and cause changes to be lost. Do you want to keep this to test the "same changes made on both sides" case? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:10: class ChromiumWPT(object): On 2016/11/01 23:21:18, jeffcarp wrote: > On 2016/11/01 at 22:07:44, foolip wrote: > > A sentence or two of documentation describing the responsibilities of this > class would be great. And the "(object)" probably isn't needed? > > The linter yells at me if classes don't inherit from object. OK, I had no idea there was a new style in town :) https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:32: return any([f.startswith(CR_WPT_DIR) and '-expected' not in f for f in files]) On 2016/11/01 22:07:44, foolip wrote: > In deps_updater.py there's an is_baseline. Using a shared definition would be > great. Add a TODO for this? https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:59: '{}'.format(sha), self.absolute_chromium_wpt_dir On 2016/11/01 23:21:18, jeffcarp wrote: > On 2016/11/01 at 22:07:44, foolip wrote: > > I think this will actually need to use some of the code in has_changes_in_wpt > to enumerate affected files. Once there's an exportable commit, using that for a > unit test would be great too. A TODO to prepend "Chromium: " if you think that's > a reasonable format too. > > Passing self.absolute_chromium_wpt_dir limits the format-patch output to only > files in imported/wpt (if I understand you correctly). It'll still include changes to expectations files though. > Re: prepending Chromium: I had assumed we'd just re-use the same footer > (Cr-Commit-Position: refs/heads/master@{#428681}) in the WPT commits. Does that > work? Sure, let's start without the "Chromium: " prefix. The purpose was to make nonsensical commit messages like "Fix crash in HTMLElement::internalThingy" slightly less nonsensical with additional context. But Gecko already exports commits without any prefix. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:21: self.host.print_('## Stop trying to make fetch happen, it\'s not going to happen') On 2016/11/01 23:21:18, jeffcarp wrote: > On 2016/11/01 at 22:07:44, foolip wrote: > > When will this message be seen? :) > > I was using this flag to avoid fetching the repo every time I tested the script. > I'll change the message to something a little less flippant. :) https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:31: self.host.print_('## Need to set up remote "github"!') On 2016/11/01 23:21:18, jeffcarp wrote: > On 2016/11/01 at 22:07:45, foolip wrote: > > Does this need to be done manually? If so, should the script exit at this > point? > > Yes but I'm not entirely sure how that's going to work at the moment. Right now > I'm using my own personal fork at jeffcarp/web-platform-tests. TODO and early exit then? The script doesn't need to really work when first landed :) https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:52: match = re.search(r'Cr-Commit-Position: (?P<cr_commit>.+)$', message, re.MULTILINE) On 2016/11/01 23:21:18, jeffcarp wrote: > On 2016/11/01 at 22:07:44, foolip wrote: > > Throw in a ^ so that it only matches at the beginning of the line? Revert > commits say things like this before the real Cr-Commit-Position: > > > Cr-Commit-Position: refs/heads/master@{#428769} > > > > Also, should this extract out just the number, or does that turn out to not be > necessary yet? > > Even better, I found `git footers --position [sha]` will give you the value of > Cr-Commit-Position. > > https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/d... > > I don't think we need to parse out the number. In my testing, git accepts revs > of the format `refs/heads/master@{#428769}`. Note that this only works if there's a local master branch, and in my testing, it doesn't actually get the same commit that has `refs/heads/master@{#428769}` as a footer. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:73: self.run(['git', 'push', 'github', ':%s' % branch_name]) On 2016/11/01 22:07:44, foolip wrote: > This might mess things up if there's an open pull request for it. When should we > expect this will happen? In the beginning, maybe err on the side of failing and > requiring manual intervention when unexpected things happen? TODO for this and the following comments in this file. (It's hard to keep track of comments in a long review, so err on the side of delaying work to make the review shorter.) https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync.py:65: host.print_('## Upload failed with error:\n{}'.format(exp)) On 2016/11/01 23:21:18, jeffcarp wrote: > On 2016/11/01 at 22:07:45, foolip wrote: > > Should it rethrow the exception, or what will happen if it tries the next > commit? > > I was imagining that even if one patch couldn't apply cleanly, others would > still be able to. However, maybe for the sake of not creating complex error > states it should just bail out immediately. Right, if we don't always apply patches in order, then it becomes more complicated to figure out just from the state of the trees what should be exported next. https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py (right): https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:18: return '\n'.join([ Did some lint script complain about """ without a blank line? https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:33: return """ These and the following in the same style as the first, whatever it turns out to be. https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:51: -expected.html Copypasta, should have been like above but with .txt? https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:45: position = self.run(['git', 'footers', '--position', sha]) Sweet! https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:46: assert position Put the assert after the strip, because I guess we don't expect a non-empty string of whitespace? https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py (right): https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py:86: # TODO: Add more specific assertions Should be TODO(jeffcarp). It doesn't mean that you're going to fix it, just that you added the TODO and know what it means.
https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py (right): https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:18: return '\n'.join([ On 2016/11/02 at 13:15:32, foolip wrote: > Did some lint script complain about """ without a blank line? No, it was has_changes_in_wpt that couldn't handle it. I changed it to be this way due to your earlier comments about not needing to test first blank lines explicitly (maybe I misunderstood). Either way, I'm about to change this to: return ("foo\n" "bar\n") Which seems cleaner and more pythonic https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:51: -expected.html On 2016/11/02 at 13:15:32, foolip wrote: > Copypasta, should have been like above but with .txt? Oops thanks for catching this! https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:46: assert position On 2016/11/02 at 13:15:33, foolip wrote: > Put the assert after the strip, because I guess we don't expect a non-empty string of whitespace? The reason it was before strip was to catch the case if .strip() were to be called on None, but that makes more sense, and the former error will serve the same purpose as an assert.
On 2016/11/03 at 22:33:00, jeffcarp wrote: > https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py (right): > > https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:18: return '\n'.join([ > On 2016/11/02 at 13:15:32, foolip wrote: > > Did some lint script complain about """ without a blank line? > > No, it was has_changes_in_wpt that couldn't handle it. I changed it to be this way due to your earlier comments about not needing to test first blank lines explicitly (maybe I misunderstood). Either way, I'm about to change this to: > > return ("foo\n" > "bar\n") > > Which seems cleaner and more pythonic > > https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:51: -expected.html > On 2016/11/02 at 13:15:32, foolip wrote: > > Copypasta, should have been like above but with .txt? > > Oops thanks for catching this! > > https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): > > https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:46: assert position > On 2016/11/02 at 13:15:33, foolip wrote: > > Put the assert after the strip, because I guess we don't expect a non-empty string of whitespace? > > The reason it was before strip was to catch the case if .strip() were to be called on None, but that makes more sense, and the former error will serve the same purpose as an assert. Ok I believe I addressed all comments with either fixes or TODOs.
lgtm
https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py (right): https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:33: return """ On 2016/11/02 13:15:33, foolip wrote: > These and the following in the same style as the first, whatever it turns out to > be. Wait, now missing newlines? (On phone, can't seem to click to write new issue.)
https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/html/semantics/scripting-1/the-template-element/template-element/node-document-changes.html (left): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/html/semantics/scripting-1/the-template-element/template-element/node-document-changes.html:17: On 2016/11/02 at 13:15:32, foolip wrote: > On 2016/11/01 22:24:42, jeffcarp wrote: > > On 2016/11/01 at 22:07:44, foolip wrote: > > > Good changes to test this with, but do you intend to land them together with > > the script? In order to experiment with the process, I would rather think that > > you'd first land these scripts, then do a separate CL with whitespace changes > > and do a dry run to get the bots green. Then, when in the office, land it and > > run the export scripts as soon as possible after landing. Make sense? > > > > This whitespace change made it into WPT master. Next time the WPT are imported > > into Chromium this diff should go away. > > Oh: https://github.com/w3c/web-platform-tests/commit/96f559b2ee6a6c7133d1de6d5715... > > That has a Cr-Commit-Position that doesn't have the corresponding change, does the script notice this and exit requiring human intervention? This was obviously deliberate, but this seems like just the thing that might happen by mistake and cause changes to be lost. > > Do you want to keep this to test the "same changes made on both sides" case? Sure, I'll keep this in mind. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:5: import unittest On 2016/11/01 at 22:57:45, qyearsley wrote: > Nit: Blank line separating import sections It seems like in the style guide that there's no blank line? https://google.github.io/styleguide/pyguide.html?showone=Imports_formatting#I... https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:24: self.host.print_('## WPT checkout exists at %s, fetching latest' % (self.path)) On 2016/11/01 at 22:57:45, qyearsley wrote: > Note, I think that rather than printing messages like this (like what deps_updater.py does right now), it's actually much nicer to use logs. > > Most modules in webkitpy have a global logger _log = logging.getLogger(__name__), and then printing of messages is done with log methods (debug, info, warning, error). Logging config can be set in a main method. > > This makes it easy to add a verbose mode, or to change how messages are printed. I'd like to change deps_updater.py to do this in the future. Changing everything to logging. It feels weird though switching from host.print_ which was inherited to _log which is module-level. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:27: self.run(['git', 'merge', '--ff-only', 'origin/master']) On 2016/11/01 at 22:57:45, qyearsley wrote: > Possible optional change: We could also make the constructor more of a no-op, and actually run these git commands in a separate setup method. What do you think? That works. Since all the class is used for is interacting with a local WPT checkout, I thought it might be redundant, but it's also not expected behavior to do a bunch of work in __init__. https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:73: self.run(['git', 'push', 'github', ':%s' % branch_name]) On 2016/11/02 at 13:15:32, foolip wrote: > On 2016/11/01 22:07:44, foolip wrote: > > This might mess things up if there's an open pull request for it. When should we > > expect this will happen? In the beginning, maybe err on the side of failing and > > requiring manual intervention when unexpected things happen? > > TODO for this and the following comments in this file. (It's hard to keep track of comments in a long review, so err on the side of delaying work to make the review shorter.) I think it should fail if the remote branch already exists. Adding a TODO to investigate. https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py (right): https://codereview.chromium.org/2439153002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:33: return """ On 2016/11/03 at 23:40:31, foolip wrote: > On 2016/11/02 13:15:33, foolip wrote: > > These and the following in the same style as the first, whatever it turns out to > > be. > > Wait, now missing newlines? (On phone, can't seem to click to write new issue.) Yes, sorry maybe I'm confused. I thought you said it was okay to remove the newlines from the test and just assert in the code.
https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py (right): https://codereview.chromium.org/2439153002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:5: import unittest On 2016/11/03 at 23:54:07, jeffcarp wrote: > On 2016/11/01 at 22:57:45, qyearsley wrote: > > Nit: Blank line separating import sections > > It seems like in the style guide that there's no blank line? https://google.github.io/styleguide/pyguide.html?showone=Imports_formatting#I... Yep, the blank line optional -- although most files in webkitpy usually have a blank line between the general library imports and webkitpy imports. https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py (right): https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:5: CR_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/' Just so it's explicit and clear for future readers, we could avoid use the full word Chromium rather than Cr in names -- e.g. CR_WPT_DIR -> CHROMIUM_WPT_DIR cr_commits -> chromium_commits cr_commits_since -> chromium_commits_since https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:19: def exportable_commits_since(self, sha): `chromium_commit` might be clearer than `sha`, since this would express that it's expected to be a commit in the Chromium repo. https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:33: assert sha Just a random note: I think that this use of assert is good :-) I think I don't normally see/use as many asserts as I could. https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:40: return any([f.startswith(CR_WPT_DIR) and '-expected' not in f for f in files]) Python any() and all() can take any iterable including generator expressions, so in expressions like this, the square brackets are optional; this can also be: return any(f.startswith(CR_WPT_DIR) and '-expected' not in f for f in files) https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:58: """This returns both the subject and body of a given revision.""" Usually docstrings start with just the verb in third-person singular, e.g. """Returns a string with the subject and body for a given commit.""" https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:64: """Get patch but only for files in LayoutTests/imported/wpt""" Likewise: """Makes a patch with just changes in files in the WPT directory in a given commit.""" https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py (right): https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:22: host.filesystem = MockFileSystem() In these three tests, I would expect that they work the same without explicitly making a new MockFileSystem since the one created in the MockHost() constructor should also be a new empty MockFileSystem. https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt_unittest.py:23: local_wpt = ChromiumWPT(host) I think I'd expect this variable be called chromium_wpt (same below). https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:16: def __init__(self, host, path=WPT_TMP_DIR, no_fetch=False, gh=False): A docstring or different argument names might make it clearer what should be passed in for `path` and `gh`. For gh, would it make sense to call it use_github? https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:33: raise Exception('Need to set up remote "github"') Possible minor restructuring: if you add a return after `_log.info('Skipping remote WPT fetch')` then you can get rid of the else branch and un-indent. https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:40: return self.run(command, **kwargs).splitlines() Would it be simpler just to add a .splitlines() in all-branches and remove this method? https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync_wpt.py (right): https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync_wpt.py:83: assert user and token In this kind of situation where a variable is is gotten from the environment/user and not passed into the function, I think I would argue that an explicit error message might be nicer, e.g. if not (user and token): _log.error('GH_USER and/or GH_TOKEN are not set.') https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/sync_wpt.py:103: print content Could use _log.info rather than print here.
https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py (right): https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:5: CR_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/' On 2016/11/04 at 17:40:32, qyearsley wrote: > Just so it's explicit and clear for future readers, we could avoid use the full word Chromium rather than Cr in names -- e.g. > > CR_WPT_DIR -> CHROMIUM_WPT_DIR > cr_commits -> chromium_commits > cr_commits_since -> chromium_commits_since Scratch out the word "avoid" in this comment...
On 2016/11/04 at 17:42:04, qyearsley wrote: > https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py (right): > > https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:5: CR_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/' > On 2016/11/04 at 17:40:32, qyearsley wrote: > > Just so it's explicit and clear for future readers, we could avoid use the full word Chromium rather than Cr in names -- e.g. > > > > CR_WPT_DIR -> CHROMIUM_WPT_DIR > > cr_commits -> chromium_commits > > cr_commits_since -> chromium_commits_since > > Scratch out the word "avoid" in this comment... Ok I addressed your CL comments and updated the spacing & comment positioning to more closely match the other files (e.g. class documentation at the top before imports)
On 2016/11/04 at 22:25:20, jeffcarp wrote: > On 2016/11/04 at 17:42:04, qyearsley wrote: > > https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... > > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py (right): > > > > https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... > > third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:5: CR_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/' > > On 2016/11/04 at 17:40:32, qyearsley wrote: > > > Just so it's explicit and clear for future readers, we could avoid use the full word Chromium rather than Cr in names -- e.g. > > > > > > CR_WPT_DIR -> CHROMIUM_WPT_DIR > > > cr_commits -> chromium_commits > > > cr_commits_since -> chromium_commits_since > > > > Scratch out the word "avoid" in this comment... > > Ok I addressed your CL comments and updated the spacing & comment positioning to more closely match the other files (e.g. class documentation at the top before imports) LGTM :-) I think that class documentation could also go at the top of the class rather than at the top before imports, but I think either way is good - either way, it helps someone who's looking through the files who wants to know "what's this file for?"
The CQ bit was checked by jeffcarp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2439153002/#ps340001 (title: "Fix spacing issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/04 at 23:03:12, qyearsley wrote: > On 2016/11/04 at 22:25:20, jeffcarp wrote: > > On 2016/11/04 at 17:42:04, qyearsley wrote: > > > https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... > > > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py (right): > > > > > > https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... > > > third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:5: CR_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/' > > > On 2016/11/04 at 17:40:32, qyearsley wrote: > > > > Just so it's explicit and clear for future readers, we could avoid use the full word Chromium rather than Cr in names -- e.g. > > > > > > > > CR_WPT_DIR -> CHROMIUM_WPT_DIR > > > > cr_commits -> chromium_commits > > > > cr_commits_since -> chromium_commits_since > > > > > > Scratch out the word "avoid" in this comment... > > > > Ok I addressed your CL comments and updated the spacing & comment positioning to more closely match the other files (e.g. class documentation at the top before imports) > > LGTM :-) > > I think that class documentation could also go at the top of the class rather than at the top before imports, but I think either way is good - either way, it helps someone who's looking through the files who wants to know "what's this file for?" Sent to the CQ. On the topic of using DepsUpdater.is_baseline - that function has a bunch of extra arguments that it doesn't use since it's being passed to file_filter. What do you think the best way is to get around that? Just pass None, None for the unused args when calling it from ChromiumWPT? Another option is to define a common base function that both use. Or could you use lambdas? instead of: file_filter=self.is_not_baseline file_filter=self.is_baseline We could do: file_filter=lambda _, _, _, f: not self.is_baseline(f) file_filter=lambda _, _, _, f: self.is_baseline(f) def is_baseline(self, file): return file.endswith('-expected.txt')
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/11/04 23:15:47, jeffcarp wrote: > On 2016/11/04 at 23:03:12, qyearsley wrote: > > On 2016/11/04 at 22:25:20, jeffcarp wrote: > > > On 2016/11/04 at 17:42:04, qyearsley wrote: > > > > > https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... > > > > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py > (right): > > > > > > > > > https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... > > > > third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:5: > CR_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/' > > > > On 2016/11/04 at 17:40:32, qyearsley wrote: > > > > > Just so it's explicit and clear for future readers, we could avoid use > the full word Chromium rather than Cr in names -- e.g. > > > > > > > > > > CR_WPT_DIR -> CHROMIUM_WPT_DIR > > > > > cr_commits -> chromium_commits > > > > > cr_commits_since -> chromium_commits_since > > > > > > > > Scratch out the word "avoid" in this comment... > > > > > > Ok I addressed your CL comments and updated the spacing & comment > positioning to more closely match the other files (e.g. class documentation at > the top before imports) > > > > LGTM :-) > > > > I think that class documentation could also go at the top of the class rather > than at the top before imports, but I think either way is good - either way, it > helps someone who's looking through the files who wants to know "what's this > file for?" > > Sent to the CQ. On the topic of using DepsUpdater.is_baseline - that function > has a bunch of extra arguments that it doesn't use since it's being passed to > file_filter. What do you think the best way is to get around that? Just pass > None, None for the unused args when calling it from ChromiumWPT? Another option > is to define a common base function that both use. Or could you use lambdas? > instead of: > > file_filter=self.is_not_baseline > file_filter=self.is_baseline > > We could do: > > file_filter=lambda _, _, _, f: not self.is_baseline(f) > file_filter=lambda _, _, _, f: self.is_baseline(f) > > def is_baseline(self, file): > return file.endswith('-expected.txt') Quinten probably has ideas, but maybe the importer should also use both ChromiumWPT and LocalWPT? At least some shared code beyond is_baseline will be needed to bail out when there are exportable commits not yet exported.
We'll pair this week and see what logic we can merge together. On Sat, Nov 5, 2016 at 3:10 PM <foolip@chromium.org> wrote: > On 2016/11/04 23:15:47, jeffcarp wrote: > > On 2016/11/04 at 23:03:12, qyearsley wrote: > > > On 2016/11/04 at 22:25:20, jeffcarp wrote: > > > > On 2016/11/04 at 17:42:04, qyearsley wrote: > > > > > > > > > https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... > > > > > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... > > > > > third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:5: > > CR_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/' > > > > > On 2016/11/04 at 17:40:32, qyearsley wrote: > > > > > > Just so it's explicit and clear for future readers, we could > avoid use > > the full word Chromium rather than Cr in names -- e.g. > > > > > > > > > > > > CR_WPT_DIR -> CHROMIUM_WPT_DIR > > > > > > cr_commits -> chromium_commits > > > > > > cr_commits_since -> chromium_commits_since > > > > > > > > > > Scratch out the word "avoid" in this comment... > > > > > > > > Ok I addressed your CL comments and updated the spacing & comment > > positioning to more closely match the other files (e.g. class > documentation at > > the top before imports) > > > > > > LGTM :-) > > > > > > I think that class documentation could also go at the top of the class > rather > > than at the top before imports, but I think either way is good - either > way, > it > > helps someone who's looking through the files who wants to know "what's > this > > file for?" > > > > Sent to the CQ. On the topic of using DepsUpdater.is_baseline - that > function > > has a bunch of extra arguments that it doesn't use since it's being > passed to > > file_filter. What do you think the best way is to get around that? Just > pass > > None, None for the unused args when calling it from ChromiumWPT? Another > option > > is to define a common base function that both use. Or could you use > lambdas? > > instead of: > > > > file_filter=self.is_not_baseline > > file_filter=self.is_baseline > > > > We could do: > > > > file_filter=lambda _, _, _, f: not self.is_baseline(f) > > file_filter=lambda _, _, _, f: self.is_baseline(f) > > > > def is_baseline(self, file): > > return file.endswith('-expected.txt') > > Quinten probably has ideas, but maybe the importer should also use both > ChromiumWPT and LocalWPT? At least some shared code beyond is_baseline > will be > needed to bail out when there are exportable commits not yet exported. > > https://codereview.chromium.org/2439153002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
We'll pair this week and see what logic we can merge together. On Sat, Nov 5, 2016 at 3:10 PM <foolip@chromium.org> wrote: > On 2016/11/04 23:15:47, jeffcarp wrote: > > On 2016/11/04 at 23:03:12, qyearsley wrote: > > > On 2016/11/04 at 22:25:20, jeffcarp wrote: > > > > On 2016/11/04 at 17:42:04, qyearsley wrote: > > > > > > > > > https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... > > > > > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2439153002/diff/260001/third_party/WebKit/Too... > > > > > third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py:5: > > CR_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/' > > > > > On 2016/11/04 at 17:40:32, qyearsley wrote: > > > > > > Just so it's explicit and clear for future readers, we could > avoid use > > the full word Chromium rather than Cr in names -- e.g. > > > > > > > > > > > > CR_WPT_DIR -> CHROMIUM_WPT_DIR > > > > > > cr_commits -> chromium_commits > > > > > > cr_commits_since -> chromium_commits_since > > > > > > > > > > Scratch out the word "avoid" in this comment... > > > > > > > > Ok I addressed your CL comments and updated the spacing & comment > > positioning to more closely match the other files (e.g. class > documentation at > > the top before imports) > > > > > > LGTM :-) > > > > > > I think that class documentation could also go at the top of the class > rather > > than at the top before imports, but I think either way is good - either > way, > it > > helps someone who's looking through the files who wants to know "what's > this > > file for?" > > > > Sent to the CQ. On the topic of using DepsUpdater.is_baseline - that > function > > has a bunch of extra arguments that it doesn't use since it's being > passed to > > file_filter. What do you think the best way is to get around that? Just > pass > > None, None for the unused args when calling it from ChromiumWPT? Another > option > > is to define a common base function that both use. Or could you use > lambdas? > > instead of: > > > > file_filter=self.is_not_baseline > > file_filter=self.is_baseline > > > > We could do: > > > > file_filter=lambda _, _, _, f: not self.is_baseline(f) > > file_filter=lambda _, _, _, f: self.is_baseline(f) > > > > def is_baseline(self, file): > > return file.endswith('-expected.txt') > > Quinten probably has ideas, but maybe the importer should also use both > ChromiumWPT and LocalWPT? At least some shared code beyond is_baseline > will be > needed to bail out when there are exportable commits not yet exported. > > https://codereview.chromium.org/2439153002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by jeffcarp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #19 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== This CL adds a script, sync-w3c-tests, that will eventually be a script that syncs our local WPT with the upstream GitHub repo in one atomic operation. This CL adds the export side of that script. BUG=657117 ========== to ========== This CL adds a script, sync-w3c-tests, that will eventually be a script that syncs our local WPT with the upstream GitHub repo in one atomic operation. This CL adds the export side of that script. BUG=657117 Committed: https://crrev.com/826ed2a741422fa8007dce9bd006da6652cad69e Cr-Commit-Position: refs/heads/master@{#430425} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/826ed2a741422fa8007dce9bd006da6652cad69e Cr-Commit-Position: refs/heads/master@{#430425}
Message was sent while issue was closed.
Just as an FYI, it looks like you edited the description and dropped the "Script for exporting WPT" line. The resulting commit doesn't have the title line at all. (A common accident, I've done it too.)
Message was sent while issue was closed.
On 2016/11/08 at 09:09:08, foolip wrote: > Just as an FYI, it looks like you edited the description and dropped the "Script for exporting WPT" line. The resulting commit doesn't have the title line at all. (A common accident, I've done it too.) Oops thanks for pointing that out, I didn't realize the first line was used that way. |
