Add unit tests for screen capture and share notification UI.

Add unit tests for screen capture and share notification UI.

BUG=246277
TEST=unit tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204974

[Tim Song, 2013-06-07 02:08:23]

Hi James. These are the unit tests for the screen capture UI patch that just
landed. Please take a look.

[James Cook, 2013-06-07 14:16:37]

Overall approach looks fine, and coverage looks good.  Some comments.

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
File ash/system/chromeos/screen_security/screen_tray_item_unittest.cc (right):

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:8: 
Either put the primary test class header include on it's own line at the top:

#include "a/s/c/s/screen_tray_item.h"

#include "foo.h"
#include "bar.h"

Or just list all the headers alphabetically in one big block.  (I prefer the
former style myself.)

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:20: const char
kTestScreenShareHelperName[] = "Ash Test";
nit: Generally we use different strings, integers, etc. in test data to make it
easier to tell failures apart, for example in the debugger.

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:31: gfx::Point
click_location = gfx::Point(view->width()/2, view->height()/2);
Spaces around /

Also, click_location should be click_location_in_local (a suffix to indicate the
coordinate system of the point, as opposed to _in_screen or _in_window)

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:40: explicit
ScreenTrayItemTest()
no explicit

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:45: uint
stop_callback_hit_count() { return stop_callback_hit_count_; }
int would be fine here.  Also, const here (but not above)

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:61:
ScreenTrayItem* tray_item_;
Make this private and use a setter.  We generally don't use protected member
variables.  This makes it a little easier to find access to the data when we
have to change the code later.

Also, comment on memory ownership (I believe it's owned by the parent view in
this case).

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:93: 
nit: one blank line

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:98:
EXPECT_EQ(test->stop_callback_hit_count(), 0u);
expected value goes first:

EXPECT_EQ(0, stop_callback_hit_count())

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:111: void
TestNotificationStartAndStop(ScreenTrayItemTest* test,
Good: nice descriptive function name

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:112: const
base::Closure& start_func,
no abbrevations: start_function, stop_function

[Tim Song, 2013-06-07 18:14:34]

Thanks for taking a look, James.

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
File ash/system/chromeos/screen_security/screen_tray_item_unittest.cc (right):

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:8: 
On 2013/06/07 14:16:37, James Cook (Chromium) wrote:
> Either put the primary test class header include on it's own line at the top:
> 
> #include "a/s/c/s/screen_tray_item.h"
> 
> #include "foo.h"
> #include "bar.h"
> 
> Or just list all the headers alphabetically in one big block.  (I prefer the
> former style myself.)

Done.

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:20: const char
kTestScreenShareHelperName[] = "Ash Test";
On 2013/06/07 14:16:37, James Cook (Chromium) wrote:
> nit: Generally we use different strings, integers, etc. in test data to make
it
> easier to tell failures apart, for example in the debugger.

Done. I just realized that it would be a good idea to use some unicode strings
here to potentially test for localization issues.

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:31: gfx::Point
click_location = gfx::Point(view->width()/2, view->height()/2);
On 2013/06/07 14:16:37, James Cook (Chromium) wrote:
> Spaces around /
> 
> Also, click_location should be click_location_in_local (a suffix to indicate
the
> coordinate system of the point, as opposed to _in_screen or _in_window)

Done.

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:40: explicit
ScreenTrayItemTest()
On 2013/06/07 14:16:37, James Cook (Chromium) wrote:
> no explicit

Done. I should really keep an eye out when copy pasting code...

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:45: uint
stop_callback_hit_count() { return stop_callback_hit_count_; }
On 2013/06/07 14:16:37, James Cook (Chromium) wrote:
> int would be fine here.  Also, const here (but not above)

Done.

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:61:
ScreenTrayItem* tray_item_;
On 2013/06/07 14:16:37, James Cook (Chromium) wrote:
> Make this private and use a setter.  We generally don't use protected member
> variables.  This makes it a little easier to find access to the data when we
> have to change the code later.
> 
> Also, comment on memory ownership (I believe it's owned by the parent view in
> this case).

Done. I added the comments about memory management in the subclasses, as we
don't actually initialize tray_item_ here.

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:93: 
On 2013/06/07 14:16:37, James Cook (Chromium) wrote:
> nit: one blank line

Done.

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:98:
EXPECT_EQ(test->stop_callback_hit_count(), 0u);
On 2013/06/07 14:16:37, James Cook (Chromium) wrote:
> expected value goes first:
> 
> EXPECT_EQ(0, stop_callback_hit_count())

Done.

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:111: void
TestNotificationStartAndStop(ScreenTrayItemTest* test,
On 2013/06/07 14:16:37, James Cook (Chromium) wrote:
> Good: nice descriptive function name

Thanks!

https://codereview.chromium.org/16472006/diff/1/ash/system/chromeos/screen_se...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:112: const
base::Closure& start_func,
On 2013/06/07 14:16:37, James Cook (Chromium) wrote:
> no abbrevations: start_function, stop_function

Done.

[James Cook, 2013-06-07 18:45:55]

LGTM assuming last comment addressed

https://codereview.chromium.org/16472006/diff/5001/ash/system/chromeos/screen...
File ash/system/chromeos/screen_security/screen_tray_item_unittest.cc (right):

https://codereview.chromium.org/16472006/diff/5001/ash/system/chromeos/screen...
ash/system/chromeos/screen_security/screen_tray_item_unittest.cc:20: const char
kTestScreenCaptureAppName[] = "Screen Capture Unit Test ಠ_ಠ";
I don't think you should put raw Unicode characters in source files. Either do
them with escape sequences or just stick with ASCII for the test.

[Tim Song, 2013-06-07 19:42:47]

I escaped the unicode characters to use the raw UTF-8 bytes of the characters.

[commit-bot: I haz the power, 2013-06-07 19:44:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/16472006/6007

[commit-bot: I haz the power, 2013-06-07 23:26:53]

Change committed as 204974

[James Cook, 2013-06-10 23:28:18]

On 2013/06/07 23:26:53, I haz the power (commit-bot) wrote:
> Change committed as 204974

The comment says the ScreenTrayItem is owned by its parent view, but in the test
the ScreenTrayItem isn't attached to a parent as far as I can tell.  It should
probably be in a scoped_ptr<>.

[Tim Song, 2013-06-21 18:13:41]

It turns out the memory leak was because I forgot to add the TrayItems created
in the tests to the SystemTray. I'm surprised that the tests pass even though
the SystemTray did not even have the tray items.

[Tim Song, 2013-06-21 19:44:20]

I also added another test that will fail if the tray items were not added to the
system tray. I had some difficulties with testability, in that we can't
explicitly check what tray items have been added, and animations introduce time
dependent states. Would it be a good idea to do some refactoring for
testability?

[James Cook, 2013-06-21 20:48:19]

On 2013/06/21 19:44:20, Tim Song wrote:
> I also added another test that will fail if the tray items were not added to
the
> system tray. I had some difficulties with testability, in that we can't
> explicitly check what tray items have been added, and animations introduce
time
> dependent states. Would it be a good idea to do some refactoring for
> testability?

This patch was reverted, right?  You might want to upload to a new patch to make
that more clear.

There are usually ways to disable animations for test, look for
DisableAnimationsForTest() in other tests or
ScopedAnimationDurationScaleMode::ZERO_DURATION.