Remove the current /prefetch switch when relaunching Chrome.

Remove the current /prefetch switch when relaunching Chrome.

Currently, when a browser process is relaunched, the /prefetch:#
switch of the current process is reused, which is wrong.

For example, when a process initially launched in background is
relaunched in foreground, it shouldn't reuse the /prefetch:#
switch dedicated to background browser processes.

With this CL, RemoveSwitchesForAutostart() removes the
/prefetch:# switch from the command line, if present.

BUG=577698
Committed: https://crrev.com/dd01ed2c79acd444ff3451a37d5b69cd8e7a5be0
Cr-Commit-Position: refs/heads/master@{#378751}

[fdoray, 2016-02-25 15:12:01]

fdoray@chromium.org changed reviewers:
+ jochen@chromium.org

[fdoray, 2016-02-25 15:12:01]

jochen@: Can you review this CL? Thanks.
cc: gab@

[gab, 2016-02-25 20:22:22]

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

[gab, 2016-02-25 20:22:23]

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

https://codereview.chromium.org/1734053002/diff/1/chrome/common/switch_utils....
chrome/common/switch_utils.cc:49: // "/prefetch:#" where # is [1, 8].
It's never appropriate to re-add one I think as we're always launching a browser
from this.

// The relaunched browser process shouldn't have a /prefetch switch, however
// it's possible the original command-line had one (e.g. a process initially
// launched in background is relaunched in foreground).

(no need to explicitly state that "we'll remove it" either as that's the purpose
of the method and it's thus implicit that if we say "it doesn't belong", we'll
also remove it)

https://codereview.chromium.org/1734053002/diff/1/chrome/common/switch_utils....
chrome/common/switch_utils.cc:59: }
This loop assumes there can be more than one prefetch switch. I don't think this
is necessary and suggest:

static const char kPrefetchSwitchPrefix[] = "prefetch:";
auto it = switch_list->lower_bound(kPrefetchSwitchPrefix);
if (it != switch_list->end() && 
    base::StartsWith(it->first, kPrefetchSwitchPrefix,
                     base::CompareCase::SENSITIVE)) {
  switch_list->erase(it);
}

https://codereview.chromium.org/1734053002/diff/1/chrome/common/switch_utils_...
File chrome/common/switch_utils_unittest.cc (right):

https://codereview.chromium.org/1734053002/diff/1/chrome/common/switch_utils_...
chrome/common/switch_utils_unittest.cc:81: FILE_PATH_LITERAL("/prefetch:2"),
As mentioned above, don't think we should support this scenario as it's an
invalid command-line (logic wise).

[fdoray, 2016-02-26 18:06:13]

Description was changed from

==========
Remove the current /prefetch switch when relaunching Chrome.

Currently, the /prefetch switch of a browser process is reused when the
process is relaunched. It can be wrong to do that. For example:

- A background Chrome process is running.
  (cmd=chrome.exe --no-startup-window /prefetch:5)
- The user clicks on the Chrome shortcut. A new process is launched.
  It rendezvous with the background Chrome process and exits.
- The user restarts Chrome via chrome://restart.
- A new process is launched. The /prefetch:5 switch is reused but
  it shouldn't (see kPrefetchArgumentBrowserBackground).
  (cmd=chrome.exe /prefetch:5).

With this CL, RemoveSwitchesForAutostart() removes /prefetch switches.
The code building a relaunch command line is responsible for adding its
own /prefetch switch afterwards when appropriate.

BUG=577698
==========

to

==========
Remove the current /prefetch switch when relaunching Chrome.

Currently, when a browser process is relaunched, the /prefetch:#
switch of the current process is reused, which is wrong.

For example, when a process initially launched in background is
relaunched in foreground, it shouldn't reuse the /prefetch:#
switch dedicated to background browser processes.

With this CL, RemoveSwitchesForAutostart() removes the
/prefetch:# switch from the command line, if present.

BUG=577698
==========

[fdoray, 2016-02-26 18:10:36]

gab@: Can you take another look? Thanks.

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

https://codereview.chromium.org/1734053002/diff/1/chrome/common/switch_utils....
chrome/common/switch_utils.cc:49: // "/prefetch:#" where # is [1, 8].
On 2016/02/25 20:22:23, gab wrote:
> It's never appropriate to re-add one I think as we're always launching a
browser
> from this.
> 
> // The relaunched browser process shouldn't have a /prefetch switch, however
> // it's possible the original command-line had one (e.g. a process initially
> // launched in background is relaunched in foreground).
> 
> (no need to explicitly state that "we'll remove it" either as that's the
purpose
> of the method and it's thus implicit that if we say "it doesn't belong", we'll
> also remove it)

Done. We do need to re-add a switch when a background process is relaunched in
background (background process relaunched in background after an update here
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...)

https://codereview.chromium.org/1734053002/diff/1/chrome/common/switch_utils....
chrome/common/switch_utils.cc:59: }
On 2016/02/25 20:22:23, gab wrote:
> This loop assumes there can be more than one prefetch switch. I don't think
this
> is necessary and suggest:
> 
> static const char kPrefetchSwitchPrefix[] = "prefetch:";
> auto it = switch_list->lower_bound(kPrefetchSwitchPrefix);
> if (it != switch_list->end() && 
>     base::StartsWith(it->first, kPrefetchSwitchPrefix,
>                      base::CompareCase::SENSITIVE)) {
>   switch_list->erase(it);
> }

Done.

https://codereview.chromium.org/1734053002/diff/1/chrome/common/switch_utils_...
File chrome/common/switch_utils_unittest.cc (right):

https://codereview.chromium.org/1734053002/diff/1/chrome/common/switch_utils_...
chrome/common/switch_utils_unittest.cc:81: FILE_PATH_LITERAL("/prefetch:2"),
On 2016/02/25 20:22:23, gab wrote:
> As mentioned above, don't think we should support this scenario as it's an
> invalid command-line (logic wise).

Done.

[gab, 2016-03-01 23:23:07]

lgtm, thanks

[jochen (gone - plz use gerrit), 2016-03-02 10:51:18]

lgtm

[fdoray, 2016-03-02 14:59:48]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-03-02 15:00:00]

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

[commit-bot: I haz the power, 2016-03-02 15:46:08]

Description was changed from

==========
Remove the current /prefetch switch when relaunching Chrome.

Currently, when a browser process is relaunched, the /prefetch:#
switch of the current process is reused, which is wrong.

For example, when a process initially launched in background is
relaunched in foreground, it shouldn't reuse the /prefetch:#
switch dedicated to background browser processes.

With this CL, RemoveSwitchesForAutostart() removes the
/prefetch:# switch from the command line, if present.

BUG=577698
==========

to

==========
Remove the current /prefetch switch when relaunching Chrome.

Currently, when a browser process is relaunched, the /prefetch:#
switch of the current process is reused, which is wrong.

For example, when a process initially launched in background is
relaunched in foreground, it shouldn't reuse the /prefetch:#
switch dedicated to background browser processes.

With this CL, RemoveSwitchesForAutostart() removes the
/prefetch:# switch from the command line, if present.

BUG=577698
==========

[commit-bot: I haz the power, 2016-03-02 15:46:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-02 15:48:03]

Description was changed from

==========
Remove the current /prefetch switch when relaunching Chrome.

Currently, when a browser process is relaunched, the /prefetch:#
switch of the current process is reused, which is wrong.

For example, when a process initially launched in background is
relaunched in foreground, it shouldn't reuse the /prefetch:#
switch dedicated to background browser processes.

With this CL, RemoveSwitchesForAutostart() removes the
/prefetch:# switch from the command line, if present.

BUG=577698
==========

to

==========
Remove the current /prefetch switch when relaunching Chrome.

Currently, when a browser process is relaunched, the /prefetch:#
switch of the current process is reused, which is wrong.

For example, when a process initially launched in background is
relaunched in foreground, it shouldn't reuse the /prefetch:#
switch dedicated to background browser processes.

With this CL, RemoveSwitchesForAutostart() removes the
/prefetch:# switch from the command line, if present.

BUG=577698

Committed: https://crrev.com/dd01ed2c79acd444ff3451a37d5b69cd8e7a5be0
Cr-Commit-Position: refs/heads/master@{#378751}
==========

[commit-bot: I haz the power, 2016-03-02 15:48:04]

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