Do not show sync promo when RestoreOnStartupURLs policy is set

Do not show sync promo when RestoreOnStartupURLs policy is set

When first_run_tabs are set through master_preferences, the sync promo
should be shown if not explicitly suppressed. When RestoreOnStartupURLs
is set through policy, the sync promo should never be shown. This is
long-standing behavior, as evidenced here:

http://crbug.com/125467#c26

That behavior recently regressed with the sync promo appearing even if
RestoreOnStartupURLs are set. The regression happened here:

https://chromiumcodereview.appspot.com/12638005

This CL fixes the regression by ensuring that when RestoreOnStartup=4 and
RestoreOnStartupURLs are set, the sync promo is not shown. The CL also
adds browser tests to protect against similar regressions in the future.

BUG=244849
TEST=Manual and new browser_tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206915

[bartfab (slow), 2013-06-05 09:23:51]

Hi Scott,

Could you please review?

Hi Brett,

Could you please review the change to base/command_line.cc?

[sail, 2013-06-05 18:38:17]

Hi, I'm not sure why restore on startup should suppress the sync promo. Can you
get feedback from UI leads about this?

Two reasons why we don't want this:
  - some platforms have restore on startup set by default. Disabling the sync
promo on these platforms is not desirable.
  - the sync promo only shows 10 times if the user doesn't explicitly skip it.
This shouldn't interfere with startup URLs.

[bartfab (slow), 2013-06-05 19:08:45]

On 2013/06/05 18:38:17, sail wrote:
> Hi, I'm not sure why restore on startup should suppress the sync promo. Can
you
> get feedback from UI leads about this?
> 
> Two reasons why we don't want this:
>   - some platforms have restore on startup set by default. Disabling the sync
> promo on these platforms is not desirable.
>   - the sync promo only shows 10 times if the user doesn't explicitly skip it.
> This shouldn't interfere with startup URLs.

The policy name RestoreOnStartupURLs is misleading. It has nothing to do with
"restore last session on startup" (which, as you say, is the default on some
platforms). Instead, this policy kicks in only when the related policy
RestoreOnStartup is set to 4, which translates to "open the following set of
pages". If the administrator has specified a set of pages by setting both of
these policies, we do not want to be adding a sync promo on top of these. That
is what the CL fixes.

[bartfab (slow), 2013-06-11 14:45:40]

Scott, Brett - Review ping.

[sky, 2013-06-11 16:32:54]

startup clearly needs to be refactored so that you don't need to write a browser
test to verify it works. *SIGH*

LGTM

https://chromiumcodereview.appspot.com/16141008/diff/40002/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right):

https://chromiumcodereview.appspot.com/16141008/diff/40002/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:999: // On a
branded Linux build, policy is required to supporess the first-run
suppress

[bartfab (slow), 2013-06-12 10:55:15]

Hi Mark, Darin, Will:

Brett seems to be out. Could any of you do an owner review of the one-line
change to base/command_line.cc?

https://chromiumcodereview.appspot.com/16141008/diff/40002/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right):

https://chromiumcodereview.appspot.com/16141008/diff/40002/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:999: // On a
branded Linux build, policy is required to supporess the first-run
On 2013/06/11 16:32:54, sky wrote:
> suppress

Done.

[bartfab (slow), 2013-06-12 10:59:40]

As requested by Sailesh, I filed a bug and added a TODO to rethink and redo the
entire logic in the future.

[Mark Mentovai, 2013-06-12 15:29:09]

LGTM for the change in base on its own, although I’m not convinced that messing
with the command line is always a good idea. What if this code winds up running
before another test that doesn’t want the command line to be munged?

[bartfab (slow), 2013-06-12 18:25:42]

On 2013/06/12 15:29:09, Mark Mentovai wrote:
> LGTM for the change in base on its own, although I’m not convinced that
messing
> with the command line is always a good idea. What if this code winds up
running
> before another test that doesn’t want the command line to be munged?

The command line gets set up by InProcessBrowserTest. So the next test should
restore the flag that this one removed.

[Mark Mentovai, 2013-06-12 18:30:15]

OK, well, my LGTM applies to base, and I have no real objections, so you should
be good to go here.

[sail, 2013-06-13 06:44:37]

lgtm

[commit-bot: I haz the power, 2013-06-17 18:24:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/16141008/94001

[commit-bot: I haz the power, 2013-06-17 22:58:42]

Retried try job too often on win_rel for step(s) chrome_frame_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-17 23:01:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/16141008/94001

[commit-bot: I haz the power, 2013-06-18 02:10:53]

Retried try job too often on win_rel for step(s) chrome_frame_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-18 06:04:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/16141008/94001

[commit-bot: I haz the power, 2013-06-18 08:19:24]

Change committed as 206915