Coverage API revamp

Coverage API supports proper filtering

(Partly) addressing concerns of https://codereview.chromium.org/346003003/

BUG=
R=johnmccutchan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37793

[Michael Lippautz (Google), 2014-06-27 00:16:48]

RFC

This partly addresses the issues that came up with
https://codereview.chromium.org/346003003/.

* filters: Leaking implementation details are fixed by using filters.
* single script in multiple libraries: Unfortunately this is valid...


Since script objects are not canonicalized (correct me if I am wrong) we could
either 

a) filter scripts by url and not actual script objects. (We could get rid of
Script::FindByUrl() again) This would solve all our present issues.

b) change the way scripts are accessed to
    
    isolate/id/libraries/id/scripts/url (and /coverage).

   However, this way we end up ``loosing'' coverage information in the case
where data is only obtained for a single script (as you only get data for the
current library context).

c) change Script::FindByUrl() to return an array of Script objects. We would
need to adjust the CoverageFilter to take arrays in this case (?)

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/coverage.h
File runtime/vm/coverage.h (right):

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/coverage.h#ne...
runtime/vm/coverage.h:28: virtual bool GetCoverage(const Library& lib,
Performance of using only one filter function (and therefore iterate over all
libraries, classes, and functions) is 3x slower than with fine-grained (separate
methods) filtering. 

Still in domain of milliseconds (~30ms).

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:154: class LibraryFilter : public CoverageFilter {
We could name those to SimpleXXXFilter and include them in coverage.h?

[Cutch, 2014-06-27 15:07:19]

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/coverage.cc
File runtime/vm/coverage.cc (right):

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/coverage.cc#n...
runtime/vm/coverage.cc:253: const GrowableObjectArray& libs =
GrowableObjectArray::Handle(
kill all the filter != NULL checks by adding:

class CoverageFilterAll : public CoverageFilter {
  virtual bool ShouldOutputCoverageFor(...) { return true; }
}

And inside this function:
CoverageFilterAll default_filter;
if (filter == NULL) {
  filter = &default_filter;
}

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/coverage.h
File runtime/vm/coverage.h (right):

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/coverage.h#ne...
runtime/vm/coverage.h:28: virtual bool GetCoverage(const Library& lib,
Maybe rename to "ShouldOutputCoverageFor"

Also, mark it as const.

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:154: class LibraryFilter : public CoverageFilter {
On 2014/06/27 00:16:48, Michael Lippautz wrote:
> We could name those to SimpleXXXFilter and include them in coverage.h?

I think it's okay to leave them here with the names:

LibraryCoverageFilter
ScriptCoverageFilter
...

[Michael Lippautz (Google), 2014-06-27 17:12:25]

PTAL

* removed helper methods from script object
* service finds the first script object (which is good enough)
* filters for script are based on url

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/coverage.cc
File runtime/vm/coverage.cc (right):

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/coverage.cc#n...
runtime/vm/coverage.cc:253: const GrowableObjectArray& libs =
GrowableObjectArray::Handle(
On 2014/06/27 15:07:18, Cutch wrote:
> kill all the filter != NULL checks by adding:
> 
> class CoverageFilterAll : public CoverageFilter {
>   virtual bool ShouldOutputCoverageFor(...) { return true; }
> }
> 
> And inside this function:
> CoverageFilterAll default_filter;
> if (filter == NULL) {
>   filter = &default_filter;
> }

Done.

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/coverage.h
File runtime/vm/coverage.h (right):

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/coverage.h#ne...
runtime/vm/coverage.h:28: virtual bool GetCoverage(const Library& lib,
On 2014/06/27 15:07:19, Cutch wrote:
> Maybe rename to "ShouldOutputCoverageFor"
> 
> Also, mark it as const.

Done.

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/351373002/diff/20001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:154: class LibraryFilter : public CoverageFilter {
On 2014/06/27 15:07:19, Cutch wrote:
> On 2014/06/27 00:16:48, Michael Lippautz wrote:
> > We could name those to SimpleXXXFilter and include them in coverage.h?
> 
> I think it's okay to leave them here with the names:
> 
> LibraryCoverageFilter
> ScriptCoverageFilter
> ...

Done.

[Cutch, 2014-06-27 17:20:12]

lgtm

https://codereview.chromium.org/351373002/diff/60001/runtime/vm/coverage.cc
File runtime/vm/coverage.cc (right):

https://codereview.chromium.org/351373002/diff/60001/runtime/vm/coverage.cc#n...
runtime/vm/coverage.cc:29: ~CoverageFilterAll() {}
Destructor is not necessary

https://codereview.chromium.org/351373002/diff/60001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/351373002/diff/60001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:157: ~LibraryCoverageFilter() {}
These destructors aren't necessary.

[Michael Lippautz (Google), 2014-06-27 17:51:25]

https://codereview.chromium.org/351373002/diff/60001/runtime/vm/coverage.cc
File runtime/vm/coverage.cc (right):

https://codereview.chromium.org/351373002/diff/60001/runtime/vm/coverage.cc#n...
runtime/vm/coverage.cc:29: ~CoverageFilterAll() {}
On 2014/06/27 17:20:12, Cutch wrote:
> Destructor is not necessary

Done.

https://codereview.chromium.org/351373002/diff/60001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/351373002/diff/60001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:157: ~LibraryCoverageFilter() {}
On 2014/06/27 17:20:12, Cutch wrote:
> These destructors aren't necessary.

Done.

[Michael Lippautz (Google), 2014-06-27 17:52:02]

Committed patchset #5 manually as r37793 (presubmit successful).