Converted Android Chromedriver buildbot scripts to recipes.

Converted Android Chromedriver buildbot scripts to recipes.

Initially adding only Android Chromedriver buildbot code
to keep the CL size down.

recipe_modules/chromedriver/api.py contains many of the
same functions (converted to recipes) from...
src/chrome/test/chromedriver/test/run_all_tests.py and
src/chrome/test/chromedriver/run_buildbot_steps.py

recipes/chromedriver.py is the android
specific parts of the main() function from run_buildbot_steps.py
plus some installation and device checking recipe steps.

BUG=

Committed: https://chromium.googlesource.com/chromium/tools/build/+/042328a7f55e8ea0373ff2fe4b366a268679ad89

[mikecase (-- gone --), 2015-08-05 23:31:47]

mikecase@chromium.org changed reviewers:
+ luqui@chromium.org

[mikecase (-- gone --), 2015-08-05 23:31:47]

Adding recipe_modules/chromedriver/api.py which is essentially the converted
chromedriver buildbot scripts and adding a recipe chromedriver_android.py to be
used on "ChromeDriver Android (dbg)" bot on the chromium.fyi waterfall

Will add more recipes in follow-up CL for other chromedriver bots.

[luqui, 2015-08-06 00:57:37]

I'm about half done with this review and my brain is out of juice.  I'll finish
up tomorrow. 

This is really good code, btw, thanks for your care. :-)

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromedriver/api.py (right):

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:16: GS_CHROMEDRIVER_BUCKET =
'chromedriver'
Be aware that such things have a way of migrating into configs.  I think here is
good for now, as long as we aren't depending on it in a way that is inaccessible
from configs.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:54: def get_build_dir(self,
required_paths):
Is it possible to compute what this ought to be instead of guessing & checking? 
Under what conditions do you expect each, and is that encodable with other info?

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:111:
pattern=self.m.path['tmp_base'].join('chromedriver_*', '*'),
How do we know the logs will be put in tmp_base?  

If it's suitably clean, consider passing the log dir to this function and the
other function that writes to it, wherever that is instead of assuming
implicitly.   You could even use a temp dir instead of tmp_base so that you
don't have to sift through other junk to find what you need.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:146: return dict([(int(v[0]),
v[1]) for v in json_dict.items()])
return { int(k): v for k, v in json_dict.items() }

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:164: """Updates the test
results log for the given platform.
This description is a bit unclear, not sure what you mean by update.  Looks like
it uploads to GS.  Mention something about GS in the description.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:173: if len(log) > 500: #
pragma: no cover
I know we're only adding one at a time so it should only go at most once, but if
you change this 'if' to 'while' your intent to truncate is clearer.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:175: assert commit_position not
in log, \
Should this really be a failure, or just a skip & warn?

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:202: chrome_min_version =
chrome_min_version_line[0].split('{')[1].split(',')[0]
Use a regex instead of 'in' and 'split'.  It'll be way more obvious.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:205: version_file_data =
self.m.file.read(
BTW we have support for nested steps now if this gets to be a bit visually
overwhelming.  See recipes/example/nested.py

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:257: pkg =
self.m.zip.make_package(root=chromedriver_dir, output=output_zip_path)
Separate these three lines by whitespace since they're really only doing one
thing.  Or factor out into a method (this pattern is used three times in the
source).   zip_one_file or smth

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromedriver/archive.py (right):

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/archive.py:19: self.api = api
Glancing through this, it appears that you only ever use api.m; i.e. you don't
use any methods of ChromedriverApi, only the recipe modules it depends on.  I
suggest passing that m directly instead of the whole ChromedriverApi object to
weaken coupling.  And then calling it self.m instead of self.api so it looks
very much like a RecipeApi.

[luqui, 2015-08-06 20:52:59]

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromedriver/api.py (right):

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:293: assert
chromedriver_platform != 'android'
Why? (answer in comment)

I'm confused, since below you are talking about getting android test results and
stuff.  I just don't understand the relationship of android to linux releases I
guess

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:469: def
wait_for_latest_snapshot(self, commit_position):
This will tie up a builder just sleeping.  How long do you expect to wait here?

If this isn't just a race-condition-breaking sort of sleep, I would recommend
cron scheduling this builder and having it abort the build if the snapshot is
not available.  If it is a race-condition-breaker, consider what happens in the
case of an error where the snapshot did not get uploaded.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:487: 'C:\\Program Files
(x86)\\Java\\ant\\bin',
Ack hardcoded paths!

I do not have a suggestion here.  Eventually we'll isolate these maybe?

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromedriver/example.py (right):

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/example.py:23: 'Android ChromeDriver
Tests (dbg)': {
This looks like a real recipe that you're planning to run.  I'd move this into
scripts/slave/recipes rather than example.py, which might be misleading. 
Eventually we are planning to ship recipes with modules, but not yet.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/example.py:39: REPO_URL =
'svn://svn-mirror.golo.chromium.org/chrome/trunk/src'
Aren't we on git now?

[samuong, 2015-08-06 21:58:38]

samuong@chromium.org changed reviewers:
+ samuong@chromium.org

[samuong, 2015-08-06 21:58:39]

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromedriver/api.py (right):

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:293: assert
chromedriver_platform != 'android'
On 2015/08/06 20:52:58, luqui wrote:
> Why? (answer in comment)
> 
> I'm confused, since below you are talking about getting android test results
and
> stuff.  I just don't understand the relationship of android to linux releases
I
> guess

ChromeDriver doesn't run on Android. It runs on a desktop, and via adb, it
drives a browser session that runs on the Android device. The Android bot is a
Linux machine with a Nexus device attached, and we use it to gate the release.
If the desktop tests pass, but the Android ones don't, we don't release.

The desktop bots are responsible for actually pushing out a release. So they
need to check if the Android test results are good, and if so, do a release. The
Android bot never does a release itself - it just uploads its test results to
GS, and lets the desktop bots handle the rest.

So the assertion makes sense; this function should never run on the Android bot.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:487: 'C:\\Program Files
(x86)\\Java\\ant\\bin',
On 2015/08/06 20:52:58, luqui wrote:
> Ack hardcoded paths!
> 
> I do not have a suggestion here.  Eventually we'll isolate these maybe?

I'd love to get rid of this too, but we'd need to somehow get ant/java installed
on our Windows bot first. I'm not sure why it wasn't done in the first place (I
wasn't around when it was set up) but what do we need to do to get this set up?
Right now we have to rdp into the bot and install it manually...

[samuong, 2015-08-28 00:14:47]

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromedriver/chromedriver_android.py (right):

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipes/c...
scripts/slave/recipes/chromedriver/chromedriver_android.py:37: 'package':
'org.chromium.chrome.shell',
Don't forget we need to replace this with ChromePublic.

[mikecase (-- gone --), 2016-04-04 21:46:39]

mikecase@chromium.org changed reviewers:
+ bpastene@chromium.org, jbudorick@chromium.org
- luqui@chromium.org

[mikecase (-- gone --), 2016-04-04 21:46:42]

Updated CL to have this replace the current auto-gen recipe.

I compared the android chromedriver expectation file to what the bot is
currently doing...
https://build.chromium.org/p/chromium.fyi/builders/Android%20ChromeDriver%20T...
...and it looks good as far as I can tell.

Made sure to update this CL based on recent changes to the
chrome/test/chromedriver code.

PTAL

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromedriver/api.py (right):

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:16: GS_CHROMEDRIVER_BUCKET =
'chromedriver'
On 2015/08/06 at 00:57:37, luqui wrote:
> Be aware that such things have a way of migrating into configs.  I think here
is good for now, as long as we aren't depending on it in a way that is
inaccessible from configs.

Ack

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:54: def get_build_dir(self,
required_paths):
On 2015/08/06 at 00:57:37, luqui wrote:
> Is it possible to compute what this ought to be instead of guessing &
checking?  Under what conditions do you expect each, and is that encodable with
other info?

I'm honestly not too familiar with this code (I'm just converting the buildbot
scripts) but this function seems unnecessary. I'm just going to get rid of it
and use api.chromium.output_dir to get the output build_dir. I expect this to
work, but if it doesn't I can always switch it back.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:111:
pattern=self.m.path['tmp_base'].join('chromedriver_*', '*'),
On 2015/08/06 at 00:57:36, luqui wrote:
> How do we know the logs will be put in tmp_base?  
> 
> If it's suitably clean, consider passing the log dir to this function and the
other function that writes to it, wherever that is instead of assuming
implicitly.   You could even use a temp dir instead of tmp_base so that you
don't have to sift through other junk to find what you need.

Done. Made a chromedriver_log_dir to store the logs.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:146: return dict([(int(v[0]),
v[1]) for v in json_dict.items()])
On 2015/08/06 at 00:57:37, luqui wrote:
> return { int(k): v for k, v in json_dict.items() }

Done

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:164: """Updates the test
results log for the given platform.
On 2015/08/06 at 00:57:37, luqui wrote:
> This description is a bit unclear, not sure what you mean by update.  Looks
like it uploads to GS.  Mention something about GS in the description.

Done, mentioned the log is stored in GS.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:173: if len(log) > 500: #
pragma: no cover
On 2015/08/06 at 00:57:36, luqui wrote:
> I know we're only adding one at a time so it should only go at most once, but
if you change this 'if' to 'while' your intent to truncate is clearer.

Agree, done.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:175: assert commit_position not
in log, \
On 2015/08/06 at 00:57:36, luqui wrote:
> Should this really be a failure, or just a skip & warn?

Changed to skip and warn.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:202: chrome_min_version =
chrome_min_version_line[0].split('{')[1].split(',')[0]
On 2015/08/06 at 00:57:37, luqui wrote:
> Use a regex instead of 'in' and 'split'.  It'll be way more obvious.

Switching to regex.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:205: version_file_data =
self.m.file.read(
On 2015/08/06 at 00:57:36, luqui wrote:
> BTW we have support for nested steps now if this gets to be a bit visually
overwhelming.  See recipes/example/nested.py

Done. Trying out the nested steps.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:257: pkg =
self.m.zip.make_package(root=chromedriver_dir, output=output_zip_path)
On 2015/08/06 at 00:57:37, luqui wrote:
> Separate these three lines by whitespace since they're really only doing one
thing.  Or factor out into a method (this pattern is used three times in the
source).   zip_one_file or smth

Done. Separated out these 3 lines w/ whitespace.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/api.py:469: def
wait_for_latest_snapshot(self, commit_position):
On 2015/08/06 at 20:52:58, luqui wrote:
> This will tie up a builder just sleeping.  How long do you expect to wait
here?
> 
> If this isn't just a race-condition-breaking sort of sleep, I would recommend
cron scheduling this builder and having it abort the build if the snapshot is
not available.  If it is a race-condition-breaker, consider what happens in the
case of an error where the snapshot did not get uploaded.

Yeah, I think that is a good idea. I'll look to see if I can find what exactly
is building the snapshots this bot needs, and trigger it from there.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromedriver/archive.py (right):

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/archive.py:19: self.api = api
On 2015/08/06 at 00:57:37, luqui wrote:
> Glancing through this, it appears that you only ever use api.m; i.e. you don't
use any methods of ChromedriverApi, only the recipe modules it depends on.  I
suggest passing that m directly instead of the whole ChromedriverApi object to
weaken coupling.  And then calling it self.m instead of self.api so it looks
very much like a RecipeApi.

Done

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromedriver/example.py (right):

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/example.py:23: 'Android ChromeDriver
Tests (dbg)': {
On 2015/08/06 at 20:52:58, luqui wrote:
> This looks like a real recipe that you're planning to run.  I'd move this into
scripts/slave/recipes rather than example.py, which might be misleading. 
Eventually we are planning to ship recipes with modules, but not yet.

Changed this example to look less like an actual recipe. Using this example.py
to mostly show how the chromedriver functions can be used and get test coverage.

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromedriver/example.py:39: REPO_URL =
'svn://svn-mirror.golo.chromium.org/chrome/trunk/src'
On 2015/08/06 at 20:52:58, luqui wrote:
> Aren't we on git now?

Yes, seems like I had copied the url from 
build/scripts/slave/recipes/android/appurify.py which has the SVN url. Maybe
that has
to be updated as well?

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromedriver/chromedriver_android.py (right):

https://codereview.chromium.org/1274723004/diff/20001/scripts/slave/recipes/c...
scripts/slave/recipes/chromedriver/chromedriver_android.py:37: 'package':
'org.chromium.chrome.shell',
On 2015/08/28 at 00:14:47, samuong wrote:
> Don't forget we need to replace this with ChromePublic.

Done.

[jbudorick, 2016-05-04 18:10:44]

I got halfway through the api.

This CL needs to be split up. There's far too much that's only being used by the
example. I imagine that will be used by the desktop bots, but that can be added
when the desktop recipes are added.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromedriver/api.py (right):

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:13: from . import archive as
chromium_archive
If you're going to alias it in the same recipe_module, just rename the module to
chromium_archive.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:29: self.archive =
chromium_archive.Archive(self.m)
Why is this public? If you have to provide public access to it, it should be
through a property.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:41: self._chromedriver_log_dir
= self.m.path.mkdtemp('chromedriver_logs')
Does this get cleaned up somewhere? Should it?

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:46: bitness = '32'
nit: 'bitness'? Why not just 'bits'?

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:50: if is_android:
Can you just do this at the beginning?

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:56: output_zip_path =
self.m.path.mkdtemp('zip_to_archive').join(
Same question w.r.t. cleaning up the temporary directory.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:128: return { int(k): v for k,
v in json_dict.items() }
nit: no spaces after { & before }

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:155: while len(log) > 500: #
pragma: no cover
yikes

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:166: version_file =
self.chromedriver_dir.join('VERSION')
Is VERSION just a text file with a string version in it?

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:186: r'int
kMinimumSupportedChromeVersion\[\] = {(\d+), \d+, \d+, \d+};')
- module-scope constant, plz
- I'm not crazy about regex searching a c++ file.
- what if the line starts with spaces?

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:192: else: # pragma: no cover
This should be coverable in example.py.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:204: max_version_re =
re.compile(r'MAJOR=(\d+)')
module-scope constant here too, plz

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:210: else: # pragma: no cover
This should also be coverable in example.py.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:236: keys =
sorted(test_results_log.keys())
nit: a few blank lines in here would make this more readable.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:239: return 'passed' if
test_results_log[commit_position] else 'failed'
You have 'failed' three times in this function. As it's part of the interface,
perhaps it'd be better to constify it at class scope.

(similarly for 'passed', and maybe 'unknown')

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:255: assert
chromedriver_platform != 'android'
?

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:260: output_zip_path =
self.m.path.mkdtemp(
temp directory cleanup, etc

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:301: assert
chromedriver_platform != 'android'
?

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:322: for line in
output.strip().split('\n'):
splitlines?

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:343: break
break at the end of a function...?

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:376: def
get_web_page_content(self, url, test_data=''):
What the?

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:603: exe_postfix = ''
I think this is cleaner if you set the affected variables directly rather than
messing with a postfix.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromedriver/archive.py (right):

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/archive.py:5: class Url(object):
Not sure I see a strong argument for namespacing these three constants like
this.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/archive.py:14: CHROME_45_REVISION =
'338390'
Why are we hard-coding these? Can we do this another way? As-is, this seems like
it'll be a maintenance nightmare.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/archive.py:18: def __init__(self, m):
Passing the modules object here seems weird but reads really cleanly below.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/archive.py:39: url = site +
'/%s/LAST_CHANGE'
Are you leaving the %s in here intentionally? Does gsutil fill that in with the
bucket or something?

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/archive.py:78: url = site +
'/%s/%s/%s.zip' % (self.get_download_platform(), revision,
Why isn't this just

  '%s/%s/%s/%s.zip' % (site, self.get_download_platform(), revision,
get_zip_name())

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/archive.py:86:
self.m.zip.unzip(step_name='unzip chrome',
nit: line break before this

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/file/api.py (right):

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/file/api.py:43: def move(self, name, source, dest,
**kwargs):
Huh, I was looking for this yesterday.

[mikecase (-- gone --), 2016-06-16 17:55:08]

Updated CL to only support Android Chromedriver recipe.

This makes is much smaller because all the Android Chromedriver bot does it run
tests and updates some log files.

Also updated CL with all comments John made on CL that are relevant to the
android bits.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromedriver/api.py (right):

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:13: from . import archive as
chromium_archive
On 2016/05/04 at 18:10:42, jbudorick wrote:
> If you're going to alias it in the same recipe_module, just rename the module
to chromium_archive.

Done

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:29: self.archive =
chromium_archive.Archive(self.m)
On 2016/05/04 at 18:10:42, jbudorick wrote:
> Why is this public? If you have to provide public access to it, it should be
through a property.

Done

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:46: bitness = '32'
On 2016/05/04 at 18:10:42, jbudorick wrote:
> nit: 'bitness'? Why not just 'bits'?

The reason is because that is what the scripts I converted had. Agree 'bits' is
a better name however.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:50: if is_android:
On 2016/05/04 at 18:10:42, jbudorick wrote:
> Can you just do this at the beginning?

Done

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:56: output_zip_path =
self.m.path.mkdtemp('zip_to_archive').join(
On 2016/05/04 at 18:10:43, jbudorick wrote:
> Same question w.r.t. cleaning up the temporary directory.

I made sure I clean up all temp dirs now.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:128: return { int(k): v for k,
v in json_dict.items() }
On 2016/05/04 at 18:10:43, jbudorick wrote:
> nit: no spaces after { & before }

done

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:155: while len(log) > 500: #
pragma: no cover
On 2016/05/04 at 18:10:43, jbudorick wrote:
> yikes

haha. This was ported from the chromedriver scripts. I can change implementation
if you want. I did at least factor out that 500 constant to be
TEST_LOG_MAX_LENGTH

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:166: version_file =
self.chromedriver_dir.join('VERSION')
On 2016/05/04 at 18:10:43, jbudorick wrote:
> Is VERSION just a text file with a string version in it?

yes. But this function is now gone as I am only adding Android support.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:186: r'int
kMinimumSupportedChromeVersion\[\] = {(\d+), \d+, \d+, \d+};')
On 2016/05/04 at 18:10:43, jbudorick wrote:
> - module-scope constant, plz
> - I'm not crazy about regex searching a c++ file.
> - what if the line starts with spaces?

This is a copy of the implementation from the Chromedriver buildbot scripts.
BUt, this function is now gone as I am only adding Android support. Will review
this comment when going back to add Linux/Windows.

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromedriver/archive.py (right):

https://codereview.chromium.org/1274723004/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/archive.py:1: # Copyright 2016 The
Chromium Authors. All rights reserved.
Making this CL Android only which means this file is no longer needed. Ill
review/address these comments when I make a follow-up CL for Linux/Windows
support.

[mikecase (-- gone --), 2016-06-21 22:46:59]

ping

[jbudorick, 2016-06-22 10:17:01]

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromedriver/api.py (right):

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:48: 
nit: -1 blank line

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:97: return {int(k): v for k, v
in json_dict.items()}
nit: iteritems

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:204: if android_packages:
So this does nothing w/o android_packages and isn't an error? Is that
intentional?

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:209: try:
nit: you're creating temp_log_dir inside the with block but removing it outside.
Be consistent -- either create it outside the with block or move the try/finally
that deletes it inside.

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:229: except
recipe_api.AggregatedStepFailure:
Why is this returning True/False and circumventing the StepFailure mechanism?

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipes/...
File scripts/slave/recipes/chromedriver/chromedriver_android.py (right):

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipes/...
scripts/slave/recipes/chromedriver/chromedriver_android.py:64:
api.adb.root_devices()
api.chromium_android.common_tests_setup_steps()?

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipes/...
scripts/slave/recipes/chromedriver/chromedriver_android.py:91:
api.chromium_android.logcat_dump()
api.chromium_android.common_tests_final_steps()?

[mikecase (-- gone --), 2016-06-23 19:14:35]

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromedriver/api.py (right):

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:48: 
On 2016/06/22 at 10:17:01, jbudorick (EMEA til June 30) wrote:
> nit: -1 blank line

Done

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:97: return {int(k): v for k, v
in json_dict.items()}
On 2016/06/22 at 10:17:01, jbudorick (EMEA til June 30) wrote:
> nit: iteritems

Done

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:204: if android_packages:
On 2016/06/22 at 10:17:01, jbudorick (EMEA til June 30) wrote:
> So this does nothing w/o android_packages and isn't an error? Is that
intentional?

eh, I mean, it was intentional, but removing for now since Im initially adding
only Android support.

When I add back in other platforms I'll have to add something similar back in,
i.e.

if <platform android>:
   run tests like this
else:
   run tests like this

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:209: try:
On 2016/06/22 at 10:17:01, jbudorick (EMEA til June 30) wrote:
> nit: you're creating temp_log_dir inside the with block but removing it
outside. Be consistent -- either create it outside the with block or move the
try/finally that deletes it inside.

Done

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:229: except
recipe_api.AggregatedStepFailure:
On 2016/06/22 at 10:17:01, jbudorick (EMEA til June 30) wrote:
> Why is this returning True/False and circumventing the StepFailure mechanism?

Its how the buildbot scripts did this. The result True/False (read, Pass/Fail)
from this step is uploaded to some logs somewhere, if some update_logs var is
true.

Going to move this try/catch into the recipes themselves. So not like...

passed=true
try:
 api.chromedriver.run_all_tests
except Stepfailure
 passed = False

is in the recipes.


Also, kinda related to some code here, is that I am updatign some names
archive_log->archive_server_log and update_log-> update_test_log to be more
descriptive.

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipes/...
File scripts/slave/recipes/chromedriver/chromedriver_android.py (right):

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipes/...
scripts/slave/recipes/chromedriver/chromedriver_android.py:64:
api.adb.root_devices()
On 2016/06/22 at 10:17:01, jbudorick (EMEA til June 30) wrote:
> api.chromium_android.common_tests_setup_steps()?

Done

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipes/...
scripts/slave/recipes/chromedriver/chromedriver_android.py:91:
api.chromium_android.logcat_dump()
On 2016/06/22 at 10:17:01, jbudorick (EMEA til June 30) wrote:
> api.chromium_android.common_tests_final_steps()?

Done

[jbudorick, 2016-06-24 13:31:09]

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromedriver/api.py (right):

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:204: if android_packages:
On 2016/06/23 19:14:35, mikecase wrote:
> On 2016/06/22 at 10:17:01, jbudorick (EMEA til June 30) wrote:
> > So this does nothing w/o android_packages and isn't an error? Is that
> intentional?
> 
> eh, I mean, it was intentional, but removing for now since Im initially adding
> only Android support.
> 
> When I add back in other platforms I'll have to add something similar back in,
> i.e.
> 
> if <platform android>:
>    run tests like this
> else:
>    run tests like this

That's fine. I'm of the opinion that, while we shouldn't paint ourselves into a
corner, we also shouldn't add things we don't need until we need them.

https://codereview.chromium.org/1274723004/diff/180001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:229: except
recipe_api.AggregatedStepFailure:
On 2016/06/23 19:14:34, mikecase wrote:
> On 2016/06/22 at 10:17:01, jbudorick (EMEA til June 30) wrote:
> > Why is this returning True/False and circumventing the StepFailure
mechanism?
> 
> Its how the buildbot scripts did this. The result True/False (read, Pass/Fail)
> from this step is uploaded to some logs somewhere, if some update_logs var is
> true.
> 
> Going to move this try/catch into the recipes themselves. So not like...
> 
> passed=true
> try:
>  api.chromedriver.run_all_tests
> except Stepfailure
>  passed = False
> 
> is in the recipes.

SG. I think it's fine for individual recipes.

> 
> 
> Also, kinda related to some code here, is that I am updatign some names
> archive_log->archive_server_log and update_log-> update_test_log to be more
> descriptive.

sgtm

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromedriver/api.py (right):

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:20: def
get_chromedriver_platform(self, is_android):
This entire function seems unnecessary at the moment.

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipes/...
File scripts/slave/recipes/chromedriver/chromedriver_android.py (right):

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/chromedriver/chromedriver_android.py:44: def
RunSteps(api):
Are you planning to have a separate recipe for non-Android? Or do you plan on
eventually unifying this?

If it's the latter (which is what I'd guess?), this should just be
recipes/chromedriver.py.

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/chromedriver/chromedriver_android.py:56: platform =
api.chromedriver.get_chromedriver_platform(bool(android_packages))
Is this necessary in a "chromedriver_android" recipe?

(somewhat depends on the answer from above)

[samuong, 2016-06-24 16:40:57]

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromedriver/api.py (right):

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:142: '--log-dir',
str(test_log_dir)
run_py_tests.py and run_java_tests.py have a --log-path switch, but not
--log-dir

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:150: if android_package:
i think this should say "if self.m.platform.is_linux:"
what matters is that chromedriver is running on linux, not that chrome is
running on android
for the android bot, xvfb is only required so that chromedriver can do keycode
conversions

[mikecase (-- gone --), 2016-06-24 18:57:50]

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromedriver/api.py (right):

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:20: def
get_chromedriver_platform(self, is_android):
On 2016/06/24 at 13:31:09, jbudorick (EMEA til June 30) wrote:
> This entire function seems unnecessary at the moment.

Yeah, and now that I think about it, maybe we should just set some
chromedriver_platform config variable for each bot instead of having this
function anyway. Removing this function (at least for now).

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:142: '--log-dir',
str(test_log_dir)
On 2016/06/24 at 16:40:57, samuong wrote:
> run_py_tests.py and run_java_tests.py have a --log-path switch, but not
--log-dir

ahh. Thanks for catching this. I had to make some changes because recipes API
has no good way of creating tempfiles that I know of. Changed it so the
run_java_tests and run_python_tests function now have an archive_server_log var.
The log file created from each test run is now uploaded after each run if this
is set.

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:150: if android_package:
On 2016/06/24 at 16:40:56, samuong wrote:
> i think this should say "if self.m.platform.is_linux:"
> what matters is that chromedriver is running on linux, not that chrome is
running on android
> for the android bot, xvfb is only required so that chromedriver can do keycode
conversions

Done. Now I add the xvfb-run command if the platform is linux

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipes/...
File scripts/slave/recipes/chromedriver/chromedriver_android.py (right):

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/chromedriver/chromedriver_android.py:44: def
RunSteps(api):
On 2016/06/24 at 13:31:09, jbudorick (EMEA til June 30) wrote:
> Are you planning to have a separate recipe for non-Android? Or do you plan on
eventually unifying this?
> 
> If it's the latter (which is what I'd guess?), this should just be
recipes/chromedriver.py.

Yeah, I guess it would be nice to just have 1 recipe. Will move to
recipes/chromedriver.py

https://codereview.chromium.org/1274723004/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/chromedriver/chromedriver_android.py:56: platform =
api.chromedriver.get_chromedriver_platform(bool(android_packages))
On 2016/06/24 at 13:31:09, jbudorick (EMEA til June 30) wrote:
> Is this necessary in a "chromedriver_android" recipe?
> 
> (somewhat depends on the answer from above)

Deleting (for now at least). When I add more platforms I may have to add this
back. Going to see if its less awkward to just have a chromedriver_platform
config var.

[mikecase (-- gone --), 2016-07-11 17:36:23]

ping

[jbudorick, 2016-07-11 17:53:48]

lgtm w/ nit

https://codereview.chromium.org/1274723004/diff/240001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromedriver/api.py (right):

https://codereview.chromium.org/1274723004/diff/240001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:150:
self._generate_test_command(
nit: indentation is off here.

https://codereview.chromium.org/1274723004/diff/240001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:169:
self._generate_test_command(
nit: indentation is also off here.

https://codereview.chromium.org/1274723004/diff/240001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:197: for package in
android_packages:
This part seems oddly android-specific given how the rest of this isn't. I'm ok
with that for now, but it seems a bit weird.

Also, since android_packages defaults to None, this should be

  for package in android_packages or []:

https://codereview.chromium.org/1274723004/diff/240001/scripts/slave/recipes/...
File scripts/slave/recipes/chromedriver.py (right):

https://codereview.chromium.org/1274723004/diff/240001/scripts/slave/recipes/...
scripts/slave/recipes/chromedriver.py:66:
api.chromium_android.common_tests_setup_steps()
nit: do this after downloading & extracting the build but before apk
installation.

[samuong, 2016-07-11 18:12:59]

lgtm

we talked about having the recipe install the chrome beta/stable apks over im,
but i'm happy to leave that for a separate cl

[mikecase (-- gone --), 2016-07-11 22:39:44]

https://codereview.chromium.org/1274723004/diff/240001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromedriver/api.py (right):

https://codereview.chromium.org/1274723004/diff/240001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:150:
self._generate_test_command(
On 2016/07/11 at 17:53:48, jbudorick wrote:
> nit: indentation is off here.

Done

https://codereview.chromium.org/1274723004/diff/240001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:169:
self._generate_test_command(
On 2016/07/11 at 17:53:48, jbudorick wrote:
> nit: indentation is also off here.

Done

https://codereview.chromium.org/1274723004/diff/240001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromedriver/api.py:197: for package in
android_packages:
On 2016/07/11 at 17:53:48, jbudorick wrote:
> This part seems oddly android-specific given how the rest of this isn't. I'm
ok with that for now, but it seems a bit weird.
> 
> Also, since android_packages defaults to None, this should be
> 
>   for package in android_packages or []:

Ack and Done

https://codereview.chromium.org/1274723004/diff/240001/scripts/slave/recipes/...
File scripts/slave/recipes/chromedriver.py (right):

https://codereview.chromium.org/1274723004/diff/240001/scripts/slave/recipes/...
scripts/slave/recipes/chromedriver.py:66:
api.chromium_android.common_tests_setup_steps()
On 2016/07/11 at 17:53:48, jbudorick wrote:
> nit: do this after downloading & extracting the build but before apk
installation.

Done

[mikecase (-- gone --), 2016-07-11 22:41:10]

Description was changed from

==========
Converted Chromedriver buildbot scripts to recipes.

recipe_modules/chromedriver/api.py contains many of the
same functions (converted to recipes) from...
src/chrome/test/chromedriver/test/run_all_tests.py and
src/chrome/test/chromedriver/run_buildbot_steps.py

recipe_modules/chromedriver/archive.py is
src/chrome/test/chromedriver/archive.py converted to recipes.

recipes/chromedriver/chromedriver_android.py is the android
specific parts of the main() function from run_buildbot_steps.py
plus some installation and device checking recipe steps.

BUG=
==========

to

==========
Converted Android Chromedriver buildbot scripts to recipes.

Initially adding only Android Chromedriver buildbot code
to keep the CL size down.

recipe_modules/chromedriver/api.py contains many of the
same functions (converted to recipes) from...
src/chrome/test/chromedriver/test/run_all_tests.py and
src/chrome/test/chromedriver/run_buildbot_steps.py

recipes/chromedriver.py is the android
specific parts of the main() function from run_buildbot_steps.py
plus some installation and device checking recipe steps.

BUG=
==========

[mikecase (-- gone --), 2016-07-11 22:42:05]

The CQ bit was checked by mikecase@chromium.org

[mikecase (-- gone --), 2016-07-11 22:42:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
samuong@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1274723004/#ps260001
(title: "Fixed nits.")

[commit-bot: I haz the power, 2016-07-11 22:42:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 22:46:13]

Description was changed from

==========
Converted Android Chromedriver buildbot scripts to recipes.

Initially adding only Android Chromedriver buildbot code
to keep the CL size down.

recipe_modules/chromedriver/api.py contains many of the
same functions (converted to recipes) from...
src/chrome/test/chromedriver/test/run_all_tests.py and
src/chrome/test/chromedriver/run_buildbot_steps.py

recipes/chromedriver.py is the android
specific parts of the main() function from run_buildbot_steps.py
plus some installation and device checking recipe steps.

BUG=
==========

to

==========
Converted Android Chromedriver buildbot scripts to recipes.

Initially adding only Android Chromedriver buildbot code
to keep the CL size down.

recipe_modules/chromedriver/api.py contains many of the
same functions (converted to recipes) from...
src/chrome/test/chromedriver/test/run_all_tests.py and
src/chrome/test/chromedriver/run_buildbot_steps.py

recipes/chromedriver.py is the android
specific parts of the main() function from run_buildbot_steps.py
plus some installation and device checking recipe steps.

BUG=

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/042328a7f55e8ea0373f...
==========

[commit-bot: I haz the power, 2016-07-11 22:46:15]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/tools/build/+/042328a7f55e8ea0373f...

[commit-bot: I haz the power, 2016-07-11 22:46:17]

CQ bit was unchecked.