Restore App window's size and maximized state from before it was closed

Restore App window's size and maximized state from before it was closed in ChromeOS.

BUG=chrome-os-partner:41173
TEST=Manually tested

Committed: https://crrev.com/359f9268d678726225fff4bbae5a5045d163aef3
Cr-Commit-Position: refs/heads/master@{#338217}

[xdai1, 2015-06-30 17:28:25]

Patchset #1 (id:1) has been deleted

[xdai1, 2015-07-07 00:40:14]

xdai@chromium.org changed reviewers:
+ oshima@chromium.org

[xdai1, 2015-07-07 00:40:15]

Oshima, do we need to fix the restoration behavior for V1 app? Could you help to
comment on the buganizer? If we do need to fix it, could you help to review the
CL please, thanks!

[oshima, 2015-07-07 21:28:04]

On 2015/07/07 00:40:15, xdai1 wrote:
> Oshima, do we need to fix the restoration behavior for V1 app? Could you help
to
> comment on the buganizer? If we do need to fix it, could you help to review
the
> CL please, thanks!

I think we should fix it, especially this works fine for browser window.
Can you instead change
the　src/chrome/browser/ui/extensions/application_launch.cc to use DEFAULT?

SHOW_STATE_NORMAL should be used for the normal window, and DEFAULT should be
more appropriate for
the initial state.

[xdai1, 2015-07-08 00:27:37]

On 2015/07/07 21:28:04, oshima wrote:
> On 2015/07/07 00:40:15, xdai1 wrote:
> > Oshima, do we need to fix the restoration behavior for V1 app? Could you
help
> to
> > comment on the buganizer? If we do need to fix it, could you help to review
> the
> > CL please, thanks!
> 
> I think we should fix it, especially this works fine for browser window.
> Can you instead change
> the　src/chrome/browser/ui/extensions/application_launch.cc to use DEFAULT?
> 
> SHOW_STATE_NORMAL should be used for the normal window, and DEFAULT should be
> more appropriate for
> the initial state.

Done. Please take another look, thanks!

[oshima, 2015-07-08 17:20:36]

can you add a test?

[xdai1, 2015-07-09 18:21:53]

On 2015/07/08 17:20:36, oshima wrote:
> can you add a test?

Addressed. Could you help take another look? Thanks.

[oshima, 2015-07-09 18:59:28]

lgtm with nits

https://codereview.chromium.org/1209033008/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
(right):

https://codereview.chromium.org/1209033008/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1318:
DCHECK(app_browser);
you don't need DCHECK because it'll fail in next line.

https://codereview.chromium.org/1209033008/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1337:
DCHECK(app_browser);
ditto

[xdai1, 2015-07-09 19:24:46]

xdai@chromium.org changed reviewers:
+ stevenjb@chromium.org

[xdai1, 2015-07-09 19:24:46]

oshima@, thanks for your review, I've addressed your comments.

+stevenjb@, could you help to review
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc file?
Thanks!

[stevenjb, 2015-07-09 19:41:09]

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
(right):

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1317: }
ASSERT_TRUE(app_browser);

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1322:
app_browser->window()->Close();
test that the window is closed here.

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1335: }
Put the above code into a helper function in an anonymous namespace since we use
it twice. (Will also make the test a bit more readable).

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1337:
EXPECT_TRUE(ash::wm::GetWindowState(window)->IsMaximized());
For completeness we should un-maximize the window, close it, open it, and ensure
that it is not maximized.

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/application_launch.cc:163: // LAUNCH_TYPE_WINDOW
launches in a normal app window.
Update the comment.

[stevenjb, 2015-07-09 19:42:24]

https://codereview.chromium.org/1209033008/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
(right):

https://codereview.chromium.org/1209033008/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1318:
DCHECK(app_browser);
On 2015/07/09 18:59:28, oshima wrote:
> you don't need DCHECK because it'll fail in next line.

In general, never use DCHECK in tests, because it is hard to debug crashing
tests. Instead, use ASSERT_TRUE(app_browser) to avoid a crash.

[oshima, 2015-07-09 20:14:05]

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
(right):

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1317: }
On 2015/07/09 19:41:09, stevenjb wrote:
> ASSERT_TRUE(app_browser);

ASSERT_TRUE is for the precondition that will otherwise execute EXPECT_XXX. In
this case, it will simply crash, so no need for ASSERT_TRUE

[stevenjb, 2015-07-09 21:13:51]

On 2015/07/09 20:14:05, oshima wrote:
>
https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
> File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
> chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1317:
}
> On 2015/07/09 19:41:09, stevenjb wrote:
> > ASSERT_TRUE(app_browser);
> 
> ASSERT_TRUE is for the precondition that will otherwise execute EXPECT_XXX. In
> this case, it will simply crash, so no need for ASSERT_TRUE

Oshima and I discussed this offline, and it really comes down to a style choice.
I prefer to add an ASSERT anywhere an assumption about another system (e.g the
browser list) could cause a crash. The advantage to an ASSERT is that other
tests in the suite will run, whereas a crash will stop execution of any other
tests in the same batch. That said, at a certain point adding lots of ASSERTS
can get cumbersome.

IMHO, I think that since the code looking up the browser by app_name is specific
to these tests, we should have an ASSERT there. Whereas,
LoadAndLaunchExtension() is presumably tested elsewhere.

That said, I am fine with or without the asserts.

[oshima, 2015-07-09 21:14:57]

On 2015/07/09 21:13:51, stevenjb wrote:
> On 2015/07/09 20:14:05, oshima wrote:
> >
>
https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
> > File
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
> >
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1317:
> }
> > On 2015/07/09 19:41:09, stevenjb wrote:
> > > ASSERT_TRUE(app_browser);
> > 
> > ASSERT_TRUE is for the precondition that will otherwise execute EXPECT_XXX.
In
> > this case, it will simply crash, so no need for ASSERT_TRUE
> 
> Oshima and I discussed this offline, and it really comes down to a style
choice.
> I prefer to add an ASSERT anywhere an assumption about another system (e.g the
> browser list) could cause a crash. The advantage to an ASSERT is that other
> tests in the suite will run, whereas a crash will stop execution of any other
> tests in the same batch. That said, at a certain point adding lots of ASSERTS
> can get cumbersome.
> 
> IMHO, I think that since the code looking up the browser by app_name is
specific
> to these tests, we should have an ASSERT there. Whereas,
> LoadAndLaunchExtension() is presumably tested elsewhere.
> 
> That said, I am fine with or without the asserts.

I'm fine with either way too.

[xdai1, 2015-07-09 22:46:45]

xdai@chromium.org changed reviewers:
+ benwells@chromium.org

[xdai1, 2015-07-09 22:46:46]

Thanks for the review and explanation about the differences between ASSERT_TURE
and EXPECT_TRUE. It seems either way is fine for this test. 

Seems I still miss an owner review... +benwells for
chrome/browser/ui/extensions/application_launch.cc file. Thanks!

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
(right):

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1322:
app_browser->window()->Close();
On 2015/07/09 19:41:09, stevenjb wrote:
> test that the window is closed here.

Done.

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1335: }
On 2015/07/09 19:41:09, stevenjb wrote:
> Put the above code into a helper function in an anonymous namespace since we
use
> it twice. (Will also make the test a bit more readable).
> 

Done.

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1337:
EXPECT_TRUE(ash::wm::GetWindowState(window)->IsMaximized());
On 2015/07/09 19:41:09, stevenjb wrote:
> For completeness we should un-maximize the window, close it, open it, and
ensure
> that it is not maximized.

Done.

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/1209033008/diff/80001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/application_launch.cc:163: // LAUNCH_TYPE_WINDOW
launches in a normal app window.
On 2015/07/09 19:41:09, stevenjb wrote:
> Update the comment.

Done.

[stevenjb, 2015-07-09 23:49:47]

Thanks! lgtm

[benwells, 2015-07-10 00:20:16]

lgtm

[xdai1, 2015-07-10 00:43:16]

The CQ bit was checked by xdai@chromium.org

[xdai1, 2015-07-10 00:43:16]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1209033008/#ps100001
(title: "Address stevenjb@'s comments.")

[commit-bot: I haz the power, 2015-07-10 00:43:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1209033008/100001

[commit-bot: I haz the power, 2015-07-10 02:40:27]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-07-10 02:41:41]

Patchset 5 (id:??) landed as
https://crrev.com/359f9268d678726225fff4bbae5a5045d163aef3
Cr-Commit-Position: refs/heads/master@{#338217}