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...

> 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

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.

> 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?