Add skeleton implementation of ExtensionPushMessagingEventRouter.

Add skeleton implementation of ExtensionPushMessagingEventRouter.

BUG=139663
TEST=none


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

[dcheng, 2012-07-31 04:29:40]

InvalidationHandler (in http://codereview.chromium.org/10837013/) is still a
work in progress and probably won't be landed for awhile since it depends on
http://codereview.chromium.org/10805002/ to land.

This change just sets up some of the groundwork that InvalidationHandler will
(hopefully) plug into.

[Munjal (Google), 2012-07-31 17:36:12]

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/invalidation_handler_observer.h
(right):

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/invalidation_handler_observer.h:5:
#ifndef
CHROME_BROWSER_EXTENSIONS_API_PUSH_MESSAGING_INVALIDATION_HANDLER_OBSERVER_H_
I think Pete is using api/push/ folder in another patch. Can you sync up with
him about the folder?

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/invalidation_handler_observer.h:12:
class InvalidationHandlerObserver {
It will be nice to have a description for this class.

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/invalidation_handler_observer.h:16:
int subchannel,
Nit: sub_channel?

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/invalidation_handler_observer.h:18:
};
Nit: I am not sure if we need DISALLOW_COPY_AND_ASSIGN for abstract classes. May
be it is good to add it?

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:48: ListValue
args;
Define args closer to its first use; before args.Append(dict);

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:58:
extension_id,
Nit: indentation.

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_API_PUSH_MESSAGING_PUSH_MESSAGING_API_H__
Name this file push_messaging_event_router.h to match with the class name.

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:16: class
ExtensionPushMessagingEventRouter : public InvalidationHandlerObserver {
Add comment for the class.

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:25: const
std::string& payload);
Test class can become a friend class and directly call OnMessage. I have seen
that pattern used a lot, and will obviate the need for a public method just for
testing.

[dcheng, 2012-07-31 19:04:11]

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/invalidation_handler_observer.h
(right):

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/invalidation_handler_observer.h:5:
#ifndef
CHROME_BROWSER_EXTENSIONS_API_PUSH_MESSAGING_INVALIDATION_HANDLER_OBSERVER_H_
On 2012/07/31 17:36:12, munjal wrote:
> I think Pete is using api/push/ folder in another patch. Can you sync up with
> him about the folder?

He's OOO this week. However, I talked to Mark about this and since we're
planning on calling it "push messaging", I made it consistent throughout the
code, documentation and file paths.

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/invalidation_handler_observer.h:12:
class InvalidationHandlerObserver {
On 2012/07/31 17:36:12, munjal wrote:
> It will be nice to have a description for this class.

Done.

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/invalidation_handler_observer.h:16:
int subchannel,
On 2012/07/31 17:36:12, munjal wrote:
> Nit: sub_channel?

I ended up picking subchannel since "subchannel" is a more popular term than
"sub channel", in both Google search (from what I could tell) and code search.

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/invalidation_handler_observer.h:18:
};
On 2012/07/31 17:36:12, munjal wrote:
> Nit: I am not sure if we need DISALLOW_COPY_AND_ASSIGN for abstract classes.
May
> be it is good to add it?

I took a look at several other pure virtual classes, and none of them used
DISALLOW_COPY_AND_ASSIGN (content::NotificationObserver,
syncer::SyncNotifierObserver, several in base/)

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:48: ListValue
args;
On 2012/07/31 17:36:12, munjal wrote:
> Define args closer to its first use; before args.Append(dict);

Done.

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:58:
extension_id,
On 2012/07/31 17:36:12, munjal wrote:
> Nit: indentation.

Done.

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_API_PUSH_MESSAGING_PUSH_MESSAGING_API_H__
On 2012/07/31 17:36:12, munjal wrote:
> Name this file push_messaging_event_router.h to match with the class name.

I opted to follow the general implementation pattern that other app/extension
APIs have used: they all define their EventRouter class in a *_api.h file. Do
you still want to rename the file in light of that?

(ChromeOS doesn't follow this covention, but CrOS extension code lives in
another directory).

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:16: class
ExtensionPushMessagingEventRouter : public InvalidationHandlerObserver {
On 2012/07/31 17:36:12, munjal wrote:
> Add comment for the class.

Done.

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:25: const
std::string& payload);
On 2012/07/31 17:36:12, munjal wrote:
> Test class can become a friend class and directly call OnMessage. I have seen
> that pattern used a lot, and will obviate the need for a public method just
for
> testing.

Done. That being said, I think the ForTest method has two obvious advantages
over friending tests with no disadvantages:
- The test code is clearer (TriggerMessageForTest is a little more obvious than
calling OnMessage, which I had to add a comment for)
- The coupling between implementation and tests isn't as tight. I'm not a big
fan of a long list of FRIEND_TEST_ALL_PREFIXES macros.

[Munjal (Google), 2012-07-31 23:09:58]

LGTM. Exciting.

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/invalidation_handler_observer.h
(right):

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/invalidation_handler_observer.h:5:
#ifndef
CHROME_BROWSER_EXTENSIONS_API_PUSH_MESSAGING_INVALIDATION_HANDLER_OBSERVER_H_
On 2012/07/31 19:04:11, dcheng wrote:
> On 2012/07/31 17:36:12, munjal wrote:
> > I think Pete is using api/push/ folder in another patch. Can you sync up
with
> > him about the folder?
> 
> He's OOO this week. However, I talked to Mark about this and since we're
> planning on calling it "push messaging", I made it consistent throughout the
> code, documentation and file paths.

Okay, SG.

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

http://codereview.chromium.org/10828087/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:25: const
std::string& payload);
On 2012/07/31 19:04:11, dcheng wrote:
> On 2012/07/31 17:36:12, munjal wrote:
> > Test class can become a friend class and directly call OnMessage. I have
seen
> > that pattern used a lot, and will obviate the need for a public method just
> for
> > testing.
> 
> Done. That being said, I think the ForTest method has two obvious advantages
> over friending tests with no disadvantages:
> - The test code is clearer (TriggerMessageForTest is a little more obvious
than
> calling OnMessage, which I had to add a comment for)
> - The coupling between implementation and tests isn't as tight. I'm not a big
> fan of a long list of FRIEND_TEST_ALL_PREFIXES macros.

Yeah, I agree about readabiity of ForTest. But I do that by adding a method to
the base test class with a good name that calls OnMessage. That way I don't ahve
to do FRIEND_TEST_ALL_PREFIXES for various tests. I just declare the base class
as friend (since that is the only one that calls OnMessage directly) and the
readability is also good.

[dcheng, 2012-07-31 23:21:24]

Adding owners:
mihaip for *extension*
ben for *gyp*

[Ben Goodger (Google), 2012-08-01 15:41:57]

lgtm

[Mihai Parparita -not on Chrome, 2012-08-01 20:05:50]

http://codereview.chromium.org/10828087/diff/5002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

http://codereview.chromium.org/10828087/diff/5002/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:22: const
char kPayloadKey[] = "payload";
The extensions infrastructure now supports auto-generated C++ classes for types,
so that yo don't need to repeat keys from your .idl file in your code. See the
permissions API changes in http://crrev.com/119405 for an example of how to do
this.

http://codereview.chromium.org/10828087/diff/5002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

http://codereview.chromium.org/10828087/diff/5002/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:19: class
ExtensionPushMessagingEventRouter : public InvalidationHandlerObserver {
Given that this class is in the "extensions" namespace, you can drop the
Extensions prefix from its name (see AppEventRouter, ProxyEventRouter,
RuntimeEventRouter, etc.)

http://codereview.chromium.org/10828087/diff/5002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc
(right):

http://codereview.chromium.org/10828087/diff/5002/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:16: class
ExtensionPushMessagingApiTest : public ExtensionApiTest {
Can remove the "Extensions" prefix here too.

http://codereview.chromium.org/10828087/diff/5002/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:24: 
Nit: extra newline.

http://codereview.chromium.org/10828087/diff/5002/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_push_messaging.idl (right):

http://codereview.chromium.org/10828087/diff/5002/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_push_messaging.idl:7: namespace
experimental.pushMessaging{
Add a [nodoc] annotation for now, so that people don't try to use it while it's
still stubbed out (and so that you don't have to regenerate docs when fleshing
it out).

http://codereview.chromium.org/10828087/diff/5002/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_push_messaging.idl:10: long
subchannel;
ID fields in API structures generally have an Id suffix (cf. processId, tabId,
socketId, vendorId, etc.)

[dcheng, 2012-08-01 21:15:33]

PTAL.

http://codereview.chromium.org/10828087/diff/5002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

http://codereview.chromium.org/10828087/diff/5002/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:22: const
char kPayloadKey[] = "payload";
On 2012/08/01 20:05:50, Mihai Parparita wrote:
> The extensions infrastructure now supports auto-generated C++ classes for
types,
> so that yo don't need to repeat keys from your .idl file in your code. See the
> permissions API changes in http://crrev.com/119405 for an example of how to do
> this.

Done. This is really neat!

http://codereview.chromium.org/10828087/diff/5002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

http://codereview.chromium.org/10828087/diff/5002/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:19: class
ExtensionPushMessagingEventRouter : public InvalidationHandlerObserver {
On 2012/08/01 20:05:50, Mihai Parparita wrote:
> Given that this class is in the "extensions" namespace, you can drop the
> Extensions prefix from its name (see AppEventRouter, ProxyEventRouter,
> RuntimeEventRouter, etc.)

Done.

http://codereview.chromium.org/10828087/diff/5002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc
(right):

http://codereview.chromium.org/10828087/diff/5002/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:16: class
ExtensionPushMessagingApiTest : public ExtensionApiTest {
On 2012/08/01 20:05:50, Mihai Parparita wrote:
> Can remove the "Extensions" prefix here too.

Done.

http://codereview.chromium.org/10828087/diff/5002/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:24: 
On 2012/08/01 20:05:50, Mihai Parparita wrote:
> Nit: extra newline.

Done.

http://codereview.chromium.org/10828087/diff/5002/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_push_messaging.idl (right):

http://codereview.chromium.org/10828087/diff/5002/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_push_messaging.idl:7: namespace
experimental.pushMessaging{
On 2012/08/01 20:05:50, Mihai Parparita wrote:
> Add a [nodoc] annotation for now, so that people don't try to use it while
it's
> still stubbed out (and so that you don't have to regenerate docs when fleshing
> it out).

Done.

http://codereview.chromium.org/10828087/diff/5002/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_push_messaging.idl:10: long
subchannel;
On 2012/08/01 20:05:50, Mihai Parparita wrote:
> ID fields in API structures generally have an Id suffix (cf. processId, tabId,
> socketId, vendorId, etc.)

Done.

[Mihai Parparita -not on Chrome, 2012-08-01 21:29:13]

LGTM

http://codereview.chromium.org/10828087/diff/8010/chrome/browser/extensions/a...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

http://codereview.chromium.org/10828087/diff/8010/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:16: using
content::BrowserThread;
Nit: this is unused.

http://codereview.chromium.org/10828087/diff/8010/chrome/browser/extensions/a...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

http://codereview.chromium.org/10828087/diff/8010/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:11: #include
"base/memory/scoped_ptr.h"
Nit: this can be removed.

[dcheng, 2012-08-01 21:37:43]

http://codereview.chromium.org/10828087/diff/8010/chrome/browser/extensions/a...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

http://codereview.chromium.org/10828087/diff/8010/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:16: using
content::BrowserThread;
On 2012/08/01 21:29:13, Mihai Parparita wrote:
> Nit: this is unused.

Done.

http://codereview.chromium.org/10828087/diff/8010/chrome/browser/extensions/a...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

http://codereview.chromium.org/10828087/diff/8010/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:11: #include
"base/memory/scoped_ptr.h"
On 2012/08/01 21:29:13, Mihai Parparita wrote:
> Nit: this can be removed.

Done.

[commit-bot: I haz the power, 2012-08-01 21:37:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10828087/8014

[commit-bot: I haz the power, 2012-08-01 22:04:58]

Try job failure for 10828087-8014 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-01 22:16:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10828087/15003

[commit-bot: I haz the power, 2012-08-01 23:54:41]

Change committed as 149536