cros: Disable the auto management logic for v1app browser window

cros: Disable the auto management logic for v1app browser window

Changes:
(1) For "open as window" browser window (v1app window), we disable auto management logic. So that they can restore the saved window bounds, not interrupted by auto placement such as centering window in work area.
(2) Simplify BrowserRemembersDockedState to test only state not bounds for reasons (a) bounds logic for app or tabbed browser window is tested in added test coverage, (b) this test will soon be removed in M-58 because of deprecation of docked window on Chrome OS.

BUG=687385
TEST=emulator tests show that v1app bounds restoring works just like v2 apps; also test coverage is added.

Review-Url: https://codereview.chromium.org/2690563005
Cr-Commit-Position: refs/heads/master@{#450216}
Committed: https://chromium.googlesource.com/chromium/src/+/2fc42ea7b1264dc128a1e454d92eebc25e1076c5

[Qiang(Joe) Xu, 2017-02-11 00:51:27]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-11 00:51:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-11 01:31:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-11 01:31:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Qiang(Joe) Xu, 2017-02-11 06:41:46]

Description was changed from

==========
cros: Disable the auto management logic for v1app browser window

BUG=687385
TEST=emulator tests
==========

to

==========
cros: Disable the auto management logic for v1app browser window

BUG=687385
TEST=emulator tests
==========

[Qiang(Joe) Xu, 2017-02-11 06:41:46]

warx@chromium.org changed reviewers:
+ oshima@chromium.org, sky@chromium.org

[Qiang(Joe) Xu, 2017-02-11 06:46:54]

Description was changed from

==========
cros: Disable the auto management logic for v1app browser window

BUG=687385
TEST=emulator tests
==========

to

==========
cros: Disable the auto management logic for v1app browser window

Changes:
(1) For "open as window" browser window (v1app window), we disable auto
management logic. So that they can restore the saved window bounds, not
interrupted by auto placement such as centering window in work area.
(2) Simplify BrowserRemembersDockedState to test only state not bounds for
reasons (a) bounds logic for app or tabbed browser window is tested in added
test coverage, (b) this test will soon be removed in M-58 because of deprecation
of docked window on Chrome OS.

BUG=687385
TEST=emulator tests show that v1app bounds restoring works just like v2 apps;
also test coverage is added.
==========

[Qiang(Joe) Xu, 2017-02-11 06:47:33]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-11 06:47:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Qiang(Joe) Xu, 2017-02-11 06:48:03]

Hi all, ptal, thanks!

[commit-bot: I haz the power, 2017-02-11 07:17:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-11 07:17:07]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Qiang(Joe) Xu, 2017-02-11 18:02:28]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-11 18:02:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-11 18:51:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-11 18:51:55]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-13 17:43:03]

https://codereview.chromium.org/2690563005/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view_interactive_uitest.cc (right):

https://codereview.chromium.org/2690563005/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_interactive_uitest.cc:66:
IN_PROC_BROWSER_TEST_P(BrowserViewTestParam,
Why does this need to be in interactive tests?

https://codereview.chromium.org/2690563005/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_interactive_uitest.cc:80:
ASSERT_TRUE(browser);
We don't assert that new returns non-null. If new fails chrome crashes.

https://codereview.chromium.org/2690563005/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_interactive_uitest.cc:102:
ASSERT_TRUE(browser);
Same comment about checking return value from new.

[Qiang(Joe) Xu, 2017-02-13 22:39:35]

Hi all, ptal, thanks!

https://codereview.chromium.org/2690563005/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view_interactive_uitest.cc (right):

https://codereview.chromium.org/2690563005/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_interactive_uitest.cc:66:
IN_PROC_BROWSER_TEST_P(BrowserViewTestParam,
On 2017/02/13 17:43:03, sky wrote:
> Why does this need to be in interactive tests?

It doesn't need. I moved it to browser_browsertest.cc.

https://codereview.chromium.org/2690563005/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_interactive_uitest.cc:80:
ASSERT_TRUE(browser);
On 2017/02/13 17:43:03, sky wrote:
> We don't assert that new returns non-null. If new fails chrome crashes.

Done.

https://codereview.chromium.org/2690563005/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_interactive_uitest.cc:102:
ASSERT_TRUE(browser);
On 2017/02/13 17:43:03, sky wrote:
> Same comment about checking return value from new.

Done.

[Qiang(Joe) Xu, 2017-02-13 22:40:36]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 22:41:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-02-13 23:35:52]

https://codereview.chromium.org/2690563005/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/2690563005/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2963: // Test that in Chrome OS, for
app browser (v1app) windows, the auto management
We try to avoid platform specific code in c/b/ui. Platform specific code goes in
the appropriate subdirectories. So, how about putting this in
browser_frame_ash_browsertest?

[Qiang(Joe) Xu, 2017-02-13 23:59:22]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[Qiang(Joe) Xu, 2017-02-14 00:00:21]

Hi all, ptal, thanks!

https://codereview.chromium.org/2690563005/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/2690563005/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2963: // Test that in Chrome OS, for
app browser (v1app) windows, the auto management
On 2017/02/13 23:35:51, sky wrote:
> We try to avoid platform specific code in c/b/ui. Platform specific code goes
in
> the appropriate subdirectories. So, how about putting this in
> browser_frame_ash_browsertest?

sure, updated.

[commit-bot: I haz the power, 2017-02-14 00:00:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-02-14 00:38:27]

LGTM - but wait for Oshima.

https://codereview.chromium.org/2690563005/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame_ash_browsertest.cc (right):

https://codereview.chromium.org/2690563005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame_ash_browsertest.cc:20: };
private: DISALLOW...

[oshima, 2017-02-14 01:09:51]

lgtm with nits

https://codereview.chromium.org/2690563005/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame_ash_browsertest.cc (right):

https://codereview.chromium.org/2690563005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame_ash_browsertest.cc:19: bool
TestApp() { return GetParam(); }
nit: CreateV1App() would be better

https://codereview.chromium.org/2690563005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame_ash_browsertest.cc:54:
EXPECT_EQ(original_bounds.ToString(), window->bounds().ToString());
nit: no need to convert to string. (Old tests use this and they need to be
updated)

[Qiang(Joe) Xu, 2017-02-14 01:19:52]

done, thanks for review

https://codereview.chromium.org/2690563005/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame_ash_browsertest.cc (right):

https://codereview.chromium.org/2690563005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame_ash_browsertest.cc:19: bool
TestApp() { return GetParam(); }
On 2017/02/14 01:09:51, oshima wrote:
> nit: CreateV1App() would be better

Done.

https://codereview.chromium.org/2690563005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame_ash_browsertest.cc:20: };
On 2017/02/14 00:38:27, sky wrote:
> private: DISALLOW...

Done.

https://codereview.chromium.org/2690563005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame_ash_browsertest.cc:54:
EXPECT_EQ(original_bounds.ToString(), window->bounds().ToString());
On 2017/02/14 01:09:51, oshima wrote:
> nit: no need to convert to string. (Old tests use this and they need to be
> updated)

Done.

[Qiang(Joe) Xu, 2017-02-14 01:20:00]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-02-14 01:20:01]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2690563005/#ps100001
(title: "nits")

[commit-bot: I haz the power, 2017-02-14 01:21:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 03:09:09]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487035200956490,
"parent_rev": "f1ebc3e6a38846d528181d052d496ba1524a1982", "commit_rev":
"2fc42ea7b1264dc128a1e454d92eebc25e1076c5"}

[commit-bot: I haz the power, 2017-02-14 03:10:05]

Description was changed from

==========
cros: Disable the auto management logic for v1app browser window

Changes:
(1) For "open as window" browser window (v1app window), we disable auto
management logic. So that they can restore the saved window bounds, not
interrupted by auto placement such as centering window in work area.
(2) Simplify BrowserRemembersDockedState to test only state not bounds for
reasons (a) bounds logic for app or tabbed browser window is tested in added
test coverage, (b) this test will soon be removed in M-58 because of deprecation
of docked window on Chrome OS.

BUG=687385
TEST=emulator tests show that v1app bounds restoring works just like v2 apps;
also test coverage is added.
==========

to

==========
cros: Disable the auto management logic for v1app browser window

Changes:
(1) For "open as window" browser window (v1app window), we disable auto
management logic. So that they can restore the saved window bounds, not
interrupted by auto placement such as centering window in work area.
(2) Simplify BrowserRemembersDockedState to test only state not bounds for
reasons (a) bounds logic for app or tabbed browser window is tested in added
test coverage, (b) this test will soon be removed in M-58 because of deprecation
of docked window on Chrome OS.

BUG=687385
TEST=emulator tests show that v1app bounds restoring works just like v2 apps;
also test coverage is added.

Review-Url: https://codereview.chromium.org/2690563005
Cr-Commit-Position: refs/heads/master@{#450216}
Committed:
https://chromium.googlesource.com/chromium/src/+/2fc42ea7b1264dc128a1e454d92e...
==========

[commit-bot: I haz the power, 2017-02-14 03:10:06]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/2fc42ea7b1264dc128a1e454d92e...