Three way experiment for "Show saved copy" button.

Three way experiment for "Show saved copy" button.

BUG=368049
Committed: https://crrev.com/78a42549731e69435c188b4eb38425af6ec7a9d5
Cr-Commit-Position: refs/heads/master@{#320113}

[megjablon, 2015-02-26 22:52:20]

Patchset #2 (id:20001) has been deleted

[megjablon, 2015-02-26 23:02:44]

megjablon@chromium.org changed reviewers:
+ mmenke@chromium.org

[megjablon, 2015-02-26 23:02:44]

Hey Matt! I'm finishing up the show saved copy experiment cl that you reviewed
about 6 months back for rdsmith. There are only a few small changes. I added
testing of post requests in ErrorPageTest.StaleCacheStatus of
errorpage_browsertest.cc. I also made it so that neterror.js has the the Show
Saved Copy button always be blue when it is the only button. Can you please take
another look?

[mmenke, 2015-02-27 20:27:14]

Looks pretty reasonable, just wonder if we can clean up the
ChromeContentBrowserClient a bit.

https://codereview.chromium.org/961443002/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/961443002/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1420:
switches::kEnableShowSavedCopyPrimary)) {
Maybe switch to a single switch with 3 different values?  Also, can't field
trials now set switches, either through finch, or about_flags.h, or something?

Then you'd just need to add one switch to kSwitchNames below, and remove this
entire block of code, I believe.

[megjablon, 2015-02-27 22:41:57]

https://codereview.chromium.org/961443002/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/961443002/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1420:
switches::kEnableShowSavedCopyPrimary)) {
On 2015/02/27 20:27:14, mmenke wrote:
> Maybe switch to a single switch with 3 different values?  Also, can't field
> trials now set switches, either through finch, or about_flags.h, or something?
> 
> Then you'd just need to add one switch to kSwitchNames below, and remove this
> entire block of code, I believe.

I changed it to a single switch with 3 values. I couldn't find a way to set
switches through field trials, but I can keep looking around. I could move all
the logic for the field trials into localized_error.cc so that we check the
switches and then the field trial there.

[mmenke, 2015-03-02 19:46:12]

On 2015/02/27 22:41:57, megjablon wrote:
>
https://codereview.chromium.org/961443002/diff/40001/chrome/browser/chrome_co...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/961443002/diff/40001/chrome/browser/chrome_co...
> chrome/browser/chrome_content_browser_client.cc:1420:
> switches::kEnableShowSavedCopyPrimary)) {
> On 2015/02/27 20:27:14, mmenke wrote:
> > Maybe switch to a single switch with 3 different values?  Also, can't field
> > trials now set switches, either through finch, or about_flags.h, or
something?
> > 
> > Then you'd just need to add one switch to kSwitchNames below, and remove
this
> > entire block of code, I believe.
> 
> I changed it to a single switch with 3 values. I couldn't find a way to set
> switches through field trials, but I can keep looking around. I could move all
> the logic for the field trials into localized_error.cc so that we check the
> switches and then the field trial there.

I can't find anything about it, either, so I must have been mistaken.

LGTM.  If we were keeping this long term, I think it would be worth trying to
clean up the ChromeContentBrowserClient more, but since this flag shouldn't be
long lived, doesn't seem worth more effort.

[megjablon, 2015-03-02 19:53:04]

megjablon@chromium.org changed reviewers:
+ asvitkine@chromium.org, thestig@chromium.org

[megjablon, 2015-03-02 19:53:05]

asvitkine: histograms.xml

thestig: net_error.*, localized_error.*, errorpage_browsertest.cc

[Lei Zhang, 2015-03-02 20:11:52]

still lgtm

[Lei Zhang, 2015-03-02 20:13:09]

whoops, wrong CL :\

[Alexei Svitkine (slow), 2015-03-02 20:16:43]

https://codereview.chromium.org/961443002/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/961443002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:53948: +  <int value="4" label="Show
Saved Copy Button Shown"/>
Just to confirm, this is changing the label of the same action? i.e. Load Stale
and Show Saved Copy is the same UI - just a different name for it in the code?
If so, this is fine.

If it's a different button, then you should add new values instead of replacing
these.

[megjablon, 2015-03-02 20:22:04]

https://codereview.chromium.org/961443002/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/961443002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:53948: +  <int value="4" label="Show
Saved Copy Button Shown"/>
On 2015/03/02 20:16:43, Alexei Svitkine wrote:
> Just to confirm, this is changing the label of the same action? i.e. Load
Stale
> and Show Saved Copy is the same UI - just a different name for it in the code?
> If so, this is fine.
> 
> If it's a different button, then you should add new values instead of
replacing
> these. 

Yep, this is just a name change, not a new button.

[Lei Zhang, 2015-03-02 20:31:04]

lgtm for reals this time.

https://codereview.chromium.org/961443002/diff/80001/chrome/browser/errorpage...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/961443002/diff/80001/chrome/browser/errorpage...
chrome/browser/errorpage_browsertest.cc:302:
command_line->AppendSwitchNative(switches::kShowSavedCopy,
Shouldn't this be AppendSwitchASCII() ?

https://codereview.chromium.org/961443002/diff/80001/chrome/common/localized_...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/961443002/diff/80001/chrome/common/localized_...
chrome/common/localized_error.cc:701: show_saved_copy_value ==
switches::kEnableShowSavedCopySecondary));
nit: I would just create a |show_saved_copy_secondary| variable so this
assignment logic is a bit more readable.

[Alexei Svitkine (slow), 2015-03-02 20:33:22]

lgtm

[megjablon, 2015-03-03 01:07:39]

https://codereview.chromium.org/961443002/diff/80001/chrome/browser/errorpage...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/961443002/diff/80001/chrome/browser/errorpage...
chrome/browser/errorpage_browsertest.cc:302:
command_line->AppendSwitchNative(switches::kShowSavedCopy,
On 2015/03/02 20:31:03, Lei Zhang wrote:
> Shouldn't this be AppendSwitchASCII() ?

Done.

https://codereview.chromium.org/961443002/diff/80001/chrome/common/localized_...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/961443002/diff/80001/chrome/common/localized_...
chrome/common/localized_error.cc:701: show_saved_copy_value ==
switches::kEnableShowSavedCopySecondary));
On 2015/03/02 20:31:03, Lei Zhang wrote:
> nit: I would just create a |show_saved_copy_secondary| variable so this
> assignment logic is a bit more readable.

Done.

[megjablon, 2015-03-11 17:30:09]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2015-03-11 17:30:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, asvitkine@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/961443002/#ps140001
(title: "histograms fix")

[commit-bot: I haz the power, 2015-03-11 17:31:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/961443002/140001

[commit-bot: I haz the power, 2015-03-11 18:23:41]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-03-11 18:24:15]

Patchset 7 (id:??) landed as
https://crrev.com/78a42549731e69435c188b4eb38425af6ec7a9d5
Cr-Commit-Position: refs/heads/master@{#320113}