Session restore shouldn't care about profile home pages.

Session restore shouldn't care about profile home pages.

The logic where we treated an empty URL as home page is outdated.

This CL contains the following additional fixes:

1) Disallow using empty URLs to denote anything (home page, new tab
page...) and force the callers of chrome::Navigate to specify which URL they
want to open. Using empty URLs was hiding bugs (see below).

2) Fixed StartupBrowserCreatorTest.UpdateWithTwoProfiles and
ProfilesWithoutPagesNotLaunched so that they don't work by accident (about:blank
used to be the home page - it wasn't restoring the previous session, but
launching the home page).

3) There was some code passing GURL("new_tab_page") around, and
comparing against that. But that's an invalid URL, so GURL will just make it
empty, and the result is not what is expected. (E.g,. GURL("foo") ==
GURL("new_tab_page")).

4) Fixed other places which were passing GURL("something_invalid") around and
pretending it's something meaningful. It was just a coincidence that nothing was
broken.


BUG=371852
R=sky@chromium.org

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

[marja, 2014-05-20 16:19:12]

sky, ptal

I might need to disable SigninBrowserTest.NotTrustedAfterRedirect for mac, since
the trivial fix (just make the URL valid, since it's clearly a mistake that the
test is using an invalid URL).

I filed a bug for that: crbug.com/375197

[marja, 2014-05-20 16:19:36]

On 2014/05/20 16:19:12, marja wrote:
> sky, ptal
> 
> I might need to disable SigninBrowserTest.NotTrustedAfterRedirect for mac,
since
> the trivial fix (just make the URL valid, since it's clearly a mistake that
the
> test is using an invalid URL).
> 
> I filed a bug for that: crbug.com/375197

... err, I mean: Since the trivial fix seems to break the test.

[sky, 2014-05-20 17:30:54]

+cpu see comment below

https://codereview.chromium.org/292713003/diff/120001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (left):

https://codereview.chromium.org/292713003/diff/120001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:894: if (it->host() ==
"new_tab_page") {
Are you sure we aren't actually using these names in some places? Say installers
or perhaps managed/custom installs? I'm adding cpu as he added these.

[cpu_(ooo_6.6-7.5), 2014-05-20 18:35:38]

https://codereview.chromium.org/292713003/diff/120001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (left):

https://codereview.chromium.org/292713003/diff/120001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:894: if (it->host() ==
"new_tab_page") {
On 2014/05/20 17:30:54, sky wrote:
> Are you sure we aren't actually using these names in some places? Say
installers
> or perhaps managed/custom installs? I'm adding cpu as he added these.

They where at some point at least. They will be in the master preferences files
that omaha carries then they become the preferences entries in tab restore or
something like that.

marga@ add grt@ to the review or at least ping him, if no answer try
robertshield@

[sky, 2014-05-20 19:23:47]

On 2014/05/20 18:35:38, cpu wrote:
>
https://codereview.chromium.org/292713003/diff/120001/chrome/browser/ui/start...
> File chrome/browser/ui/startup/startup_browser_creator_impl.cc (left):
> 
>
https://codereview.chromium.org/292713003/diff/120001/chrome/browser/ui/start...
> chrome/browser/ui/startup/startup_browser_creator_impl.cc:894: if (it->host()
==
> "new_tab_page") {
> On 2014/05/20 17:30:54, sky wrote:
> > Are you sure we aren't actually using these names in some places? Say
> installers
> > or perhaps managed/custom installs? I'm adding cpu as he added these.
> 
> They where at some point at least. They will be in the master preferences
files
> that omaha carries then they become the preferences entries in tab restore or
> something like that.
> 
> marga@ add grt@ to the review or at least ping him, if no answer try
> robertshield@

+grt and +robershield to see if they know if "new_tab_page" and the like are
still needed (see
https://codereview.chromium.org/292713003/diff/120001/chrome/browser/ui/start...
)

[robertshield, 2014-05-20 19:36:25]

https://codereview.chromium.org/292713003/diff/120001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (left):

https://codereview.chromium.org/292713003/diff/120001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:894: if (it->host() ==
"new_tab_page") {
On 2014/05/20 18:35:38, cpu wrote:
> On 2014/05/20 17:30:54, sky wrote:
> > Are you sure we aren't actually using these names in some places? Say
> installers
> > or perhaps managed/custom installs? I'm adding cpu as he added these.
> 
> They where at some point at least. They will be in the master preferences
files
> that omaha carries then they become the preferences entries in tab restore or
> something like that.

What Carlos said: these magic values are used by distribution partners,
enterprise admins, etc. who wish to customize Chrome at first run using
master_preferences files. 

Is there a reason for deleting them from here?

> 
> marga@ add grt@ to the review or at least ping him, if no answer try
> robertshield@

[sky, 2014-05-20 19:48:49]

On Tue, May 20, 2014 at 12:36 PM,  <robertshield@chromium.org> wrote:
>
>
https://codereview.chromium.org/292713003/diff/120001/chrome/browser/ui/start...
> File chrome/browser/ui/startup/startup_browser_creator_impl.cc (left):
>
>
https://codereview.chromium.org/292713003/diff/120001/chrome/browser/ui/start...
> chrome/browser/ui/startup/startup_browser_creator_impl.cc:894: if
> (it->host() == "new_tab_page") {
> On 2014/05/20 18:35:38, cpu wrote:
>>
>> On 2014/05/20 17:30:54, sky wrote:
>> > Are you sure we aren't actually using these names in some places?
>
> Say
>>
>> installers
>> > or perhaps managed/custom installs? I'm adding cpu as he added
>
> these.
>
>> They where at some point at least. They will be in the master
>
> preferences files
>>
>> that omaha carries then they become the preferences entries in tab
>
> restore or
>>
>> something like that.
>
>
> What Carlos said: these magic values are used by distribution partners,
> enterprise admins, etc. who wish to customize Chrome at first run using
> master_preferences files.
>
> Is there a reason for deleting them from here?

I don't think Marja realized they were needed. At a minimum there
should be comments describing the magic values. Better would be a
header that it makes it more obvious these values, while not used
anywhere in the code base, are important and not to be deleted.

  -Scott

>
>
>
>> marga@ add grt@ to the review or at least ping him, if no answer try
>> robertshield@
>
>
> https://codereview.chromium.org/292713003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[marja, 2014-05-21 09:23:40]

Thanks for noticing the bug! I didn't realize we might have correct magic URLs
coming from the outside.

I went overboard with the deletion, since I saw
browser_creator.AddFirstRunTab(GURL("new_tab_page")); in one test - that's an
invalid URL which won't pass the url.host() == "new_tab_page" check (probably
the test passed because invalid URL was treated as the home page). I didn't
realize that GURL("http://new_tab_page") will work.

Are the external users using this correctly with scheme://? Or does the scheme
get added internally? I'm worried, because in the test it says:

  // Simulate the following master_preferences:
  // {
  //  "first_run_tabs" : [
  //    "new_tab_page",
  //    "files/title1.html"
  //  ],

and if those are just converted to GURLs trivially, without adding the scheme,
it won't work :(

Anyhow, I've now restored the magic host code. Could you take another look?

[grt (UTC plus 2), 2014-05-21 10:46:32]

On 2014/05/21 09:23:40, marja wrote:
> Thanks for noticing the bug! I didn't realize we might have correct magic URLs
> coming from the outside.
> 
> I went overboard with the deletion, since I saw
> browser_creator.AddFirstRunTab(GURL("new_tab_page")); in one test - that's an
> invalid URL which won't pass the url.host() == "new_tab_page" check (probably
> the test passed because invalid URL was treated as the home page). I didn't
> realize that GURL("http://new_tab_page") will work.
> 
> Are the external users using this correctly with scheme://?

Yes. The recommendation is to use "http://" for the scheme as you have above.

Thanks!

> Or does the scheme
> get added internally? I'm worried, because in the test it says:
> 
>   // Simulate the following master_preferences:
>   // {
>   //  "first_run_tabs" : [
>   //    "new_tab_page",
>   //    "files/title1.html"
>   //  ],
> 
> and if those are just converted to GURLs trivially, without adding the scheme,
> it won't work :(
> 
> Anyhow, I've now restored the magic host code. Could you take another look?

[sky, 2014-05-21 16:03:10]

LGTM

[marja, 2014-05-22 07:37:17]

Thanks for review!

The latest patch set disables 2 tests on mac (the fixed versions of these tests
fail):

1) StartupBrowserCreatorTest.UpdateWithTwoProfiles

This is probably failing on mac because of some testing infra problem (hang
while waiting for the profiles to be restored??) crbug.com/376184

2) SigninBrowserTest.NotTrustedAfterRedirect

This feature is going to deprecated soon (says rogerta@ in crbug.com/375197).

[marja, 2014-05-22 07:37:24]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2014-05-22 07:38:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/marja@chromium.org/292713003/200001

[marja, 2014-05-22 14:53:23]

On 2014/05/22 07:38:24, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/marja@chromium.org/292713003/200001

Hmm, this has been in the commit queue for 7 hours. All other try jobs are
green, except win_gpu which hasn't ran for some reason. I'll commit this by
hand.

[marja, 2014-05-22 14:53:27]

The CQ bit was unchecked by marja@chromium.org

[marja, 2014-05-22 15:04:08]

Committed patchset #12 manually as r272182 (presubmit successful).