Fix: after updating, restore tabs from all profiles.

Fix: after updating, restore tabs from all profiles.

Also, launch the profiles in the order they became active. (Windows are
still not launched in the original order, since that information is lost.)

BUG=109816
TEST=See bug & BrowserInitTest.UpdateWithTwoProfiles


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=118286

[marja, 2012-01-16 15:55:16]

Hi,

This is a fix to http://codereview.chromium.org/9087009 .

pkasting: Could you review this, too? Thanks!

[Peter Kasting, 2012-01-16 20:13:05]

LGTM

Your comment makes me wonder if we can save and restore the window Z-order
within a profile as well.

http://codereview.chromium.org/9232007/diff/6001/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/9232007/diff/6001/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:1694: if (*it == last_used_profile) {
Nit: Shorter and (IMO) clearer:

      if (!browser_init->LaunchBrowser((*it == last_used_profile) ?
          command_line : command_line_without_urls, *it, cur_dir,
          is_process_startup, is_first_run, return_code))
        return false;

[marja, 2012-01-18 17:08:17]

Thanks for the comments!

In addition, I did the following fixes:

- The BrowserInitTest::UpdateWithTwoProfiles was incorrect; it was not
simulating the "browser was restarted" corerctly, and it was passing both before
and after the bugfix. Fixed that.

- After this CL, the kWasRestarted preference is always read during browser
creation. This makes InProcessBrowserTest read it so early on that the
ReadWasRestartedAfter(NormaS|Res)tart were too late trying to set the
preference. I fixed this by making was_restarted_read_ a static member variable
of BrowserInit instead of a static variable inside BrowserInit::WasRestarted, so
that the tests can set it to false and force re-reading the pref.

- last_used_profile might be the default profile and in that case it won't be in
last_opened_profiles, modified BrowserInit::ProcessCmdLineImpl to take that into
account. (All ui tests were failing because of this.)

pkasting, can you check if this still LGTY after these fixes?

Afaics, the remaining "windows not in the correct order" problem is that windows
from multiple profiles don't appear in the correct order. It's the
responsibility of SessionService to restore each profile correctly, but if the
windows from different profiles are interleaved, they aren't restored in the
right order since the restoring is done for each profile separately.

http://codereview.chromium.org/9232007/diff/6001/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/9232007/diff/6001/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:1694: if (*it == last_used_profile) {
On 2012/01/16 20:13:05, Peter Kasting wrote:
> Nit: Shorter and (IMO) clearer:
> 
>       if (!browser_init->LaunchBrowser((*it == last_used_profile) ?
>           command_line : command_line_without_urls, *it, cur_dir,
>           is_process_startup, is_first_run, return_code))
>         return false;

Done.

[Peter Kasting, 2012-01-18 17:30:03]

On 2012/01/18 17:08:17, marja wrote:
> Afaics, the remaining "windows not in the correct order" problem is that
windows
> from multiple profiles don't appear in the correct order. It's the
> responsibility of SessionService to restore each profile correctly, but if the
> windows from different profiles are interleaved, they aren't restored in the
> right order since the restoring is done for each profile separately.

OK, I had assumed that would be a problem, and was mostly interested in ensuring
that within each profile the windows were ordered correctly.

However I suppose another thing we could potentially do would be to keep a
global window order list so we could restore all the windows in the correct
order.  Presumably the easiest way to do this would be to make restoring a
profile a two-stage process, one where we bring up the profile and its data
structures, and a second where we restore windows.  Then we could bring the
profiles up in any order, and once they were all up, run through the window list
and restore each.  This would also do the correct thing for the common,
single-active-profile case.

http://codereview.chromium.org/9232007/diff/17002/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/9232007/diff/17002/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1695: command_line :
command_line_without_urls, *it, cur_dir,
Nit: Indent 4, not 8

http://codereview.chromium.org/9232007/diff/17002/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1702: // |last_opened_profiles|. Make sure
it's launched.
Is launching this last correct, or do we need to launch it first?

Maybe it would be better to make |last_opened_profiles| always contain all
opened profiles in the correct order, rather than omitting the default profile
if it was present.  This seems more consistent and comprehensible and less
error-prone.

[marja, 2012-01-18 18:01:12]

Thanks for comments!

http://codereview.chromium.org/9232007/diff/17002/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/9232007/diff/17002/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1695: command_line :
command_line_without_urls, *it, cur_dir,
On 2012/01/18 17:30:03, Peter Kasting wrote:
> Nit: Indent 4, not 8

Done.

http://codereview.chromium.org/9232007/diff/17002/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1702: // |last_opened_profiles|. Make sure
it's launched.
On 2012/01/18 17:30:03, Peter Kasting wrote:
> Is launching this last correct, or do we need to launch it first?
> 
> Maybe it would be better to make |last_opened_profiles| always contain all
> opened profiles in the correct order, rather than omitting the default profile
> if it was present.  This seems more consistent and comprehensible and less
> error-prone.

Clarified this: .empty() instead of find(), and in that case it doesn't matter
if it's launched before or after going through the list. ProfileManager not
changed, based on our discussion...

[Peter Kasting, 2012-01-18 18:04:30]

LGTM

http://codereview.chromium.org/9232007/diff/24001/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/9232007/diff/24001/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1705: if (last_opened_profiles.empty()) {
Nit: We might also want to DCHECK(is_first_run) inside this block?  Also, all
the |command_line_without_urls| code above (as well as the for loop) could go in
an else block of this conditional so that we don't bother to do it if we don't
need it.

[marja, 2012-01-19 08:38:22]

Thanks for the review!

http://codereview.chromium.org/9232007/diff/24001/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/9232007/diff/24001/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1705: if (last_opened_profiles.empty()) {
On 2012/01/18 18:04:30, Peter Kasting wrote:
> Nit: We might also want to DCHECK(is_first_run) inside this block?  Also, all
> the |command_line_without_urls| code above (as well as the for loop) could go
in
> an else block of this conditional so that we don't bother to do it if we don't
> need it.

Done.

[commit-bot: I haz the power, 2012-01-19 08:52:09]

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

[marja, 2012-01-19 10:27:02]

http://codereview.chromium.org/9232007/diff/24001/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/9232007/diff/24001/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1705: if (last_opened_profiles.empty()) {
On 2012/01/19 08:38:23, marja wrote:
> On 2012/01/18 18:04:30, Peter Kasting wrote:
> > Nit: We might also want to DCHECK(is_first_run) inside this block?  Also,
all
> > the |command_line_without_urls| code above (as well as the for loop) could
go
> in
> > an else block of this conditional so that we don't bother to do it if we
don't
> > need it.
> 
> Done.

Oops, I was wrong. The "first run" is not the only case when
last_opened_profiles is empty. The other case is after the user has exited the
browser by closing all windows for all profiles. In that case, no profiles are
open exit time, and the correct behaviour is to launch the last used profile (as
the code already does). I removed the DCHECK.

[commit-bot: I haz the power, 2012-01-19 10:27:17]

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

[commit-bot: I haz the power, 2012-01-19 11:35:02]

Change committed as 118286

[Peter Kasting, 2012-01-19 18:41:17]

On 2012/01/19 10:27:02, marja wrote:
> Oops, I was wrong. The "first run" is not the only case when
> last_opened_profiles is empty. The other case is after the user has exited the
> browser by closing all windows for all profiles.

Does this mean Wrench->Exit?  Or does this refer to manually closing windows one
by one?

If the former, then I'm confused how we can have a shutdown with multiple "last
opened" profiles except when recovering from a crash.  If the latter, and the
former causes us to reopen all windows on restart, this is fine.

[marja, 2012-01-20 07:04:20]

On 2012/01/19 18:41:17, Peter Kasting wrote:
> On 2012/01/19 10:27:02, marja wrote:
> > Oops, I was wrong. The "first run" is not the only case when
> > last_opened_profiles is empty. The other case is after the user has exited
the
> > browser by closing all windows for all profiles.
> 
> Does this mean Wrench->Exit?  Or does this refer to manually closing windows
one
> by one?
> 
> If the former, then I'm confused how we can have a shutdown with multiple
"last
> opened" profiles except when recovering from a crash.  If the latter, and the
> former causes us to reopen all windows on restart, this is fine.

Yes, the latter, and the former causes us to reopen all windows.