Delete the auto-launch trial.

Delete the auto-launch trial.

BUG=436543, 398813
TBR=thakis@chromium.org

Committed: https://crrev.com/f2901f6da9887b9ca5cdd576191bde0dc6676d63
Cr-Commit-Position: refs/heads/master@{#359299}

[gab, 2015-11-10 03:21:05]

Description was changed from

==========
Delete the AutoLaunchExperiment.

BUG=436543
==========

to

==========
Delete the auto-launch trial.

BUG=436543
==========

[gab, 2015-11-10 03:22:48]

gab@chromium.org changed reviewers:
+ pkasting@chromium.org

[gab, 2015-11-10 03:22:49]

Peter PTAL, as discussed on http://crbug.com/436543.

Thanks,
Gab

PS: Also planning to remove auto_launch_util et al. in a follow-up but this CL
is big enough and nicely self-contained as-is.

[Peter Kasting, 2015-11-10 05:17:27]

RS LGTM

I'm not intimately familiar with this feature so I'm unable to give you a more
thorough review as to whether you missed important things.

[gab, 2015-11-10 14:01:59]

gab@chromium.org changed reviewers:
+ finnur@chromium.org

[gab, 2015-11-10 14:02:37]

On 2015/11/10 05:17:27, Peter Kasting wrote:
> RS LGTM
> 
> I'm not intimately familiar with this feature so I'm unable to give you a more
> thorough review as to whether you missed important things.

@finnur: this was your feature if I'm reading the archives correctly, mind
taking a look?

Thanks,
Gab

[gab, 2015-11-10 14:04:05]

gab@chromium.org changed reviewers:
+ bauerb@chromium.org

[gab, 2015-11-10 14:05:14]

gab@chromium.org changed reviewers:
+ asvitkine@chromium.org, thakis@chromium.org

[gab, 2015-11-10 14:06:07]

On 2015/11/10 14:02:37, gab wrote:
> On 2015/11/10 05:17:27, Peter Kasting wrote:
> > RS LGTM
> > 
> > I'm not intimately familiar with this feature so I'm unable to give you a
more
> > thorough review as to whether you missed important things.
> 
> @finnur: this was your feature if I'm reading the archives correctly, mind
> taking a look?
> 
> Thanks,
> Gab

@asvitkine for chrome/browser/chrome_browser_field_trials_desktop.cc
@thakis for chrome/browser/auto_launch_trial.(cc|h)
@bauerb for chrome/browser/resources/* and chrome/browser/prefs/*

[Bernhard Bauer, 2015-11-10 16:14:14]

LGTM w/ nits:

https://codereview.chromium.org/1432033003/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/1432033003/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:228: // Deprecated 11/15.
Use the four-digit year.

https://codereview.chromium.org/1432033003/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:229: const char
kShownAutoLaunchInfobarDeprecated[] =
Can you add a TODO with fixed date / milestone to remove this and a bug number?

https://codereview.chromium.org/1432033003/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:232: }  // namespace
Kinda unrelated, but the anonymous namespace is unnecessary.

[Alexei Svitkine (slow), 2015-11-10 16:17:05]

lgtm

https://codereview.chromium.org/1432033003/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/1432033003/diff/1/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1601: // auto-launch" infobar.
Delete the comment too.

[gab, 2015-11-10 16:29:03]

Thanks, done.

@Nico for chrome/auto_launch_trial.(cc|h) rubberstamp.

https://codereview.chromium.org/1432033003/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/1432033003/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:228: // Deprecated 11/15.
On 2015/11/10 16:14:13, Bernhard Bauer wrote:
> Use the four-digit year.

Done.

https://codereview.chromium.org/1432033003/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:229: const char
kShownAutoLaunchInfobarDeprecated[] =
On 2015/11/10 16:14:13, Bernhard Bauer wrote:
> Can you add a TODO with fixed date / milestone to remove this and a bug
number?

Done, no bug number though, I tend to just make it a rule for myself or anything
I review that touches this file to ask them to cleanup past due removal code at
the same time (like I did for kLegacyProfileResetPromptMemento in this CL).

https://codereview.chromium.org/1432033003/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:232: }  // namespace
On 2015/11/10 16:14:13, Bernhard Bauer wrote:
> Kinda unrelated, but the anonymous namespace is unnecessary.

True, but Chromium style tends to put everything that is local in an anonymous
namespace, so I'd prefer to keep it that way.

https://codereview.chromium.org/1432033003/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/1432033003/diff/1/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1601: // auto-launch" infobar.
On 2015/11/10 16:17:05, Alexei Svitkine (slow) wrote:
> Delete the comment too.

Oops, good catch, thanks

[gab, 2015-11-10 16:29:17]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-10 16:29:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432033003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432033003/40001

[commit-bot: I haz the power, 2015-11-10 16:40:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-10 16:40:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2015-11-10 19:06:31]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-10 19:08:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432033003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432033003/60001

[commit-bot: I haz the power, 2015-11-10 19:55:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-10 19:55:27]

Dry run: This issue passed the CQ dry run.

[Finnur, 2015-11-11 09:54:49]

LGTM.

And thanks for doing this! Sorry I didn't get to it -- it wasn't forgotten, it
just never quite reached the top of my long list of things to do.

[Finnur, 2015-11-11 09:55:53]

Forgot the comments... (wish Rietveld would warn when you reply without
publishing comments -- or ask if the drafts should also be published).

https://codereview.chromium.org/1432033003/diff/60001/chrome/browser/auto_lau...
File chrome/browser/auto_launch_trial.h (left):

https://codereview.chromium.org/1432033003/diff/60001/chrome/browser/auto_lau...
chrome/browser/auto_launch_trial.h:22: //   - brand RNGQ does not.
I don't know if we still send out these builds, but can you double check with
someone to see if we need to notify someone that we should not be sending out
branded builds, expecting this brand code to do something?

https://codereview.chromium.org/1432033003/diff/60001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (left):

https://codereview.chromium.org/1432033003/diff/60001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:235: // TODO(engedy): Remove this and
usages in M42 or later. See crbug.com/398813.
You should add BUG=398813 to this CL's description (and close the bug
afterwards, I presume).

https://codereview.chromium.org/1432033003/diff/60001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:555:
local_state->ClearPref(kLegacyProfileResetPromptMemento);
It is hard for me to know if these (and some of the ones above) can be
removed...

https://codereview.chromium.org/1432033003/diff/60001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/1432033003/diff/60001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:515: // go here.
This looks a bit weird, depending on what you are aiming for, either:

1) Add a linebreak between 515 and 516,
2) Remove the indentation of lines 514 and 515
OR
3) Move the comment inside the #if

[gab, 2015-11-11 21:23:38]

Description was changed from

==========
Delete the auto-launch trial.

BUG=436543
==========

to

==========
Delete the auto-launch trial.

BUG=436543, 398813
==========

[gab, 2015-11-11 21:30:03]

Thanks, responses inline, landing.

https://codereview.chromium.org/1432033003/diff/60001/chrome/browser/auto_lau...
File chrome/browser/auto_launch_trial.h (left):

https://codereview.chromium.org/1432033003/diff/60001/chrome/browser/auto_lau...
chrome/browser/auto_launch_trial.h:22: //   - brand RNGQ does not.
On 2015/11/11 09:55:53, Finnur wrote:
> I don't know if we still send out these builds, but can you double check with
> someone to see if we need to notify someone that we should not be sending out
> branded builds, expecting this brand code to do something?

I sent a notification to chrome-ui-review and to arnoldkim (who's listed as
owner of that brand in google3), no one seems to care so far (and given the
*extremely low* UMA numbers for foreground auto-launch -- ref. issue 553790 and
follow-up CL https://codereview.chromium.org/1439803002/ -- I highly doubt
anyone is using this).

https://codereview.chromium.org/1432033003/diff/60001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (left):

https://codereview.chromium.org/1432033003/diff/60001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:235: // TODO(engedy): Remove this and
usages in M42 or later. See crbug.com/398813.
On 2015/11/11 09:55:53, Finnur wrote:
> You should add BUG=398813 to this CL's description (and close the bug
> afterwards, I presume).

Done.

https://codereview.chromium.org/1432033003/diff/60001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:555:
local_state->ClearPref(kLegacyProfileResetPromptMemento);
On 2015/11/11 09:55:53, Finnur wrote:
> It is hard for me to know if these (and some of the ones above) can be
> removed...

Based on comment on line 546: anything a year+ old should be removed.

The other ones are more than just this file so I didn't touch them in this CL,
but kLegacyProfileResetPromptMemento was well-timed as it highlighted why it's
done the same way for the newly deprecated pref.

https://codereview.chromium.org/1432033003/diff/60001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/1432033003/diff/60001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:515: // go here.
On 2015/11/11 09:55:53, Finnur wrote:
> This looks a bit weird, depending on what you are aiming for, either:
> 
> 1) Add a linebreak between 515 and 516,
> 2) Remove the indentation of lines 514 and 515
> OR
> 3) Move the comment inside the #if

This follows the same format as the code removed on line 361, but agree that (1)
will make it cleaner, done.

[gab, 2015-11-11 21:32:06]

TBR thakis for deletion of top-level c/b files that really didn't belong in
top-level in the first place :-)

[gab, 2015-11-11 21:32:28]

Description was changed from

==========
Delete the auto-launch trial.

BUG=436543, 398813
==========

to

==========
Delete the auto-launch trial.

BUG=436543, 398813
TBR=thakis@chromium.org
==========

[gab, 2015-11-11 21:32:34]

The CQ bit was checked by gab@chromium.org

[gab, 2015-11-11 21:32:35]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
asvitkine@chromium.org, pkasting@chromium.org, finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/1432033003/#ps80001
(title: "+line break")

[commit-bot: I haz the power, 2015-11-11 21:36:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432033003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432033003/80001

[commit-bot: I haz the power, 2015-11-11 21:51:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-11 21:51:37]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Finnur, 2015-11-12 10:12:28]

I got the same error yesterday when trying to land through the GC and it just
needed a retry, so I'll check the button for you.

[Finnur, 2015-11-12 10:12:35]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2015-11-12 10:12:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432033003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432033003/80001

[commit-bot: I haz the power, 2015-11-12 10:37:01]

Committed patchset #5 (id:80001)

[gab, 2015-11-12 13:38:08]

On 2015/11/12 10:12:28, Finnur wrote:
> I got the same error yesterday when trying to land through the GC and it just
> needed a retry, so I'll check the button for you.

Thanks :-)!

[commit-bot: I haz the power, 2015-11-12 20:06:33]

Patchset 5 (id:??) landed as
https://crrev.com/f2901f6da9887b9ca5cdd576191bde0dc6676d63
Cr-Commit-Position: refs/heads/master@{#359299}