Trace revision and v8 version for IterationInfo

Trace revision and v8 version for IterationInfo

Metadata now contains fields including
{os-arch: "x86_64",
 os-name: "Linux",
 os-version: "3.13.0-76-generic",
 product-version: "Chrome/53.0.2771.0",
 v8-version: "5.3.332.1",
 revision: "1641c3f49dbd670a0278e0e5eecb56d96dbf2a9d-refs/heads/master@{#400465}",
 user-agent: "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2771.0 Safari/537.36"}

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrpH8/edit#

https://github.com/catapult-project/catapult/issues/2405
BUG=catapult:#2405

Committed: https://crrev.com/e213694e24fef0519e8796ce271b7816cab730de
Cr-Commit-Position: refs/heads/master@{#406605}

[benjhayden, 2016-06-17 20:17:52]

Description was changed from

==========
Trace revision and blink version

BUG=catapult:#2405
==========

to

==========
Trace revision and blink version for IterationInfo

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

BUG=catapult:#2405
==========

[benjhayden, 2016-06-17 20:21:47]

Description was changed from

==========
Trace revision and blink version for IterationInfo

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

BUG=catapult:#2405
==========

to

==========
Trace revision and blink version for IterationInfo

Metadata now contains fields including
{blink-version: "537.36 (@1641c3f49dbd670a0278e0e5eecb56d96dbf2a9d)",
 os-arch: "x86_64",
 os-name: "Linux",
 os-version: "3.13.0-76-generic",
 product-version: "Chrome/53.0.2771.0",
 revision:
"1641c3f49dbd670a0278e0e5eecb56d96dbf2a9d-refs/heads/master@{#400465}",
 user-agent: "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like
Gecko) Chrome/53.0.2771.0 Safari/537.36"}

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

BUG=catapult:#2405
==========

[benjhayden, 2016-06-17 20:24:31]

benjhayden@chromium.org changed reviewers:
+ nednguyen@google.com, primiano@chromium.org

[benjhayden, 2016-06-17 20:24:32]

Some of these composite fields contain redundant data. Should the sub-fields
(blink-major, blink-minor, revision, product-name, product-version, etc.) be
split out to reduce duplication?

[nednguyen, 2016-06-17 20:38:12]

lgtm but of course Primiano's the code owner here

https://codereview.chromium.org/2072383002/diff/1/content/browser/tracing/tra...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/2072383002/diff/1/content/browser/tracing/tra...
content/browser/tracing/tracing_controller_impl.cc:119:
metadata_dict->SetString("revision", GetContentClient()->GetLastChange());
Can we also have v8 revision? :)

[Primiano Tucci (use gerrit), 2016-06-21 06:34:09]

https://codereview.chromium.org/2072383002/diff/20001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2072383002/diff/20001/chrome/common/chrome_co...
chrome/common/chrome_content_client.cc:616: return
version_info::GetLastChange();
Why this plumbing here? This seems to defeat the purpose of version_info.h being
a component.

https://codereview.chromium.org/2072383002/diff/20001/content/browser/tracing...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/2072383002/diff/20001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl.cc:118:
metadata_dict->SetString("blink-version", content::GetWebKitVersion());
what do we need this for now that blink lives in the chrome repo? why isn't the
revision enough?

[benjhayden, 2016-06-21 23:30:36]

Thanks! PTAL -- should I add a rule to let content include version_info? I am
unfamiliar with components.

https://codereview.chromium.org/2072383002/diff/20001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2072383002/diff/20001/chrome/common/chrome_co...
chrome/common/chrome_content_client.cc:616: return
version_info::GetLastChange();
On 2016/06/21 at 06:34:09, Primiano Tucci wrote:
> Why this plumbing here? This seems to defeat the purpose of version_info.h
being a component.

** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  content/browser/tracing/tracing_controller_impl.cc
      Illegal include: "components/version_info/version_info.h"
    Because of no rule applying.

Should I add a rule?

https://codereview.chromium.org/2072383002/diff/20001/content/browser/tracing...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/2072383002/diff/20001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl.cc:118:
metadata_dict->SetString("blink-version", content::GetWebKitVersion());
On 2016/06/21 at 06:34:09, Primiano Tucci wrote:
> what do we need this for now that blink lives in the chrome repo? why isn't
the revision enough?

Done.

[benjhayden, 2016-06-21 23:39:06]

Description was changed from

==========
Trace revision and blink version for IterationInfo

Metadata now contains fields including
{blink-version: "537.36 (@1641c3f49dbd670a0278e0e5eecb56d96dbf2a9d)",
 os-arch: "x86_64",
 os-name: "Linux",
 os-version: "3.13.0-76-generic",
 product-version: "Chrome/53.0.2771.0",
 revision:
"1641c3f49dbd670a0278e0e5eecb56d96dbf2a9d-refs/heads/master@{#400465}",
 user-agent: "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like
Gecko) Chrome/53.0.2771.0 Safari/537.36"}

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

BUG=catapult:#2405
==========

to

==========
Trace revision and v8 version for IterationInfo

Metadata now contains fields including
{os-arch: "x86_64",
 os-name: "Linux",
 os-version: "3.13.0-76-generic",
 product-version: "Chrome/53.0.2771.0",
 revision:
"1641c3f49dbd670a0278e0e5eecb56d96dbf2a9d-refs/heads/master@{#400465}",
 user-agent: "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like
Gecko) Chrome/53.0.2771.0 Safari/537.36"}

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

BUG=catapult:#2405
==========

[Primiano Tucci (use gerrit), 2016-06-22 08:29:10]

speculative LGTM.
Probably better if we directly use version_info as discussed above. But LGTM
anyways even if for some reason that should turn out to be a bad idea / not
feasible whatever.

https://codereview.chromium.org/2072383002/diff/20001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2072383002/diff/20001/chrome/common/chrome_co...
chrome/common/chrome_content_client.cc:616: return
version_info::GetLastChange();
On 2016/06/21 23:30:36, benjhayden_chromium wrote:
> On 2016/06/21 at 06:34:09, Primiano Tucci wrote:
> > Why this plumbing here? This seems to defeat the purpose of version_info.h
> being a component.
> 
> ** Presubmit ERRORS **
> You added one or more #includes that violate checkdeps rules.
>   content/browser/tracing/tracing_controller_impl.cc
>       Illegal include: "components/version_info/version_info.h"
>     Because of no rule applying.
> 
> Should I add a rule?

Yup, add it to content/browser/tracing/DEPS and then you'll need a stamp from
one of the version_info owners (See components/version_info/OWNERS)

[benjhayden, 2016-06-22 17:19:00]

benjhayden@chromium.org changed reviewers:
+ thakis@chromium.org

[benjhayden, 2016-06-22 17:19:00]

+thakis, PTAL -- stamp for DEPS on version_info?

[Nico, 2016-06-22 18:11:12]

lgtm

[benjhayden, 2016-06-22 19:07:46]

The CQ bit was checked by benjhayden@chromium.org

[benjhayden, 2016-06-22 19:07:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2072383002/#ps60001
(title: "use version_info component")

[commit-bot: I haz the power, 2016-06-22 19:08:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2072383002/60001

[commit-bot: I haz the power, 2016-06-22 19:13:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-22 19:13:13]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[benjhayden, 2016-06-23 22:15:07]

The CQ bit was checked by benjhayden@chromium.org

[benjhayden, 2016-06-23 22:15:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, nednguyen@google.com, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2072383002/#ps80001
(title: "dep")

[commit-bot: I haz the power, 2016-06-23 22:16:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2072383002/80001

[commit-bot: I haz the power, 2016-06-23 22:24:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-23 22:24:12]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[benjhayden, 2016-06-23 22:54:40]

benjhayden@chromium.org changed reviewers:
+ clamy@chromium.org

[benjhayden, 2016-06-23 22:54:40]

+clamy PTAL -- stamp for content/ owners?

[clamy, 2016-06-24 11:32:12]

https://codereview.chromium.org/2072383002/diff/80001/content/browser/tracing...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/2072383002/diff/80001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl.cc:119:
metadata_dict->SetString("revision", version_info::GetLastChange());
How does this work with non-Chrome embedders?

[Primiano Tucci (use gerrit), 2016-06-24 12:39:43]

https://codereview.chromium.org/2072383002/diff/80001/content/browser/tracing...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/2072383002/diff/80001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl.cc:119:
metadata_dict->SetString("revision", version_info::GetLastChange());
On 2016/06/24 11:32:12, clamy wrote:
> How does this work with non-Chrome embedders?

version_info is not a chrome thingy. It's a component.

[clamy, 2016-06-24 14:29:43]

Having content/ depend on components/ seems to me to be the wrong way around. I
know that there are some exceptions for html_viewer (aka mandoline), but they
should be removed. I'll check with other content/ owners whether this is
something we want to allow at all.

[Primiano Tucci (use gerrit), 2016-06-24 15:36:29]

On 2016/06/24 14:29:43, clamy wrote:
> Having content/ depend on components/ seems to me to be the wrong way around.
I
> know that there are some exceptions for html_viewer (aka mandoline), but they
> should be removed.
There seems to be quite a few
(https://cs.chromium.org/search/?q=f:%5Esrc/content.*DEPS+components/&sq=packa...)
My line of reasoning here was that version_info is an independent standalong
thing which doesn't belong neither to chrome nor to content, and that various
pieces of the codebase could use without pulling in weird unneeded dependencies.
In essence something like base/ which doesn't need to live in base.

[jam, 2016-06-27 16:10:44]

It seems this information is redundant. i.e. product-version has enough
information to get back to the revision of all the repos that we bring in via
DEPS.

[benjhayden, 2016-06-27 18:00:07]

On 2016/06/27 at 16:10:44, jam wrote:
> It seems this information is redundant. i.e. product-version has enough
information to get back to the revision of all the repos that we bring in via
DEPS.

This information is for telemetry, which can be run at any revision, not just
product versions.

[jam, 2016-06-27 19:52:07]

On 2016/06/27 18:00:07, benjhayden_chromium wrote:
> On 2016/06/27 at 16:10:44, jam wrote:
> > It seems this information is redundant. i.e. product-version has enough
> information to get back to the revision of all the repos that we bring in via
> DEPS.
> 
> This information is for telemetry, which can be run at any revision, not just
> product versions.

To make sure I'm understanding correctly: so you want data from user builds or
bots etc where the version isn't updated for every deps roll right?

There's already a way to let the embedder specify this:
TracingDelegate::TracingDelegate. So you can add the
version_info::GetLastChange() in ChromeTracingDelegate::GenerateMetadataDict.
That should be sufficient, and also enough to work the v8 version from right?

[benjhayden, 2016-06-28 00:24:16]

On 2016/06/27 at 19:52:07, jam wrote:
> On 2016/06/27 18:00:07, benjhayden_chromium wrote:
> > On 2016/06/27 at 16:10:44, jam wrote:
> > > It seems this information is redundant. i.e. product-version has enough
> > information to get back to the revision of all the repos that we bring in
via
> > DEPS.
> > 
> > This information is for telemetry, which can be run at any revision, not
just
> > product versions.
> 
> To make sure I'm understanding correctly: so you want data from user builds or
bots etc where the version isn't updated for every deps roll right?
> 
> There's already a way to let the embedder specify this:
TracingDelegate::TracingDelegate. So you can add the
version_info::GetLastChange() in ChromeTracingDelegate::GenerateMetadataDict.
That should be sufficient, and also enough to work the v8 version from right?

Is ChromeTracingDelegate linked into webview? content shell?

[Primiano Tucci (use gerrit), 2016-06-28 07:20:58]

On 2016/06/28 00:24:16, benjhayden_chromium wrote:
> Is ChromeTracingDelegate linked into webview? content shell?

Don't think so. jam@, I think this is the culprit. Ben needs this information in
something embedded-neutral. Unfortunately the tracing code in base/ cannot know
about version info because that's too much for base. 
If depending on version_info is breaking some layering I'm happy from my side to
have this patch to go back plumbing via content_client, like for the V8
revision.

[jam, 2016-06-28 15:02:21]

On 2016/06/28 07:20:58, Primiano Tucci wrote:
> On 2016/06/28 00:24:16, benjhayden_chromium wrote:
> > Is ChromeTracingDelegate linked into webview? content shell?
> 
> Don't think so. jam@, I think this is the culprit. Ben needs this information
in
> something embedded-neutral. 

I didn't know that. There's a bug link that doesn't work, and the design doc
doesn't describe which products it runs in.

Can you explain when/where you need this to work?

If you say you need it in all embedders of content, then that's different from
what this cl does which is only update chrome's ContentClient implementation.


> Unfortunately the tracing code in base/ cannot know
> about version info because that's too much for base. 
> If depending on version_info is breaking some layering I'm happy from my side
to
> have this patch to go back plumbing via content_client, like for the V8
> revision.

[benjhayden, 2016-06-28 20:37:39]

Description was changed from

==========
Trace revision and v8 version for IterationInfo

Metadata now contains fields including
{os-arch: "x86_64",
 os-name: "Linux",
 os-version: "3.13.0-76-generic",
 product-version: "Chrome/53.0.2771.0",
 revision:
"1641c3f49dbd670a0278e0e5eecb56d96dbf2a9d-refs/heads/master@{#400465}",
 user-agent: "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like
Gecko) Chrome/53.0.2771.0 Safari/537.36"}

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

BUG=catapult:#2405
==========

to

==========
Trace revision and v8 version for IterationInfo

Metadata now contains fields including
{os-arch: "x86_64",
 os-name: "Linux",
 os-version: "3.13.0-76-generic",
 product-version: "Chrome/53.0.2771.0",
 revision:
"1641c3f49dbd670a0278e0e5eecb56d96dbf2a9d-refs/heads/master@{#400465}",
 user-agent: "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like
Gecko) Chrome/53.0.2771.0 Safari/537.36"}

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

https://github.com/catapult-project/catapult/issues/2405
BUG=catapult:#2405
==========

[benjhayden, 2016-06-28 22:08:09]

On 2016/06/28 at 15:02:21, jam wrote:
> On 2016/06/28 07:20:58, Primiano Tucci wrote:
> > On 2016/06/28 00:24:16, benjhayden_chromium wrote:
> > > Is ChromeTracingDelegate linked into webview? content shell?
> > 
> > Don't think so. jam@, I think this is the culprit. Ben needs this
information in
> > something embedded-neutral. 
> 
> I didn't know that. There's a bug link that doesn't work, and the design doc
doesn't describe which products it runs in.

Sorry! The bug link works in the new version of codereview, but I added a direct
link. I don't think that the bug doesn't contain anything particularly relevant
to this discussion.
https://github.com/catapult-project/catapult/issues/2405

> 
> Can you explain when/where you need this to work?
> 
> If you say you need it in all embedders of content, then that's different from
what this cl does which is only update chrome's ContentClient implementation.

So maybe GetV8Version() should be in the ContentClient base implementation?

> 
> 
> > Unfortunately the tracing code in base/ cannot know
> > about version info because that's too much for base. 
> > If depending on version_info is breaking some layering I'm happy from my
side to
> > have this patch to go back plumbing via content_client, like for the V8
> > revision.

[jam, 2016-06-28 22:58:03]

On 2016/06/28 22:08:09, benjhayden_chromium wrote:
> On 2016/06/28 at 15:02:21, jam wrote:
> > On 2016/06/28 07:20:58, Primiano Tucci wrote:
> > > On 2016/06/28 00:24:16, benjhayden_chromium wrote:
> > > > Is ChromeTracingDelegate linked into webview? content shell?
> > > 
> > > Don't think so. jam@, I think this is the culprit. Ben needs this
> information in
> > > something embedded-neutral. 
> > 
> > I didn't know that. There's a bug link that doesn't work, and the design doc
> doesn't describe which products it runs in.
> 
> Sorry! The bug link works in the new version of codereview, but I added a
direct
> link. I don't think that the bug doesn't contain anything particularly
relevant
> to this discussion.
> https://github.com/catapult-project/catapult/issues/2405
> 
> > 
> > Can you explain when/where you need this to work?

The link doesn't quite describe this to me.
-does this need to run on all waterfalls? for content_shell, chrome, webview?
-it also discusses recipes sending data. is this still happening?
-will this link to the buildbot run? that would have information on revision

can we VC when you're free so I can better understand this?

> > 
> > If you say you need it in all embedders of content, then that's different
from
> what this cl does which is only update chrome's ContentClient implementation.
> 
> So maybe GetV8Version() should be in the ContentClient base implementation?
> 
> > 
> > 
> > > Unfortunately the tracing code in base/ cannot know
> > > about version info because that's too much for base. 
> > > If depending on version_info is breaking some layering I'm happy from my
> side to
> > > have this patch to go back plumbing via content_client, like for the V8
> > > revision.

[jam, 2016-06-28 23:24:22]

Ok Ned and I discussed this in person and he helped me understand this better.
He said we only need this for webview & chrome.

In the interest of not adding more dependencies from content to components
(we're trying to reduce these), especially ones related to version which we've
historically kept content oblivious too:
-v8::V8::GetVersion() can be called from content/ (it already includes v8/, and
I'm not sure I understand the difference between content/browser including v8 vs
chrome/common, since both are running in the browser process)
-metadata_dict->SetString("revision", version_info::GetLastChange()); can be put
in chrome's ChromeTracingDelegate::GenerateMetadataDict and an implementation
that goes in webview. that's one line of duplication, but it does avoid content
growing more dependencies.

[nednguyen, 2016-06-28 23:45:13]

Description was changed from

==========
Trace revision and v8 version for IterationInfo

Metadata now contains fields including
{os-arch: "x86_64",
 os-name: "Linux",
 os-version: "3.13.0-76-generic",
 product-version: "Chrome/53.0.2771.0",
 revision:
"1641c3f49dbd670a0278e0e5eecb56d96dbf2a9d-refs/heads/master@{#400465}",
 user-agent: "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like
Gecko) Chrome/53.0.2771.0 Safari/537.36"}

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

https://github.com/catapult-project/catapult/issues/2405
BUG=catapult:#2405
==========

to

==========
Trace revision and v8 version for IterationInfo

Metadata now contains fields including
{os-arch: "x86_64",
 os-name: "Linux",
 os-version: "3.13.0-76-generic",
 product-version: "Chrome/53.0.2771.0",
 revision:
"1641c3f49dbd670a0278e0e5eecb56d96dbf2a9d-refs/heads/master@{#400465}",
 user-agent: "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like
Gecko) Chrome/53.0.2771.0 Safari/537.36"}

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

https://github.com/catapult-project/catapult/issues/2405
BUG=catapult:#2405
==========

[nednguyen, 2016-06-28 23:45:13]

nednguyen@google.com changed reviewers:
+ vmiura@chromium.org

[benjhayden, 2016-06-28 23:45:14]

On 2016/06/28 at 23:24:22, jam wrote:
> Ok Ned and I discussed this in person and he helped me understand this better.
He said we only need this for webview & chrome.
> 
> In the interest of not adding more dependencies from content to components
(we're trying to reduce these), especially ones related to version which we've
historically kept content oblivious too:
> -v8::V8::GetVersion() can be called from content/ (it already includes v8/,
and I'm not sure I understand the difference between content/browser including
v8 vs chrome/common, since both are running in the browser process)
> -metadata_dict->SetString("revision", version_info::GetLastChange()); can be
put in chrome's ChromeTracingDelegate::GenerateMetadataDict and an
implementation that goes in webview. that's one line of duplication, but it does
avoid content growing more dependencies.

Sure, that makes sense. Clarifying question: by "an implementation that goes in
webview", do you mean that I need to implement a WebviewContentBrowserClient
with a GetTracingDelegate() that returns a WebviewTracingDelegate with a
GenerateMetadataDict() that calls version_info::GetLastChange()?

[nednguyen, 2016-06-28 23:47:49]

nednguyen@google.com changed reviewers:
- vmiura@chromium.org

[benjhayden, 2016-06-29 18:04:58]

Talked with jam offline, AwContentBrowserClient already exists, it just need a
TracingDelegate.
I'll get a new patch up soon.
Thanks!

[benjhayden, 2016-07-01 18:27:00]

Description was changed from

==========
Trace revision and v8 version for IterationInfo

Metadata now contains fields including
{os-arch: "x86_64",
 os-name: "Linux",
 os-version: "3.13.0-76-generic",
 product-version: "Chrome/53.0.2771.0",
 revision:
"1641c3f49dbd670a0278e0e5eecb56d96dbf2a9d-refs/heads/master@{#400465}",
 user-agent: "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like
Gecko) Chrome/53.0.2771.0 Safari/537.36"}

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

https://github.com/catapult-project/catapult/issues/2405
BUG=catapult:#2405
==========

to

==========
Trace revision and v8 version for IterationInfo

Metadata now contains fields including
{os-arch: "x86_64",
 os-name: "Linux",
 os-version: "3.13.0-76-generic",
 product-version: "Chrome/53.0.2771.0",
 v8-version: "5.3.332.1",
 revision:
"1641c3f49dbd670a0278e0e5eecb56d96dbf2a9d-refs/heads/master@{#400465}",
 user-agent: "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like
Gecko) Chrome/53.0.2771.0 Safari/537.36"}

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

https://github.com/catapult-project/catapult/issues/2405
BUG=catapult:#2405
==========

[benjhayden, 2016-07-01 20:01:55]

The CQ bit was checked by benjhayden@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-01 20:02:33]

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

[benjhayden, 2016-07-01 20:03:57]

benjhayden@chromium.org changed reviewers:
+ jam@chromium.org
- clamy@chromium.org

[benjhayden, 2016-07-01 20:03:58]

jam, ptal?

[commit-bot: I haz the power, 2016-07-01 20:09:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-01 20:09:02]

Dry run: Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[benjhayden, 2016-07-08 17:23:22]

Patchset #7 (id:120001) has been deleted

[benjhayden, 2016-07-08 17:24:08]

Patchset #7 (id:140001) has been deleted

[benjhayden, 2016-07-10 02:53:30]

Patchset #8 (id:180001) has been deleted

[benjhayden, 2016-07-10 02:53:39]

Patchset #7 (id:160001) has been deleted

[benjhayden, 2016-07-10 02:53:51]

Patchset #6 (id:100001) has been deleted

[benjhayden, 2016-07-10 03:31:44]

Patchset #6 (id:200001) has been deleted

[benjhayden, 2016-07-10 05:15:02]

Patchset #6 (id:220001) has been deleted

[jam, 2016-07-12 21:28:24]

https://codereview.chromium.org/2072383002/diff/240001/android_webview/browse...
File android_webview/browser/tracing/aw_tracing_delegate.h (right):

https://codereview.chromium.org/2072383002/diff/240001/android_webview/browse...
android_webview/browser/tracing/aw_tracing_delegate.h:19: 
nit: per convention, put content::TracingDelegate implementation: before line
17. also no blank lines between overridden methods on line 19

https://codereview.chromium.org/2072383002/diff/240001/android_webview/browse...
android_webview/browser/tracing/aw_tracing_delegate.h:22: private:
nit: no need

https://codereview.chromium.org/2072383002/diff/240001/chrome/common/chrome_c...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2072383002/diff/240001/chrome/common/chrome_c...
chrome/common/chrome_content_client.cc:623: return v8::V8::GetVersion();
no need to create a new ContentClient method. content/ can already include v8/,
so it should just do this directly.

[benjhayden, 2016-07-15 00:10:49]

The CQ bit was checked by benjhayden@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 00:11:08]

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

[benjhayden, 2016-07-15 01:22:09]

jam, ptal, is it ok to change content/DEPS from -v8 to +v8?

https://codereview.chromium.org/2072383002/diff/240001/chrome/common/chrome_c...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2072383002/diff/240001/chrome/common/chrome_c...
chrome/common/chrome_content_client.cc:623: return v8::V8::GetVersion();
On 2016/07/12 at 21:28:24, jam wrote:
> no need to create a new ContentClient method. content/ can already include
v8/, so it should just do this directly.

content/DEPS had "-v8" in its include_rules, so I removed that so that I could
do what you suggest.

[commit-bot: I haz the power, 2016-07-15 01:23:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-15 01:23:26]

Dry run: This issue passed the CQ dry run.

[jam, 2016-07-19 21:05:09]

On 2016/07/15 01:22:09, benjhayden_chromium wrote:
> jam, ptal, is it ok to change content/DEPS from -v8 to +v8?
> 
>
https://codereview.chromium.org/2072383002/diff/240001/chrome/common/chrome_c...
> File chrome/common/chrome_content_client.cc (right):
> 
>
https://codereview.chromium.org/2072383002/diff/240001/chrome/common/chrome_c...
> chrome/common/chrome_content_client.cc:623: return v8::V8::GetVersion();
> On 2016/07/12 at 21:28:24, jam wrote:
> > no need to create a new ContentClient method. content/ can already include
> v8/, so it should just do this directly.
> 
> content/DEPS had "-v8" in its include_rules, so I removed that so that I could
> do what you suggest.

since other than this one call, content/browser doens't call v8, i prefer a
specific DEPS entry so that we can ensure no other calls (i.e. ones which could
be expensive) creep in. so can you change content/browser/tracing/DEPS to add a
per-file dependency just for tracing_controller_impl.cc? and with that lgtm,
thanks

[nednguyen, 2016-07-19 21:12:17]

On 2016/07/19 21:05:09, jam wrote:
> On 2016/07/15 01:22:09, benjhayden_chromium wrote:
> > jam, ptal, is it ok to change content/DEPS from -v8 to +v8?
> > 
> >
>
https://codereview.chromium.org/2072383002/diff/240001/chrome/common/chrome_c...
> > File chrome/common/chrome_content_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/2072383002/diff/240001/chrome/common/chrome_c...
> > chrome/common/chrome_content_client.cc:623: return v8::V8::GetVersion();
> > On 2016/07/12 at 21:28:24, jam wrote:
> > > no need to create a new ContentClient method. content/ can already include
> > v8/, so it should just do this directly.
> > 
> > content/DEPS had "-v8" in its include_rules, so I removed that so that I
could
> > do what you suggest.
> 
> since other than this one call, content/browser doens't call v8, i prefer a
> specific DEPS entry so that we can ensure no other calls (i.e. ones which
could
> be expensive) creep in. so can you change content/browser/tracing/DEPS to add
a
> per-file dependency just for tracing_controller_impl.cc? and with that lgtm,
> thanks

Ben is out for a month, is this CL landable as-is? If not, who can take point on
landing this on his behalf?

[benjhayden, 2016-07-20 04:18:58]

The CQ bit was checked by benjhayden@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 04:19:30]

Dry run: 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-20 05:58:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 05:58:31]

Dry run: This issue passed the CQ dry run.

[benjhayden, 2016-07-20 17:37:53]

The CQ bit was checked by benjhayden@chromium.org

[benjhayden, 2016-07-20 17:37:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, nednguyen@google.com, thakis@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2072383002/#ps280001
(title: "specific_include_rules tracing_controller_impl.cc +v8")

[commit-bot: I haz the power, 2016-07-20 17:38:48]

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-20 17:44:27]

Committed patchset #8 (id:280001)

[commit-bot: I haz the power, 2016-07-20 17:44:54]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 17:47:05]

Description was changed from

==========
Trace revision and v8 version for IterationInfo

Metadata now contains fields including
{os-arch: "x86_64",
 os-name: "Linux",
 os-version: "3.13.0-76-generic",
 product-version: "Chrome/53.0.2771.0",
 v8-version: "5.3.332.1",
 revision:
"1641c3f49dbd670a0278e0e5eecb56d96dbf2a9d-refs/heads/master@{#400465}",
 user-agent: "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like
Gecko) Chrome/53.0.2771.0 Safari/537.36"}

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

https://github.com/catapult-project/catapult/issues/2405
BUG=catapult:#2405
==========

to

==========
Trace revision and v8 version for IterationInfo

Metadata now contains fields including
{os-arch: "x86_64",
 os-name: "Linux",
 os-version: "3.13.0-76-generic",
 product-version: "Chrome/53.0.2771.0",
 v8-version: "5.3.332.1",
 revision:
"1641c3f49dbd670a0278e0e5eecb56d96dbf2a9d-refs/heads/master@{#400465}",
 user-agent: "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like
Gecko) Chrome/53.0.2771.0 Safari/537.36"}

Design doc:
https://docs.google.com/document/d/12KvpnDUFUuCEgId7H-QL5PoRcsp99CXdqbZc-EOrp...

https://github.com/catapult-project/catapult/issues/2405
BUG=catapult:#2405

Committed: https://crrev.com/e213694e24fef0519e8796ce271b7816cab730de
Cr-Commit-Position: refs/heads/master@{#406605}
==========

[commit-bot: I haz the power, 2016-07-20 17:47:06]

Patchset 8 (id:??) landed as
https://crrev.com/e213694e24fef0519e8796ce271b7816cab730de
Cr-Commit-Position: refs/heads/master@{#406605}