A unit test that creates a situation that crashed (bug 130306)

A unit test that creates a situation that crashed (bug 130306)
verifying that ash_shell terminates successfully when a 
window watcher is attached.

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

[Harry McCleave, 2012-08-06 19:57:56]

Could you take a look at this. The test runs fine with ToT, but if the fix in
question is reverted it seg-faults, is this sufficient or is there something to
be done for unit tests to catch and better report this?

[sky, 2012-08-06 21:21:22]

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/ash.gyp
File ash/ash.gyp (right):

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/ash.gyp#newcode393
ash/ash.gyp:393: '../ui/views/views.gyp:views_examples_lib',
Why are you adding this dependency?

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/ash.gyp#newcode400
ash/ash.gyp:400: '../ui/app_list/app_list_item_model.cc',
Why are you explicitly adding this? It should be covered by adding a dependency
on app_list above.

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/shell_unittest.cc
File ash/shell_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/shell_unittest.cc#...
ash/shell_unittest.cc:344: #if defined(OS_LINUX) && defined(OS_CHROMEOS)
Why is this specific to linux and chromeos?

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/shell_unittest.cc#...
ash/shell_unittest.cc:345: class WindowWatcherTest : public test::AshTestBase {
This test is testing WindowWatcher and not so much shell. The test should be
there.

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/shell_unittest.cc#...
ash/shell_unittest.cc:346: public:
nit: spacing is off here, 350 and 353. These should be indented one space.

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/shell_unittest.cc#...
ash/shell_unittest.cc:369: window_watcher_.reset();
Don't you need to do a elegate->SetWatcher(NULL) too?

[Harry McCleave, 2012-08-07 18:17:23]

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/ash.gyp
File ash/ash.gyp (right):

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/ash.gyp#newcode393
ash/ash.gyp:393: '../ui/views/views.gyp:views_examples_lib',
On 2012/08/06 21:21:22, sky wrote:
> Why are you adding this dependency?
as discussed. (window_type_launcher can reference examples)

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/ash.gyp#newcode400
ash/ash.gyp:400: '../ui/app_list/app_list_item_model.cc',
On 2012/08/06 21:21:22, sky wrote:
> Why are you explicitly adding this? It should be covered by adding a
dependency
> on app_list above.

Done.

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/shell_unittest.cc
File ash/shell_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/shell_unittest.cc#...
ash/shell_unittest.cc:344: #if defined(OS_LINUX) && defined(OS_CHROMEOS)
On 2012/08/06 21:21:22, sky wrote:
> Why is this specific to linux and chromeos?

Done.

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/shell_unittest.cc#...
ash/shell_unittest.cc:345: class WindowWatcherTest : public test::AshTestBase {
On 2012/08/06 21:21:22, sky wrote:
> This test is testing WindowWatcher and not so much shell. The test should be
> there.

Done.

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/shell_unittest.cc#...
ash/shell_unittest.cc:346: public:
On 2012/08/06 21:21:22, sky wrote:
> nit: spacing is off here, 350 and 353. These should be indented one space.

Done.

https://chromiumcodereview.appspot.com/10834185/diff/1/ash/shell_unittest.cc#...
ash/shell_unittest.cc:369: window_watcher_.reset();
On 2012/08/06 21:21:22, sky wrote:
> Don't you need to do a elegate->SetWatcher(NULL) too?

Done. Oddly in this case you don't as the desctructor/cleanup does not attempt
to destroy the local reference to the watcher, but I suppose it is good practice
so it has been added.

[sky, 2012-08-07 20:31:06]

https://chromiumcodereview.appspot.com/10834185/diff/2002/ash/shell/window_wa...
File ash/shell/window_watcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834185/diff/2002/ash/shell/window_wa...
ash/shell/window_watcher_unittest.cc:9: #include "ash/shell/window_watcher.h"
Includes order for test should be the same as that of any other cc. So, this
should be the first include, newline, rest of includes.

https://chromiumcodereview.appspot.com/10834185/diff/2002/ash/shell/window_wa...
ash/shell/window_watcher_unittest.cc:14: class WindowWatcherTest : public
test::AshTestBase {
There's no ndeed for this class, instead typedef WindowWatcherTest as
test::AshTestBase and moved the scoped_ptr into your test.

https://chromiumcodereview.appspot.com/10834185/diff/2002/ash/shell_unittest.cc
File ash/shell_unittest.cc (left):

https://chromiumcodereview.appspot.com/10834185/diff/2002/ash/shell_unittest....
ash/shell_unittest.cc:341: 
Keep the newline.

[Harry McCleave, 2012-08-07 20:59:28]

https://chromiumcodereview.appspot.com/10834185/diff/2002/ash/shell/window_wa...
File ash/shell/window_watcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834185/diff/2002/ash/shell/window_wa...
ash/shell/window_watcher_unittest.cc:9: #include "ash/shell/window_watcher.h"
On 2012/08/07 20:31:06, sky wrote:
> Includes order for test should be the same as that of any other cc. So, this
> should be the first include, newline, rest of includes.

Done.

https://chromiumcodereview.appspot.com/10834185/diff/2002/ash/shell/window_wa...
ash/shell/window_watcher_unittest.cc:14: class WindowWatcherTest : public
test::AshTestBase {
On 2012/08/07 20:31:06, sky wrote:
> There's no ndeed for this class, instead typedef WindowWatcherTest as
> test::AshTestBase and moved the scoped_ptr into your test.

Done.

https://chromiumcodereview.appspot.com/10834185/diff/2002/ash/shell_unittest.cc
File ash/shell_unittest.cc (left):

https://chromiumcodereview.appspot.com/10834185/diff/2002/ash/shell_unittest....
ash/shell_unittest.cc:341: 
On 2012/08/07 20:31:06, sky wrote:
> Keep the newline.

Done.

[sky, 2012-08-07 22:31:24]

https://chromiumcodereview.appspot.com/10834185/diff/5007/ash/shell/window_wa...
File ash/shell/window_watcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834185/diff/5007/ash/shell/window_wa...
ash/shell/window_watcher_unittest.cc:5: #include "ash/test/ash_test_base.h"
window_watcher.h should be the first include.

https://chromiumcodereview.appspot.com/10834185/diff/5007/ash/shell/window_wa...
ash/shell/window_watcher_unittest.cc:20: scoped_ptr<ash::shell::WindowWatcher>
window_watcher_;
remove trailing _

[Harry McCleave, 2012-08-09 00:16:13]

https://chromiumcodereview.appspot.com/10834185/diff/5007/ash/shell/window_wa...
File ash/shell/window_watcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834185/diff/5007/ash/shell/window_wa...
ash/shell/window_watcher_unittest.cc:5: #include "ash/test/ash_test_base.h"
On 2012/08/07 22:31:24, sky wrote:
> window_watcher.h should be the first include.

Done.

https://chromiumcodereview.appspot.com/10834185/diff/5007/ash/shell/window_wa...
ash/shell/window_watcher_unittest.cc:20: scoped_ptr<ash::shell::WindowWatcher>
window_watcher_;
On 2012/08/07 22:31:24, sky wrote:
> remove trailing _

Done.

[sky, 2012-08-09 00:41:38]

LGTM

[commit-bot: I haz the power, 2012-08-09 00:42:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/10834185/7002

[commit-bot: I haz the power, 2012-08-09 03:35:26]

Change committed as 150709