Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(420)

Issue 2797433002: Include Google Update integration details in crash keys and about:version. (Closed)

Created:
3 years, 8 months ago by grt (UTC plus 2)
Modified:
3 years, 8 months ago
CC:
chromium-reviews, grt+watch_chromium.org, oshima+watch_chromium.org, pennymac+watch_chromium.org, wfh+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Include Google Update integration details in crash keys and about:version. Specifically: - Include the raw value of Chrome's "ap" and cohort "name" values in crash keys -- these are the magic values by which Chrome determines which update channel it is on. Having the values in crash keys will give us data to confirm that the channel identification logic is correct. - Include the cohort "name" value in about:version for convenient access. BUG=579504 Review-Url: https://codereview.chromium.org/2797433002 Cr-Commit-Position: refs/heads/master@{#462018} Committed: https://chromium.googlesource.com/chromium/src/+/97d39c267e8389053e8a5c3b3c6731d6054965bd

Patch Set 1 #

Total comments: 4

Patch Set 2 : review comments and better tests #

Patch Set 3 : sync to position 461540 #

Patch Set 4 : register new crash keys in installer #

Patch Set 5 : skip new unittests for chromium builds since they don't apply #

Total comments: 2

Patch Set 6 : components/version_ui/OWNERS now own version_ui_strings.grdp #

Unified diffs Side-by-side diffs Delta from patch set Stats (+232 lines, -46 lines) Patch
M chrome/app/chrome_crash_reporter_client_win.cc View 2 chunks +7 lines, -0 lines 0 comments Download
M chrome/browser/chrome_browser_main_win.cc View 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/version_ui.cc View 1 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/common/crash_keys.h View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/common/crash_keys.cc View 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/install_static/install_details.h View 3 chunks +34 lines, -0 lines 0 comments Download
M chrome/install_static/install_util.h View 1 chunk +7 lines, -2 lines 0 comments Download
M chrome/install_static/install_util.cc View 1 4 chunks +42 lines, -31 lines 0 comments Download
M chrome/install_static/product_install_details.cc View 1 chunk +10 lines, -1 line 0 comments Download
M chrome/install_static/product_install_details_unittest.cc View 1 2 3 4 2 chunks +55 lines, -0 lines 0 comments Download
M chrome/installer/setup/installer_crash_reporting.cc View 1 2 3 4 chunks +24 lines, -11 lines 0 comments Download
M chrome/installer/setup/setup_install_details.cc View 1 chunk +9 lines, -1 line 0 comments Download
M components/OWNERS View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M components/version_ui/resources/about_version.html View 1 1 chunk +3 lines, -0 lines 0 comments Download
M components/version_ui/version_ui_constants.h View 1 chunk +1 line, -0 lines 0 comments Download
M components/version_ui/version_ui_constants.cc View 1 chunk +1 line, -0 lines 0 comments Download
M components/version_ui_strings.grdp View 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 37 (24 generated)
grt (UTC plus 2)
Hi. Would y'all please take a look? Review assignments: scottmg: overall review tommycli: version_ui stuff ...
3 years, 8 months ago (2017-04-03 13:09:44 UTC) #6
tommycli
lgtm sans below https://codereview.chromium.org/2797433002/diff/20001/components/version_ui/resources/about_version.html File components/version_ui/resources/about_version.html (right): https://codereview.chromium.org/2797433002/diff/20001/components/version_ui/resources/about_version.html#newcode53 components/version_ui/resources/about_version.html:53: <span i18n-content="update_cohort_name"></span> Does this also need ...
3 years, 8 months ago (2017-04-03 15:36:36 UTC) #7
Robert Sesek
crash_keys lgtm
3 years, 8 months ago (2017-04-03 17:43:58 UTC) #8
scottmg
Any additional tests you could add to see that these get plumbed through? lgtm https://codereview.chromium.org/2797433002/diff/20001/chrome/browser/ui/webui/version_ui.cc ...
3 years, 8 months ago (2017-04-03 18:02:55 UTC) #9
grt (UTC plus 2)
Thanks for the review. Scott: PTAL. https://codereview.chromium.org/2797433002/diff/20001/chrome/browser/ui/webui/version_ui.cc File chrome/browser/ui/webui/version_ui.cc (right): https://codereview.chromium.org/2797433002/diff/20001/chrome/browser/ui/webui/version_ui.cc#newcode160 chrome/browser/ui/webui/version_ui.cc:160: #endif // defined(OS_WIN) ...
3 years, 8 months ago (2017-04-03 19:54:34 UTC) #10
scottmg
lgtm moreso!
3 years, 8 months ago (2017-04-03 19:57:59 UTC) #13
grt (UTC plus 2)
jochen: Please review chrome\browser\chrome_browser_main_win.cc and components\version_ui_strings.grdp. Please take the liberty of checking the CQ box ...
3 years, 8 months ago (2017-04-03 21:19:36 UTC) #15
jochen (gone - plz use gerrit)
lgtm with suggestion https://codereview.chromium.org/2797433002/diff/100001/components/version_ui_strings.grdp File components/version_ui_strings.grdp (right): https://codereview.chromium.org/2797433002/diff/100001/components/version_ui_strings.grdp#newcode55 components/version_ui_strings.grdp:55: <message name="IDS_VERSION_UI_VARIATIONS" desc="label for the variations ...
3 years, 8 months ago (2017-04-05 07:46:21 UTC) #26
grt (UTC plus 2)
Thanks. https://codereview.chromium.org/2797433002/diff/100001/components/version_ui_strings.grdp File components/version_ui_strings.grdp (right): https://codereview.chromium.org/2797433002/diff/100001/components/version_ui_strings.grdp#newcode55 components/version_ui_strings.grdp:55: <message name="IDS_VERSION_UI_VARIATIONS" desc="label for the variations list on ...
3 years, 8 months ago (2017-04-05 07:57:50 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2797433002/120001
3 years, 8 months ago (2017-04-05 07:58:11 UTC) #30
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/264949)
3 years, 8 months ago (2017-04-05 08:51:58 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2797433002/120001
3 years, 8 months ago (2017-04-05 09:00:34 UTC) #34
commit-bot: I haz the power
3 years, 8 months ago (2017-04-05 09:42:37 UTC) #37
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/97d39c267e8389053e8a5c3b3c67...

Powered by Google App Engine
This is Rietveld 408576698