Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
813873005:
Add field trial and flag to mark HTTP as non-secure. (Closed)
DescriptionAdd field trial and flag to mark HTTP as non-secure.
BUG=444242
Committed: https://crrev.com/601fe3c76c5ae5a0fab68766a95b9313c1b408ec
Cr-Commit-Position: refs/heads/master@{#312920}
Patch Set 1 #
Total comments: 7
Patch Set 2 : de-format, meet field trial style on the field of style trial #
Total comments: 6
Patch Set 3 : Fix histogram stuff; refactor helper function. #Patch Set 4 : Rebase. #Patch Set 5 : Only badge if SECURITY_STYLE_UNAUTHENTICATED && unauthenticated network transport. (E.g. not chrome… #
Total comments: 9
Patch Set 6 : Respond to comments. #Patch Set 7 : Describe when the flag's counter is emitted. #
Total comments: 1
Patch Set 8 : s/counter/action/ #
Total comments: 4
Patch Set 9 : Rename to "Mark Non-Secure As ...", use named constants. #
Total comments: 4
Patch Set 10 : Rebase and fix style nit. #Patch Set 11 : Fix histogram name. #Patch Set 12 : Consistently use "mark non secure as". #Patch Set 13 : Gotta have a default option. #
Messages
Total messages: 49 (15 generated)
palmer@chromium.org changed reviewers: + egm@chromium.org, felt@chromium.org
Might need more work, but, have fun.
jww@chromium.org changed reviewers: + jww@chromium.org
Drive by review! https://codereview.chromium.org/813873005/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/813873005/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:191: const Experiment::Choice kMarkHttpAsNonSecureChoices[] = { I know, I know, it sucks that this file isn't formatted correctly, but the unfortunate rule is that we don't git cl format about_flags :-( You just reset this file and make your new addition fit the (bad) formatting that's already in here. https://codereview.chromium.org/813873005/diff/1/chrome/browser/ui/toolbar/to... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/813873005/diff/1/chrome/browser/ui/toolbar/to... chrome/browser/ui/toolbar/toolbar_model_impl.cc:97: if (GetSecurityLevelForFieldTrialGroup( According to Finch documentation, you should actually place this call *before* you call GetSwitchValueASCII, because this registers the group for the user, and we want to have that info for UMA purposes, even if they override the behavior with a switch. Frankly, I'd just throw the command line stuff into the GetSecurityLevelForFieldTrialGroup function anyway and call it there. Then you could have just one, nice function GetSecurityStyle that returns the appropriate style based on field trial and switches. https://codereview.chromium.org/813873005/diff/1/chrome/common/chrome_switche... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/813873005/diff/1/chrome/common/chrome_switche... chrome/common/chrome_switches.cc:802: const char kMarkHttpAsNonSecure[] = "mark-http-as-non-secure"; nit: If I'm not mistaken, another one of those files with weird formatting :-P I think these equals signs are supposed to be aligned in a column.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/813873005/diff/1/chrome/browser/ui/toolbar/to... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/813873005/diff/1/chrome/browser/ui/toolbar/to... chrome/browser/ui/toolbar/toolbar_model_impl.cc:97: if (GetSecurityLevelForFieldTrialGroup( On 2014/12/20 01:21:57, jww wrote: > According to Finch documentation, you should actually place this call *before* > you call GetSwitchValueASCII, because this registers the group for the user, and > we want to have that info for UMA purposes, even if they override the behavior > with a switch. > > Frankly, I'd just throw the command line stuff into the > GetSecurityLevelForFieldTrialGroup function anyway and call it there. Then you > could have just one, nice function GetSecurityStyle that returns the appropriate > style based on field trial and switches. See https://goto.google.com/finch-and-flags
https://codereview.chromium.org/813873005/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/813873005/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:191: const Experiment::Choice kMarkHttpAsNonSecureChoices[] = { On 2014/12/20 01:21:57, jww wrote: > I know, I know, it sucks that this file isn't formatted correctly, but the > unfortunate rule is that we don't git cl format about_flags :-( You just reset > this file and make your new addition fit the (bad) formatting that's already in > here. Done. https://codereview.chromium.org/813873005/diff/1/chrome/browser/ui/toolbar/to... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/813873005/diff/1/chrome/browser/ui/toolbar/to... chrome/browser/ui/toolbar/toolbar_model_impl.cc:97: if (GetSecurityLevelForFieldTrialGroup( OK, I think I did this right. I'm sure you'll let me know if not. :) https://codereview.chromium.org/813873005/diff/1/chrome/common/chrome_switche... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/813873005/diff/1/chrome/common/chrome_switche... chrome/common/chrome_switches.cc:802: const char kMarkHttpAsNonSecure[] = "mark-http-as-non-secure"; On 2014/12/20 01:21:57, jww wrote: > nit: If I'm not mistaken, another one of those files with weird formatting :-P I > think these equals signs are supposed to be aligned in a column. Done.
https://codereview.chromium.org/813873005/diff/20001/chrome/browser/about_fla... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/813873005/diff/20001/chrome/browser/about_fla... chrome/browser/about_flags.cc:2101: "mark-http-as-non-secure", // FLAGS:RECORD_UMA Somewhere at the top of this array, there are instructions on how to update histograms.xml appropriately. You'll need to do that before passing the presubmit, I believe. https://codereview.chromium.org/813873005/diff/20001/chrome/browser/ui/toolba... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/813873005/diff/20001/chrome/browser/ui/toolba... chrome/browser/ui/toolbar/toolbar_model_impl.cc:68: bool GetSecurityLevelForHttpFieldTrial(ToolbarModel::SecurityLevel* level) { This certainly works, but instead of returning a bool, why not just return the ToolbarModel::SecurityLevel? This would also address my comment below... https://codereview.chromium.org/813873005/diff/20001/chrome/browser/ui/toolba... chrome/browser/ui/toolbar/toolbar_model_impl.cc:81: return true; This is unreachable code.
https://codereview.chromium.org/813873005/diff/20001/chrome/browser/about_fla... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/813873005/diff/20001/chrome/browser/about_fla... chrome/browser/about_flags.cc:2101: "mark-http-as-non-secure", // FLAGS:RECORD_UMA On 2014/12/20 02:07:58, jww wrote: > Somewhere at the top of this array, there are instructions on how to update > histograms.xml appropriately. You'll need to do that before passing the > presubmit, I believe. Done. https://codereview.chromium.org/813873005/diff/20001/chrome/browser/ui/toolba... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/813873005/diff/20001/chrome/browser/ui/toolba... chrome/browser/ui/toolbar/toolbar_model_impl.cc:68: bool GetSecurityLevelForHttpFieldTrial(ToolbarModel::SecurityLevel* level) { > This certainly works, but instead of returning a bool, why not just return the > ToolbarModel::SecurityLevel? This would also address my comment below... I modeled this after GetSecurityLevelForFieldTrialGroup, above. I agree that it makes much more sense to return ToolbarModel::SecurityLevel, but I assumed sleevi had his reasons. But I'll return a SecurityLevel. https://codereview.chromium.org/813873005/diff/20001/chrome/browser/ui/toolba... chrome/browser/ui/toolbar/toolbar_model_impl.cc:81: return true; > This is unreachable code. I'm not sure why?
palmer@chromium.org changed reviewers: + mpearson@google.com, msw@chromium.org
msw: OWNER for Chrome UI mpearson: OWNER for UMA/histograms
https://codereview.chromium.org/813873005/diff/80001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/813873005/diff/80001/chrome/app/generated_res... chrome/app/generated_resources.grd:5690: + Do not mark HTTP as non-secure or as dubious. This one breaks the pattern of the others. "Mark HTTP as neutral."? https://codereview.chromium.org/813873005/diff/80001/chrome/browser/ui/toolba... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/813873005/diff/80001/chrome/browser/ui/toolba... chrome/browser/ui/toolbar/toolbar_model_impl.cc:73: if (choice == "no" || group == "no") Doesn't this mean that as long as the Finch trial is serving "no" (which will be the initial default), there won't be any way to opt in to another treatment?
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
https://codereview.chromium.org/813873005/diff/80001/tools/metrics/actions/ac... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/813873005/diff/80001/tools/metrics/actions/ac... tools/metrics/actions/actions.xml:186: <owner>palmer@chromium.org, felt@chromium.org</owner> owner is a repeated field, one e-mail address per entry. https://codereview.chromium.org/813873005/diff/80001/tools/metrics/actions/ac... tools/metrics/actions/actions.xml:188: This flag lets users opt into or out of marking HTTP as Dubious or as Actions need to clearly state when they are emitted. More importantly, after a quick glance over your changelist, I don't see where this action is emitted. Did you forget to add the code?
blelump@gmail.com changed reviewers: + blelump@gmail.com
https://codereview.chromium.org/813873005/diff/80001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/813873005/diff/80001/chrome/app/generated_res... chrome/app/generated_resources.grd:5690: + Do not mark HTTP as non-secure or as dubious. On 2014/12/23 01:34:58, felt wrote: > This one breaks the pattern of the others. "Mark HTTP as neutral."? Done. https://codereview.chromium.org/813873005/diff/80001/chrome/browser/ui/toolba... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/813873005/diff/80001/chrome/browser/ui/toolba... chrome/browser/ui/toolbar/toolbar_model_impl.cc:73: if (choice == "no" || group == "no") > Doesn't this mean that as long as the Finch trial is serving "no" (which will be > the initial default), there won't be any way to opt in to another treatment? You're right, thanks. Changing it so that the user's choice always takes precedence. https://codereview.chromium.org/813873005/diff/80001/tools/metrics/actions/ac... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/813873005/diff/80001/tools/metrics/actions/ac... tools/metrics/actions/actions.xml:186: <owner>palmer@chromium.org, felt@chromium.org</owner> On 2015/01/02 19:14:28, Mark P wrote: > owner is a repeated field, one e-mail address per entry. Done. https://codereview.chromium.org/813873005/diff/80001/tools/metrics/actions/ac... tools/metrics/actions/actions.xml:188: This flag lets users opt into or out of marking HTTP as Dubious or as > Actions need to clearly state when they are emitted. > > More importantly, after a quick glance over your changelist, I don't see where > this action is emitted. Did you forget to add the code? I'm not sure what you mean. I think I am following the instructions in chrome/browser/about_flags.cc, under "RECORDING USER METRICS FOR FLAGS".
did you forget to upload the new patchset?
Also, please upload a new patchset to reflect your past changes. --mark https://codereview.chromium.org/813873005/diff/80001/tools/metrics/actions/ac... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/813873005/diff/80001/tools/metrics/actions/ac... tools/metrics/actions/actions.xml:188: This flag lets users opt into or out of marking HTTP as Dubious or as On 2015/01/07 00:59:00, Chromium Palmer wrote: > > Actions need to clearly state when they are emitted. > > > > More importantly, after a quick glance over your changelist, I don't see where > > this action is emitted. Did you forget to add the code? > > I'm not sure what you mean. I think I am following the instructions in > chrome/browser/about_flags.cc, under "RECORDING USER METRICS FOR FLAGS". Ah, I didn't notice this was using that action recording system. In that case, you need to revise your comment explain when this action is emitted. In this case, it's once per startup when the flag is active. Also, why do you want this? I believe it will get recorded if the user sets it regardless of what value the user sets it to. Is that useful for you? You'll know the usage of the flag but not what people are using it for...
> did you forget to upload the new patchset? I was waiting for it to build correctly before uploading. Coming soon.
> In that case, you need to revise your comment explain when this action is > emitted. In this case, it's once per startup when the flag is active. > > Also, why do you want this? I believe it will get recorded if the user sets it > regardless of what value the user sets it to. Is that useful for you? You'll > know the usage of the flag but not what people are using it for... Mainly, "it seems like what you're supposed to do". If it's not valuable, I'll gladly remove it. But, it might be nice to know "a bunch of people interacted with the flag, indicating people are interested in how HTTP will look, indicating that site operators maybe are engaged".
On 2015/01/07 23:38:57, Chromium Palmer wrote: > > In that case, you need to revise your comment explain when this action is > > emitted. In this case, it's once per startup when the flag is active. > > > > Also, why do you want this? I believe it will get recorded if the user sets > it > > regardless of what value the user sets it to. Is that useful for you? You'll > > know the usage of the flag but not what people are using it for... > > Mainly, "it seems like what you're supposed to do". It's not common. If you look down about_flags.cc, you'll rarely see flags marked with FLAGS:RECORD_UMA. That said ... > If it's not valuable, I'll > gladly remove it. But, it might be nice to know "a bunch of people interacted > with the flag, indicating people are interested in how HTTP will look, > indicating that site operators maybe are engaged". ... if you want to know the "it might be nice to know " you might as well keep it. If you're keeping it, please revise the action description as I mentioned above. --mark
> If you're keeping it, please revise the action description as I mentioned above. OK, done.
xml files lgtm baring one minor comment https://codereview.chromium.org/813873005/diff/120001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/813873005/diff/120001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:197: Non-Secure. The counter is emitted once per startup when the flag is active. nit: ^counter^action
lgtm % nit https://codereview.chromium.org/813873005/diff/140001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/813873005/diff/140001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_model_impl.cc:71: if (choice == "no") nit: I'd recommend making these values constants. That's the normal pattern for field trial strings. It also ensures that you use the same ones for the command line and field trial, since you're using a single constant variable twice instead of repeating the string twice in this method.
I'm surprised and shocked that you don't have to trust the website settings UI bubble when changing the toolbar UI. When I was working on this for SHA-1, I found the two to be tightly coupled in behaviour - and could easily crash if assumptions are violated. Have you checked to make sure this is fine? Where are the unittests for this? https://codereview.chromium.org/813873005/diff/140001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/813873005/diff/140001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_model_impl.cc:115: if (url.SchemeIs("http") || url.SchemeIs("ftp")) This (marking FTP here) is _not_ what I'd expect from the rest of this CL, from your variables, from the command-line, or from the field trial. Either remove this or let's work on better naming for all the *Http* stuff.
https://codereview.chromium.org/813873005/diff/140001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/813873005/diff/140001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_model_impl.cc:71: if (choice == "no") On 2015/01/08 00:46:56, felt wrote: > nit: I'd recommend making these values constants. That's the normal pattern for > field trial strings. It also ensures that you use the same ones for the command > line and field trial, since you're using a single constant variable twice > instead of repeating the string twice in this method. Done. https://codereview.chromium.org/813873005/diff/140001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_model_impl.cc:115: if (url.SchemeIs("http") || url.SchemeIs("ftp")) > This (marking FTP here) is _not_ what I'd expect from the rest of this CL, from > your variables, from the command-line, or from the field trial. > > Either remove this or let's work on better naming for all the *Http* stuff. I'd prefer the latter — it would be confusing for the 2 people who have ever seen ftp:// URLs. I'll rename *Http* as kMarkNonSecureAs*, et c.
> I'm surprised and shocked that you don't have to trust the website settings UI > bubble when changing the toolbar UI. I'm not sure what you mean by this. > When I was working on this for SHA-1, I > found the two to be tightly coupled in behaviour - and could easily crash if > assumptions are violated. > > Have you checked to make sure this is fine? It seems to work for me, including interacting with the website settings/Page Info/Origin Info Bubble. > Where are the unittests for this? Nowhere yet. Do you have tests I should model mine after?
palmer@chromium.org changed reviewers: + sky@chromium.org - blelump@gmail.com, msw@chromium.org
Since msw will be out, +sky for chrome/browser/ui/toolbar/toolbar_model_impl.cc OWNERS.
palmer@chromium.org changed reviewers: - mpearson@google.com
You only need a review of chrome/browser/ui/toolbar/toolbar_model_impl.cc ? Right? https://codereview.chromium.org/813873005/diff/160001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/813873005/diff/160001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_model_impl.cc:73: else if (choice == switches::kMarkNonSecureAsDubious) nit: (chromium) style guide say no else after return. https://codereview.chromium.org/813873005/diff/160001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_model_impl.cc:78: std::string group = base::FieldTrialList::FindFullName("MarkNonSecureAs"); Is it common not to use constants for this?
Yes, just the 1 file. Thanks, sky! https://codereview.chromium.org/813873005/diff/160001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/813873005/diff/160001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_model_impl.cc:73: else if (choice == switches::kMarkNonSecureAsDubious) On 2015/01/22 21:56:47, sky wrote: > nit: (chromium) style guide say no else after return. Done. https://codereview.chromium.org/813873005/diff/160001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_model_impl.cc:78: std::string group = base::FieldTrialList::FindFullName("MarkNonSecureAs"); > Is it common not to use constants for this? Searching the code shows about 50/50.
LGTM
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/813873005/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/813873005/200001
The CQ bit was unchecked by palmer@chromium.org
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/813873005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...) Try jobs failed on following builders: linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/813873005/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/601fe3c76c5ae5a0fab68766a95b9313c1b408ec Cr-Commit-Position: refs/heads/master@{#312920} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
