Remove logic for unused foreground auto-launch mode on Windows.

Remove logic for unused foreground auto-launch mode on Windows.

And distill auto_launch_util.cc to essentially the very simple thing it is
doing once all the if(in_foreground) code is out of the way..!

TBR=atwilson (metrics change in chrome/browser/background/background_mode_manager.cc already reviewed by metrics OWNER rkaplow@)
TBR=pkasting (side-effect of switch removal on chrome/browser/ui/startup/startup_browser_creator_impl.cc already reviewed by grt@)
TBR=gene (side-effect of switch removal on cloud_print -- that switch was a no-op for cloud_print anyways)
BUG=553790
TEST=Install pre-CL chrome, enable bg auto-launch, install new Chrome, confirm it uses the same key for bg auto-launch and works with it.

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

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

Description was changed from

==========
Remove logic for unused foreground auto-launch mode on Windows.

BUG=553790
==========

to

==========
Remove logic for unused foreground auto-launch mode on Windows.

And distill auto_launch_util.cc to essentially the very simple thing it was
doing if you remove all the if(in_foreground) code..

BUG=553790
==========

[gab, 2015-11-11 21:04:14]

gab@chromium.org changed reviewers:
+ atwilson@chromium.org, grt@chromium.org

[gab, 2015-11-11 21:04:15]

Greg PTAL

We can remove a ton of unused auto-launch code following
https://codereview.chromium.org/1432033003/ :-).

See bug for stats and reasoning!

Thanks,
Gab

https://codereview.chromium.org/1439803002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1439803002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:2642: +    enum="BooleanBackgroundMode">
@rkaplow: Is this sane? Essentially we're going from an enum (0-3) to a 0-1
(i.e., a bool) where 0/1 still mean the same thing they used to but 2/3 are no
more. Can we thus re-use the same histogram? That would make for a better
continuity in the data if so (note, report levels for 2/3 were SO low (< 0.00%)
that it shouldn't affect bucket counts for 0/1 when comparing before/after).

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

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

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

On 2015/11/11 21:04:15, gab wrote:
> Greg PTAL

+Drew for background mode changes.

+rkaplow for histograms (see Q below)

> 
> We can remove a ton of unused auto-launch code following
> https://codereview.chromium.org/1432033003/ :-).
> 
> See bug for stats and reasoning!
> 
> Thanks,
> Gab
> 
>
https://codereview.chromium.org/1439803002/diff/1/tools/metrics/histograms/hi...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1439803002/diff/1/tools/metrics/histograms/hi...
> tools/metrics/histograms/histograms.xml:2642: +   
enum="BooleanBackgroundMode">
> @rkaplow: Is this sane? Essentially we're going from an enum (0-3) to a 0-1
> (i.e., a bool) where 0/1 still mean the same thing they used to but 2/3 are no
> more. Can we thus re-use the same histogram? That would make for a better
> continuity in the data if so (note, report levels for 2/3 were SO low (<
0.00%)
> that it shouldn't affect bucket counts for 0/1 when comparing before/after).

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

Description was changed from

==========
Remove logic for unused foreground auto-launch mode on Windows.

And distill auto_launch_util.cc to essentially the very simple thing it was
doing if you remove all the if(in_foreground) code..

BUG=553790
==========

to

==========
Remove logic for unused foreground auto-launch mode on Windows.

And distill auto_launch_util.cc to essentially the very simple thing it was
doing once all the if(in_foreground) code is out of the way..!

BUG=553790
==========

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

Description was changed from

==========
Remove logic for unused foreground auto-launch mode on Windows.

And distill auto_launch_util.cc to essentially the very simple thing it was
doing once all the if(in_foreground) code is out of the way..!

BUG=553790
==========

to

==========
Remove logic for unused foreground auto-launch mode on Windows.

And distill auto_launch_util.cc to essentially the very simple thing it is
doing once all the if(in_foreground) code is out of the way..!

BUG=553790
==========

[gab, 2015-11-11 21:07:55]

Description was changed from

==========
Remove logic for unused foreground auto-launch mode on Windows.

And distill auto_launch_util.cc to essentially the very simple thing it is
doing once all the if(in_foreground) code is out of the way..!

BUG=553790
==========

to

==========
Remove logic for unused foreground auto-launch mode on Windows.

And distill auto_launch_util.cc to essentially the very simple thing it is
doing once all the if(in_foreground) code is out of the way..!

BUG=553790
TEST=Install pre-CL chrome, enable bg auto-launch, install new Chrome, confirm
it uses the same key for bg auto-launch and works with it.
==========

[grt (UTC plus 2), 2015-11-12 17:49:03]

looks good. nice to see unused code getting the axe!

https://codereview.chromium.org/1439803002/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1439803002/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:500: // don't open any
browser windows, unless kAutoLaunchAtStartup is also
please update this comment

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
File chrome/installer/util/auto_launch_util.cc (right):

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:25: // The prefix of the Chrome
Auto-launch key under the Run key.
nit: put this and GetAutoLaunchKeyName() into the unnamed namespace.

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:41: uint8 hash[16];
nit: uint8_t and #include <stdint.h>

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:42: crypto::SHA256HashString(input,
hash, sizeof(hash));
sizeof -> arraysize and #include "base/macros.h"

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:43: std::string hash_string =
base::HexEncode(hash, sizeof(hash));
nit: inline HexEncode below

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:50:
base::CommandLine::ForCurrentProcess()->GetProgram());
i think it's better to use DIR_EXE + kChromeExe as before. it's possible that
the command line could have been new_chrome.exe or old_chrome.exe or some other
junk we wouldn't want to persist in the registry.

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/util_...
File chrome/installer/util/util_constants.h (right):

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/util_...
chrome/installer/util/util_constants.h:121: // CONFIGURE_AUTO_LAUNCH = 16,   //
Deprecated.
nit: please remove the "// Deprecated." comment for consistency with the other
deprecated values.

[rkaplow, 2015-11-12 18:00:26]

https://codereview.chromium.org/1439803002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1439803002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:2642: +    enum="BooleanBackgroundMode">
On 2015/11/11 21:04:15, gab wrote:
> @rkaplow: Is this sane? Essentially we're going from an enum (0-3) to a 0-1
> (i.e., a bool) where 0/1 still mean the same thing they used to but 2/3 are no
> more. Can we thus re-use the same histogram? That would make for a better
> continuity in the data if so (note, report levels for 2/3 were SO low (<
0.00%)
> that it shouldn't affect bucket counts for 0/1 when comparing before/after).


We generally don't really like having data coming in untracked - we would prefer
if you left the enum, and changed the labels to DEPRECATED_*.
I realize it's unfortunate that our dashboards will still show the old values
(in dropdowns, etc.). I think that's a bug in the frontends I can report
separately.

[gab, 2015-11-12 18:44:26]

Done. PTAL (@rob: see reply below)

https://codereview.chromium.org/1439803002/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1439803002/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:500: // don't open any
browser windows, unless kAutoLaunchAtStartup is also
On 2015/11/12 17:49:02, grt wrote:
> please update this comment

Done.

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
File chrome/installer/util/auto_launch_util.cc (right):

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:25: // The prefix of the Chrome
Auto-launch key under the Run key.
On 2015/11/12 17:49:02, grt wrote:
> nit: put this and GetAutoLaunchKeyName() into the unnamed namespace.

Done.

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:41: uint8 hash[16];
On 2015/11/12 17:49:02, grt wrote:
> nit: uint8_t and #include <stdint.h>

Done.

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:42: crypto::SHA256HashString(input,
hash, sizeof(hash));
On 2015/11/12 17:49:02, grt wrote:
> sizeof -> arraysize and #include "base/macros.h"

Done.

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:43: std::string hash_string =
base::HexEncode(hash, sizeof(hash));
On 2015/11/12 17:49:02, grt wrote:
> nit: inline HexEncode below

Done.

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:50:
base::CommandLine::ForCurrentProcess()->GetProgram());
On 2015/11/12 17:49:02, grt wrote:
> i think it's better to use DIR_EXE + kChromeExe as before. it's possible that
> the command line could have been new_chrome.exe or old_chrome.exe or some
other
> junk we wouldn't want to persist in the registry.

Good point, done.

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/util_...
File chrome/installer/util/util_constants.h (right):

https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/util_...
chrome/installer/util/util_constants.h:121: // CONFIGURE_AUTO_LAUNCH = 16,   //
Deprecated.
On 2015/11/12 17:49:03, grt wrote:
> nit: please remove the "// Deprecated." comment for consistency with the other
> deprecated values.

Done.

https://codereview.chromium.org/1439803002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1439803002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:2642: +    enum="BooleanBackgroundMode">
On 2015/11/12 18:00:26, rkaplow wrote:
> On 2015/11/11 21:04:15, gab wrote:
> > @rkaplow: Is this sane? Essentially we're going from an enum (0-3) to a 0-1
> > (i.e., a bool) where 0/1 still mean the same thing they used to but 2/3 are
no
> > more. Can we thus re-use the same histogram? That would make for a better
> > continuity in the data if so (note, report levels for 2/3 were SO low (<
> 0.00%)
> > that it shouldn't affect bucket counts for 0/1 when comparing before/after).
> 
> 
> We generally don't really like having data coming in untracked - we would
prefer
> if you left the enum, and changed the labels to DEPRECATED_*.
> I realize it's unfortunate that our dashboards will still show the old values
> (in dropdowns, etc.). I think that's a bug in the frontends I can report
> separately.
> 
> 

Ok, brought back the enum in histograms.xml. Can I still only use
UMA_HISTOGRAM_BOOLEAN in the code (it's a 0/1 enum in practice IIRC, right?).

[grt (UTC plus 2), 2015-11-12 18:51:39]

lgtm w/ fix below

https://codereview.chromium.org/1439803002/diff/20001/chrome/installer/util/a...
File chrome/installer/util/auto_launch_util.cc (right):

https://codereview.chromium.org/1439803002/diff/20001/chrome/installer/util/a...
chrome/installer/util/auto_launch_util.cc:8: 
nit: no newline here as per example in
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

https://codereview.chromium.org/1439803002/diff/20001/chrome/installer/util/a...
chrome/installer/util/auto_launch_util.cc:58:
application_path.Append(installer::kChromeExe);
ha! you've been fooled by the confusing FilePath API! Append() is a const method
that returns a new FilePath. i suggest renaming application_path to
application_dir and putting the Append call inline below like:
  base::CommandLine cmd_line(application_dir.Append(installer::kChromeExe));

[gab, 2015-11-12 18:56:31]

https://codereview.chromium.org/1439803002/diff/20001/chrome/installer/util/a...
File chrome/installer/util/auto_launch_util.cc (right):

https://codereview.chromium.org/1439803002/diff/20001/chrome/installer/util/a...
chrome/installer/util/auto_launch_util.cc:58:
application_path.Append(installer::kChromeExe);
On 2015/11/12 18:51:39, grt wrote:
> ha! you've been fooled by the confusing FilePath API! Append() is a const
method
> that returns a new FilePath. i suggest renaming application_path to
> application_dir and putting the Append call inline below like:
>   base::CommandLine cmd_line(application_dir.Append(installer::kChromeExe));

Indeed, good eyes ;-), had caught this myself as well in my manual test, fixed!

[gab, 2015-11-12 18:59:14]

https://codereview.chromium.org/1439803002/diff/20001/chrome/installer/util/a...
File chrome/installer/util/auto_launch_util.cc (right):

https://codereview.chromium.org/1439803002/diff/20001/chrome/installer/util/a...
chrome/installer/util/auto_launch_util.cc:58:
application_path.Append(installer::kChromeExe);
On 2015/11/12 18:56:30, gab wrote:
> On 2015/11/12 18:51:39, grt wrote:
> > ha! you've been fooled by the confusing FilePath API! Append() is a const
> method
> > that returns a new FilePath. i suggest renaming application_path to
> > application_dir and putting the Append call inline below like:
> >   base::CommandLine cmd_line(application_dir.Append(installer::kChromeExe));
> 
> Indeed, good eyes ;-), had caught this myself as well in my manual test,
fixed!

(too bad we don't have WARN_UNUSED_RESULT protection on Windows :-( -- 
https://code.google.com/p/chromium/codesearch#chromium/src/base/compiler_spec...)

[gab, 2015-11-12 19:14:17]

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

[gab, 2015-11-12 19:14:17]

+gene for cloud_print/ side-effects
+pkasting for side-effects in
chrome/browser/ui/startup/startup_browser_creator_impl.cc

Thanks,
Gab

[rkaplow, 2015-11-13 16:05:28]

On 2015/11/12 18:44:26, gab wrote:
> Done. PTAL (@rob: see reply below)
> 
>
https://codereview.chromium.org/1439803002/diff/1/chrome/browser/ui/startup/s...
> File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):
> 
>
https://codereview.chromium.org/1439803002/diff/1/chrome/browser/ui/startup/s...
> chrome/browser/ui/startup/startup_browser_creator_impl.cc:500: // don't open
any
> browser windows, unless kAutoLaunchAtStartup is also
> On 2015/11/12 17:49:02, grt wrote:
> > please update this comment
> 
> Done.
> 
>
https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
> File chrome/installer/util/auto_launch_util.cc (right):
> 
>
https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
> chrome/installer/util/auto_launch_util.cc:25: // The prefix of the Chrome
> Auto-launch key under the Run key.
> On 2015/11/12 17:49:02, grt wrote:
> > nit: put this and GetAutoLaunchKeyName() into the unnamed namespace.
> 
> Done.
> 
>
https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
> chrome/installer/util/auto_launch_util.cc:41: uint8 hash[16];
> On 2015/11/12 17:49:02, grt wrote:
> > nit: uint8_t and #include <stdint.h>
> 
> Done.
> 
>
https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
> chrome/installer/util/auto_launch_util.cc:42: crypto::SHA256HashString(input,
> hash, sizeof(hash));
> On 2015/11/12 17:49:02, grt wrote:
> > sizeof -> arraysize and #include "base/macros.h"
> 
> Done.
> 
>
https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
> chrome/installer/util/auto_launch_util.cc:43: std::string hash_string =
> base::HexEncode(hash, sizeof(hash));
> On 2015/11/12 17:49:02, grt wrote:
> > nit: inline HexEncode below
> 
> Done.
> 
>
https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/auto_...
> chrome/installer/util/auto_launch_util.cc:50:
> base::CommandLine::ForCurrentProcess()->GetProgram());
> On 2015/11/12 17:49:02, grt wrote:
> > i think it's better to use DIR_EXE + kChromeExe as before. it's possible
that
> > the command line could have been new_chrome.exe or old_chrome.exe or some
> other
> > junk we wouldn't want to persist in the registry.
> 
> Good point, done.
> 
>
https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/util_...
> File chrome/installer/util/util_constants.h (right):
> 
>
https://codereview.chromium.org/1439803002/diff/1/chrome/installer/util/util_...
> chrome/installer/util/util_constants.h:121: // CONFIGURE_AUTO_LAUNCH = 16,  
//
> Deprecated.
> On 2015/11/12 17:49:03, grt wrote:
> > nit: please remove the "// Deprecated." comment for consistency with the
other
> > deprecated values.
> 
> Done.
> 
>
https://codereview.chromium.org/1439803002/diff/1/tools/metrics/histograms/hi...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1439803002/diff/1/tools/metrics/histograms/hi...
> tools/metrics/histograms/histograms.xml:2642: +   
enum="BooleanBackgroundMode">
> On 2015/11/12 18:00:26, rkaplow wrote:
> > On 2015/11/11 21:04:15, gab wrote:
> > > @rkaplow: Is this sane? Essentially we're going from an enum (0-3) to a
0-1
> > > (i.e., a bool) where 0/1 still mean the same thing they used to but 2/3
are
> no
> > > more. Can we thus re-use the same histogram? That would make for a better
> > > continuity in the data if so (note, report levels for 2/3 were SO low (<
> > 0.00%)
> > > that it shouldn't affect bucket counts for 0/1 when comparing
before/after).
> > 
> > 
> > We generally don't really like having data coming in untracked - we would
> prefer
> > if you left the enum, and changed the labels to DEPRECATED_*.
> > I realize it's unfortunate that our dashboards will still show the old
values
> > (in dropdowns, etc.). I think that's a bug in the frontends I can report
> > separately.
> > 
> > 
> 
> Ok, brought back the enum in histograms.xml. Can I still only use
> UMA_HISTOGRAM_BOOLEAN in the code (it's a 0/1 enum in practice IIRC, right?).

right

[rkaplow, 2015-11-13 16:05:35]

lgtm

[gab, 2015-11-13 16:09:16]

Description was changed from

==========
Remove logic for unused foreground auto-launch mode on Windows.

And distill auto_launch_util.cc to essentially the very simple thing it is
doing once all the if(in_foreground) code is out of the way..!

BUG=553790
TEST=Install pre-CL chrome, enable bg auto-launch, install new Chrome, confirm
it uses the same key for bg auto-launch and works with it.
==========

to

==========
Remove logic for unused foreground auto-launch mode on Windows.

And distill auto_launch_util.cc to essentially the very simple thing it is
doing once all the if(in_foreground) code is out of the way..!

TBR=atwilson (metrics change in
chrome/browser/background/background_mode_manager.cc already reviewed by metrics
OWNER rkaplow@)
TBR=pkasting (side-effect of switch removal on
chrome/browser/ui/startup/startup_browser_creator_impl.cc already reviewed by
grt@)
TBR=gene (side-effect of switch removal on cloud_print -- that switch was a
no-op for cloud_print anyways)
BUG=553790
TEST=Install pre-CL chrome, enable bg auto-launch, install new Chrome, confirm
it uses the same key for bg auto-launch and works with it.
==========

[gab, 2015-11-13 16:09:43]

TBR atwilson (metrics change in
chrome/browser/background/background_mode_manager.cc already reviewed by metrics
OWNER rkaplow@)

TBR pkasting (side-effect of switch removal on
chrome/browser/ui/startup/startup_browser_creator_impl.cc already reviewed by
grt@)

TBR gene (side-effect of switch removal on cloud_print -- that switch was a
no-op for cloud_print anyways)

Thanks,
Gab

[gab, 2015-11-13 16:09:48]

The CQ bit was checked by gab@chromium.org

[gab, 2015-11-13 16:09:48]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1439803002/#ps40001
(title: "oops")

[commit-bot: I haz the power, 2015-11-13 16:09:58]

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

[commit-bot: I haz the power, 2015-11-13 16:24:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-13 16:24:58]

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

[Peter Kasting, 2015-11-14 01:19:33]

LGTM

[Andrew T Wilson (Slow), 2015-11-16 11:13:12]

background/ lgtm