[Android] Sets up a coverage system for java using EMMA

Sets up a coverage system for java using EMMA

This includes:
* Adds emma instrumentation to classes in java and APK build targets.
* Adds --coverage_dir flags to instrumentation tests.
* Creates a script to take in coverage and metadata directories to
  produce HTML output.
* Changes fyi dbg buildbot configs to build and run tests with coverage enabled.

To build with emma coverage, add 'emma_coverage=1' to the GYP_DEFINES
environment variable.

BUG=255644

[gkanwar1, 2013-07-25 01:19:54]

ptal

[frankf, 2013-07-25 18:53:43]

Adding Chris for gyp changes

https://codereview.chromium.org/20210002/diff/4001/build/android/pylib/instru...
File build/android/pylib/instrumentation/test_runner.py (right):

https://codereview.chromium.org/20210002/diff/4001/build/android/pylib/instru...
build/android/pylib/instrumentation/test_runner.py:135: def
_GetInstrumentationArgs(self, test):
where test used?

https://codereview.chromium.org/20210002/diff/4001/build/android/pylib/instru...
build/android/pylib/instrumentation/test_runner.py:188: self.coverage_basename =
test + '.ec'
Use string formatter: '%s.ec' % test
also below

https://codereview.chromium.org/20210002/diff/4001/build/android/pylib/instru...
build/android/pylib/instrumentation/test_runner.py:189: self.coverage_file =
(self.adb.GetExternalStorage() + '/' +
-> coverage_device_file

https://codereview.chromium.org/20210002/diff/4001/build/android/pylib/instru...
build/android/pylib/instrumentation/test_runner.py:191: self.coverage_basename)
define coverage_host_file here

https://codereview.chromium.org/20210002/diff/4001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/20210002/diff/4001/build/android/test_runner....
build/android/test_runner.py:289: 'EMMA coverage files.'))
Just use coverage_dir to enable coverage

[gkanwar1, 2013-07-25 19:06:38]

Made some fixes, ptal.

https://codereview.chromium.org/20210002/diff/4001/build/android/pylib/instru...
File build/android/pylib/instrumentation/test_runner.py (right):

https://codereview.chromium.org/20210002/diff/4001/build/android/pylib/instru...
build/android/pylib/instrumentation/test_runner.py:135: def
_GetInstrumentationArgs(self, test):
On 2013/07/25 18:53:44, frankf wrote:
> where test used?

Oops, this a remnant of something else I was doing here. Removed.

https://codereview.chromium.org/20210002/diff/4001/build/android/pylib/instru...
build/android/pylib/instrumentation/test_runner.py:188: self.coverage_basename =
test + '.ec'
On 2013/07/25 18:53:44, frankf wrote:
> Use string formatter: '%s.ec' % test
> also below

Done. Fixed in several other places in this file also.

https://codereview.chromium.org/20210002/diff/4001/build/android/pylib/instru...
build/android/pylib/instrumentation/test_runner.py:189: self.coverage_file =
(self.adb.GetExternalStorage() + '/' +
On 2013/07/25 18:53:44, frankf wrote:
> -> coverage_device_file

Done.

https://codereview.chromium.org/20210002/diff/4001/build/android/pylib/instru...
build/android/pylib/instrumentation/test_runner.py:191: self.coverage_basename)
On 2013/07/25 18:53:44, frankf wrote:
> define coverage_host_file here

Done.

https://codereview.chromium.org/20210002/diff/4001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/20210002/diff/4001/build/android/test_runner....
build/android/test_runner.py:289: 'EMMA coverage files.'))
On 2013/07/25 18:53:44, frankf wrote:
> Just use coverage_dir to enable coverage

Good point. Updated.

[frankf, 2013-07-25 20:28:19]

https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
File build/android/emma_html_output.py (right):

https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
build/android/emma_html_output.py:1: #! /usr/bin/env python
Rename this to generate_emma_html.py

https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
build/android/emma_html_output.py:46: 'coverage metadata (.em) files.'))
So you also need the path to the src passed in. But we really don't want to
hardcode a list of the src files across the tree.

https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
build/android/emma_html_output.py:57: if not options.output:
Just combine these conditionals into one and print that all options are
mandatory

https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
build/android/emma_html_output.py:61: android_sdk_root =
os.environ.get('ANDROID_SDK_ROOT')
This is defined in constant.py

https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
build/android/emma_html_output.py:66: emma_file = os.path.join(android_sdk_root,
'tools', 'lib', 'emma.jar')
-> emma_jar

https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
build/android/emma_html_output.py:71: file_args = []
input_args

https://codereview.chromium.org/20210002/diff/11001/build/android/pylib/instr...
File build/android/pylib/instrumentation/test_runner.py (right):

https://codereview.chromium.org/20210002/diff/11001/build/android/pylib/instr...
build/android/pylib/instrumentation/test_runner.py:186: self.coverage_basename =
'%s.ec' % (test)
No need for ()

https://codereview.chromium.org/20210002/diff/11001/build/android/pylib/instr...
build/android/pylib/instrumentation/test_runner.py:186: self.coverage_basename =
'%s.ec' % (test)
This var is local now

https://codereview.chromium.org/20210002/diff/11001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/20210002/diff/11001/build/android/test_runner...
build/android/test_runner.py:284: option_parser.add_option('--coverage_dir',
default=None,
No need for default=None

[gkanwar1, 2013-07-25 20:33:23]

On 2013/07/25 20:28:19, frankf wrote:
>
https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
> File build/android/emma_html_output.py (right):
> 
>
https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
> build/android/emma_html_output.py:1: #! /usr/bin/env python
> Rename this to generate_emma_html.py
> 
>
https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
> build/android/emma_html_output.py:46: 'coverage metadata (.em) files.'))
> So you also need the path to the src passed in. But we really don't want to
> hardcode a list of the src files across the tree.
> 
>
https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
> build/android/emma_html_output.py:57: if not options.output:
> Just combine these conditionals into one and print that all options are
> mandatory
> 
>
https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
> build/android/emma_html_output.py:61: android_sdk_root =
> os.environ.get('ANDROID_SDK_ROOT')
> This is defined in constant.py
> 
>
https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
> build/android/emma_html_output.py:66: emma_file =
os.path.join(android_sdk_root,
> 'tools', 'lib', 'emma.jar')
> -> emma_jar
> 
>
https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
> build/android/emma_html_output.py:71: file_args = []
> input_args
> 
>
https://codereview.chromium.org/20210002/diff/11001/build/android/pylib/instr...
> File build/android/pylib/instrumentation/test_runner.py (right):
> 
>
https://codereview.chromium.org/20210002/diff/11001/build/android/pylib/instr...
> build/android/pylib/instrumentation/test_runner.py:186: self.coverage_basename
=
> '%s.ec' % (test)
> No need for ()
> 
>
https://codereview.chromium.org/20210002/diff/11001/build/android/pylib/instr...
> build/android/pylib/instrumentation/test_runner.py:186: self.coverage_basename
=
> '%s.ec' % (test)
> This var is local now
> 
>
https://codereview.chromium.org/20210002/diff/11001/build/android/test_runner.py
> File build/android/test_runner.py (right):
> 
>
https://codereview.chromium.org/20210002/diff/11001/build/android/test_runner...
> build/android/test_runner.py:284: option_parser.add_option('--coverage_dir',
> default=None,
> No need for default=None

It looks like the coverage data is not including all the files that we want. I'm
going to play with this more and try to figure out what's up. Please hold off on
review while I work on fixing this (I'll address the outstanding comments as
well). Thanks!

[gkanwar1, 2013-08-01 02:14:35]

Made several changes:
* Non-apk java for ContentShell is now covered. Test java is not.
* Lists of source directories are automatically generated and aggregated via
generate_emma_html.py
* Finer grained control over which targets get instrumented exists now. We
specify which targets to instrument via the GYP files.

ptal when you get the chance.

https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
File build/android/emma_html_output.py (right):

https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
build/android/emma_html_output.py:1: #! /usr/bin/env python
On 2013/07/25 20:28:19, frankf wrote:
> Rename this to generate_emma_html.py

Done.

https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
build/android/emma_html_output.py:46: 'coverage metadata (.em) files.'))
On 2013/07/25 20:28:19, frankf wrote:
> So you also need the path to the src passed in. But we really don't want to
> hardcode a list of the src files across the tree.

See the latest patch set. I introduced an emma_sources.txt file accompanying
each metadata file, so this file accumulates those filenames and passes them all
to the HTML generator.

https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
build/android/emma_html_output.py:57: if not options.output:
On 2013/07/25 20:28:19, frankf wrote:
> Just combine these conditionals into one and print that all options are
> mandatory

Done.

https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
build/android/emma_html_output.py:61: android_sdk_root =
os.environ.get('ANDROID_SDK_ROOT')
On 2013/07/25 20:28:19, frankf wrote:
> This is defined in constant.py

Done.

https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
build/android/emma_html_output.py:66: emma_file = os.path.join(android_sdk_root,
'tools', 'lib', 'emma.jar')
On 2013/07/25 20:28:19, frankf wrote:
> -> emma_jar

Done.

https://codereview.chromium.org/20210002/diff/11001/build/android/emma_html_o...
build/android/emma_html_output.py:71: file_args = []
On 2013/07/25 20:28:19, frankf wrote:
> input_args

Done.

https://codereview.chromium.org/20210002/diff/11001/build/android/pylib/instr...
File build/android/pylib/instrumentation/test_runner.py (right):

https://codereview.chromium.org/20210002/diff/11001/build/android/pylib/instr...
build/android/pylib/instrumentation/test_runner.py:186: self.coverage_basename =
'%s.ec' % (test)
On 2013/07/25 20:28:19, frankf wrote:
> No need for ()

Done.

https://codereview.chromium.org/20210002/diff/11001/build/android/pylib/instr...
build/android/pylib/instrumentation/test_runner.py:186: self.coverage_basename =
'%s.ec' % (test)
On 2013/07/25 20:28:19, frankf wrote:
> This var is local now

Done.

https://codereview.chromium.org/20210002/diff/11001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/20210002/diff/11001/build/android/test_runner...
build/android/test_runner.py:284: option_parser.add_option('--coverage_dir',
default=None,
On 2013/07/25 20:28:19, frankf wrote:
> No need for default=None

Done.

[frankf, 2013-08-01 19:57:41]

If it's not expensive, let's just build separate instrumented apks, and enable
instrumentation all the time.

I'll let Chris comment on the gyp specifics.

https://codereview.chromium.org/20210002/diff/28001/build/android/ant/apk-pac...
File build/android/ant/apk-package.xml (right):

https://codereview.chromium.org/20210002/diff/28001/build/android/ant/apk-pac...
build/android/ant/apk-package.xml:89: <package-helper>
remove tabs

https://codereview.chromium.org/20210002/diff/28001/build/android/generate_em...
File build/android/generate_emma_html.py (right):

https://codereview.chromium.org/20210002/diff/28001/build/android/generate_em...
build/android/generate_emma_html.py:7: """Accumulates EMMA coverage files to
produce html output."""
Aggregates

https://codereview.chromium.org/20210002/diff/28001/build/android/generate_em...
build/android/generate_emma_html.py:28: 
no blank line

https://codereview.chromium.org/20210002/diff/28001/build/android/generate_em...
build/android/generate_emma_html.py:41: option_parser.add_option('-c',
'--coverage_dir', default=None,
- not _
also below

https://codereview.chromium.org/20210002/diff/28001/build/android/generate_em...
build/android/generate_emma_html.py:63: sources_files.extend(f.read().split('
'))
can you just split()

https://codereview.chromium.org/20210002/diff/28001/build/android/generate_em...
build/android/generate_emma_html.py:73: return cmd_helper.RunCmd(['java', '-cp',
emma_jar, 'emma',
align on next line

https://codereview.chromium.org/20210002/diff/28001/build/android/pylib/instr...
File build/android/pylib/instrumentation/test_runner.py (right):

https://codereview.chromium.org/20210002/diff/28001/build/android/pylib/instr...
build/android/pylib/instrumentation/test_runner.py:234:
self.adb.RunShellCommand('rm %s' % self.coverage_device_file)
rm -f

https://codereview.chromium.org/20210002/diff/28001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/20210002/diff/28001/build/android/test_runner...
build/android/test_runner.py:253: option_parser.add_option('--coverage_dir',
--coverage-dir

https://codereview.chromium.org/20210002/diff/28001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/20210002/diff/28001/build/java_apk.gypi#newco...
build/java_apk.gypi:99: 'emma_device_jar':
'<(android_sdk_root)/tools/lib/emma_device.jar',
This is defined in bunch of places. Is there a way to define it once?

[gkanwar1, 2013-08-07 19:24:56]

cjhopman -- I made the changes you recommended (linearizing the actinons, and
converting the ant script to python), ptal and let me know if there are other
things I should change with the gyp files.

This changes also makes changes the fyi dbg buildbot to run instrumentation with
coverage. The only component missing at this point is getting the HTML to the
log.

https://codereview.chromium.org/20210002/diff/28001/build/android/ant/apk-pac...
File build/android/ant/apk-package.xml (right):

https://codereview.chromium.org/20210002/diff/28001/build/android/ant/apk-pac...
build/android/ant/apk-package.xml:89: <package-helper>
On 2013/08/01 19:57:41, frankf wrote:
> remove tabs

Done.

https://codereview.chromium.org/20210002/diff/28001/build/android/generate_em...
File build/android/generate_emma_html.py (right):

https://codereview.chromium.org/20210002/diff/28001/build/android/generate_em...
build/android/generate_emma_html.py:7: """Accumulates EMMA coverage files to
produce html output."""
On 2013/08/01 19:57:41, frankf wrote:
> Aggregates

Done.

https://codereview.chromium.org/20210002/diff/28001/build/android/generate_em...
build/android/generate_emma_html.py:28: 
On 2013/08/01 19:57:41, frankf wrote:
> no blank line

Done.

https://codereview.chromium.org/20210002/diff/28001/build/android/generate_em...
build/android/generate_emma_html.py:41: option_parser.add_option('-c',
'--coverage_dir', default=None,
On 2013/08/01 19:57:41, frankf wrote:
> - not _
> also below

Done.

https://codereview.chromium.org/20210002/diff/28001/build/android/generate_em...
build/android/generate_emma_html.py:63: sources_files.extend(f.read().split('
'))
On 2013/08/01 19:57:41, frankf wrote:
> can you just split()

Good point. That's more robust also.

https://codereview.chromium.org/20210002/diff/28001/build/android/generate_em...
build/android/generate_emma_html.py:73: return cmd_helper.RunCmd(['java', '-cp',
emma_jar, 'emma',
On 2013/08/01 19:57:41, frankf wrote:
> align on next line

I assume by this you mean to move all args down to the next line, and indent 4
spaces? If so, done.

https://codereview.chromium.org/20210002/diff/28001/build/android/pylib/instr...
File build/android/pylib/instrumentation/test_runner.py (right):

https://codereview.chromium.org/20210002/diff/28001/build/android/pylib/instr...
build/android/pylib/instrumentation/test_runner.py:234:
self.adb.RunShellCommand('rm %s' % self.coverage_device_file)
On 2013/08/01 19:57:41, frankf wrote:
> rm -f

Done.

https://codereview.chromium.org/20210002/diff/28001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/20210002/diff/28001/build/android/test_runner...
build/android/test_runner.py:253: option_parser.add_option('--coverage_dir',
On 2013/08/01 19:57:41, frankf wrote:
> --coverage-dir

Done.

https://codereview.chromium.org/20210002/diff/28001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/20210002/diff/28001/build/java_apk.gypi#newco...
build/java_apk.gypi:99: 'emma_device_jar':
'<(android_sdk_root)/tools/lib/emma_device.jar',
On 2013/08/01 19:57:41, frankf wrote:
> This is defined in bunch of places. Is there a way to define it once?

Unfortunately I don't think there's a simple way to share this constant between
GYP and the ant files. That said, I removed it from java.gypi, since we don't
need it in there any more. It's now referred to once in java_apk.gypi and once
in apk-package.xml. I think that's the best we can get.

[cjhopman, 2013-08-09 18:54:37]

https://codereview.chromium.org/20210002/diff/40001/build/android/ant/apk-pac...
File build/android/ant/apk-package.xml (right):

https://codereview.chromium.org/20210002/diff/40001/build/android/ant/apk-pac...
build/android/ant/apk-package.xml:89: <package-helper>
Nit: replace tabs with spaces

https://codereview.chromium.org/20210002/diff/40001/build/android/ant/apk-pac...
build/android/ant/apk-package.xml:91: <jarfile
path="${sdk.dir}/tools/lib/emma_device.jar" />
this path can be passed to ant by adding the flag
'-DEMMA_JAR=<(emma_device_jar)' in the gyp action.

However, it's possible that this can be done in a cleaner way. My guess is that
apkbuilder just merges the extrajars into the dex file at dex path and then
packages that into the apk. If that's the case, then you should be able to just
merge the emma jar into the classes.dex file yourself and not modify this file
at all.

https://codereview.chromium.org/20210002/diff/40001/build/android/dex_action....
File build/android/dex_action.gypi (right):

https://codereview.chromium.org/20210002/diff/40001/build/android/dex_action....
build/android/dex_action.gypi:36: 'dex_no_locals': [],
gyp typically uses 0 or 1 for flags like this, let's do that and not expose
setting python flags to includers of this file. Then below just pass
'--no-locals=<(no_locals)' and parse it in the python script.

https://codereview.chromium.org/20210002/diff/40001/build/android/generate_em...
File build/android/generate_emma_html.py (right):

https://codereview.chromium.org/20210002/diff/40001/build/android/generate_em...
build/android/generate_emma_html.py:29: basenames = [f for f in filenames if
os.path.splitext(f)[1] == ext]
basenames = fnmatch.filter(filenames, '*.' + ext)

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.py
File build/android/gyp/instr.py (right):

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:15: from pylib import cmd_helper
we try to minimize the use of build/android/pylib in build/android/gyp. This is
mostly done so that there is more control over the dependencies of these python
build actions. If it is required to use these you should specify them (and
anything they import) as inputs to any gyp actions that use this script.

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:16: from pylib import constants
This looks like it's just used for paths. Pass those in from gyp instead.

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:61: cmd_helper.RunCmd(['cp', '-R',
options.input_path, options.output_path])
shutil.copy(options.input_path, options.output_path) if these aren't
directories, otherwise shutil.copytree should work. Either way, do the copying
in python.

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:73: s = s[1:-1]
sources = build_utils.ParseGypList(sources_string)

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:82: f.write(' '.join(relative_sources))
I would write this json formatted, but not required.

json.dump(relative_sources, f)

and to read:

relative_sources = json.load(f)

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:92: emma_jar =
os.path.join(constants.ANDROID_SDK_ROOT, 'tools', 'lib',
Same as below.

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:101: cmd_helper.RunCmd(['cp'] +
glob.glob(os.path.join(
Do this copying in python. Can possibly just shutil.copytree

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:116: emma_jar =
os.path.join(constants.ANDROID_SDK_ROOT, 'tools', 'lib',
Don't calculate the path here, pass it in as an argument.

https://codereview.chromium.org/20210002/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/20210002/diff/40001/build/common.gypi#newcode911
build/common.gypi:911: 'emma_instrument%': 0,
If this is set by individual targets (i.e. not configurable globally), set the
default in the .gypi files for the targets, not here.

[gkanwar1, 2013-08-09 23:32:53]

https://codereview.chromium.org/20210002/diff/40001/build/android/ant/apk-pac...
File build/android/ant/apk-package.xml (right):

https://codereview.chromium.org/20210002/diff/40001/build/android/ant/apk-pac...
build/android/ant/apk-package.xml:89: <package-helper>
On 2013/08/09 18:54:37, cjhopman wrote:
> Nit: replace tabs with spaces

Done.

https://codereview.chromium.org/20210002/diff/40001/build/android/ant/apk-pac...
build/android/ant/apk-package.xml:91: <jarfile
path="${sdk.dir}/tools/lib/emma_device.jar" />
On 2013/08/09 18:54:37, cjhopman wrote:
> this path can be passed to ant by adding the flag
> '-DEMMA_JAR=<(emma_device_jar)' in the gyp action.
> 
> However, it's possible that this can be done in a cleaner way. My guess is
that
> apkbuilder just merges the extrajars into the dex file at dex path and then
> packages that into the apk. If that's the case, then you should be able to
just
> merge the emma jar into the classes.dex file yourself and not modify this file
> at all.

It looks like apkbuilder actually adds the java resources from the .jar to the
final APK. Looking through some of the sdklib source for Android I wasn't able
to figure exactly how extracting the java resources and adding them to the APK
manually would work. It looks like it would likely be more complicated unless we
already have something for this.

https://codereview.chromium.org/20210002/diff/40001/build/android/dex_action....
File build/android/dex_action.gypi (right):

https://codereview.chromium.org/20210002/diff/40001/build/android/dex_action....
build/android/dex_action.gypi:36: 'dex_no_locals': [],
On 2013/08/09 18:54:37, cjhopman wrote:
> gyp typically uses 0 or 1 for flags like this, let's do that and not expose
> setting python flags to includers of this file. Then below just pass
> '--no-locals=<(no_locals)' and parse it in the python script.

Done.

https://codereview.chromium.org/20210002/diff/40001/build/android/generate_em...
File build/android/generate_emma_html.py (right):

https://codereview.chromium.org/20210002/diff/40001/build/android/generate_em...
build/android/generate_emma_html.py:29: basenames = [f for f in filenames if
os.path.splitext(f)[1] == ext]
On 2013/08/09 18:54:37, cjhopman wrote:
> basenames = fnmatch.filter(filenames, '*.' + ext)

Done.

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.py
File build/android/gyp/instr.py (right):

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:15: from pylib import cmd_helper
On 2013/08/09 18:54:37, cjhopman wrote:
> we try to minimize the use of build/android/pylib in build/android/gyp. This
is
> mostly done so that there is more control over the dependencies of these
python
> build actions. If it is required to use these you should specify them (and
> anything they import) as inputs to any gyp actions that use this script.

Removed references to cmd_helper and constants. command_options_parser makes the
option parsing much cleaner, and doesn't depend on anything else, so I'm going
to leave that as a dependency if that's okay.

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:16: from pylib import constants
On 2013/08/09 18:54:37, cjhopman wrote:
> This looks like it's just used for paths. Pass those in from gyp instead.

Done.

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:61: cmd_helper.RunCmd(['cp', '-R',
options.input_path, options.output_path])
On 2013/08/09 18:54:37, cjhopman wrote:
> shutil.copy(options.input_path, options.output_path) if these aren't
> directories, otherwise shutil.copytree should work. Either way, do the copying
> in python.

Done.

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:73: s = s[1:-1]
On 2013/08/09 18:54:37, cjhopman wrote:
> sources = build_utils.ParseGypList(sources_string)

Done.

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:82: f.write(' '.join(relative_sources))
On 2013/08/09 18:54:37, cjhopman wrote:
> I would write this json formatted, but not required.
> 
> json.dump(relative_sources, f)
> 
> and to read:
> 
> relative_sources = json.load(f)

Sounds like a good idea. Done.

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:92: emma_jar =
os.path.join(constants.ANDROID_SDK_ROOT, 'tools', 'lib',
On 2013/08/09 18:54:37, cjhopman wrote:
> Same as below.

Done.

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:101: cmd_helper.RunCmd(['cp'] +
glob.glob(os.path.join(
On 2013/08/09 18:54:37, cjhopman wrote:
> Do this copying in python. Can possibly just shutil.copytree

Done.

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:116: emma_jar =
os.path.join(constants.ANDROID_SDK_ROOT, 'tools', 'lib',
On 2013/08/09 18:54:37, cjhopman wrote:
> Don't calculate the path here, pass it in as an argument.

Fixed here and in generate_emma_html.py

https://codereview.chromium.org/20210002/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/20210002/diff/40001/build/common.gypi#newcode911
build/common.gypi:911: 'emma_instrument%': 0,
On 2013/08/09 18:54:37, cjhopman wrote:
> If this is set by individual targets (i.e. not configurable globally), set the
> default in the .gypi files for the targets, not here.

This is only set to 1 by targets we want to instrument, and left alone by other
targets (there's a lot of targets that we don't want to instrument, so I feel
like a whitelist works best here). Is there a better way to do this than setting
a global default?

[cjhopman, 2013-08-10 00:11:47]

https://codereview.chromium.org/20210002/diff/40001/build/android/ant/apk-pac...
File build/android/ant/apk-package.xml (right):

https://codereview.chromium.org/20210002/diff/40001/build/android/ant/apk-pac...
build/android/ant/apk-package.xml:91: <jarfile
path="${sdk.dir}/tools/lib/emma_device.jar" />
On 2013/08/09 23:32:53, gkanwar1 wrote:
> On 2013/08/09 18:54:37, cjhopman wrote:
> > this path can be passed to ant by adding the flag
> > '-DEMMA_JAR=<(emma_device_jar)' in the gyp action.
> > 
> > However, it's possible that this can be done in a cleaner way. My guess is
> that
> > apkbuilder just merges the extrajars into the dex file at dex path and then
> > packages that into the apk. If that's the case, then you should be able to
> just
> > merge the emma jar into the classes.dex file yourself and not modify this
file
> > at all.
> 
> It looks like apkbuilder actually adds the java resources from the .jar to the
> final APK. Looking through some of the sdklib source for Android I wasn't able
> to figure exactly how extracting the java resources and adding them to the APK
> manually would work. It looks like it would likely be more complicated unless
we
> already have something for this.

Yeah, don't worry about it then, this is fine.

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.py
File build/android/gyp/instr.py (right):

https://codereview.chromium.org/20210002/diff/40001/build/android/gyp/instr.p...
build/android/gyp/instr.py:15: from pylib import cmd_helper
On 2013/08/09 23:32:53, gkanwar1 wrote:
> On 2013/08/09 18:54:37, cjhopman wrote:
> > we try to minimize the use of build/android/pylib in build/android/gyp. This
> is
> > mostly done so that there is more control over the dependencies of these
> python
> > build actions. If it is required to use these you should specify them (and
> > anything they import) as inputs to any gyp actions that use this script.
> 
> Removed references to cmd_helper and constants. command_options_parser makes
the
> option parsing much cleaner, and doesn't depend on anything else, so I'm going
> to leave that as a dependency if that's okay.

That's fine.

https://codereview.chromium.org/20210002/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/20210002/diff/40001/build/common.gypi#newcode911
build/common.gypi:911: 'emma_instrument%': 0,
On 2013/08/09 23:32:53, gkanwar1 wrote:
> On 2013/08/09 18:54:37, cjhopman wrote:
> > If this is set by individual targets (i.e. not configurable globally), set
the
> > default in the .gypi files for the targets, not here.
> 
> This is only set to 1 by targets we want to instrument, and left alone by
other
> targets (there's a lot of targets that we don't want to instrument, so I feel
> like a whitelist works best here). Is there a better way to do this than
setting
> a global default?

You can set the default in the .gypi files that use the variable (i.e.
build/java.gypi and build/java_apk.gypi). The target itself can still override
the default in the included .gypi. That is, you can move this line into those
gypi files and there is no need to change any of the targets.

https://codereview.chromium.org/20210002/diff/56001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/20210002/diff/56001/build/java.gypi#newcode281
build/java.gypi:281: 'action_name': 'instr_jar_<(_target_name)',
Extract this action to its own file in build/android (like
build/android/dex_action.gypi). Then expose those variables that should be
configured by the caller (mostly the things that are different between the use
here and the one in java.gypi).

[gkanwar1, 2013-08-12 18:19:10]

This also adds the newly created buckets. I will try testing this by temporary
changing the try tests to the fyi-tests bot config, and change it back once it
works.

https://codereview.chromium.org/20210002/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/20210002/diff/40001/build/common.gypi#newcode911
build/common.gypi:911: 'emma_instrument%': 0,
On 2013/08/10 00:11:47, cjhopman wrote:
> On 2013/08/09 23:32:53, gkanwar1 wrote:
> > On 2013/08/09 18:54:37, cjhopman wrote:
> > > If this is set by individual targets (i.e. not configurable globally), set
> the
> > > default in the .gypi files for the targets, not here.
> > 
> > This is only set to 1 by targets we want to instrument, and left alone by
> other
> > targets (there's a lot of targets that we don't want to instrument, so I
feel
> > like a whitelist works best here). Is there a better way to do this than
> setting
> > a global default?
> 
> You can set the default in the .gypi files that use the variable (i.e.
> build/java.gypi and build/java_apk.gypi). The target itself can still override
> the default in the included .gypi. That is, you can move this line into those
> gypi files and there is no need to change any of the targets.
> 

Ah, got it. Done.

https://codereview.chromium.org/20210002/diff/56001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/20210002/diff/56001/build/java.gypi#newcode281
build/java.gypi:281: 'action_name': 'instr_jar_<(_target_name)',
On 2013/08/10 00:11:47, cjhopman wrote:
> Extract this action to its own file in build/android (like
> build/android/dex_action.gypi). Then expose those variables that should be
> configured by the caller (mostly the things that are different between the use
> here and the one in java.gypi). 

Done.

[gkanwar1, 2013-08-12 18:23:20]

On 2013/08/12 18:19:10, gkanwar1 wrote:
> This also adds the newly created buckets. I will try testing this by temporary
> changing the try tests to the fyi-tests bot config, and change it back once it
> works.
> 
> https://codereview.chromium.org/20210002/diff/40001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/20210002/diff/40001/build/common.gypi#newcode911
> build/common.gypi:911: 'emma_instrument%': 0,
> On 2013/08/10 00:11:47, cjhopman wrote:
> > On 2013/08/09 23:32:53, gkanwar1 wrote:
> > > On 2013/08/09 18:54:37, cjhopman wrote:
> > > > If this is set by individual targets (i.e. not configurable globally),
set
> > the
> > > > default in the .gypi files for the targets, not here.
> > > 
> > > This is only set to 1 by targets we want to instrument, and left alone by
> > other
> > > targets (there's a lot of targets that we don't want to instrument, so I
> feel
> > > like a whitelist works best here). Is there a better way to do this than
> > setting
> > > a global default?
> > 
> > You can set the default in the .gypi files that use the variable (i.e.
> > build/java.gypi and build/java_apk.gypi). The target itself can still
override
> > the default in the included .gypi. That is, you can move this line into
those
> > gypi files and there is no need to change any of the targets.
> > 
> 
> Ah, got it. Done.
> 
> https://codereview.chromium.org/20210002/diff/56001/build/java.gypi
> File build/java.gypi (right):
> 
> https://codereview.chromium.org/20210002/diff/56001/build/java.gypi#newcode281
> build/java.gypi:281: 'action_name': 'instr_jar_<(_target_name)',
> On 2013/08/10 00:11:47, cjhopman wrote:
> > Extract this action to its own file in build/android (like
> > build/android/dex_action.gypi). Then expose those variables that should be
> > configured by the caller (mostly the things that are different between the
use
> > here and the one in java.gypi). 
> 
> Done.

Actually, looking over the buildbot config again, this shouldn't require any
changes to the try-bots, since we have try-fyi-tests and try-fyi-builder-dbg
which copy the fyi-tests and fyi-builder-dbg config in the code.

[frankf, 2013-08-13 23:33:38]

https://codereview.chromium.org/20210002/diff/91001/build/android/buildbot/bb...
File build/android/buildbot/bb_device_steps.py (right):

https://codereview.chromium.org/20210002/diff/91001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:30: COVERAGE_DIR =
os.path.join(CHROME_SRC, 'out', 'coverage')
Move this to constants.py

https://codereview.chromium.org/20210002/diff/91001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:287: '--metadata-dir',
os.path.join('out', options.target),
How does this work? Did yo forget constants.DIR_SOURCE_ROOT?

https://codereview.chromium.org/20210002/diff/91001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:302: (options.coverage_bucket,
revision)])
As it stands, you're going to upload from all fyi bots and trybots. Can this
conflict?

https://codereview.chromium.org/20210002/diff/91001/build/android/gyp/instr.py
File build/android/gyp/instr.py (right):

https://codereview.chromium.org/20210002/diff/91001/build/android/gyp/instr.p...
build/android/gyp/instr.py:1: #!/usr/bin/env python
Can you pick a better module name than 'instr'?

https://codereview.chromium.org/20210002/diff/91001/build/android/gyp/instr.p...
build/android/gyp/instr.py:7: """Instruments classes and jar files."""
Expand this comment.

https://codereview.chromium.org/20210002/diff/91001/build/android/gyp/instr.p...
build/android/gyp/instr.py:23: option_parser.add_option('--input-path',
help='Path to input file(s).')
Define what 'input' files are

https://codereview.chromium.org/20210002/diff/91001/build/android/gyp/instr.p...
build/android/gyp/instr.py:25: help='Path to output final file(s) to.')
Same here

https://codereview.chromium.org/20210002/diff/91001/build/android/gyp/instr.p...
build/android/gyp/instr.py:75: """Adds all normalized source directories to
|sources_file|."""
Add Args section here and below

https://codereview.chromium.org/20210002/diff/91001/build/android/pylib/utils...
File build/android/pylib/utils/command_option_parser.py (right):

https://codereview.chromium.org/20210002/diff/91001/build/android/pylib/utils...
build/android/pylib/utils/command_option_parser.py:59: argv: Command line
arguments. If None, automatically draw from sys.argv.
2 spaces, same below

https://codereview.chromium.org/20210002/diff/91001/build/android/pylib/utils...
build/android/pylib/utils/command_option_parser.py:59: argv: Command line
arguments. If None, automatically draw from sys.argv.
Can you just pass argv everywhere instead of relying sys.argv?

[gkanwar1, 2013-08-15 19:49:20]

Fixed several things, ptal.

https://codereview.chromium.org/20210002/diff/91001/build/android/buildbot/bb...
File build/android/buildbot/bb_device_steps.py (right):

https://codereview.chromium.org/20210002/diff/91001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:30: COVERAGE_DIR =
os.path.join(CHROME_SRC, 'out', 'coverage')
On 2013/08/13 23:33:39, frankf wrote:
> Move this to constants.py

Done.

https://codereview.chromium.org/20210002/diff/91001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:287: '--metadata-dir',
os.path.join('out', options.target),
On 2013/08/13 23:33:39, frankf wrote:
> How does this work? Did yo forget constants.DIR_SOURCE_ROOT?

It's relative to the python running directory, which is at DIR_SOURCE_ROOT.
Still worth making this more explicit, so I added that.

https://codereview.chromium.org/20210002/diff/91001/build/android/buildbot/bb...
build/android/buildbot/bb_device_steps.py:302: (options.coverage_bucket,
revision)])
On 2013/08/13 23:33:39, frankf wrote:
> As it stands, you're going to upload from all fyi bots and trybots. Can this
> conflict?

Yep, good point. Changing url to gs://<bucket>/<bot-id>/<rev-id>.

https://codereview.chromium.org/20210002/diff/91001/build/android/gyp/instr.py
File build/android/gyp/instr.py (right):

https://codereview.chromium.org/20210002/diff/91001/build/android/gyp/instr.p...
build/android/gyp/instr.py:1: #!/usr/bin/env python
On 2013/08/13 23:33:39, frankf wrote:
> Can you pick a better module name than 'instr'?

Renamed to emma_instr.

https://codereview.chromium.org/20210002/diff/91001/build/android/gyp/instr.p...
build/android/gyp/instr.py:7: """Instruments classes and jar files."""
On 2013/08/13 23:33:39, frankf wrote:
> Expand this comment.

Done.

https://codereview.chromium.org/20210002/diff/91001/build/android/gyp/instr.p...
build/android/gyp/instr.py:23: option_parser.add_option('--input-path',
help='Path to input file(s).')
On 2013/08/13 23:33:39, frankf wrote:
> Define what 'input' files are

Done.

https://codereview.chromium.org/20210002/diff/91001/build/android/gyp/instr.p...
build/android/gyp/instr.py:25: help='Path to output final file(s) to.')
On 2013/08/13 23:33:39, frankf wrote:
> Same here

Done.

https://codereview.chromium.org/20210002/diff/91001/build/android/gyp/instr.p...
build/android/gyp/instr.py:75: """Adds all normalized source directories to
|sources_file|."""
On 2013/08/13 23:33:39, frankf wrote:
> Add Args section here and below

Done.

https://codereview.chromium.org/20210002/diff/91001/build/android/pylib/utils...
File build/android/pylib/utils/command_option_parser.py (right):

https://codereview.chromium.org/20210002/diff/91001/build/android/pylib/utils...
build/android/pylib/utils/command_option_parser.py:59: argv: Command line
arguments. If None, automatically draw from sys.argv.
On 2013/08/13 23:33:39, frankf wrote:
> 2 spaces, same below

Done.

https://codereview.chromium.org/20210002/diff/91001/build/android/pylib/utils...
build/android/pylib/utils/command_option_parser.py:59: argv: Command line
arguments. If None, automatically draw from sys.argv.
On 2013/08/13 23:33:39, frankf wrote:
> Can you just pass argv everywhere instead of relying sys.argv?

I did this to parallel optparser.OptionParser.parse_args() --
http://docs.python.org/2/library/optparse.html#tutorial (see the bit about
parse_args). I could change this to passing it in if you think that would be
better.

[gkanwar1, 2013-08-15 19:50:19]

Adding Marcus for review on the buildbot changes.

[gkanwar1, 2013-08-15 21:38:12]

On 2013/08/15 19:50:19, gkanwar1 wrote:
> Adding Marcus for review on the buildbot changes.

Looks like there's still some issues to sort out. Please hold off on review for
now.

[cjhopman, 2013-08-20 15:27:23]

https://codereview.chromium.org/20210002/diff/170001/android_webview/android_...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/20210002/diff/170001/android_webview/android_...
android_webview/android_webview.gyp:203: 'emma_should_instrument': 1,
Maybe this should default to 1 and be disabled if there are any targets that we
don't want to have instrumented.

https://codereview.chromium.org/20210002/diff/170001/build/android/gyp/dex.py
File build/android/gyp/dex.py (left):

https://codereview.chromium.org/20210002/diff/170001/build/android/gyp/dex.py...
build/android/gyp/dex.py:19: # See http://crbug.com/272064 for context on
--force-jumbo.
Nit: keep this comment

https://codereview.chromium.org/20210002/diff/170001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/20210002/diff/170001/build/java.gypi#newcode89
build/java.gypi:89: 'javac_jar_path': '<(jar_path)'
I'm not sure that you can expand jar_path here (since it's defined in an outer
variables block).

[gkanwar1, 2013-08-20 17:30:13]

Coverage generation and uploading is now working. I would wait for the full
suite of trybots to pass before asking for review, but since I'm pretty crunched
on time right now (I'm leaving Friday) it would be great if I could get a review
on this ASAP. My apologies for all the delays...

Patch Set 11 ("Fixes gsutil upload") has a successful run:
http://build.chromium.org/p/tryserver.chromium/builders/android_fyi_dbg_trigg....
The link is emitted incorrectly, and should point to
https://storage.googleapis.com/chromium-code-coverage/try-fyi-tests-dbg/21847...
(fixed in PS 12).

Also, adding joth for review on the change to CleanupReferenceTest.

https://codereview.chromium.org/20210002/diff/170001/android_webview/android_...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/20210002/diff/170001/android_webview/android_...
android_webview/android_webview.gyp:203: 'emma_should_instrument': 1,
On 2013/08/20 15:27:24, cjhopman wrote:
> Maybe this should default to 1 and be disabled if there are any targets that
we
> don't want to have instrumented.

I went with an inclusive system because there are *lots* of libraries that we
don't want to instrument, and only a few java sources that we actually do want.

https://codereview.chromium.org/20210002/diff/170001/build/android/gyp/dex.py
File build/android/gyp/dex.py (left):

https://codereview.chromium.org/20210002/diff/170001/build/android/gyp/dex.py...
build/android/gyp/dex.py:19: # See http://crbug.com/272064 for context on
--force-jumbo.
On 2013/08/20 15:27:24, cjhopman wrote:
> Nit: keep this comment

Oops, must have lost that during a rebase. Fixed.

https://codereview.chromium.org/20210002/diff/170001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/20210002/diff/170001/build/java.gypi#newcode89
build/java.gypi:89: 'javac_jar_path': '<(jar_path)'
On 2013/08/20 15:27:24, cjhopman wrote:
> I'm not sure that you can expand jar_path here (since it's defined in an outer
> variables block).

But doesn't the previous code also try to expand jar_name, which is defined in
the outer block? As far as I can tell this hasn't broken anything, but maybe
progaurd_preprocess is 1 by default?

[bulach, 2013-08-20 17:42:34]

thanks!!! I was writing some comments, but then realized this is just way, way
too big, and will require way too many reviewers at once, so then we'll cross
each other's stream (I ended up reviewing a previous patchset) and won't be
productive for anyone.

let's split this into multiple, semi-independent steps, and then it'll be easier
to digest...

1) add "emma" support to gyp ==> cjhopman can review that, and it shouldn't
change any existing behavior..

2) add "coverage" option to test runner ==> frankf is the best person... again,
it's an "option" that won't be exercised by any bot yet, so shouldn't change
existing behavior..


3) add buildbot support ==> ilevy.. this one will then exercise all of the
above, but will hopefully be just adding a special config to an existing bot, so
shouldn't be too controversial..

please let me know your thoughts on this workflow, I hope it will help
everybody.. even if it seems a bit more work at first, it'll hopefully finish
earlier than a monolithic patch. :)

https://codereview.chromium.org/20210002/diff/179001/android_webview/android_...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/20210002/diff/179001/android_webview/android_...
android_webview/android_webview.gyp:203: 'emma_should_instrument': 1,
hmm... it seems that "emma_should_instrument" actually depends on emma_coverage,
is that right? could we just use "emma_coverage" via a gyp defines?

it looks really weird this "emma_should_instrument" being unconditionally set to
1 here :) I think by default it should _not_ be instrumented, and it's not clear
that this will not really instrument if emma_coverage is not set..

https://codereview.chromium.org/20210002/diff/179001/build/android/buildbot/b...
File build/android/buildbot/bb_device_steps.py (right):

https://codereview.chromium.org/20210002/diff/179001/build/android/buildbot/b...
build/android/buildbot/bb_device_steps.py:30: REVISION_FILENAME =
'FULL_BUILD_REVISION'
what's this?
it'd be nice to reuse the one we have for downstream, see:
${DOWNSTREAM}/build/perf_generate_rev_lsit.py

https://codereview.chromium.org/20210002/diff/179001/build/android/buildbot/b...
build/android/buildbot/bb_device_steps.py:57: annotation='SmallTest'),
why only SmallTest?

https://codereview.chromium.org/20210002/diff/179001/build/android/buildbot/b...
build/android/buildbot/bb_device_steps.py:63:
host_driven_root=constants.CHROMIUM_TEST_SHELL_HOST_DRIVEN_DIR,
unrelated?

https://codereview.chromium.org/20210002/diff/179001/build/android/buildbot/b...
build/android/buildbot/bb_device_steps.py:292: ['ls', '-l',
'build/android/generate_emma_html.py'])
not needed?

https://codereview.chromium.org/20210002/diff/179001/build/android/buildbot/b...
File build/android/buildbot/bb_run_bot.py (right):

https://codereview.chromium.org/20210002/diff/179001/build/android/buildbot/b...
build/android/buildbot/bb_run_bot.py:52: init_env['BUILDBOT_ID'] = bot_id
what is this? is it the same as BUILDBOT_BUILDERNAME?

https://codereview.chromium.org/20210002/diff/179001/build/android/buildbot/b...
build/android/buildbot/bb_run_bot.py:120: std_tests = ['ui']
what about unit?

https://codereview.chromium.org/20210002/diff/179001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/20210002/diff/179001/build/common.gypi#newcod...
build/common.gypi:915: # Set to 1 to enable native code coverage.  In addition
to build changes
"native" is an android concept, this is common to all platform. please keep
it...

https://codereview.chromium.org/20210002/diff/179001/content/public/android/j...
File
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java
(right):

https://codereview.chromium.org/20210002/diff/179001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java:58:
@DisabledTest
?!

[gkanwar1, 2013-08-20 17:50:59]

Marcus -- Thanks for the suggestion! I think you're right that breaking this up
will speed things up, and that division seems good to me. I'm going to work on
doing that now. If possible I'd like to shoot for landing both the GYP and
Python changes today. The buildbot changes should be small enough that once the
two other changes are done, we can land it quickly either tonight or tomorrow
morning.

Regarding your comments -- many of the things you pointed our were temporary
changes for testing, which I reverted in the latest patch set. I'll make some
fixes for things that do still need to be fixed in the new CLs.

[bulach, 2013-08-20 18:31:13]

thanks!! btw, I understand you're time-constrained, and I'm not your host :) but
if it serves of any help: I truly appreciate this work, and even if you don't
manage to land all the parts, it'll be pretty easy for anyone to pick it up and
land the remaining bits... 

so rather than feeling pressured to "land" everything, make sure you have all
the parts clearly documented and that someone will be able to pick this up
without much pain..

[bulach, 2013-08-20 18:33:32]

oh, and finally: don't be blocked on me.. I think all of these changes have
OWNERS in your timezone..

[joth, 2013-08-20 18:34:22]

yes I think you can land the .java changes in its own patch

https://codereview.chromium.org/20210002/diff/143004/content/public/android/j...
File
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java
(left):

https://codereview.chromium.org/20210002/diff/143004/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java:57:
ReferredObject instance = new ReferredObject();
Use AtomicReference here, and then you won't need to make it a member.

(Maybe)


AtomicReference<ReferredObject> instance =
        new AtomicReference<ReferredObject>(new ReferredObject());

https://codereview.chromium.org/20210002/diff/143004/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java:60:
instance = null;
instance.set(null);
instance = null;

https://codereview.chromium.org/20210002/diff/143004/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java:61:
collectGarbage();
Can also try

instance == null
AssertTrue(instance == null);   // ensure compiler isn't futzing the assignment

[gkanwar1, 2013-08-20 20:04:00]

On 2013/08/20 18:34:22, joth wrote:
> yes I think you can land the .java changes in its own patch
> 
>
https://codereview.chromium.org/20210002/diff/143004/content/public/android/j...
> File
>
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java
> (left):
> 
>
https://codereview.chromium.org/20210002/diff/143004/content/public/android/j...
>
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java:57:
> ReferredObject instance = new ReferredObject();
> Use AtomicReference here, and then you won't need to make it a member.
> 
> (Maybe)
> 
> 
> AtomicReference<ReferredObject> instance =
>         new AtomicReference<ReferredObject>(new ReferredObject());
> 
>
https://codereview.chromium.org/20210002/diff/143004/content/public/android/j...
>
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java:60:
> instance = null;
> instance.set(null);
> instance = null;
> 
>
https://codereview.chromium.org/20210002/diff/143004/content/public/android/j...
>
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java:61:
> collectGarbage();
> Can also try
> 
> instance == null
> AssertTrue(instance == null);   // ensure compiler isn't futzing the
assignment

The following CLs exist:
* Python -- Reviewed by Frank and landed:
https://codereview.chromium.org/23293006/
* Small Java changes -- Reviewed by Joth, waiting on trybots to land:
https://codereview.chromium.org/23253005/#ps6001
* GYP changes -- Waiting on review from Chris:
https://codereview.chromium.org/22870021/
* Buildbot changes -- Not published yet, waiting on GYP changes:
https://codereview.chromium.org/23345003/

Looks like the latest patch set in this overall CL passed, and outputted correct
coverage data, so we once these get landed everything should be in working
order!

[gkanwar1, 2013-08-24 01:57:28]

On 2013/08/20 20:04:00, gkanwar1 wrote:
> On 2013/08/20 18:34:22, joth wrote:
> > yes I think you can land the .java changes in its own patch
> > 
> >
>
https://codereview.chromium.org/20210002/diff/143004/content/public/android/j...
> > File
> >
>
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java
> > (left):
> > 
> >
>
https://codereview.chromium.org/20210002/diff/143004/content/public/android/j...
> >
>
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java:57:
> > ReferredObject instance = new ReferredObject();
> > Use AtomicReference here, and then you won't need to make it a member.
> > 
> > (Maybe)
> > 
> > 
> > AtomicReference<ReferredObject> instance =
> >         new AtomicReference<ReferredObject>(new ReferredObject());
> > 
> >
>
https://codereview.chromium.org/20210002/diff/143004/content/public/android/j...
> >
>
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java:60:
> > instance = null;
> > instance.set(null);
> > instance = null;
> > 
> >
>
https://codereview.chromium.org/20210002/diff/143004/content/public/android/j...
> >
>
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java:61:
> > collectGarbage();
> > Can also try
> > 
> > instance == null
> > AssertTrue(instance == null);   // ensure compiler isn't futzing the
> assignment
> 
> The following CLs exist:
> * Python -- Reviewed by Frank and landed:
> https://codereview.chromium.org/23293006/
> * Small Java changes -- Reviewed by Joth, waiting on trybots to land:
> https://codereview.chromium.org/23253005/#ps6001
> * GYP changes -- Waiting on review from Chris:
> https://codereview.chromium.org/22870021/
> * Buildbot changes -- Not published yet, waiting on GYP changes:
> https://codereview.chromium.org/23345003/
> 
> Looks like the latest patch set in this overall CL passed, and outputted
correct
> coverage data, so we once these get landed everything should be in working
> order!

Closing this meta-patch, since all the pieces have landed.