mash: Test ChromeLauncherControllerImpl more like production use.

mash: Test ChromeLauncherControllerImpl more like production use.

Adjust the ownership of the instances used by most unit test fixtures.
The test Shell owns CLC as the ShelfDelegate, just like in production.
Fix up test helper functions and remove ProxyShelfDelegate.

Make BrowserWithTestWindowTest::TearDown more like production shutdown.
(first tear down the browser instance, then the Shell, then the profile)
Remove DestroyBrowserAndProfile, inline/recreate similar behavior.

Add a workaround for DefaultApps test teardown crash (crbug.com/709297).

BUG=557406
TEST=Automated tests still pass; more closely match production.
R=jamescook@chromium.org,sky@chromium.org

Review-Url: https://codereview.chromium.org/2804913002
Cr-Commit-Position: refs/heads/master@{#462880}
Committed: https://chromium.googlesource.com/chromium/src/+/e148e7ab0ffb1b5facad8c6c18a34af43f3bc1b3

[msw, 2017-04-06 00:52:22]

Description was changed from

==========
Make ChromeLauncherControllerImpl lifetime in tests better match production.

BUG=
==========

to

==========
mash: Test ChromeLauncherControllerImpl more like production use.

BUG=
==========

[msw, 2017-04-06 19:51:09]

Patchset #2 (id:20001) has been deleted

[msw, 2017-04-06 19:51:15]

Patchset #1 (id:1) has been deleted

[msw, 2017-04-06 23:24:00]

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

[commit-bot: I haz the power, 2017-04-06 23:24:40]

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

[msw, 2017-04-07 00:25:44]

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

[commit-bot: I haz the power, 2017-04-07 00:26:22]

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-04-07 00:29:17]

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

[commit-bot: I haz the power, 2017-04-07 00:29:18]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[msw, 2017-04-07 00:38:13]

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

[commit-bot: I haz the power, 2017-04-07 00:38:51]

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-04-07 02:15:42]

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

[commit-bot: I haz the power, 2017-04-07 02:15:44]

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

[msw, 2017-04-07 09:32:47]

Description was changed from

==========
mash: Test ChromeLauncherControllerImpl more like production use.

BUG=
==========

to

==========
mash: Test ChromeLauncherControllerImpl more like production use.

Adjust the ownership of the instances used by most unit test fixtures.
The test Shell owns CLC as the ShelfDelegate, just like in production.
Fix up test helper functions and remove ProxyShelfDelegate.

Make BrowserWithTestWindowTest::TearDown more like production shutdown.
(first tear down the browser instance, then the Shell, then the profile)
Remove DestroyBrowserAndProfile, inline/recreate similar behavior.

BUG=557406
TEST=Automated tests still pass; more closely match production.
R=jamescook@chromium.org
==========

[msw, 2017-04-07 09:32:47]

msw@chromium.org changed reviewers:
+ jamescook@chromium.org

[msw, 2017-04-07 09:38:02]

Patchset #5 (id:120001) has been deleted

[msw, 2017-04-07 09:39:31]

Description was changed from

==========
mash: Test ChromeLauncherControllerImpl more like production use.

Adjust the ownership of the instances used by most unit test fixtures.
The test Shell owns CLC as the ShelfDelegate, just like in production.
Fix up test helper functions and remove ProxyShelfDelegate.

Make BrowserWithTestWindowTest::TearDown more like production shutdown.
(first tear down the browser instance, then the Shell, then the profile)
Remove DestroyBrowserAndProfile, inline/recreate similar behavior.

BUG=557406
TEST=Automated tests still pass; more closely match production.
R=jamescook@chromium.org
==========

to

==========
mash: Test ChromeLauncherControllerImpl more like production use.

Adjust the ownership of the instances used by most unit test fixtures.
The test Shell owns CLC as the ShelfDelegate, just like in production.
Fix up test helper functions and remove ProxyShelfDelegate.

Make BrowserWithTestWindowTest::TearDown more like production shutdown.
(first tear down the browser instance, then the Shell, then the profile)
Remove DestroyBrowserAndProfile, inline/recreate similar behavior.

Add a workaround for DefaultApps test teardown crash (crbug.com/709297).

BUG=557406
TEST=Automated tests still pass; more closely match production.
R=jamescook@chromium.org
==========

[msw, 2017-04-07 09:40:29]

Description was changed from

==========
mash: Test ChromeLauncherControllerImpl more like production use.

Adjust the ownership of the instances used by most unit test fixtures.
The test Shell owns CLC as the ShelfDelegate, just like in production.
Fix up test helper functions and remove ProxyShelfDelegate.

Make BrowserWithTestWindowTest::TearDown more like production shutdown.
(first tear down the browser instance, then the Shell, then the profile)
Remove DestroyBrowserAndProfile, inline/recreate similar behavior.

Add a workaround for DefaultApps test teardown crash (crbug.com/709297).

BUG=557406
TEST=Automated tests still pass; more closely match production.
R=jamescook@chromium.org
==========

to

==========
mash: Test ChromeLauncherControllerImpl more like production use.

Adjust the ownership of the instances used by most unit test fixtures.
The test Shell owns CLC as the ShelfDelegate, just like in production.
Fix up test helper functions and remove ProxyShelfDelegate.

Make BrowserWithTestWindowTest::TearDown more like production shutdown.
(first tear down the browser instance, then the Shell, then the profile)
Remove DestroyBrowserAndProfile, inline/recreate similar behavior.

Add a workaround for DefaultApps test teardown crash (crbug.com/709297).

BUG=557406
TEST=Automated tests still pass; more closely match production.
R=jamescook@chromium.org,sky@chromium.org
==========

[msw, 2017-04-07 09:40:30]

msw@chromium.org changed reviewers:
+ sky@chromium.org

[msw, 2017-04-07 09:40:51]

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

[commit-bot: I haz the power, 2017-04-07 09:41:07]

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

[msw, 2017-04-07 09:43:54]

Hey James and Scott, please take a look; thanks!
James: chrome_launcher_controller_impl_unittest.cc
Scott: the rest ([clc]_impl_unittest.cc too, if interested)

[commit-bot: I haz the power, 2017-04-07 10:10:26]

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

[commit-bot: I haz the power, 2017-04-07 10:10:27]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-04-07 14:15:40]

LGTM++

https://codereview.chromium.org/2804913002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(left):

https://codereview.chromium.org/2804913002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:320:
};
Glad to see this going away.

https://codereview.chromium.org/2804913002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2804913002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:509:
// Returns a pointer to the initialized controller, which is owned by Shell.
nit: Not relevant anymore

https://codereview.chromium.org/2804913002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1315:
launcher_controller_ = nullptr;
optional: utility method ResetLauncherController() for these two lines?

[sky, 2017-04-07 14:51:39]

LGTM

https://codereview.chromium.org/2804913002/diff/140001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/2804913002/diff/140001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.cc:91: if (browser_.get())
if (browser_)?

[msw, 2017-04-07 15:38:41]

https://codereview.chromium.org/2804913002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(left):

https://codereview.chromium.org/2804913002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:320:
};
On 2017/04/07 14:15:40, James Cook wrote:
> Glad to see this going away.

Acknowledged.

https://codereview.chromium.org/2804913002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2804913002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:509:
// Returns a pointer to the initialized controller, which is owned by Shell.
On 2017/04/07 14:15:40, James Cook wrote:
> nit: Not relevant anymore

Done.

https://codereview.chromium.org/2804913002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1315:
launcher_controller_ = nullptr;
On 2017/04/07 14:15:40, James Cook wrote:
> optional: utility method ResetLauncherController() for these two lines?

Done.

https://codereview.chromium.org/2804913002/diff/140001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/2804913002/diff/140001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.cc:91: if (browser_.get())
On 2017/04/07 14:51:39, sky wrote:
> if (browser_)?

Done.

[msw, 2017-04-07 15:38:46]

The CQ bit was checked by msw@chromium.org

[msw, 2017-04-07 15:38:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2804913002/#ps160001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-04-07 15:39:14]

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-04-07 16:11:37]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491579526834860,
"parent_rev": "39a85508e33a3bac8b8a7ffbdd1abfb5c569a547", "commit_rev":
"e148e7ab0ffb1b5facad8c6c18a34af43f3bc1b3"}

[commit-bot: I haz the power, 2017-04-07 16:12:29]

Description was changed from

==========
mash: Test ChromeLauncherControllerImpl more like production use.

Adjust the ownership of the instances used by most unit test fixtures.
The test Shell owns CLC as the ShelfDelegate, just like in production.
Fix up test helper functions and remove ProxyShelfDelegate.

Make BrowserWithTestWindowTest::TearDown more like production shutdown.
(first tear down the browser instance, then the Shell, then the profile)
Remove DestroyBrowserAndProfile, inline/recreate similar behavior.

Add a workaround for DefaultApps test teardown crash (crbug.com/709297).

BUG=557406
TEST=Automated tests still pass; more closely match production.
R=jamescook@chromium.org,sky@chromium.org
==========

to

==========
mash: Test ChromeLauncherControllerImpl more like production use.

Adjust the ownership of the instances used by most unit test fixtures.
The test Shell owns CLC as the ShelfDelegate, just like in production.
Fix up test helper functions and remove ProxyShelfDelegate.

Make BrowserWithTestWindowTest::TearDown more like production shutdown.
(first tear down the browser instance, then the Shell, then the profile)
Remove DestroyBrowserAndProfile, inline/recreate similar behavior.

Add a workaround for DefaultApps test teardown crash (crbug.com/709297).

BUG=557406
TEST=Automated tests still pass; more closely match production.
R=jamescook@chromium.org,sky@chromium.org

Review-Url: https://codereview.chromium.org/2804913002
Cr-Commit-Position: refs/heads/master@{#462880}
Committed:
https://chromium.googlesource.com/chromium/src/+/e148e7ab0ffb1b5facad8c6c18a3...
==========

[commit-bot: I haz the power, 2017-04-07 16:12:30]

Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/e148e7ab0ffb1b5facad8c6c18a3...