Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1945803002:
SyzyASAN: Export experiment state to finch synthetic field trials. (Closed)
|
Created:
4 years, 7 months ago by Sigurður Ásgeirsson Modified:
4 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSyzyASAN: Export experiment state to synthetic field trials.
This needs corresponding server-side configuration, coming up.
R=chrisha@chromium.org
BUG=608810
Committed: https://crrev.com/acb7d5253569d0d3ce17d4a39bcb87f656a43116
Cr-Commit-Position: refs/heads/master@{#391836}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address Chris' comments. #
Total comments: 2
Patch Set 3 : Now even compiles under SYZYASAN defined. #
Total comments: 1
Patch Set 4 : Address Alexei's nit. #Patch Set 5 : Fix export name to match the SyzyASAN-side name change. #
Messages
Total messages: 21 (8 generated)
Description was changed from ========== SyzyASAN: Export experiment state to finch synthetic field trials. This needs corresponding server-side configuration, coming up. R=chrisha@chromium.org BUG=608810 ========== to ========== SyzyASAN: Export experiment state to finch synthetic field trials. This needs corresponding server-side configuration, coming up. R=chrisha@chromium.org BUG=608810 ==========
siggi@chromium.org changed reviewers: + asvitkine@chromium.org
Awesome, this lgtm! https://codereview.chromium.org/1945803002/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1945803002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1237: typedef VOID(WINAPI* SyzyasanEnumFeaturesFn)(SyzyASANExperimentCallbackFn); SyzyASAN, for consistency? https://codereview.chromium.org/1945803002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1238: // Export the SyzyASAN experiements as synthetic field trials. experiments* https://codereview.chromium.org/1945803002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1266: ubernit: Remove this blank line?
siggi@chromium.org changed reviewers: + jochen@chromium.org
Jochen for owners please? https://codereview.chromium.org/1945803002/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1945803002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1237: typedef VOID(WINAPI* SyzyasanEnumFeaturesFn)(SyzyASANExperimentCallbackFn); On 2016/05/03 20:56:36, chrisha (slow) wrote: > SyzyASAN, for consistency? Done. https://codereview.chromium.org/1945803002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1238: // Export the SyzyASAN experiements as synthetic field trials. On 2016/05/03 20:56:36, chrisha (slow) wrote: > experiments* Done. https://codereview.chromium.org/1945803002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1266: On 2016/05/03 20:56:36, chrisha (slow) wrote: > ubernit: Remove this blank line? Done.
lgtm
Besides the comment below, please change the CL description to avoid using the term "Finch" which is an internal codename. https://codereview.chromium.org/1945803002/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1945803002/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1231: ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial(name, status); Does this compile? I believe you need to add a friend declaration to ChromeMetricsServiceAccessor - which is the mechanism we use to ensure all the users of this API have been reviewed by us.
Description was changed from ========== SyzyASAN: Export experiment state to finch synthetic field trials. This needs corresponding server-side configuration, coming up. R=chrisha@chromium.org BUG=608810 ========== to ========== SyzyASAN: Export experiment state to synthetic field trials. This needs corresponding server-side configuration, coming up. R=chrisha@chromium.org BUG=608810 ==========
On 2016/05/04 15:01:46, Alexei Svitkine wrote: > Besides the comment below, please change the CL description to avoid using the > term "Finch" which is an internal codename. > > https://codereview.chromium.org/1945803002/diff/20001/chrome/browser/chrome_b... > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/1945803002/diff/20001/chrome/browser/chrome_b... > chrome/browser/chrome_browser_main.cc:1231: > ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial(name, status); > Does this compile? I believe you need to add a friend declaration to > ChromeMetricsServiceAccessor - which is the mechanism we use to ensure all the > users of this API have been reviewed by us. Erhm - I was going to say, "yes this compiles", cause it does. Turns out it's important to try with the SYZYASAN define enabled. Fix coming right up :/.
mkay - now EVEN compiles. <slinks away in shame to test this in combo with new SyzyASAN instrumentation> https://codereview.chromium.org/1945803002/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1945803002/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1231: ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial(name, status); On 2016/05/04 15:01:45, Alexei Svitkine wrote: > Does this compile? I believe you need to add a friend declaration to > ChromeMetricsServiceAccessor - which is the mechanism we use to ensure all the > users of this API have been reviewed by us. Done.
lgtm https://codereview.chromium.org/1945803002/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1945803002/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1230: // by ChromeMEtricsServiceAccessor. Nit: ME -> Me
Chris - one last look? I've verified that this works against a locally-built SyzyASAN runtime.
lgtm, assuming all compiles this time :)
The CQ bit was checked by siggi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1945803002/#ps80001 (title: "Fix export name to match the SyzyASAN-side name change.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1945803002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1945803002/80001
Message was sent while issue was closed.
Description was changed from ========== SyzyASAN: Export experiment state to synthetic field trials. This needs corresponding server-side configuration, coming up. R=chrisha@chromium.org BUG=608810 ========== to ========== SyzyASAN: Export experiment state to synthetic field trials. This needs corresponding server-side configuration, coming up. R=chrisha@chromium.org BUG=608810 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== SyzyASAN: Export experiment state to synthetic field trials. This needs corresponding server-side configuration, coming up. R=chrisha@chromium.org BUG=608810 ========== to ========== SyzyASAN: Export experiment state to synthetic field trials. This needs corresponding server-side configuration, coming up. R=chrisha@chromium.org BUG=608810 Committed: https://crrev.com/acb7d5253569d0d3ce17d4a39bcb87f656a43116 Cr-Commit-Position: refs/heads/master@{#391836} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/acb7d5253569d0d3ce17d4a39bcb87f656a43116 Cr-Commit-Position: refs/heads/master@{#391836} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
