Record executable version details to stability file

Record executable version details to stability file

BUG=620813
Committed: https://crrev.com/09c0c274ade68914487c187b2e99ab85f43b99a0
Cr-Commit-Position: refs/heads/master@{#437719}

[manzagop (departed), 2016-12-06 16:17:08]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 16:17:20]

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

[manzagop (departed), 2016-12-06 16:17:36]

[email protected] changed reviewers:
+ [email protected]

[manzagop (departed), 2016-12-06 16:17:37]

Hi Brian,
Please have a look!
Thanks,
Pierre

[commit-bot: I haz the power, 2016-12-06 17:19:07]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-12-06 17:19:07]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2016-12-06 19:28:43]

https://codereview.chromium.org/2556813002/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2556813002/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_field_trials_desktop.cc:130:
global_data.SetString(browser_watcher::kStabilityProduct,
Feel free to create a SetString method inside ActivityUserData that takes a
StringPiece16 and converts it to UTF8 internally.

https://codereview.chromium.org/2556813002/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_field_trials_desktop.cc:139:
global_data.SetString(browser_watcher::kStabilityPlatform, "Win32");
Shouldn't these be x86 and x64, given that the defined symbols are CPU
architectures and not strictly Windows architectures?

https://codereview.chromium.org/2556813002/diff/1/components/browser_watcher/...
File components/browser_watcher/stability_data_names.cc (right):

https://codereview.chromium.org/2556813002/diff/1/components/browser_watcher/...
components/browser_watcher/stability_data_names.cc:9: const char
kStabilityChannel[] = "channel";
Since these are global, is it worth having the strings indicate such?  e.g.
Chrome-Channel
Chrome-Platform
etc.

Or similar.  Just wondering if there might end up being, for example, a
"version" of something else that is also global.

[manzagop (departed), 2016-12-06 21:38:44]

The CQ bit was checked by [email protected] to run a CQ dry run

[manzagop (departed), 2016-12-06 21:39:03]

Thanks! Another look?

https://codereview.chromium.org/2556813002/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2556813002/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_field_trials_desktop.cc:130:
global_data.SetString(browser_watcher::kStabilityProduct,
On 2016/12/06 19:28:43, bcwhite wrote:
> Feel free to create a SetString method inside ActivityUserData that takes a
> StringPiece16 and converts it to UTF8 internally.

Done.

https://codereview.chromium.org/2556813002/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_field_trials_desktop.cc:139:
global_data.SetString(browser_watcher::kStabilityPlatform, "Win32");
On 2016/12/06 19:28:43, bcwhite wrote:
> Shouldn't these be x86 and x64, given that the defined symbols are CPU
> architectures and not strictly Windows architectures?

Yeah... These are the "platform" strings that the crash frontend expects from
windows clients. In fact, the above way of getting the product name/version/etc
is also windows specific.

https://codereview.chromium.org/2556813002/diff/1/components/browser_watcher/...
File components/browser_watcher/stability_data_names.cc (right):

https://codereview.chromium.org/2556813002/diff/1/components/browser_watcher/...
components/browser_watcher/stability_data_names.cc:9: const char
kStabilityChannel[] = "channel";
On 2016/12/06 19:28:43, bcwhite wrote:
> Since these are global, is it worth having the strings indicate such?  e.g.
> Chrome-Channel
> Chrome-Platform
> etc.
> 
> Or similar.  Just wondering if there might end up being, for example, a
> "version" of something else that is also global.

I think I prefer keeping these short and we can qualify more the more specific
ones. Sg?

[commit-bot: I haz the power, 2016-12-06 21:39:37]

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

[bcwhite, 2016-12-06 22:37:32]

lgtm

https://codereview.chromium.org/2556813002/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2556813002/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_field_trials_desktop.cc:139:
global_data.SetString(browser_watcher::kStabilityPlatform, "Win32");
On 2016/12/06 21:39:03, manzagop wrote:
> On 2016/12/06 19:28:43, bcwhite wrote:
> > Shouldn't these be x86 and x64, given that the defined symbols are CPU
> > architectures and not strictly Windows architectures?
> 
> Yeah... These are the "platform" strings that the crash frontend expects from
> windows clients. In fact, the above way of getting the product
name/version/etc
> is also windows specific.

Acknowledged.

https://codereview.chromium.org/2556813002/diff/1/components/browser_watcher/...
File components/browser_watcher/stability_data_names.cc (right):

https://codereview.chromium.org/2556813002/diff/1/components/browser_watcher/...
components/browser_watcher/stability_data_names.cc:9: const char
kStabilityChannel[] = "channel";
On 2016/12/06 21:39:03, manzagop wrote:
> On 2016/12/06 19:28:43, bcwhite wrote:
> > Since these are global, is it worth having the strings indicate such?  e.g.
> > Chrome-Channel
> > Chrome-Platform
> > etc.
> > 
> > Or similar.  Just wondering if there might end up being, for example, a
> > "version" of something else that is also global.
> 
> I think I prefer keeping these short and we can qualify more the more specific
> ones. Sg?

Your call.

https://codereview.chromium.org/2556813002/diff/20001/base/debug/activity_tra...
File base/debug/activity_tracker.h (right):

https://codereview.chromium.org/2556813002/diff/20001/base/debug/activity_tra...
base/debug/activity_tracker.h:337: SetString(name, base::UTF16ToUTF8(value));
This module is part of base so you can remove the base:: qualifier.

[commit-bot: I haz the power, 2016-12-06 23:17:16]

The CQ bit was unchecked by [email protected]

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

Dry run: This issue passed the CQ dry run.

[manzagop (departed), 2016-12-07 14:50:36]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 14:50:46]

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

[manzagop (departed), 2016-12-07 14:51:10]

Thanks!

https://codereview.chromium.org/2556813002/diff/20001/base/debug/activity_tra...
File base/debug/activity_tracker.h (right):

https://codereview.chromium.org/2556813002/diff/20001/base/debug/activity_tra...
base/debug/activity_tracker.h:337: SetString(name, base::UTF16ToUTF8(value));
On 2016/12/06 22:37:32, bcwhite wrote:
> This module is part of base so you can remove the base:: qualifier.

Done.

[manzagop (departed), 2016-12-07 14:52:03]

[email protected] changed reviewers:
+ [email protected]

[manzagop (departed), 2016-12-07 14:52:04]

Hi Jesse!

Could you have an OWNERS' look at
chrome\browser\chrome_browser_field_trials_desktop.cc?

Thanks!

Pierre

[commit-bot: I haz the power, 2016-12-07 15:40:51]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-12-07 15:40:51]

Dry run: This issue passed the CQ dry run.

[jwd, 2016-12-09 21:52:25]

lgtm

[manzagop (departed), 2016-12-09 21:58:38]

Thanks for the review!

[manzagop (departed), 2016-12-09 21:58:44]

The CQ bit was checked by [email protected]

[manzagop (departed), 2016-12-09 21:58:44]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected]
Link to the patchset: https://codereview.chromium.org/2556813002/#ps40001
(title: "Address bcwhite's comments")

[commit-bot: I haz the power, 2016-12-09 21:59:32]

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-12-09 22:03:44]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-12-09 22:03:45]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[manzagop (departed), 2016-12-09 22:54:44]

The CQ bit was checked by [email protected]

[manzagop (departed), 2016-12-09 22:54:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
[email protected], [email protected]
Link to the patchset: https://codereview.chromium.org/2556813002/#ps60001
(title: "Merge")

[commit-bot: I haz the power, 2016-12-09 22:55:26]

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-12-10 01:59:37]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481324084535560,
"parent_rev": "e8f9ba1bc4ad3088877a87443ce19b103268ec17", "commit_rev":
"d90b8a37cd28fa908bc6c349dfbe14c200a7d548"}

[commit-bot: I haz the power, 2016-12-10 02:00:02]

Description was changed from

==========
Record executable version details to stability file

BUG=620813
==========

to

==========
Record executable version details to stability file

BUG=620813

Review-Url: https://codereview.chromium.org/2556813002
==========

[commit-bot: I haz the power, 2016-12-10 02:00:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-12 14:59:32]

Description was changed from

==========
Record executable version details to stability file

BUG=620813

Review-Url: https://codereview.chromium.org/2556813002
==========

to

==========
Record executable version details to stability file

BUG=620813

Committed: https://crrev.com/09c0c274ade68914487c187b2e99ab85f43b99a0
Cr-Commit-Position: refs/heads/master@{#437719}
==========

[commit-bot: I haz the power, 2016-12-12 14:59:33]

Patchset 4 (id:??) landed as
https://crrev.com/09c0c274ade68914487c187b2e99ab85f43b99a0
Cr-Commit-Position: refs/heads/master@{#437719}