Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1291073005:
Update full-form autofill config to always query the field trial state. (Closed)
|
Created:
5 years, 4 months ago by Justin Donnelly Modified:
5 years, 4 months ago CC:
chromium-reviews, estade+watch_chromium.org, browser-components-watch_chromium.org, rouslan+autofillwatch_chromium.org, bondd Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate full-form autofill config to always query the field trial state.
This is necessary for correct metrics reporting. Also, add a comment to FindFullName in field_trial.h mentioning its role in metrics reporting.
BUG=504644
Committed: https://crrev.com/ff51160943ee86667cecda60eb284ad2fcf507ee
Cr-Commit-Position: refs/heads/master@{#343534}
Patch Set 1 #Patch Set 2 : Add comment to field_trial.h #
Total comments: 2
Patch Set 3 : Re-flow comment text #
Messages
Total messages: 31 (12 generated)
jdonnelly@chromium.org changed reviewers: + thestig@chromium.org
Can we add a note to base/metrics/field_trial.h?
jdonnelly@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine, can you suggest some language to add to base/metrics/field_trial.h to explain the issue here to future experimenters? To be honest, I'm not totally sure I understand the way this works, I'm just following the instructions at https://g3doc.corp.google.com/analysis/uma/g3doc/finch-experiments/guide-for-...
Please update the CL description to not use the internal codename "Finch". Otherwise, how about the following note to add at the end of the comment of FindFullName() in field_trial.h: "The first call of this function on a given field trial will mark it as active, so that its state will be reported with usage metrics, crashes, etc."
On 2015/08/14 15:50:13, Alexei Svitkine (OOO Aug15-24) wrote: > Please update the CL description to not use the internal codename "Finch". Done. > Otherwise, how about the following note to add at the end of the comment of > FindFullName() in field_trial.h: > > "The first call of this function on a given field trial will mark it as active, > so that its state will be reported with usage metrics, crashes, etc." Great, thanks. Done.
lgtm https://codereview.chromium.org/1291073005/diff/20001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/1291073005/diff/20001/base/metrics/field_tria... base/metrics/field_trial.h:407: // Returns the group name chosen for the named trial, or the Nit: Seems like some of the following line can still fit on this one - can you clean this up too while you're updating the comment?
https://codereview.chromium.org/1291073005/diff/20001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/1291073005/diff/20001/base/metrics/field_tria... base/metrics/field_trial.h:407: // Returns the group name chosen for the named trial, or the On 2015/08/14 16:15:36, Alexei Svitkine (OOO Aug15-24) wrote: > Nit: Seems like some of the following line can still fit on this one - can you > clean this up too while you're updating the comment? Done.
lgtm
The CQ bit was checked by jdonnelly@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1291073005/#ps40001 (title: "Re-flow comment text")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291073005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291073005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_arm_compile on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm_compi...)
The CQ bit was checked by jdonnelly@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291073005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291073005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_arm_compile on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm_compi...)
The CQ bit was checked by jdonnelly@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291073005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291073005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_arm_compile on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm_compi...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291073005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291073005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_arm_compile on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm_compi...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291073005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291073005/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ff51160943ee86667cecda60eb284ad2fcf507ee Cr-Commit-Position: refs/heads/master@{#343534} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
