Plumb invalidations from Tango to the extensions code for the Push Messaging API.

Plumb invalidations from Tango to the extensions code for the Push Messaging API.

BUG=139661, 139663
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152877

[dcheng, 2012-08-09 20:43:53]

Mostly ready, with the unfortunate problem that it causes all
sync_integration_tests to fail. =)

+munjal and petewil for the overall review
+akalin for *invalidation_handler* and *sync* changes (I'm assuming your patch
will land before mine; I will update and rebase as appropriate)
+mihaip for push_messaging_api.cc and permissions changes and some
extension-related questions.

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

http://codereview.chromium.org/10826156/diff/2030/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:46: const
ExtensionSet* extensions =
It's unclear from the comments whether this is only enabled apps/extensions, or
if it includes disabled ones as well. I don't see an obvious method on Extension
to indicate whether or not it's loaded, so I assume that presence in
ExtensionService::extensions() means it's enabled.

http://codereview.chromium.org/10826156/diff/2030/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:99: case
chrome::NOTIFICATION_EXTENSION_UNLOADED: {
I believe this doesn't get called when a Profile is shutdown; can you please
confirm this Mihai?

[akalin, 2012-08-10 23:07:10]

overall, this looks good.  I'd like to take another look after you rebase off my
latest patch, though.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h
(right):

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:24:
// Tango registrations, and dispatches callbacks for any invalidations received.
tango is an internal code name.  maybe 'invalidation registrations'?

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/sync/invali...
File chrome/browser/sync/invalidation_service.h (right):

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/sync/invali...
chrome/browser/sync/invalidation_service.h:15: class InvalidationService {
this name is confusing, since eventually non-service things can (presumably)
implement this.

i don't have a better name yet. :( I'll try thinking of one.

[Mihai Parparita -not on Chrome, 2012-08-11 00:47:26]

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

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:101: case
chrome::NOTIFICATION_EXTENSION_UNLOADED: {
There's also NOTIFICATION_EXTENSION_PERMISSIONS_UPDATED (when permissions are
granted via the optional permissions API).

[Pete Williamson, 2012-08-13 18:24:09]

I'm still getting up to speed on the Chromium style guidelines, feel free to
ignore comments about style where the style guidelines contradict me.

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

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:38: 
Might be good to add a comment here:
// Find out about all the extensions, both already installed and those that get
installed during this session, put them into a list, and register their
associated object IDs.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:45: 
Might be good to add a comment:
// Register for all extension load and unload events so we can tell when a new
push-messaging capable extension installs or uninstalls

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:52:
std::set<std::string> push_messaging_extensions;
// Iterate over the list of installed extensions and make a list of those who
use push-messaging

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:59: }
//  now register all the object IDs for the push messaging extensions

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc
(right):

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:42: 
Should we also have a unit test for the "Init()" method?

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
(right):

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:20:

Nit - the "kXxxIndex" constants below might be a bit clearer as kXxxStartIndex,
to imply that this is the starting index of a portion of a string instead of an
index into an array

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:67:
}
Might be good to add a comment here:
// TODO: Modify the Server side code to take a format type of "V" then come back
and change this "U" to "V"

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:71:
}
Does this "PM" need to be registered somewhere so that nobody else tries to use
it?  If so, let's start a thread to make that happen, and add a TODO: so we
don't forget.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:89:

Coding standards question:  I personally think it is a good idea for each
function to have a one line header comment describing the intent of the
function.  Is that done here (either in the header or the cpp file?)  Also, I
think we should mention that this class just makes a copy of the service and
observer, and relies upon the caller to ensure they are valid for the lifetime
of this class.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:105:

Is this removing the registration when the DTOR fires, or completing it?  I'm
not quite sure what the intent is here (perhaps a comment would help the reader)

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:143:
// Nothing to do.
Should we DCHECK(false); here since this shouldn't happen, and disables our
system?

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h
(right):

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:12:
#include "base/compiler_specific.h"
It's a bit scary that we need to have some compiler specific behavior.  A
comment might help the maintenance programmer understand why we need this header
file.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:18:

It might be nice to replace the word Invalidations in file and class names with
"Message", or "Notification" or "Event".  Invalidations is a kind of strange
concept for the maintenance programmer to understand, and an artifact of the
underlying layer.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:29:
// enabled.
It might be good to comment on the ownership of the service and observer objects
- are they taken over by the handler, or assumed to remain valid over the
lifetime of the Handler?

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:40:
// SyncNotifierObserver implementation.
A bit more comment here about what a SyncNotifierObserver is might help. 
"Observes Notifications coming from Sync Layer, and routes them" perhaps?

http://codereview.chromium.org/10826156/diff/14016/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/push_messaging/background.js (right):

http://codereview.chromium.org/10826156/diff/14016/chrome/test/data/extension...
chrome/test/data/extensions/api_test/push_messaging/background.js:1: 
I don't see any changes - does this file need to be removed from the patch?

[dcheng, 2012-08-13 20:44:14]

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

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:38: 
On 2012/08/13 18:24:09, petewil1 wrote:
> Might be good to add a comment here:
> // Find out about all the extensions, both already installed and those that
get
> installed during this session, put them into a list, and register their
> associated object IDs.

In general, we try to avoid comments that describe what the code does.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:45: 
On 2012/08/13 18:24:09, petewil1 wrote:
> Might be good to add a comment:
> // Register for all extension load and unload events so we can tell when a new
> push-messaging capable extension installs or uninstalls

Done.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:52:
std::set<std::string> push_messaging_extensions;
On 2012/08/13 18:24:09, petewil1 wrote:
> // Iterate over the list of installed extensions and make a list of those who
> use push-messaging

See above.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:59: }
On 2012/08/13 18:24:09, petewil1 wrote:
> //  now register all the object IDs for the push messaging extensions

See above.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:101: case
chrome::NOTIFICATION_EXTENSION_UNLOADED: {
On 2012/08/11 00:47:27, Mihai Parparita wrote:
> There's also NOTIFICATION_EXTENSION_PERMISSIONS_UPDATED (when permissions are
> granted via the optional permissions API).

Hmm. Thinking about it more, I'm not sure if push messaging makes a good
optional permission. It seems like something you'd always want on, whereas
optional permission seems to imply that you should drop it as soon as you don't
need it...

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc
(right):

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:42: 
On 2012/08/13 18:24:09, petewil1 wrote:
> Should we also have a unit test for the "Init()" method?

This wasn't originally possible since it would require a test that could span
restarts... but jam has added some infrastructure that might make it possible.
I'll have to investigate.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
(right):

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:20:

On 2012/08/13 18:24:09, petewil1 wrote:
> Nit - the "kXxxIndex" constants below might be a bit clearer as
kXxxStartIndex,
> to imply that this is the starting index of a portion of a string instead of
an
> index into an array

It's also the index into a string. I named it "Index" instead of "StartIndex"
since there's no complementary "EndIndex".

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:67:
}
On 2012/08/13 18:24:09, petewil1 wrote:
> Might be good to add a comment here:
> // TODO: Modify the Server side code to take a format type of "V" then come
back
> and change this "U" to "V"

I avoided making that comment since that would require changing this code again
if my current server side change is accepted as is.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:71:
}
On 2012/08/13 18:24:09, petewil1 wrote:
> Does this "PM" need to be registered somewhere so that nobody else tries to
use
> it?  If so, let's start a thread to make that happen, and add a TODO: so we
> don't forget.

This is an ad-hoc registration. We rely on good citizenship =)

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:89:

On 2012/08/13 18:24:09, petewil1 wrote:
> Coding standards question:  I personally think it is a good idea for each
> function to have a one line header comment describing the intent of the
> function.  Is that done here (either in the header or the cpp file?)  Also, I
> think we should mention that this class just makes a copy of the service and
> observer, and relies upon the caller to ensure they are valid for the lifetime
> of this class.

I've updated the header with comments about the lifetime of pointers that are
passed in.

WRT to commenting functions, I'll comment non-obvious functions or non-obvious
behaviors. For the most part, since this is a relatively straightforward class,
the interface is pretty simple and self-documenting.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:105:

On 2012/08/13 18:24:09, petewil1 wrote:
> Is this removing the registration when the DTOR fires, or completing it?  I'm
> not quite sure what the intent is here (perhaps a comment would help the
reader)

I've rebased to akalin's latest set of changes. It should be clearer now, since
it uses the common pattern of Registering yourself as an Observer in the ctor
and Unregistering yourself in the dtor. A lot of the documentation for this is
already in profile_sync_service.h as well, so I wanted to avoid duplicating that
information here.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:143:
// Nothing to do.
On 2012/08/13 18:24:09, petewil1 wrote:
> Should we DCHECK(false); here since this shouldn't happen, and disables our
> system?

That just means that sync has been disabled. In that case, there's not too much
we can do, but DCHECK/CHECKing probably isn't what we want to do.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h
(right):

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:12:
#include "base/compiler_specific.h"
On 2012/08/13 18:24:09, petewil1 wrote:
> It's a bit scary that we need to have some compiler specific behavior.  A
> comment might help the maintenance programmer understand why we need this
header
> file.

It is used for the OVERRIDE keyword. Since this is so prevalent, it's generally
not commented upon.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:18:

On 2012/08/13 18:24:09, petewil1 wrote:
> It might be nice to replace the word Invalidations in file and class names
with
> "Message", or "Notification" or "Event".  Invalidations is a kind of strange
> concept for the maintenance programmer to understand, and an artifact of the
> underlying layer.

Notification, Message, and Event are all a little too generic here IMO.
"Invalidation" fits well since this is the class that maps between the
invalidation model and the extension API model.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:24:
// Tango registrations, and dispatches callbacks for any invalidations received.
On 2012/08/10 23:07:10, akalin wrote:
> tango is an internal code name.  maybe 'invalidation registrations'?

Done. "tango" is mentioned in the google-cache-invalidation repository though
which is why I thought it'd be OK.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:29:
// enabled.
On 2012/08/13 18:24:09, petewil1 wrote:
> It might be good to comment on the ownership of the service and observer
objects
> - are they taken over by the handler, or assumed to remain valid over the
> lifetime of the Handler?

Done.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:40:
// SyncNotifierObserver implementation.
On 2012/08/13 18:24:09, petewil1 wrote:
> A bit more comment here about what a SyncNotifierObserver is might help. 
> "Observes Notifications coming from Sync Layer, and routes them" perhaps?

Usually the documentation for the interface lives in the file that defines the
interface. Otherwise it'd create a lot of duplicate comments.

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/sync/invali...
File chrome/browser/sync/invalidation_service.h (right):

http://codereview.chromium.org/10826156/diff/14016/chrome/browser/sync/invali...
chrome/browser/sync/invalidation_service.h:15: class InvalidationService {
On 2012/08/10 23:07:10, akalin wrote:
> this name is confusing, since eventually non-service things can (presumably)
> implement this.
> 
> i don't have a better name yet. :( I'll try thinking of one.

I agree that this is not a great name. I am taking suggestions for better ones
=)

http://codereview.chromium.org/10826156/diff/14016/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/push_messaging/background.js (right):

http://codereview.chromium.org/10826156/diff/14016/chrome/test/data/extension...
chrome/test/data/extensions/api_test/push_messaging/background.js:1: 
On 2012/08/13 18:24:09, petewil1 wrote:
> I don't see any changes - does this file need to be removed from the patch?

This is a new file. It just has one newline in it since I didn't need any other
behavior. The side-by-side diff on Rietveld looks odd, though the actual content
is correct. I suspect this is partially due to git's move detection algorithm.

[Munjal (Google), 2012-08-14 18:56:37]

Apart from teh comments below, I was also thinking if there is a way to remove
the dependency on ProfileSyncService for push messaging. But I will delegate it
to Fred to comment on that since I am not sure how easy or hard it is to do
that.

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
(right):

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:43:
std::string name = base::StringPrintf("UPM%s%d", extension_id.c_str(), i);
How about using some separator (e.g. |) in the object id? E.g.
UPM|<ext_id>|sub_channel_id

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:85:
return *subchannel <= 3;
The logic in this method will be much more robust if we use separators instead
of relying on the length of each field.

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:134:
service_->UpdateRegisteredInvalidationIds(this, registered_ids_);
If we are keeping track of registered_ids_ just to tell the invalidation servie
what objects ids we want to register for, we can just keep a list of extension
ids instead and then compute the list of object ids from that every time we need
ot tell the invalidation service. The logic to remove an extension id will
become much simpler.

Or is it possible to tell InvalidatinoService to add/remove specific object ids
instead of having ot give it a full list every time?

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h
(right):

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:21:
class PushMessagingInvalidationHandlerObserver;
I am wondering if we should have a browser/push_messaging folder where we put
most of our push messaging code in a push_messaging namespace. This has two
benefits:
- Class names won't be very long.
- Most of the push messaging stuff can be in one place and then we will just
have a thin layer for extension event routing.

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:527:
push_messaging_event_router_->Shutdown();
Can't we clean up the event router in the destructor instead of adding an
explicit method call?

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/sync/invalid...
File chrome/browser/sync/invalidation_service.h (right):

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/sync/invalid...
chrome/browser/sync/invalidation_service.h:14: // TODO(dcheng): Add an
interesting comment here.
Did you mean to add a comment here before committing?

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/sync/invalid...
chrome/browser/sync/invalidation_service.h:19: // and it must already be
registered.
This comment is a bit confusing. Comment says the handler must alredy be
registered, but this method seems to be the one that can register a new handler.
?

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/sync/invalid...
chrome/browser/sync/invalidation_service.h:32: const syncer::ObjectIdSet& ids) =
0;
Could we instead expose methods to add and remove object ids the handler is
listening for?

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/sync/invalid...
chrome/browser/sync/invalidation_service.h:36: // associated with |handler|.
So are object ids shared across handlers? Or in other words, can multiple
handlers listen to the same object ids?

If that is the case, what happens when one handler stops listening to some
object ids? Does InvalidationService unregister for that object id? Or does it
keep track of all handlers for that object id and unregister for the object id
if there is no handler for that?

I am just trying to understand if there is a way to simplify the semantics here
a little bit.

http://codereview.chromium.org/10826156/diff/6005/chrome/common/extensions/ap...
File chrome/common/extensions/api/_permission_features.json (right):

http://codereview.chromium.org/10826156/diff/6005/chrome/common/extensions/ap...
chrome/common/extensions/api/_permission_features.json:233: "extension_types":
["platform_app"]
I think we should allow this feature for packaged apps, platform apps and
extensions.

[dcheng, 2012-08-14 21:39:58]

There are future plans to decouple invalidations from ProfileSyncService. For
now, that's the interface though =)

I've also added a rudimentary browser test for restarts. It's somewhat imprecise
though, and it makes me sad because it requires some ugly casts...

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
(right):

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:43:
std::string name = base::StringPrintf("UPM%s%d", extension_id.c_str(), i);
On 2012/08/14 18:56:37, munjal wrote:
> How about using some separator (e.g. |) in the object id? E.g.
> UPM|<ext_id>|sub_channel_id

The Chrome components format didn't allow for non-alphanumeric characters, nor
did it use separators. Now that we're going to have our own source ID, we can do
something else of course; I'll take a look other object ID factories to see how
they do it.

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:85:
return *subchannel <= 3;
On 2012/08/14 18:56:37, munjal wrote:
> The logic in this method will be much more robust if we use separators instead
> of relying on the length of each field.

See above.

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:134:
service_->UpdateRegisteredInvalidationIds(this, registered_ids_);
On 2012/08/14 18:56:37, munjal wrote:
> If we are keeping track of registered_ids_ just to tell the invalidation
servie
> what objects ids we want to register for, we can just keep a list of extension
> ids instead and then compute the list of object ids from that every time we
need
> ot tell the invalidation service. The logic to remove an extension id will
> become much simpler.
> 

Done.

> Or is it possible to tell InvalidatinoService to add/remove specific object
ids
> instead of having ot give it a full list every time?

It is not possible to pass a diff to InvalidationService. It is possible that
this is something we want to address in the future, depending on how consumers
of the API choose to use it.

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h
(right):

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:21:
class PushMessagingInvalidationHandlerObserver;
On 2012/08/14 18:56:37, munjal wrote:
> I am wondering if we should have a browser/push_messaging folder where we put
> most of our push messaging code in a push_messaging namespace. This has two
> benefits:
> - Class names won't be very long.
> - Most of the push messaging stuff can be in one place and then we will just
> have a thin layer for extension event routing.

That seems like a good idea. The class names are ridiculously long, since I
didn't feel comfortable putting generic names like "InvalidationHandler" into
the extensions namespace. I'd like to pursue that move in a separate patch
though.

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:527:
push_messaging_event_router_->Shutdown();
On 2012/08/14 18:56:37, munjal wrote:
> Can't we clean up the event router in the destructor instead of adding an
> explicit method call?

We cannot. We have dependencies such that PSS depends on ExtensionSystem. That
means that PSS is destroyed before ExtensionSystem, so we need to drop our
references to PSS in Shutdown() before it is destroyed.

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/sync/invalid...
File chrome/browser/sync/invalidation_service.h (right):

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/sync/invalid...
chrome/browser/sync/invalidation_service.h:14: // TODO(dcheng): Add an
interesting comment here.
On 2012/08/14 18:56:37, munjal wrote:
> Did you mean to add a comment here before committing?

I've replaced this with a slightly more helpful comment.

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/sync/invalid...
chrome/browser/sync/invalidation_service.h:19: // and it must already be
registered.
On 2012/08/14 18:56:37, munjal wrote:
> This comment is a bit confusing. Comment says the handler must alredy be
> registered, but this method seems to be the one that can register a new
handler.
> ?

Hmm. I copied these comments frmo ProfileSyncService... but it does read a
little oddly. I'll talk with Fred about this.

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/sync/invalid...
chrome/browser/sync/invalidation_service.h:32: const syncer::ObjectIdSet& ids) =
0;
On 2012/08/14 18:56:37, munjal wrote:
> Could we instead expose methods to add and remove object ids the handler is
> listening for?

This is how the interface looks for now. It was chosen since it was likely that
most handlers would have to keep track of what was registered themselves anyway.

If it simplifies the implementation of a lot of SyncNotifierObservers, it's
possible we'll change this but that won't happen in this CL.

http://codereview.chromium.org/10826156/diff/6005/chrome/browser/sync/invalid...
chrome/browser/sync/invalidation_service.h:36: // associated with |handler|.
On 2012/08/14 18:56:37, munjal wrote:
> So are object ids shared across handlers? Or in other words, can multiple
> handlers listen to the same object ids?
> 

No. "An ID must be registered for at most one handler."

> If that is the case, what happens when one handler stops listening to some
> object ids? Does InvalidationService unregister for that object id? Or does it
> keep track of all handlers for that object id and unregister for the object id
> if there is no handler for that?
> 
> I am just trying to understand if there is a way to simplify the semantics
here
> a little bit.

This is used for shutdown. When Unregister() is called, all existing
registrations for a handler are orphaned. The intended use is to avoid a lot of
re-registration traffic when Chrome is restarted. See
http://code.google.com/p/chromium/issues/detail?id=140325

http://codereview.chromium.org/10826156/diff/6005/chrome/common/extensions/ap...
File chrome/common/extensions/api/_permission_features.json (right):

http://codereview.chromium.org/10826156/diff/6005/chrome/common/extensions/ap...
chrome/common/extensions/api/_permission_features.json:233: "extension_types":
["platform_app"]
On 2012/08/14 18:56:37, munjal wrote:
> I think we should allow this feature for packaged apps, platform apps and
> extensions.

On #crx, I think Aaron mentioned that all the new permissions should be platform
app (he explicitly mentioned push messaging as one of those). I'm not sure what
the rationale was.

[dcheng, 2012-08-16 19:55:07]

Hello all,

Please take another look.

I've added a new browser test. I've also talked to akalin about the comments in
ProfileSyncService; I'm just going to clone the comments from PSS from now and
update it when akalin fixes the typo in PSS.

[Munjal (Google), 2012-08-20 19:26:00]

LGTM with nits and optional suggestions you can do in another patch.

http://codereview.chromium.org/10826156/diff/6006/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h
(right):

http://codereview.chromium.org/10826156/diff/6006/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:48:
syncer::IncomingNotificationSource source) OVERRIDE;
I think you can make the OVERRIDE methods private. That way the public interface
remains clean.

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

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:68:
handler_.reset();
Please add a comment describing why we have explicit Shutdown.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
(right):

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:27:
syncer::ObjectIdSet ExtensionIdToObjectIds(const std::string& extension_id) {
Nit: Should we pass in ObjectIdSet* instead of returning to avoid making a copy?

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:79:
PushMessagingInvalidationHandlerObserver* observer,
I just realized that "observer" is a more appropriate name when multiple
observers are allowed. "delegate" is a more appropriate term in this case, since
only one is allowed at a time.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:97:
DCHECK_EQ(32U, extension_id.size());
There are few places in this file which use 32 as extendino id size. At the
least, we need a constant for that in this file. Better still, if there is a
constant somewhere in extension code for this, we can use that.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h
(right):

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:38:
// PushMessagingInvalidationMapper implementation.
Make these methods private/protected to reduce the public interface.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:42:
// SyncNotifierObserver implementation.
Make these methods private/protected to reduce the public interface.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_unittest.cc
(right):

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_unittest.cc:23:
namespace {
NIt: Put hte anonymous namesapce outside of extensions namespace?

[akalin, 2012-08-20 20:35:01]

mostly nits, but I thought of a better name for InvalidationService

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (left):

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:34: Profile*
profile_;
const pointer?

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
(right):

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:27:
syncer::ObjectIdSet ExtensionIdToObjectIds(const std::string& extension_id) {
On 2012/08/20 19:26:00, munjal wrote:
> Nit: Should we pass in ObjectIdSet* instead of returning to avoid making a
copy?

not really -- it's a microoptimization that's usually done by the compiler.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:30:
std::string name = base::StringPrintf("U/%s/%d", extension_id.c_str(), i);
Consider just doing:

const std::string& name = "U/" + extension_id + "/" + base::IntToString(i);

as it's more type-safe.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:55:
if (components[0] != "U") {
check components size?

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:59:
if (components[1].size() != 32) {
I think there's an IsExtensionIdValid() function somewhere that you can use
instead

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:97:
DCHECK_EQ(32U, extension_id.size());
On 2012/08/20 19:26:00, munjal wrote:
> There are few places in this file which use 32 as extendino id size. At the
> least, we need a constant for that in this file. Better still, if there is a
> constant somewhere in extension code for this, we can use that.

probably should just use IsExtensionIdValid()

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:105:
DCHECK_EQ(32U, extension_id.size());
here too

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:140:
registered_extensions_.begin(); it != registered_extensions_.end();
i usually try to break after semicolons

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h
(right):

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:38:
// PushMessagingInvalidationMapper implementation.
On 2012/08/20 19:26:00, munjal wrote:
> Make these methods private/protected to reduce the public interface.

I think the current best practice is to not bother; there's no protection
afforded since you can just (inadvertently) cast to the parent class and call
the function.  And there's no point in publicly inheriting from a class only to
hide the inherited functions.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:42:
// SyncNotifierObserver implementation.
On 2012/08/20 19:26:00, munjal wrote:
> Make these methods private/protected to reduce the public interface.

same as above.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:58:
InvalidationService* service_;
const pointer?

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:60:
PushMessagingInvalidationHandlerObserver* observer_;
const pointer?

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_observer.h
(right):

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_observer.h:17:
virtual ~PushMessagingInvalidationHandlerObserver() {}
protected destructor?

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/sync/invali...
File chrome/browser/sync/invalidation_service.h (right):

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/sync/invali...
chrome/browser/sync/invalidation_service.h:16: class InvalidationService {
Okay, how about the name InvalidationFrontend?  Suggested comment:

// Interface for classes that handle invalidation registrations and send out
invalidations to registered handlers.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/sync/invali...
chrome/browser/sync/invalidation_service.h:18: virtual ~InvalidationService() {
}
make destructor protected

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/sync/invali...
chrome/browser/sync/invalidation_service.h:20: // and it must already be
registered.
may as well fix the comment in this patch

[dcheng, 2012-08-20 20:58:20]

Addressed all the comments... I think.

http://codereview.chromium.org/10826156/diff/6006/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h
(right):

http://codereview.chromium.org/10826156/diff/6006/chrome/browser/extensions/a...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:48:
syncer::IncomingNotificationSource source) OVERRIDE;
On 2012/08/20 19:26:00, munjal wrote:
> I think you can make the OVERRIDE methods private. That way the public
interface
> remains clean.

See above. Or below. Or wherever the actual comment went.

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

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:68:
handler_.reset();
On 2012/08/20 19:26:00, munjal wrote:
> Please add a comment describing why we have explicit Shutdown.

Done.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (left):

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:34: Profile*
profile_;
On 2012/08/20 20:35:01, akalin wrote:
> const pointer?

Done.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
(right):

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:27:
syncer::ObjectIdSet ExtensionIdToObjectIds(const std::string& extension_id) {
On 2012/08/20 19:26:00, munjal wrote:
> Nit: Should we pass in ObjectIdSet* instead of returning to avoid making a
copy?

Code that deals with ObjectIdSets (e.g. in sync) generally depends on RVO/NRVO
to elide the additional copy. I'm happy to change this if you feel strongly
about it.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:30:
std::string name = base::StringPrintf("U/%s/%d", extension_id.c_str(), i);
On 2012/08/20 20:35:01, akalin wrote:
> Consider just doing:
> 
> const std::string& name = "U/" + extension_id + "/" + base::IntToString(i);
> 
> as it's more type-safe.

Done.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:55:
if (components[0] != "U") {
On 2012/08/20 20:35:01, akalin wrote:
> check components size?

Oops. Done.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:59:
if (components[1].size() != 32) {
On 2012/08/20 20:35:01, akalin wrote:
> I think there's an IsExtensionIdValid() function somewhere that you can use
> instead

Done.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:79:
PushMessagingInvalidationHandlerObserver* observer,
On 2012/08/20 19:26:00, munjal wrote:
> I just realized that "observer" is a more appropriate name when multiple
> observers are allowed. "delegate" is a more appropriate term in this case,
since
> only one is allowed at a time.

Done.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:97:
DCHECK_EQ(32U, extension_id.size());
On 2012/08/20 20:35:01, akalin wrote:
> On 2012/08/20 19:26:00, munjal wrote:
> > There are few places in this file which use 32 as extendino id size. At the
> > least, we need a constant for that in this file. Better still, if there is a
> > constant somewhere in extension code for this, we can use that.
> 
> probably should just use IsExtensionIdValid()

Using Extension::IdIsValid() here and several other places now instead of using
32U.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:105:
DCHECK_EQ(32U, extension_id.size());
On 2012/08/20 20:35:01, akalin wrote:
> here too

Done.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:140:
registered_extensions_.begin(); it != registered_extensions_.end();
On 2012/08/20 20:35:01, akalin wrote:
> i usually try to break after semicolons

It doesn't fit on one line.
<embarrassed look>

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h
(right):

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:38:
// PushMessagingInvalidationMapper implementation.
On 2012/08/20 20:35:01, akalin wrote:
> On 2012/08/20 19:26:00, munjal wrote:
> > Make these methods private/protected to reduce the public interface.
> 
> I think the current best practice is to not bother; there's no protection
> afforded since you can just (inadvertently) cast to the parent class and call
> the function.  And there's no point in publicly inheriting from a class only
to
> hide the inherited functions.

Opted to leave it the same. Eliminates FRIEND_TEST_ALL_PREFIXES lines as well.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:42:
// SyncNotifierObserver implementation.
On 2012/08/20 20:35:01, akalin wrote:
> On 2012/08/20 19:26:00, munjal wrote:
> > Make these methods private/protected to reduce the public interface.
> 
> same as above.

Ditto.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:58:
InvalidationService* service_;
On 2012/08/20 20:35:01, akalin wrote:
> const pointer?

Done.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:60:
PushMessagingInvalidationHandlerObserver* observer_;
On 2012/08/20 20:35:01, akalin wrote:
> const pointer?

Done.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_observer.h
(right):

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_observer.h:17:
virtual ~PushMessagingInvalidationHandlerObserver() {}
On 2012/08/20 20:35:01, akalin wrote:
> protected destructor?

Done.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_unittest.cc
(right):

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_unittest.cc:23:
namespace {
On 2012/08/20 19:26:00, munjal wrote:
> NIt: Put hte anonymous namesapce outside of extensions namespace?

This is for convenience: that way, I don't need to say
extensions::PushMessagingINvalidationHandlerObserver, for example.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/sync/invali...
File chrome/browser/sync/invalidation_service.h (right):

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/sync/invali...
chrome/browser/sync/invalidation_service.h:16: class InvalidationService {
On 2012/08/20 20:35:01, akalin wrote:
> Okay, how about the name InvalidationFrontend?  Suggested comment:
> 
> // Interface for classes that handle invalidation registrations and send out
> invalidations to registered handlers.

Done.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/sync/invali...
chrome/browser/sync/invalidation_service.h:18: virtual ~InvalidationService() {
}
On 2012/08/20 20:35:01, akalin wrote:
> make destructor protected

Done.

http://codereview.chromium.org/10826156/diff/13013/chrome/browser/sync/invali...
chrome/browser/sync/invalidation_service.h:20: // and it must already be
registered.
On 2012/08/20 20:35:01, akalin wrote:
> may as well fix the comment in this patch

Done.

[Pete Williamson, 2012-08-20 22:41:05]

http://codereview.chromium.org/10826156/diff/15029/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
(right):

http://codereview.chromium.org/10826156/diff/15029/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:27:
syncer::ObjectIdSet ExtensionIdToObjectIds(const std::string& extension_id) {
Should we add unit tests for this and for ObjectIdToExtensionAndSubchannel?

http://codereview.chromium.org/10826156/diff/15029/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h
(right):

http://codereview.chromium.org/10826156/diff/15029/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:29:
// |service| and |observer| must remain valid for the lifetime of this object.
I don't see a parameter named |observer| - is this comment out of date?

[dcheng, 2012-08-20 22:45:28]

http://codereview.chromium.org/10826156/diff/15029/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
(right):

http://codereview.chromium.org/10826156/diff/15029/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:27:
syncer::ObjectIdSet ExtensionIdToObjectIds(const std::string& extension_id) {
On 2012/08/20 22:41:05, Pete Williamson wrote:
> Should we add unit tests for this and for ObjectIdToExtensionAndSubchannel?

This is tested through the RegisterUnregister and Dispatch tests. I think it's
probably not worth it to break it down into smaller tests in this case.

http://codereview.chromium.org/10826156/diff/15029/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h
(right):

http://codereview.chromium.org/10826156/diff/15029/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h:29:
// |service| and |observer| must remain valid for the lifetime of this object.
On 2012/08/20 22:41:05, Pete Williamson wrote:
> I don't see a parameter named |observer| - is this comment out of date?

Done.

[akalin, 2012-08-21 21:01:38]

sync lgtm

http://codereview.chromium.org/10826156/diff/11097/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
(right):

http://codereview.chromium.org/10826156/diff/11097/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:58:
if (components[0] != "U") {
didn't check the size?

[Mihai Parparita -not on Chrome, 2012-08-21 22:24:10]

LGTM

[dcheng, 2012-08-21 22:56:43]

+ben for OWNERS review of gyp changes.

[Nico, 2012-08-22 00:52:02]

gyp files lgtm stamp, but one question below

http://codereview.chromium.org/10826156/diff/19002/chrome/chrome_browser_exte...
File chrome/chrome_browser_extensions.gypi (right):

http://codereview.chromium.org/10826156/diff/19002/chrome/chrome_browser_exte...
chrome/chrome_browser_extensions.gypi:609: # less intertwined in the main
codebase.
Are you sure you need to add your file below? Sounds like this list is supposed
to grow shorter, not longer.

[dcheng, 2012-08-22 00:59:59]

http://codereview.chromium.org/10826156/diff/19002/chrome/chrome_browser_exte...
File chrome/chrome_browser_extensions.gypi (right):

http://codereview.chromium.org/10826156/diff/19002/chrome/chrome_browser_exte...
chrome/chrome_browser_extensions.gypi:609: # less intertwined in the main
codebase.
On 2012/08/22 00:52:02, Nico wrote:
> Are you sure you need to add your file below? Sounds like this list is
supposed
> to grow shorter, not longer.

It's needed to make the Android bot happy. =/

[commit-bot: I haz the power, 2012-08-22 02:35:17]

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

[commit-bot: I haz the power, 2012-08-22 02:35:34]

Failed to apply patch for
chrome/test/data/extensions/api_test/push_messaging/background.js:
While running svn propset svn:eol-style LF
chrome/test/data/extensions/api_test/push_messaging/background.js --config-dir
/b/commit-queue/subversion_config --non-interactive;
svn: File 'chrome/test/data/extensions/api_test/push_messaging/background.js'
has binary mime type property

[commit-bot: I haz the power, 2012-08-22 05:59:04]

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

[commit-bot: I haz the power, 2012-08-22 06:23:54]

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

[commit-bot: I haz the power, 2012-08-22 06:30:50]

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

[commit-bot: I haz the power, 2012-08-22 08:20:36]

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

[commit-bot: I haz the power, 2012-08-22 11:46:40]

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

[commit-bot: I haz the power, 2012-08-22 14:12:27]

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

[Ben Goodger (Google), 2012-08-22 16:05:12]

gyp lgtm

[commit-bot: I haz the power, 2012-08-22 17:32:30]

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

[commit-bot: I haz the power, 2012-08-22 18:40:31]

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

[commit-bot: I haz the power, 2012-08-22 21:09:38]

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

[commit-bot: I haz the power, 2012-08-22 23:55:17]

Change committed as 152877