Makes session restore on Chrome OS restore popups.

Makes session restore on Chrome OS restore popups.

BUG=18862
TEST=none


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

[Paweł Hajdan Jr., 2009-10-26 21:00:17]

Drive-by.

http://codereview.chromium.org/337030/diff/1/11
File chrome/browser/sessions/session_restore_uitest.cc (right):

http://codereview.chromium.org/337030/diff/1/11#newcode292
Line 292: if (type1 == BrowserProxy::TYPE_NORMAL) {
Is it possible to remove this "if"? I.e. to make browser_proxy1 always be the
same type, and similar with browser_proxy2? Or does it depend on how fast they
are restored?

http://codereview.chromium.org/337030/diff/1/2
File chrome/test/automation/automation_messages_internal.h (right):

http://codereview.chromium.org/337030/diff/1/2#newcode373
Line 373: bool /* if true tabbed window, otherwise popup */,
We have more than two types now. Please consider making this parameter int.

http://codereview.chromium.org/337030/diff/1/4
File chrome/test/automation/automation_proxy.h (right):

http://codereview.chromium.org/337030/diff/1/4#newcode96
Line 96: // False likely indicates an IPC error.
nit: Please fix the comment here and above: "Returns true on success.". You can
include the "IPC error" part if you like.

[brettw, 2009-10-26 21:10:33]

http://codereview.chromium.org/337030/diff/1/15
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/337030/diff/1/15#newcode1290
Line 1290: void AutomationProvider::OpenNewBrowserWindow(bool normal,
To me, this would be easier to understand and explain if it was "is_popup"
rather than "normal" (with the inverted meaning). It also seems more obvious
from the caller perspective that you don't want any options (like popup) so pass
false.

http://codereview.chromium.org/337030/diff/1/9
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/337030/diff/1/9#newcode233
Line 233: // Closing a window can never make has_open_trackable_browsers_ go
from false
This line is now too long.

[sky, 2009-10-26 21:28:08]

I couldn't nuke the if statement in that ui test though as it really
varies from run to run as which window gets activated first and
assigned to browser window 0.

Everything else was updated.

Thanks,

  -Scott

On Mon, Oct 26, 2009 at 2:10 PM,  <brettw@chromium.org> wrote:
>
> http://codereview.chromium.org/337030/diff/1/15
> File chrome/browser/automation/automation_provider.cc (right):
>
> http://codereview.chromium.org/337030/diff/1/15#newcode1290
> Line 1290: void AutomationProvider::OpenNewBrowserWindow(bool normal,
> To me, this would be easier to understand and explain if it was
> "is_popup" rather than "normal" (with the inverted meaning). It also
> seems more obvious from the caller perspective that you don't want any
> options (like popup) so pass false.
>
> http://codereview.chromium.org/337030/diff/1/9
> File chrome/browser/sessions/session_service.cc (right):
>
> http://codereview.chromium.org/337030/diff/1/9#newcode233
> Line 233: // Closing a window can never make
> has_open_trackable_browsers_ go from false
> This line is now too long.
>
> http://codereview.chromium.org/337030
>

[Paweł Hajdan Jr., 2009-10-26 21:32:39]

Automation and test bits LGTM.

[brettw, 2009-10-26 21:35:39]

LGTM with one comment addition.

http://codereview.chromium.org/337030/diff/5001/6009
File chrome/browser/sessions/session_service.h (right):

http://codereview.chromium.org/337030/diff/5001/6009#newcode309
Line 309: // match |window_id| with our profile.
Could you add a sentence here about what "trackable" means?