Add a browser test for functions in notification provider API

Add a browser test for functions in notification provider API

Add a basic browser test for functions in notification provider API.
This is mainly to test the connection and provide a skeleton for future
tests. The functions are not fully implemented yet, and more tests will
be added as the functions are implemented.

BUG=

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

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

[liyanhou, 2014-08-04 17:56:33]

Could you take a look at this? Thank you so much!

[Pete Williamson, 2014-08-04 20:30:46]

https://codereview.chromium.org/416423006/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc
(right):

https://codereview.chromium.org/416423006/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:15:
void CreateNotificationOptionsForTest(
Great job refactoring this!  This is exactly the kind of thing we want to do as
we add more code to a project.

https://codereview.chromium.org/416423006/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:43:
extensions::NotificationProviderEventRouter* event_router =
Why remove the use of scoped_ptr here?  If we did mean to remove it, we should
have a delete, but probably better still to put it back.

https://codereview.chromium.org/416423006/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:69:
const extensions::Extension* extension = LoadExtension(
Do we ever free this memory?  Might be good to use a smart pointer.  While the
bar is lower for tests, we should still prevent memory leaks when running the
test.  If nothing else, it will help us know when a leak check of a test fails
that the problem is likely to be in the code under test instead of the test
itself.

https://codereview.chromium.org/416423006/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:73:
extensions::NotificationProviderEventRouter* event_router =
ccoped_ptr here too.

https://codereview.chromium.org/416423006/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/notification_provider/basic_usage/manifest.json
(right):

https://codereview.chromium.org/416423006/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/notification_provider/basic_usage/manifest.json:7:
"scripts": ["background.js"]
Are there any tests in test.js that are now no longer run?  (Or was it empty to
start with?)

[liyanhou, 2014-08-04 23:24:16]

Could you take another look at this? Thank you so much!

https://codereview.chromium.org/416423006/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc
(right):

https://codereview.chromium.org/416423006/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:15:
void CreateNotificationOptionsForTest(
On 2014/08/04 20:30:45, Pete Williamson wrote:
> Great job refactoring this!  This is exactly the kind of thing we want to do
as
> we add more code to a project.

Thank you!

https://codereview.chromium.org/416423006/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:43:
extensions::NotificationProviderEventRouter* event_router =
On 2014/08/04 20:30:45, Pete Williamson wrote:
> Why remove the use of scoped_ptr here?  If we did mean to remove it, we should
> have a delete, but probably better still to put it back.

Done. Sorry that was a mistake.

https://codereview.chromium.org/416423006/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:69:
const extensions::Extension* extension = LoadExtension(
On 2014/08/04 20:30:46, Pete Williamson wrote:
> Do we ever free this memory?  Might be good to use a smart pointer.  While the
> bar is lower for tests, we should still prevent memory leaks when running the
> test.  If nothing else, it will help us know when a leak check of a test fails
> that the problem is likely to be in the code under test instead of the test
> itself.

Do we need to free this memory if it's not new'd?

https://codereview.chromium.org/416423006/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:73:
extensions::NotificationProviderEventRouter* event_router =
On 2014/08/04 20:30:45, Pete Williamson wrote:
> ccoped_ptr here too.

Done.

https://codereview.chromium.org/416423006/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/notification_provider/basic_usage/manifest.json
(right):

https://codereview.chromium.org/416423006/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/notification_provider/basic_usage/manifest.json:7:
"scripts": ["background.js"]
On 2014/08/04 20:30:46, Pete Williamson wrote:
> Are there any tests in test.js that are now no longer run?  (Or was it empty
to
> start with?)

Somehow, this diff turned out to be between events/manifest.json and
basic_usage/manifest.json. But basic_usage is a new directory. So, yes, test.js
has tests. But it will run when another browser test runs, and is not relevant
to this test.

[Pete Williamson, 2014-08-05 00:46:42]

LGTM.

You're right, it looks like we do not need to free what LoadExtension returns.

[liyanhou, 2014-08-05 17:35:13]

kalman@ Could you take a look at the notification_provider_apitest.cc file?
Thank you so much!

[not at google - send to devlin, 2014-08-05 18:12:27]

lgtm

[liyanhou, 2014-08-05 18:50:16]

The CQ bit was checked by liyanhou@chromium.org

[commit-bot: I haz the power, 2014-08-05 18:51:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/liyanhou@chromium.org/416423006/20001

[commit-bot: I haz the power, 2014-08-05 22:23:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-05 23:26:13]

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

[commit-bot: I haz the power, 2014-08-05 23:26:14]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dewittj, 2014-08-05 23:34:14]

The CQ bit was checked by dewittj@chromium.org

[commit-bot: I haz the power, 2014-08-05 23:36:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/liyanhou@chromium.org/416423006/20001

[commit-bot: I haz the power, 2014-08-06 05:57:35]

Change committed as 287692

[Adam Rice, 2014-08-06 07:11:49]

Reverted as 287722.

Leaks memory. See

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...

https://codereview.chromium.org/416423006/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc
(right):

https://codereview.chromium.org/416423006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:77:
extension->id(), sender_id1, notification_id1, *options.release());
lol. no.

[liyanhou, 2014-08-07 04:19:21]

The CQ bit was checked by liyanhou@chromium.org

[commit-bot: I haz the power, 2014-08-07 04:20:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/liyanhou@chromium.org/416423006/40001

[commit-bot: I haz the power, 2014-08-07 05:08:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-07 06:58:41]

Change committed as 287972