Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1512113002:
Read feature param for Physical Web experiment (Closed)
DescriptionRead feature param for Physical Web experiment
BUG=529962
Committed: https://crrev.com/842ece221157e8d2082181282d14788e099f46a7
Cr-Commit-Position: refs/heads/master@{#367025}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Actually compiled. Doh! #
Total comments: 4
Patch Set 3 : Use default value #
Total comments: 4
Patch Set 4 : Changed description in commit message #Patch Set 5 : Used new feature API #
Total comments: 12
Patch Set 6 : Reorganized some code #
Total comments: 6
Patch Set 7 : Reorganize code again #Patch Set 8 : Rebased #
Messages
Total messages: 62 (23 generated)
cco3@chromium.org changed reviewers: + nyquist@chromium.org, rkaplow@chromium.org
https://codereview.chromium.org/1512113002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/1512113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:88: private static boolean getBooleanParam(String paramName, boolean defaultValue) { defaultValue doesn't seem to be used? In the call above you don't seem to be passing it either. Did you try compiling this?
https://codereview.chromium.org/1512113002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/1512113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:88: private static boolean getBooleanParam(String paramName, boolean defaultValue) { On 2015/12/10 00:04:06, nyquist wrote: > defaultValue doesn't seem to be used? In the call above you don't seem to be > passing it either. Did you try compiling this? Done.
https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:90: private static boolean getBooleanParam(String paramName, boolean defaultValue) { So defaultValue is still unused. Is that still intentional? https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:94: return TextUtils.equals(ENABLED_VALUE, Should this be: String paramValue = VariationsAssociatedData.getVariationParamValue(FIELD_TRIAL_NAME, paramName); if (TextUtils.isEmpty(paramValue)) return defaultValue; return TextUtils.equals(ENABLED_VALUE, paramValue);
mmocny@chromium.org changed reviewers: + mmocny@chromium.org
https://codereview.chromium.org/1512113002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/1512113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:29: // TODO(cco3): Remove chrome://flag after Finch is experiment is more in place. I thought we still want to make the chrome://flag available to allow explicit opt-in?
https://codereview.chromium.org/1512113002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/1512113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:29: // TODO(cco3): Remove chrome://flag after Finch is experiment is more in place. On 2015/12/10 15:07:04, mmocny wrote: > I thought we still want to make the chrome://flag available to allow explicit > opt-in? That's what I thought too, but this seriously complicates things because it's not clear whether the flag was turned off intentionally as a hard signal that the feature is to be turned off. Matt might be able to comment further since he's been working with this logic. People can still turn it on with a command line flag though. https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:90: private static boolean getBooleanParam(String paramName, boolean defaultValue) { On 2015/12/10 01:45:22, nyquist wrote: > So defaultValue is still unused. Is that still intentional? Oh, in the method body...right. I had fixed it in the invocation. https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:94: return TextUtils.equals(ENABLED_VALUE, On 2015/12/10 01:45:22, nyquist wrote: > Should this be: > String paramValue = > VariationsAssociatedData.getVariationParamValue(FIELD_TRIAL_NAME, paramName); > if (TextUtils.isEmpty(paramValue)) return defaultValue; > return TextUtils.equals(ENABLED_VALUE, paramValue); Done. (Sorry, I had gotten this code from another project...if you tell me where I could consolidate these helper functions, I can make a change to do that.)
On 2015/12/11 21:32:19, cco3 wrote: > https://codereview.chromium.org/1512113002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java > (right): > > https://codereview.chromium.org/1512113002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:29: > // TODO(cco3): Remove chrome://flag after Finch is experiment is more in place. > On 2015/12/10 15:07:04, mmocny wrote: > > I thought we still want to make the chrome://flag available to allow explicit > > opt-in? > > That's what I thought too, but this seriously complicates things because it's > not clear whether the flag was turned off intentionally as a hard signal that > the feature is to be turned off. Matt might be able to comment further since > he's been working with this logic. > > People can still turn it on with a command line flag though. That's really unfortunate. I'll reach out to Robert to see if he has suggestions on this. I think partners will want really easy ways to try this out, and a command line flag is super hard to flip for mere mortals. > > https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java > (right): > > https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:90: > private static boolean getBooleanParam(String paramName, boolean defaultValue) { > On 2015/12/10 01:45:22, nyquist wrote: > > So defaultValue is still unused. Is that still intentional? > > Oh, in the method body...right. I had fixed it in the invocation. > > https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:94: > return TextUtils.equals(ENABLED_VALUE, > On 2015/12/10 01:45:22, nyquist wrote: > > Should this be: > > String paramValue = > > VariationsAssociatedData.getVariationParamValue(FIELD_TRIAL_NAME, paramName); > > if (TextUtils.isEmpty(paramValue)) return defaultValue; > > return TextUtils.equals(ENABLED_VALUE, paramValue); > > Done. (Sorry, I had gotten this code from another project...if you tell me > where I could consolidate these helper functions, I can make a change to do > that.)
On Fri, Dec 11, 2015 at 1:35 PM, <mmocny@chromium.org> wrote: > On 2015/12/11 21:32:19, cco3 wrote: > > https://codereview.chromium.org/1512113002/diff/1/chrome/android/java/src/org... >> >> File > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java >> >> (right): > > > > https://codereview.chromium.org/1512113002/diff/1/chrome/android/java/src/org... > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:29: >> >> // TODO(cco3): Remove chrome://flag after Finch is experiment is more in > > place. >> >> On 2015/12/10 15:07:04, mmocny wrote: >> > I thought we still want to make the chrome://flag available to allow > > explicit >> >> > opt-in? > > >> That's what I thought too, but this seriously complicates things because >> it's >> not clear whether the flag was turned off intentionally as a hard signal >> that >> the feature is to be turned off. Matt might be able to comment further >> since >> he's been working with this logic. > > >> People can still turn it on with a command line flag though. > > > That's really unfortunate. I'll reach out to Robert to see if he has > suggestions on this. I think partners will want really easy ways to try > this > out, and a command line flag is super hard to flip for mere mortals. Paging mattreynolds for suggestions. > > > > > https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... >> >> File > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java >> >> (right): > > > > https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:90: >> >> private static boolean getBooleanParam(String paramName, boolean >> defaultValue) > > { >> >> On 2015/12/10 01:45:22, nyquist wrote: >> > So defaultValue is still unused. Is that still intentional? > > >> Oh, in the method body...right. I had fixed it in the invocation. > > > > https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:94: >> >> return TextUtils.equals(ENABLED_VALUE, >> On 2015/12/10 01:45:22, nyquist wrote: >> > Should this be: >> > String paramValue = >> > VariationsAssociatedData.getVariationParamValue(FIELD_TRIAL_NAME, > > paramName); >> >> > if (TextUtils.isEmpty(paramValue)) return defaultValue; >> > return TextUtils.equals(ENABLED_VALUE, paramValue); > > >> Done. (Sorry, I had gotten this code from another project...if you tell >> me >> where I could consolidate these helper functions, I can make a change to >> do >> that.) > > > > > https://codereview.chromium.org/1512113002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The previous best practices for flags was the have an "enable" and a "disable" flag. There is a new way which is much simpler, use the new Chrome Feature API which nativly supports flags and finch. You can read about it here: https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/featur... I think this should make it more clear what to do. On 2015/12/11 21:43:36, chromium-reviews wrote: > On Fri, Dec 11, 2015 at 1:35 PM, <mailto:mmocny@chromium.org> wrote: > > On 2015/12/11 21:32:19, cco3 wrote: > > > > > https://codereview.chromium.org/1512113002/diff/1/chrome/android/java/src/org... > >> > >> File > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java > >> > >> (right): > > > > > > > > > https://codereview.chromium.org/1512113002/diff/1/chrome/android/java/src/org... > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:29: > >> > >> // TODO(cco3): Remove chrome://flag after Finch is experiment is more in > > > > place. > >> > >> On 2015/12/10 15:07:04, mmocny wrote: > >> > I thought we still want to make the chrome://flag available to allow > > > > explicit > >> > >> > opt-in? > > > > > >> That's what I thought too, but this seriously complicates things because > >> it's > >> not clear whether the flag was turned off intentionally as a hard signal > >> that > >> the feature is to be turned off. Matt might be able to comment further > >> since > >> he's been working with this logic. > > > > > >> People can still turn it on with a command line flag though. > > > > > > That's really unfortunate. I'll reach out to Robert to see if he has > > suggestions on this. I think partners will want really easy ways to try > > this > > out, and a command line flag is super hard to flip for mere mortals. > > Paging mattreynolds for suggestions. > > > > > > > > > > > > https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... > >> > >> File > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java > >> > >> (right): > > > > > > > > > https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:90: > >> > >> private static boolean getBooleanParam(String paramName, boolean > >> defaultValue) > > > > { > >> > >> On 2015/12/10 01:45:22, nyquist wrote: > >> > So defaultValue is still unused. Is that still intentional? > > > > > >> Oh, in the method body...right. I had fixed it in the invocation. > > > > > > > > > https://codereview.chromium.org/1512113002/diff/20001/chrome/android/java/src... > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:94: > >> > >> return TextUtils.equals(ENABLED_VALUE, > >> On 2015/12/10 01:45:22, nyquist wrote: > >> > Should this be: > >> > String paramValue = > >> > VariationsAssociatedData.getVariationParamValue(FIELD_TRIAL_NAME, > > > > paramName); > >> > >> > if (TextUtils.isEmpty(paramValue)) return defaultValue; > >> > return TextUtils.equals(ENABLED_VALUE, paramValue); > > > > > >> Done. (Sorry, I had gotten this code from another project...if you tell > >> me > >> where I could consolidate these helper functions, I can make a change to > >> do > >> that.) > > > > > > > > > > https://codereview.chromium.org/1512113002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
Description was changed from ========== Read Finch param for Physical Web experiment BUG=529962 ========== to ========== Read Finch param for Physical Web experiment BUG=529962 ==========
rkaplow@chromium.org changed reviewers: + asvitkine@google.com - rkaplow@chromium.org
rkaplow@chromium.org changed reviewers: + rkaplow@chromium.org
going on vacation - adding alexei to do next round of review
> The previous best practices for flags was the have an "enable" and a "disable" > flag. > There is a new way which is much simpler, use the new Chrome Feature API which > nativly supports flags and finch. I think we should add the "disable-physical-web" flag but plan to remove both (enable and disable) once the Finch experiment reaches 100% enrollment and we no longer need a kill switch. The Feature API does seem simpler but it's not clear to me how to migrate an existing flag or how to easily access the FeatureList state from Java.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
Some comments below. It might still be better to switch to Feature API, because there's a lot of gotchas when not using it (see my comments below) which go away when using Feature API. However, we do need to expose a Java accessor to it. I can work on it if you're receptive to using it. In terms of users of the existing flag, it's true that for these folks, the flag will be toggled off once they get the new version that uses feature API. But presumably there shouldn't be too many people toggling things in about:flags anyway. They can always toggle the flag again. https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:31: // TODO(cco3): Remove chrome://flag after Finch is experiment is more in place. "Finch" is an internal codename. Don't use it in code or in the CL description. https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:35: CommandLine.getInstance().hasSwitch(ChromeSwitches.ENABLE_PHYSICAL_WEB); You should also have a disable switch, so that if the experiment is putting users into the enable group, they can disable it for debugging. In about_flags.cc, you can use ENABLE_DISABLE_VALUE_TYPE() macro. https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:37: return (allowedChannel && switchEnabled) || getBooleanParam(ENABLED_PARAM, false); The current logic won't integrate well with go/variations-flags set up. Specifically, you won't be able to tag users who are using the flag in a separate group. Which might be OK for you - such users will not show up in either group on the variations dashboard. If you want to see them in a separate group, you need to query the field trial regardless of the state of the flags as per the example in the go link.
I agree that it would be great if this feature used the new Feature API. https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:84: * command-line switch with the same name, for easy local testing. Couldn't you force the trial through a command line instead?
Description was changed from ========== Read Finch param for Physical Web experiment BUG=529962 ========== to ========== Read feature param for Physical Web experiment BUG=529962 ==========
On 2015/12/17 02:02:23, nyquist wrote: > I agree that it would be great if this feature used the new Feature API. > > https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java > (right): > > https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:84: > * command-line switch with the same name, for easy local testing. > Couldn't you force the trial through a command line instead? So how do I get started on getting the feature api working in java?
I've opened https://code.google.com/p/chromium/issues/detail?id=570461 to track adding the Java API. I can work on that tomorrow and send out the CL. Alternatively, if you prefer, you can do it as part of your CL. Either one works for me. On Thu, Dec 17, 2015 at 5:12 PM, <cco3@chromium.org> wrote: > On 2015/12/17 02:02:23, nyquist wrote: > >> I agree that it would be great if this feature used the new Feature API. >> > > > > https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... > >> File >> > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java > >> (right): >> > > > > https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:84: > >> * command-line switch with the same name, for easy local testing. >> Couldn't you force the trial through a command line instead? >> > > So how do I get started on getting the feature api working in java? > > https://codereview.chromium.org/1512113002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/12/17 22:14:35, chromium-reviews wrote: > I've opened https://code.google.com/p/chromium/issues/detail?id=570461 to > track adding the Java API. > > I can work on that tomorrow and send out the CL. Alternatively, if you > prefer, you can do it as part of your CL. Either one works for me. > > On Thu, Dec 17, 2015 at 5:12 PM, <mailto:cco3@chromium.org> wrote: > > > On 2015/12/17 02:02:23, nyquist wrote: > > > >> I agree that it would be great if this feature used the new Feature API. > >> > > > > > > > > > https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... > > > >> File > >> > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java > > > >> (right): > >> > > > > > > > > > https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:84: > > > >> * command-line switch with the same name, for easy local testing. > >> Couldn't you force the trial through a command line instead? > >> > > > > So how do I get started on getting the feature api working in java? > > > > https://codereview.chromium.org/1512113002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. If you already have a plan in mind, I'll let you implement it.
On 2015/12/17 22:14:35, chromium-reviews wrote: > I've opened https://code.google.com/p/chromium/issues/detail?id=570461 to > track adding the Java API. > > I can work on that tomorrow and send out the CL. Alternatively, if you > prefer, you can do it as part of your CL. Either one works for me. > > On Thu, Dec 17, 2015 at 5:12 PM, <mailto:cco3@chromium.org> wrote: > > > On 2015/12/17 02:02:23, nyquist wrote: > > > >> I agree that it would be great if this feature used the new Feature API. > >> > > > > > > > > > https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... > > > >> File > >> > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java > > > >> (right): > >> > > > > > > > > > https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:84: > > > >> * command-line switch with the same name, for easy local testing. > >> Couldn't you force the trial through a command line instead? > >> > > > > So how do I get started on getting the feature api working in java? > > > > https://codereview.chromium.org/1512113002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. If you already have a plan in mind, I'll let you implement it.
Sounds good. I should have it ready tomorrow. On Thu, Dec 17, 2015 at 5:23 PM, <cco3@chromium.org> wrote: > On 2015/12/17 22:14:35, chromium-reviews wrote: > >> I've opened https://code.google.com/p/chromium/issues/detail?id=570461 to >> track adding the Java API. >> > > I can work on that tomorrow and send out the CL. Alternatively, if you >> prefer, you can do it as part of your CL. Either one works for me. >> > > On Thu, Dec 17, 2015 at 5:12 PM, <mailto:cco3@chromium.org> wrote: >> > > > On 2015/12/17 02:02:23, nyquist wrote: >> > >> >> I agree that it would be great if this feature used the new Feature >> API. >> >> >> > >> > >> > >> > >> > > > https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... > >> > >> >> File >> >> >> > >> > >> > >> > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java > >> > >> >> (right): >> >> >> > >> > >> > >> > >> > > > https://codereview.chromium.org/1512113002/diff/40001/chrome/android/java/src... > >> > >> > >> > >> > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:84: > >> > >> >> * command-line switch with the same name, for easy local testing. >> >> Couldn't you force the trial through a command line instead? >> >> >> > >> > So how do I get started on getting the feature api working in java? >> > >> > https://codereview.chromium.org/1512113002/ >> > >> > > -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > If you already have a plan in mind, I'll let you implement it. > > https://codereview.chromium.org/1512113002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I've sent out https://codereview.chromium.org/1538573004/ for review. Let me know if that works for you!
On 2015/12/18 18:01:02, Alexei Svitkine (slow) wrote: > I've sent out https://codereview.chromium.org/1538573004/ for review. Let me > know if that works for you! OK, Alexei, I've updated the change to use your code. Let me know if I've done it correctly.
overall looks good, just one comments. How does a user manage their feature list? https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.cc:45: const base::Feature kPhysicalWebFeature { I don't expect its a good idea to use static initialization of a non-POD type like this. I'm not sure what the style guide for chromium says about it, but typically you either wrap in a Get*() function which itself holds the static data, or create a global plain *pointer* to the object. https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.h (right): https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.h:17: extern const base::Feature kPhysicalWebFeature; Is this really the first Clank feature?
lgtm % comments Thanks! https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.cc:47: }; Nit: I'd put this after line 27. https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.h (right): https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.h:17: extern const base::Feature kPhysicalWebFeature; Nit: I'd put this on after line 14. https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.h:17: extern const base::Feature kPhysicalWebFeature; On 2015/12/21 15:45:55, mmocny wrote: > Is this really the first Clank feature? First one using the new Feature API. I only landed the CL to expose the Java API for base/feature_list.h on Friday. https://codereview.chromium.org/1512113002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1512113002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:68537: - <int value="-1075089382" label="enable-physical-web"/> Keep this one here, as it's still used by UMA to decode UMA data from older versions.
(still lgtm % my comments) https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:26: #include "chrome/browser/android/chrome_feature_list.h" I think this needs to be in ifdef android, since it includes jni headers. When you add the ifdef, also move it out to its own block below. https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.cc:45: const base::Feature kPhysicalWebFeature { On 2015/12/21 15:45:55, mmocny wrote: > I don't expect its a good idea to use static initialization of a non-POD type > like this. > > I'm not sure what the style guide for chromium says about it, but typically you > either wrap in a Get*() function which itself holds the static data, or create a > global plain *pointer* to the object. It's a POD type and this is the correct way to initialize it. (See the docs in base/feature_list.h)
On 2015/12/21 15:45:55, mmocny wrote: > overall looks good, just one comments. > > How does a user manage their feature list? flags, command-line, remote feature control...the whole point of this feature api is that it does all that resolution for us. To answer the concern you most likely have, this does preserve our flag. It changes it to a drop down that has a "default" entry. That's how we know if someone has intentionally enabled the feature via the flag. > > https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... > File chrome/browser/android/chrome_feature_list.cc (right): > > https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... > chrome/browser/android/chrome_feature_list.cc:45: const base::Feature > kPhysicalWebFeature { > I don't expect its a good idea to use static initialization of a non-POD type > like this. > > I'm not sure what the style guide for chromium says about it, but typically you > either wrap in a Get*() function which itself holds the static data, or create a > global plain *pointer* to the object. > > https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... > File chrome/browser/android/chrome_feature_list.h (right): > > https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... > chrome/browser/android/chrome_feature_list.h:17: extern const base::Feature > kPhysicalWebFeature; > Is this really the first Clank feature?
https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:26: #include "chrome/browser/android/chrome_feature_list.h" On 2015/12/21 15:50:50, Alexei Svitkine (slow) wrote: > I think this needs to be in ifdef android, since it includes jni headers. > > When you add the ifdef, also move it out to its own block below. Done. https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.cc:47: }; On 2015/12/21 15:48:18, Alexei Svitkine (slow) wrote: > Nit: I'd put this after line 27. Done. https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.h (right): https://codereview.chromium.org/1512113002/diff/80001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.h:17: extern const base::Feature kPhysicalWebFeature; On 2015/12/21 15:48:18, Alexei Svitkine (slow) wrote: > Nit: I'd put this on after line 14. Done. https://codereview.chromium.org/1512113002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1512113002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:68537: - <int value="-1075089382" label="enable-physical-web"/> On 2015/12/21 15:48:18, Alexei Svitkine (slow) wrote: > Keep this one here, as it's still used by UMA to decode UMA data from older > versions. Done.
The CQ bit was checked by cco3@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/1512113002/#ps100001 (title: "Reorganized some code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512113002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512113002/100001
https://codereview.chromium.org/1512113002/diff/100001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1512113002/diff/100001/chrome/browser/about_f... chrome/browser/about_flags.cc:69: #include "chrome/browser/android/chrome_feature_list.h" This is a !defined(OS_ANDROID) block. You want a new block that's defined(OS_ANDROID) or have an else for this block (and probably reverse the cond so defined is first). https://codereview.chromium.org/1512113002/diff/100001/chrome/browser/android... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1512113002/diff/100001/chrome/browser/android... chrome/browser/android/chrome_feature_list.cc:45: const base::Feature kPhysicalWebFeature { I meant to move this to below the anon namespace and keep the Register function where it was. https://codereview.chromium.org/1512113002/diff/100001/chrome/browser/android... File chrome/browser/android/chrome_feature_list.h (right): https://codereview.chromium.org/1512113002/diff/100001/chrome/browser/android... chrome/browser/android/chrome_feature_list.h:15: extern const base::Feature kPhysicalWebFeature; Nit: Add an empty line below.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1512113002/diff/100001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1512113002/diff/100001/chrome/browser/about_f... chrome/browser/about_flags.cc:69: #include "chrome/browser/android/chrome_feature_list.h" On 2015/12/21 19:02:17, Alexei Svitkine (slow) wrote: > This is a !defined(OS_ANDROID) block. You want a new block that's > defined(OS_ANDROID) or have an else for this block (and probably reverse the > cond so defined is first). Done. https://codereview.chromium.org/1512113002/diff/100001/chrome/browser/android... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1512113002/diff/100001/chrome/browser/android... chrome/browser/android/chrome_feature_list.cc:45: const base::Feature kPhysicalWebFeature { On 2015/12/21 19:02:17, Alexei Svitkine (slow) wrote: > I meant to move this to below the anon namespace and keep the Register function > where it was. Done. https://codereview.chromium.org/1512113002/diff/100001/chrome/browser/android... File chrome/browser/android/chrome_feature_list.h (right): https://codereview.chromium.org/1512113002/diff/100001/chrome/browser/android... chrome/browser/android/chrome_feature_list.h:15: extern const base::Feature kPhysicalWebFeature; On 2015/12/21 19:02:17, Alexei Svitkine (slow) wrote: > Nit: Add an empty line below. Done.
cco3@chromium.org changed reviewers: + newt@chromium.org
Adding newt@ for android ownership (nyquist@ had already made some comments)
The CQ bit was checked by cco3@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512113002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512113002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
cco3@chromium.org changed reviewers: + @chromium.org
dfalcantara@chromium.org changed reviewers: + dfalcantara@chromium.org
lgtm
The CQ bit was checked by conleyo@google.com
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/1512113002/#ps120001 (title: "Reorganize code again")
On 2015/12/28 22:43:35, dfalcantara wrote: > lgtm Thank you!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512113002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512113002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
cco3@chromium.org changed reviewers: - @chromium.org, conleyo@google.com
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/1512113002/#ps140001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512113002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512113002/140001
Message was sent while issue was closed.
Description was changed from ========== Read feature param for Physical Web experiment BUG=529962 ========== to ========== Read feature param for Physical Web experiment BUG=529962 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Read feature param for Physical Web experiment BUG=529962 ========== to ========== Read feature param for Physical Web experiment BUG=529962 Committed: https://crrev.com/842ece221157e8d2082181282d14788e099f46a7 Cr-Commit-Position: refs/heads/master@{#367025} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/842ece221157e8d2082181282d14788e099f46a7 Cr-Commit-Position: refs/heads/master@{#367025} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
