Restore all profiles which were active when restoring the last open pages.

Restore all profiles which were active when restoring the last open pages.

Upon startup, all profiles which had browsers open at the last exit are restored.
This will restore the last open pages if the profile settings so dictate.

BUG=99088
TEST=see bug & ProfileManagerTest.LastActiveProfiles(AtShutdown)?, BrowserInitTest.StartupURLsForTwoProfiles


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

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

[marja, 2012-01-04 14:58:03]

Hi Jochen,

This is my first attempt to restore all profiles when restoring last open pages.
I'm not superconfident that the restoring code is at the right place
(BrowserInit::ProcessCmndLineImpl). Wdyt?

[jochen (gone - plz use gerrit), 2012-01-05 13:07:58]

lg so far

now some tests would be nice :)

[marja, 2012-01-09 10:39:48]

On 2012/01/05 13:07:58, jochen wrote:
> now some tests would be nice :)

Tests added. (Also, some of my earlier BrowserInitTests were disabled for mac,
by accident, so I enabled them in this CL.)

[jochen (gone - plz use gerrit), 2012-01-09 10:51:23]

lgtm

http://codereview.chromium.org/9087009/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_browsertest.h (right):

http://codereview.chromium.org/9087009/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_browsertest.h:140: protected:
why is this needed?

[marja, 2012-01-09 11:04:32]

Thanks for the review!

http://codereview.chromium.org/9087009/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_browsertest.h (right):

http://codereview.chromium.org/9087009/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_browsertest.h:140: protected:
On 2012/01/09 10:51:23, jochen wrote:
> why is this needed?

It allows BrowserInitTest::StartupURLsForTwoProfiles to use temp_dir_.

[marja, 2012-01-09 13:33:20]

Hi OWNERS {mirandac,pkasting}, could you review the following parts:

mirandac:
chrome/browser/profiles/*

pkasting:
chrome/browser/ui/*

Thanks!

[Miranda Callahan, 2012-01-09 14:38:54]

Thanks, Marja -- LGTM with a couple comments.

http://codereview.chromium.org/9087009/diff/12014/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/9087009/diff/12014/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:498: default:
default block should also have braces.

http://codereview.chromium.org/9087009/diff/12014/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.h (right):

http://codereview.chromium.org/9087009/diff/12014/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.h:91: Profile* GetLastUsedProfile(const
FilePath& user_data_dir);
Could you add a comment here (or below) making it clear exactly how
"GetLastUsedProfile" differs from "GetLastActiveProfiles"? Or can we get rid of
"GetLastUsedProfile" and substitute getting the first item from
"GetLastActiveProfiles" now? If not, why not? I think future maintainers/coders
may be confused about which to use and why, so a sentence of explanation would
be helpful.

[Peter Kasting, 2012-01-10 02:20:12]

LGTM with nits

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

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1675: CommandLine::SwitchMap::const_iterator
switch_it;
Nit: Declare loop iterators/indexes inside the loop declaration (2 places)

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1686: command_line_without_urls, *it, cur_dir,
is_process_startup,
Nit: This indenting is strange:
* Since you're not doing one arg per line (fine), put as many args on the first
line as possible, as well as on the subsequent lines (2 places)
* Indent subsequent lines 4 or even, but not "4 from parens"

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1687: is_first_run, return_code)) {
Nit: No need for {}

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init.h (right):

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.h:46: bool Start(const CommandLine& cmd_line,
const FilePath& cur_dir,
Nit: One arg per line on declarations (2 places)

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.h:48: const std::vector<Profile*>&
other_profiles, int* return_code) {
Nit: I suggest typedefing "std::vector<Profile*>" to Profiles or some such.

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init_browsertest.cc (right):

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init_browsertest.cc:75: Browser** out_other_browser) {
Nit: Return a Browser* rather than using an outparam.

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init_browsertest.cc:80: other_browser = *i;
Nit: "break" here instead of using the "!other_browser" conditional in the loop
declaration (more common style, easier to read)

Note that after addressing the above nit, this line will just become a direct
return anyway.

[marja, 2012-01-10 14:12:02]

Thanks for review & comments!

(I also did a tiny test fix, the added BrowserInit test was failing on mac.
Patch set 9 is the test fix and a rebase.)

http://codereview.chromium.org/9087009/diff/12014/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/9087009/diff/12014/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:498: default:
On 2012/01/09 14:38:55, Miranda Callahan wrote:
> default block should also have braces.

Done.

http://codereview.chromium.org/9087009/diff/12014/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.h (right):

http://codereview.chromium.org/9087009/diff/12014/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.h:91: Profile* GetLastUsedProfile(const
FilePath& user_data_dir);
On 2012/01/09 14:38:55, Miranda Callahan wrote:
> Could you add a comment here (or below) making it clear exactly how
> "GetLastUsedProfile" differs from "GetLastActiveProfiles"? Or can we get rid
of
> "GetLastUsedProfile" and substitute getting the first item from
> "GetLastActiveProfiles" now? If not, why not? I think future
maintainers/coders
> may be confused about which to use and why, so a sentence of explanation would
> be helpful.

Clarified the comments:
For GetLastUsedProfile: Get the Profile last used (the Profile to which owns the
most recently focused window) with this Chrome build. If no signed profile has
been stored in Local State, hand back the Default profile.

For GetLastActiveProfiles: Get the Profiles which are currently active, i.e.,
have open browsers, or were active the last time Chrome was running. The
Profiles appear in the order they became active. The last used profile will be
on the list, but its index on the list will depend on when it became active (so,
it is not necessarily the last one).

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

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1675: CommandLine::SwitchMap::const_iterator
switch_it;
On 2012/01/10 02:20:12, Peter Kasting wrote:
> Nit: Declare loop iterators/indexes inside the loop declaration (2 places)

Done.

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1686: command_line_without_urls, *it, cur_dir,
is_process_startup,
On 2012/01/10 02:20:12, Peter Kasting wrote:
> Nit: This indenting is strange:
> * Since you're not doing one arg per line (fine), put as many args on the
first
> line as possible, as well as on the subsequent lines (2 places)
> * Indent subsequent lines 4 or even, but not "4 from parens"

Done.

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1687: is_first_run, return_code)) {
On 2012/01/10 02:20:12, Peter Kasting wrote:
> Nit: No need for {}

Done.

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init.h (right):

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.h:46: bool Start(const CommandLine& cmd_line,
const FilePath& cur_dir,
On 2012/01/10 02:20:12, Peter Kasting wrote:
> Nit: One arg per line on declarations (2 places)

Done.

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.h:48: const std::vector<Profile*>&
other_profiles, int* return_code) {
On 2012/01/10 02:20:12, Peter Kasting wrote:
> Nit: I suggest typedefing "std::vector<Profile*>" to Profiles or some such.

Done.

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init_browsertest.cc (right):

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init_browsertest.cc:75: Browser** out_other_browser) {
On 2012/01/10 02:20:12, Peter Kasting wrote:
> Nit: Return a Browser* rather than using an outparam.

Done.

http://codereview.chromium.org/9087009/diff/12016/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init_browsertest.cc:80: other_browser = *i;
On 2012/01/10 02:20:12, Peter Kasting wrote:
> Nit: "break" here instead of using the "!other_browser" conditional in the
loop
> declaration (more common style, easier to read)
> 
> Note that after addressing the above nit, this line will just become a direct
> return anyway.

Done.

[commit-bot: I haz the power, 2012-01-10 14:13:06]

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

[commit-bot: I haz the power, 2012-01-10 16:10:09]

Change committed as 117033

[marja, 2012-01-11 14:48:57]

This was committed but reverted because of www.crbug.com/109812 . Patch set 10
contains a fix, and while at it, I fixed confusing variable naming, too.

pkasting@, could you have a look if this CL still LGTY? Thanks!

[Peter Kasting, 2012-01-11 18:38:01]

LGTM

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

http://codereview.chromium.org/9087009/diff/27001/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1682: is_first_run, return_code)) {
Nit: No need for {}; could condense args onto two lines by indenting 4 instead
of even

http://codereview.chromium.org/9087009/diff/27001/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1689: if (*it == last_used_profile) {
Nit: Why not just make the loop body:

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

http://codereview.chromium.org/9087009/diff/27001/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init.h (right):

http://codereview.chromium.org/9087009/diff/27001/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.h:50: Profile* last_used_profile,
Nit: If you're going to mess with the names, I think |last_active_profile| and
|last_opened_profiles| (or |other_opened_profiles|) makes more sense.

[marja, 2012-01-12 09:20:22]

Thanks for comments again!

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

http://codereview.chromium.org/9087009/diff/27001/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1682: is_first_run, return_code)) {
On 2012/01/11 18:38:01, Peter Kasting wrote:
> Nit: No need for {}; could condense args onto two lines by indenting 4 instead
> of even

Done. (The style guides are unclear about whether I should indent 4 deeper than
the function call or 4 deeper than the "if", but from a previous comment I
inferred you mean the latter.)

http://codereview.chromium.org/9087009/diff/27001/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:1689: if (*it == last_used_profile) {
On 2012/01/11 18:38:01, Peter Kasting wrote:
> Nit: Why not just make the loop body:
> 
>       if ((*it != last_used_profile) &&
>           !browser_init->LaunchBrowser(command_line_without_urls, *it,
cur_dir,
>               is_process_startup, is_first_run, return_code))
>         return false;

Done.

http://codereview.chromium.org/9087009/diff/27001/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init.h (right):

http://codereview.chromium.org/9087009/diff/27001/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.h:50: Profile* last_used_profile,
On 2012/01/11 18:38:01, Peter Kasting wrote:
> Nit: If you're going to mess with the names, I think |last_active_profile| and
> |last_opened_profiles| (or |other_opened_profiles|) makes more sense.

Yes. ProfileManager already had LastUsedProfile() before this CL, so, I changed
"last active profiles" to "last opened profiles", so,
ProfileManager::LastOpenedProfiles() and the corresponding changes in parameter
names in BrowserInit.

[commit-bot: I haz the power, 2012-01-12 09:20:50]

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

[commit-bot: I haz the power, 2012-01-12 09:55:02]

Try job failure for 9087009-32001 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-01-12 10:08:54]

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

[commit-bot: I haz the power, 2012-01-12 11:48:49]

Change committed as 117420