Add webusb notification UI unit tests

Add webusb notification UI unit tests

This patch added webusb notification UI unit tests code.

BUG=492204
Committed: https://crrev.com/a764fd3e6431e3932b20d1c364a8d92a55120167
Cr-Commit-Position: refs/heads/master@{#347866}

[juncai, 2015-09-04 21:02:19]

juncai@chromium.org changed reviewers:
+ caitkp@chromium.org, reillyg@chromium.org, thestig@chromium.org

[juncai, 2015-09-04 21:02:20]

thestig@chromium.org: Please review changes in 
chrome/chrome_tests_unit.gypi

caitkp@chromium.org: Please review changes in 
components/BUILD.gn
components/components_tests.gyp

reillyg@chromium.org: Please review changes in 
chrome/browser/chrome_webusb_browser_client.h
chrome/browser/chrome_webusb_browser_client_unittest.cc
components/webusb/BUILD.gn
components/webusb/webusb_browser_client.h
components/webusb/webusb_detector_unittest.cc
device/usb/mock_usb_device.h
device/usb/mock_usb_device.cc

[Lei Zhang, 2015-09-04 21:10:18]

chrome/ build files lgtm

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
File chrome/browser/chrome_webusb_browser_client_unittest.cc (right):

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:35: void
set_usb_service(device::UsbService* service) { usb_service_ = service; }
Just roll this into the ctor?

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:92:
EXPECT_TRUE(message_center != nullptr);
You can omit the != nullptr, ditto below.

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:100:
base::UTF8ToUTF16(product_string));
nit: you can use ASCIIToUTF16()

[Reilly Grant (use Gerrit), 2015-09-04 21:23:34]

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
File chrome/browser/chrome_webusb_browser_client_unittest.cc (right):

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:57:
usb_service_.reset(new device::MockUsbService());
The pattern in other tests that need a MockUsbService is to make it a member of
the FakeDeviceClient (usually called TestDeviceClient). See
https://codereview.chromium.org/1314273002/diff/100001/extensions/browser/api....

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:92:
EXPECT_TRUE(message_center != nullptr);
ASSERT_TRUE so that later code doesn't crash dereferencing the nullptr.

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:99:
l10n_util::GetStringFUTF16(IDS_WEBUSB_DEVICE_DETECTED_NOTIFICATION_TITLE,
Use the literal English strings here (with base::ASCIIToUTF16 to convert). This
makes the test fail if the string is changed accidentally.

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:109:
EXPECT_TRUE(notification_delegate != nullptr);
EXPECT_TRUE(notification->delegate() != nullptr);

https://codereview.chromium.org/1326003006/diff/40001/components/webusb/webus...
File components/webusb/webusb_detector_unittest.cc (right):

https://codereview.chromium.org/1326003006/diff/40001/components/webusb/webus...
components/webusb/webusb_detector_unittest.cc:36: device::UsbService*
usb_service_;
Same suggestion to use MockUsbService directly here. Makes setup simpler.

https://codereview.chromium.org/1326003006/diff/40001/components/webusb/webus...
components/webusb/webusb_detector_unittest.cc:209: InvokeUsbAdded();
It's weird for this test to have a UsbService but not have a UsbService. How
about not initializing it at all in this case?

https://codereview.chromium.org/1326003006/diff/40001/components/webusb/webus...
components/webusb/webusb_detector_unittest.cc:297: InvokeUsbRemoved();
This is kind of a bug in the mocks... it shouldn't be possible for a device to
be removed multiple times.

[juncai, 2015-09-08 21:14:01]

reillyg@chromium.org: Please review changes.

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
File chrome/browser/chrome_webusb_browser_client_unittest.cc (right):

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:35: void
set_usb_service(device::UsbService* service) { usb_service_ = service; }
On 2015/09/04 21:10:18, Lei Zhang wrote:
> Just roll this into the ctor?

Done.

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:57:
usb_service_.reset(new device::MockUsbService());
On 2015/09/04 21:23:34, Reilly Grant wrote:
> The pattern in other tests that need a MockUsbService is to make it a member
of
> the FakeDeviceClient (usually called TestDeviceClient). See
>
https://codereview.chromium.org/1314273002/diff/100001/extensions/browser/api....

Done.

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:92:
EXPECT_TRUE(message_center != nullptr);
On 2015/09/04 21:10:18, Lei Zhang wrote:
> You can omit the != nullptr, ditto below.

Done.

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:92:
EXPECT_TRUE(message_center != nullptr);
On 2015/09/04 21:23:34, Reilly Grant wrote:
> ASSERT_TRUE so that later code doesn't crash dereferencing the nullptr.

Done.

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:99:
l10n_util::GetStringFUTF16(IDS_WEBUSB_DEVICE_DETECTED_NOTIFICATION_TITLE,
On 2015/09/04 21:23:34, Reilly Grant wrote:
> Use the literal English strings here (with base::ASCIIToUTF16 to convert).
This
> makes the test fail if the string is changed accidentally.

Done.

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:100:
base::UTF8ToUTF16(product_string));
On 2015/09/04 21:10:18, Lei Zhang wrote:
> nit: you can use ASCIIToUTF16()

Done.

https://codereview.chromium.org/1326003006/diff/40001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:109:
EXPECT_TRUE(notification_delegate != nullptr);
On 2015/09/04 21:23:34, Reilly Grant wrote:
> EXPECT_TRUE(notification->delegate() != nullptr);

Done.

https://codereview.chromium.org/1326003006/diff/40001/components/webusb/webus...
File components/webusb/webusb_detector_unittest.cc (right):

https://codereview.chromium.org/1326003006/diff/40001/components/webusb/webus...
components/webusb/webusb_detector_unittest.cc:36: device::UsbService*
usb_service_;
On 2015/09/04 21:23:34, Reilly Grant wrote:
> Same suggestion to use MockUsbService directly here. Makes setup simpler.

Done.

https://codereview.chromium.org/1326003006/diff/40001/components/webusb/webus...
components/webusb/webusb_detector_unittest.cc:209: InvokeUsbAdded();
On 2015/09/04 21:23:34, Reilly Grant wrote:
> It's weird for this test to have a UsbService but not have a UsbService. How
> about not initializing it at all in this case?

Done.

https://codereview.chromium.org/1326003006/diff/40001/components/webusb/webus...
components/webusb/webusb_detector_unittest.cc:297: InvokeUsbRemoved();
On 2015/09/04 21:23:34, Reilly Grant wrote:
> This is kind of a bug in the mocks... it shouldn't be possible for a device to
> be removed multiple times.

Done.

[Reilly Grant (use Gerrit), 2015-09-08 21:45:37]

lgtm with one comment.

https://codereview.chromium.org/1326003006/diff/80001/chrome/browser/chrome_w...
File chrome/browser/chrome_webusb_browser_client_unittest.cc (right):

https://codereview.chromium.org/1326003006/diff/80001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:90:
l10n_util::GetStringUTF16(IDS_WEBUSB_DEVICE_DETECTED_NOTIFICATION);
Please hard-code the expected string instead of querying it through the l10n
system. This helps guard against bugs creeping into UI strings without anyone
noticing.

[juncai, 2015-09-08 23:13:29]

https://codereview.chromium.org/1326003006/diff/80001/chrome/browser/chrome_w...
File chrome/browser/chrome_webusb_browser_client_unittest.cc (right):

https://codereview.chromium.org/1326003006/diff/80001/chrome/browser/chrome_w...
chrome/browser/chrome_webusb_browser_client_unittest.cc:90:
l10n_util::GetStringUTF16(IDS_WEBUSB_DEVICE_DETECTED_NOTIFICATION);
On 2015/09/08 21:45:37, Reilly Grant wrote:
> Please hard-code the expected string instead of querying it through the l10n
> system. This helps guard against bugs creeping into UI strings without anyone
> noticing.

Done.

[Cait (Slow), 2015-09-09 02:19:53]

components/BUILD.gn, components/components_tests.gyp LGTM

[juncai, 2015-09-09 02:29:43]

The CQ bit was checked by juncai@chromium.org

[juncai, 2015-09-09 02:29:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/1326003006/#ps100001
(title: "hardcoded expected string for testing")

[commit-bot: I haz the power, 2015-09-09 02:30:29]

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

[commit-bot: I haz the power, 2015-09-09 09:04:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-09 09:05:43]

Patchset 6 (id:??) landed as
https://crrev.com/a764fd3e6431e3932b20d1c364a8d92a55120167
Cr-Commit-Position: refs/heads/master@{#347866}