Provide a mechanism for the GCM driver to send message receipts to GCM.

Provide a mechanism for the GCM driver to send message receipts to GCM.

As part of an effort to provide better message tracking for GCM messages,
this CL establishes a basic system for sending receipts to GCM. The
SendMessageReceipt is implemented on the GCMDriver and will send back an
integer representing a status, which GCM can use to aggregate data
about final message results.

This CL also implements receipts for the various GCM level failures that
messages could encounter. A future CL will provide a callback to send
receipts to GCMAppHandlers so that receipts can provide full end-to-end
visibility into errors and successes.

BUG=674131

[harkness, 2016-12-14 14:12:54]

harkness@chromium.org changed reviewers:
+ peter@chromium.org, zea@chromium.org

[harkness, 2016-12-14 14:12:54]

Not 100% done. I still need to connect the SendMessageReceiptCallback and it
needs a lot more tests, but if you want an early look at the implementation,
here it is.

I don't particularly like the enum being in GCMClient, but it seemed like the
only place to put it that both driver and client could access.

Thoughts?

[Peter Beverloo, 2016-12-15 19:24:25]

Some initial feedback - thanks for doing this!

At a higher level, we should not land this until we've determined the opt-in.
(Because we'd start sending data following this CL.) I have some ideas that boil
down to a combination of Finch and a GCM-controlled kill switch, let's chat :-).

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/commo...
File components/gcm_driver/common/gcm_messages.h (right):

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/commo...
components/gcm_driver/common/gcm_messages.h:40: std::string id;
I'd very much prefer not to expose the message ID if we can avoid it - it's an
entirely internal implementation detail. Is there any way we can bind it to the
callback as opposed to stashing it in here?

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_c...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client.h:75: enum GCMMessageStatus {
Let's move this to its own file. It's not clear to me what usage like the
following means ("for what"):

    GCMClient::GCM_SUCCESS

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client.h:79: // 1 - 99 reserved for GCMClient and
GCMDriver error status.
nit: Let's make sure that we pull this separate from our class separation, and
have comments that can be reused elsewhere. ("The GCM Client"?)

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_c...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client_impl.cc:1411: GCMClient::GCM_INVALID_SUBTYPE);
It's a bit unfortunate that we need to do the GCMClient -> GCMDriver ->
GCMClient hoop. Could we instead have SendMessageReceipt() live in the GCMClient
and propagate the callback to the GCMDriver? (I guess this means that the
callback has to be declared in the same file as the enumeration.)

An additional benefit here is that we can avoid constructing an OutgoingMessage
object for Android, and instead use more specialized handling.

(At least until we need _real_ support for outgoing messages, because now you're
signing yourself up to do that work.)

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_d...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_d...
components/gcm_driver/gcm_driver.cc:27: const char* kMessageReceiptType =
"message_recipt";
recipt -> receipt

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_d...
components/gcm_driver/gcm_driver.cc:30: const char* kReceiptGCMDestinationID =
"1029510549786@google.com";
micro nit:

    const char kFoo[] = "bar";

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_d...
components/gcm_driver/gcm_driver.cc:30: const char* kReceiptGCMDestinationID =
"1029510549786@google.com";
Also what is this number? Would other users of the GCM Driver (e.g. Samsung or
Opera) want to change it?

Would be good to add some docs.

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_d...
components/gcm_driver/gcm_driver.cc:192:
base::Int64ToString(base::Time::NowFromSystemTime().ToInternalValue());
Why is the difference between Now() and NowFromSystemTime() significant?

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_d...
components/gcm_driver/gcm_driver.cc:202: void
GCMDriver::SendMessageReceiptCallback(const std::string& message_id,
unused

[harkness, 2016-12-21 17:23:38]

Very im progress work at the moment. This is to give Peter a concrete view of
the threading question I sent him.

Functionally, the gcm_client_impl_unittests are passing, but there's a threading
issue with fake_gcm_client (separate from general gcm_client threading
questions) which is breaking one of the gcm_driver_desktop_unittests.

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/commo...
File components/gcm_driver/common/gcm_messages.h (right):

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/commo...
components/gcm_driver/common/gcm_messages.h:40: std::string id;
On 2016/12/15 19:24:24, Peter Beverloo wrote:
> I'd very much prefer not to expose the message ID if we can avoid it - it's an
> entirely internal implementation detail. Is there any way we can bind it to
the
> callback as opposed to stashing it in here?

Now that the callback is created in the GCMClient, I could remove this.

Done.

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_c...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client.h:75: enum GCMMessageStatus {
On 2016/12/15 19:24:24, Peter Beverloo (OOO - Jan 3rd) wrote:
> Let's move this to its own file. It's not clear to me what usage like the
> following means ("for what"):
> 
>     GCMClient::GCM_SUCCESS

currently still in the GCMClient, but I'm planning to move it. Next upload.

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client.h:79: // 1 - 99 reserved for GCMClient and
GCMDriver error status.
On 2016/12/15 19:24:24, Peter Beverloo (OOO - Jan 3rd) wrote:
> nit: Let's make sure that we pull this separate from our class separation, and
> have comments that can be reused elsewhere. ("The GCM Client"?)

Done.

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_c...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client_impl.cc:1411: GCMClient::GCM_INVALID_SUBTYPE);
On 2016/12/15 19:24:24, Peter Beverloo (OOO - Jan 3rd) wrote:
> It's a bit unfortunate that we need to do the GCMClient -> GCMDriver ->
> GCMClient hoop. Could we instead have SendMessageReceipt() live in the
GCMClient
> and propagate the callback to the GCMDriver? (I guess this means that the
> callback has to be declared in the same file as the enumeration.)
> 
> An additional benefit here is that we can avoid constructing an
OutgoingMessage
> object for Android, and instead use more specialized handling.
> 
> (At least until we need _real_ support for outgoing messages, because now
you're
> signing yourself up to do that work.)

I've mostly moved things to the GCMClient now. It does simplify things to not
need to pass them through the Delegate.

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_d...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_d...
components/gcm_driver/gcm_driver.cc:27: const char* kMessageReceiptType =
"message_recipt";
On 2016/12/15 19:24:24, Peter Beverloo wrote:
> recipt -> receipt

Done.

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_d...
components/gcm_driver/gcm_driver.cc:30: const char* kReceiptGCMDestinationID =
"1029510549786@google.com";
On 2016/12/15 19:24:24, Peter Beverloo wrote:
> micro nit:
> 
>     const char kFoo[] = "bar";

Done.

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_d...
components/gcm_driver/gcm_driver.cc:30: const char* kReceiptGCMDestinationID =
"1029510549786@google.com";
On 2016/12/15 19:24:24, Peter Beverloo (OOO - Jan 3rd) wrote:
> Also what is this number? Would other users of the GCM Driver (e.g. Samsung or
> Opera) want to change it?
> 
> Would be good to add some docs.

Done.

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_d...
components/gcm_driver/gcm_driver.cc:192:
base::Int64ToString(base::Time::NowFromSystemTime().ToInternalValue());
On 2016/12/15 19:24:24, Peter Beverloo (OOO - Jan 3rd) wrote:
> Why is the difference between Now() and NowFromSystemTime() significant?

I used it for consistency with Send(), but according to the documentation in
time.h, it's also more reliable for unit tests. However, the actual
implementation for NowFromSystemTime() just calls Now(), so it seems like the
comments in time.h are just out of date.

I can change it if you prefer, leave it for consistency, or change both here and
Send().  zea, is there any background on why the code doesn't use Now()?

https://codereview.chromium.org/2578583002/diff/1/components/gcm_driver/gcm_d...
components/gcm_driver/gcm_driver.cc:202: void
GCMDriver::SendMessageReceiptCallback(const std::string& message_id,
On 2016/12/15 19:24:24, Peter Beverloo (OOO - Jan 3rd) wrote:
> unused

Yup, this was the callback stuff that I mentioned was incomplete. I've removed
it for now, although this raised the issue that I've been questioning Costin
about.

[harkness, 2017-01-10 14:00:59]

Ready for code review now please.  :)

[Peter Beverloo, 2017-01-13 01:46:12]

Thanks, this is heading in a good direction :).

Jen, could you make sure that Nicolas is included on all communication with the
GCM team on this?

Nicolas, do you have an opinion on whether it's a good idea to use
BindToCurrentThread here? In short, the callback is needed on the IO thread for
GCMClientImpl and in the UI thread for GCMDriverAndroid. It's not reasonable to
ask app handlers to deal with this.

The alternative is to always bind to GCMDriver, which lives on the UI thread,
avoiding this problem at the cost of some additional plumbing. What do you think
about that, Jen?

https://codereview.chromium.org/2578583002/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_unittest.cc (right):

https://codereview.chromium.org/2578583002/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_unittest.cc:253: //
messages,
nit: weird linebreak?

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client.h:143: typedef
base::Callback<void(GCMMessageStatus)> MessageReceiptCallback;
nit: C++11ize

using MessageReceiptCallback = base::Callback<void(GCMMessageStatus)>;

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client.h:143: typedef
base::Callback<void(GCMMessageStatus)> MessageReceiptCallback;
Architecturally, why did you choose to keep this in the GCMClient rather than
defining it in gcm_message_status.h?

My thinking is that users of receipt callbacks will need to include
gcm_message_status.h anyway. That way we can avoid having to include
gcm_client.h, which really is more of an implementation detail. (Given that it
doesn't exist on Android, for example.)

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client.h:182: const MessageReceiptCallback& callback)
= 0;
(This will largely apply to app handlers, but it's important to document it
here, too.)

App handlers are certainly not required to invoke |callback|. Delivery receipts
should only be used by feature teams who would benefit from having additional
information available. That should be reflected in both the documentation
(:179), as well as the parameter name (:182).

I'd suggest using |receipt_callback| for the parameter name, although
|optional_receipt_callback| would work too if you're not afraid of long names.

In addition, please also document the threading guarantees.

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client.h:289: GCMMessageStatus status) = 0;
This doesn't have to live in the gcm_client.h interface, and can instead live as
a private method on GCMClientImpl.

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:30: #include
"components/gcm_driver/gcm_message_status.h"
This file is now included trice - once in gcm_client.h, once in
gcm_client_impl.h and one in the .cc file. Once will do. (Also see the comment
in that file.)

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:59: // upstream receipt messages.
Who owns it? Where does the value come from? Maybe there's an e-mail address we
can include here?

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:113:
base::Bind(&GCMDriverAndroid::SendMessageReceipt,
Why isn't this BindToCurrentThread? If we're telling AppHandlers that it's OK to
invoke the callback on any thread, we need to be consistent about that.

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:114: weak_ptr_factory_.GetWeakPtr(),
app_id, ""));
Why the empty string for |message_id|? Can we just not pass it until it's
available?

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:120: // TODO(harkness) Implement
message receipts in android.
nit: please mention a bug

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_message_status.h (right):

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_message_status.h:11: enum GCMMessageStatus {
Can this be an "enum class"? That would allow us to forward declare it in the
other header files, allowing this file only to be included where it's actually
needed.

[Nicolas Zea, 2017-01-18 01:13:05]

On 2017/01/13 01:46:12, Peter Beverloo (OOO- Jan 19th) wrote:
> Thanks, this is heading in a good direction :).
> 
> Jen, could you make sure that Nicolas is included on all communication with
the
> GCM team on this?
> 
> Nicolas, do you have an opinion on whether it's a good idea to use
> BindToCurrentThread here? In short, the callback is needed on the IO thread
for
> GCMClientImpl and in the UI thread for GCMDriverAndroid. It's not reasonable
to
> ask app handlers to deal with this.
> 
> The alternative is to always bind to GCMDriver, which lives on the UI thread,
> avoiding this problem at the cost of some additional plumbing. What do you
think
> about that, Jen?
> 

I'm a bit hesitant to add this dependency on Sync. Ideally GCM would be
completely independent from Sync (the dependency that exists today should
probably go away).

Binding to GCMDriver does seem like a cleaner abstraction. Alternatively, moving
bind_to_task_runner into base/ might make sense (note that the one in Sync is
just a copy of the one in src/chromecast). Maybe now that it's needed in three
places it makes more sense?

[harkness, 2017-01-19 13:20:42]

New patch responded to previous comments.

Re BindToCurrentThread: I definitely feel like BindToCurrentThread is really
nifty functionality. The fact that three bits of code now all want to use it
makes it feel like something that should be in base. I'm happy to move it to
base as a separate CL either before or after I land this one.

https://codereview.chromium.org/2578583002/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_unittest.cc (right):

https://codereview.chromium.org/2578583002/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_unittest.cc:253: //
messages,
On 2017/01/13 01:46:11, Peter Beverloo (OOO- Jan 19th) wrote:
> nit: weird linebreak?

Done.

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client.h:143: typedef
base::Callback<void(GCMMessageStatus)> MessageReceiptCallback;
On 2017/01/13 01:46:11, Peter Beverloo (OOO- Jan 19th) wrote:
> Architecturally, why did you choose to keep this in the GCMClient rather than
> defining it in gcm_message_status.h?
> 
> My thinking is that users of receipt callbacks will need to include
> gcm_message_status.h anyway. That way we can avoid having to include
> gcm_client.h, which really is more of an implementation detail. (Given that it
> doesn't exist on Android, for example.)

The main architectural reason was to have it in the same file that defined
SendMessageReceipt, so if someone was reading that code and wanted to look up
the definition, it would be right there.

I did a bit of looking, and I think all the files which need the definition
already had included gcm_client.h because of other reasons  (often
GCMClient::Result), and actually there are a lot of files that just passively
pass the callback through without needing to know the message status enum.

I do think it makes more sense in the gcm_client.h, but I don't feel super
strongly about it so if you want I can move it. Your Android point is a good
one.

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client.h:143: typedef
base::Callback<void(GCMMessageStatus)> MessageReceiptCallback;
On 2017/01/13 01:46:11, Peter Beverloo (OOO- Jan 19th) wrote:
> nit: C++11ize
> 
> using MessageReceiptCallback = base::Callback<void(GCMMessageStatus)>;

Done.

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client.h:182: const MessageReceiptCallback& callback)
= 0;
On 2017/01/13 01:46:11, Peter Beverloo (OOO- Jan 19th) wrote:
> (This will largely apply to app handlers, but it's important to document it
> here, too.)
> 
> App handlers are certainly not required to invoke |callback|. Delivery
receipts
> should only be used by feature teams who would benefit from having additional
> information available. That should be reflected in both the documentation
> (:179), as well as the parameter name (:182).
> 
> I'd suggest using |receipt_callback| for the parameter name, although
> |optional_receipt_callback| would work too if you're not afraid of long names.
> 
> In addition, please also document the threading guarantees.

Done.

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client.h:289: GCMMessageStatus status) = 0;
On 2017/01/13 01:46:11, Peter Beverloo (OOO- Jan 19th) wrote:
> This doesn't have to live in the gcm_client.h interface, and can instead live
as
> a private method on GCMClientImpl.

Good point! I've moved it.

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:30: #include
"components/gcm_driver/gcm_message_status.h"
On 2017/01/13 01:46:12, Peter Beverloo (OOO- Jan 19th) wrote:
> This file is now included trice - once in gcm_client.h, once in
> gcm_client_impl.h and one in the .cc file. Once will do. (Also see the comment
> in that file.)

Once I changed it to enum class, it's only included once and forward declared in
the other cases.

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:59: // upstream receipt messages.
On 2017/01/13 01:46:12, Peter Beverloo (OOO- Jan 19th) wrote:
> Who owns it? Where does the value come from? Maybe there's an e-mail address
we
> can include here?

I just asked Pinar and she generated the receiver ID and gave it to me. I'm not
sure what information you want in the comment? Would adding "Generated by GCM
developers" be appropriate?

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:113:
base::Bind(&GCMDriverAndroid::SendMessageReceipt,
On 2017/01/13 01:46:12, Peter Beverloo (OOO- Jan 19th) wrote:
> Why isn't this BindToCurrentThread? If we're telling AppHandlers that it's OK
to
> invoke the callback on any thread, we need to be consistent about that.

Done.

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:114: weak_ptr_factory_.GetWeakPtr(),
app_id, ""));
On 2017/01/13 01:46:12, Peter Beverloo (OOO- Jan 19th) wrote:
> Why the empty string for |message_id|? Can we just not pass it until it's
> available?

Sure, that's a good idea. Done.

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:120: // TODO(harkness) Implement
message receipts in android.
On 2017/01/13 01:46:12, Peter Beverloo (OOO- Jan 19th) wrote:
> nit: please mention a bug

Done.

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_message_status.h (right):

https://codereview.chromium.org/2578583002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_message_status.h:11: enum GCMMessageStatus {
On 2017/01/13 01:46:12, Peter Beverloo (OOO- Jan 19th) wrote:
> Can this be an "enum class"? That would allow us to forward declare it in the
> other header files, allowing this file only to be included where it's actually
> needed.

Done.

[Nicolas Zea, 2017-01-20 18:14:11]

Would probably be good to start a thread with some of the base owners about
adding bind_to_taskrunner. I'm fine with having it included here for now
(although would be good to add a TODO comment in the DEPS file explaining that
it's only temporary.

[harkness, 2017-02-07 15:15:10]

Peter and I discussed the bind_to_taskrunner, and he remembered that there had
been issues with making it more generally accessible, so I added a callback
entry point to GCMDriver. That callback will always need to be called on the UI
thread, and then a task is posted to the GCMDriver::IOWorker to make the call
into the GCMClient.

[Peter Beverloo, 2017-02-08 17:09:09]

https://codereview.chromium.org/2578583002/diff/120001/chrome/browser/service...
File chrome/browser/services/gcm/fake_gcm_profile_service.h (right):

https://codereview.chromium.org/2578583002/diff/120001/chrome/browser/service...
chrome/browser/services/gcm/fake_gcm_profile_service.h:13: #include
"components/gcm_driver/gcm_client.h"
note: you'll get some interesting merge conflicts w/
https://codereview.chromium.org/2675293003/

https://codereview.chromium.org/2578583002/diff/120001/chrome/browser/service...
chrome/browser/services/gcm/fake_gcm_profile_service.h:50: const
GCMClient::MessageReceiptCallback& callback);
Please use |receipt_callback| instead of |callback| for MessageReceiptCallback
everywhere.

https://codereview.chromium.org/2578583002/diff/120001/chrome/browser/service...
chrome/browser/services/gcm/fake_gcm_profile_service.h:50: const
GCMClient::MessageReceiptCallback& callback);
Did you compile this? MessageReceiptCallback is not a type of the GCMClient
class... (There's various occurrences of this.)

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/fake_gcm_client.cc (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/fake_gcm_client.cc:248:
base::Bind(&FakeGCMClient::SendMessageReceipt,
nit: generally we have a mild preference of writing nested base::Bind()
callbacks to a local, because it significantly reduces nesting.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/fake_gcm_client.h (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/fake_gcm_client.h:101: GCMMessageStatus receipt_status() {
return receipt_status_; }
nit: these methods can be const

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/fake_gcm_driver.h (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/fake_gcm_driver.h:67: void DoSendMessageReceipt(const
MessageReceiptCallback& callback,
// GCMDriver implementation:

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_account_mapper.h (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_account_mapper.h:37: DispatchMessageCallback;
nit: I know you're just passing by, but mind updating these typedefs to the
newer `using` syntax?

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_client.h:180: // run on the UI thread.
[The callback can...the UI thread]

That seems wrong. The GCM Client lives on the IO thread, and since it uses
Send() it *must* be invoked on the IO thread.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:60: const int kReceiptTTLInSeconds =
10;
micro nit: unify with the constant block around lines 93-105

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1428:
GCMMessageStatus::GCM_INVALID_SUBTYPE);
INVALID_SUBTYPE is not really accurate - INVALID_REGISTRATION would be better,
since that's what |registered| actually represents. Alternatively you need to
consider the pair of [was_subtype, registered, registration type (v3/IID)] to
choose the appropriate value.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1498: DCHECK(!app_id.empty());
Could |app_id| (or |message_id|, which should get the same treatment) be the
empty string at this point because of a server error? If so, we should silently
bail out (maybe w/ LOG(ERROR)) instead of crashing the browser.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1500: // Prepare a message to send to
GCM which will log the status of the received
What do you think about guarding this behind a finch roll-out? We can have three
settings -

  1) No receipts.
  2) Only unsuccessful receipts.
  3) All receipts.

That'll allow us to roll the feature out progressively and give us a kill switch
at the same time.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1505:
base::Int64ToString(base::Time::NowFromSystemTime().ToInternalValue());
How was this decided? base::Time stores milliseconds, so collisions are entirely
possible here. (Unlike other scheduled sub-systems such as heartbeat.)

Should we use a more unique value? base::GenerateGUID() can be your friend.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.h (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.h:159: virtual void
SendMessageReceipt(const std::string& message_id,
This shouldn't be declared as `virtual` - nothing overrides it.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_driver.cc:16: #include
"components/gcm_driver/gcm_message_status.h"
nit: this is already included in the header

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_driver.cc:311: DoSendMessageReceipt(callback,
GCMMessageStatus::GCM_ENCRYPTION_FAILURE);
I'd argue that this should just be SendMessageReceipt() that lives on the UI
thread.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_driver.h:325: GCMMessageStatus status) = 0;
Instead of defining this in the base class (GCMDriver), let's just define it
twice in the implementation classes (GCMDriver{Android,Desktop}) with the
signatures they need?

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_driver_android.cc:127: NOTIMPLEMENTED();
nit: drop the NOTIMPLEMENTED() -- that'll just spam the console

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_driver_android.h (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_driver_android.h:102: GCMMessageSatus status);
nit: this won't compile (missing `override`). I hope we can merge these two
methods in either case.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_driver_desktop.cc:1238: DispatchMessage(app_id,
message, callback);
The |callback| passed to DispatchMessage any beyond *must* be invoked on the UI
thread. The one that's being passed now *must* be invoked on the IO thread.

The simplest path here would be to -

GCMDriverDesktop
    void SendMessageReceipt(
        const MessageReceiptCallback& io_receipt_callback,
        GCMMessageStatus status);

--> DispatchMessage(..., base::Bind(&SendMessageReceipt, callback))

GCMDriverAndroid
    void SendMessageReceipt(
        const std::string& message_id,
        const std::string& app_id,
        GCMMessageStatus status);

--> DispatchMessage(..., base::Bind(&SendMessageReceipt, message_id,
                                    app_id))

That way you don't need DoSendMessageReceipt either, since SendMessageReceipt
can take care of posting the task if needs be (desktop).

[harkness, 2017-02-09 16:27:30]

Threading and nits taken care of. Finch coming soon.

https://codereview.chromium.org/2578583002/diff/120001/chrome/browser/service...
File chrome/browser/services/gcm/fake_gcm_profile_service.h (right):

https://codereview.chromium.org/2578583002/diff/120001/chrome/browser/service...
chrome/browser/services/gcm/fake_gcm_profile_service.h:13: #include
"components/gcm_driver/gcm_client.h"
On 2017/02/08 17:09:08, Peter Beverloo wrote:
> note: you'll get some interesting merge conflicts w/
> https://codereview.chromium.org/2675293003/

Acknowledged.

https://codereview.chromium.org/2578583002/diff/120001/chrome/browser/service...
chrome/browser/services/gcm/fake_gcm_profile_service.h:50: const
GCMClient::MessageReceiptCallback& callback);
On 2017/02/08 17:09:08, Peter Beverloo wrote:
> Did you compile this? MessageReceiptCallback is not a type of the GCMClient
> class... (There's various occurrences of this.)

For my iterations, I've just been compiling components_unittests, I'd forgotten
about the changes in chrome/browser. I'll update those.

https://codereview.chromium.org/2578583002/diff/120001/chrome/browser/service...
chrome/browser/services/gcm/fake_gcm_profile_service.h:50: const
GCMClient::MessageReceiptCallback& callback);
On 2017/02/08 17:09:08, Peter Beverloo wrote:
> Please use |receipt_callback| instead of |callback| for MessageReceiptCallback
> everywhere.

Done.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/fake_gcm_client.cc (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/fake_gcm_client.cc:248:
base::Bind(&FakeGCMClient::SendMessageReceipt,
On 2017/02/08 17:09:08, Peter Beverloo wrote:
> nit: generally we have a mild preference of writing nested base::Bind()
> callbacks to a local, because it significantly reduces nesting.

Done.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/fake_gcm_client.h (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/fake_gcm_client.h:101: GCMMessageStatus receipt_status() {
return receipt_status_; }
On 2017/02/08 17:09:08, Peter Beverloo wrote:
> nit: these methods can be const

Done.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/fake_gcm_driver.h (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/fake_gcm_driver.h:67: void DoSendMessageReceipt(const
MessageReceiptCallback& callback,
On 2017/02/08 17:09:08, Peter Beverloo wrote:
> // GCMDriver implementation:

Done.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_account_mapper.h (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_account_mapper.h:37: DispatchMessageCallback;
On 2017/02/08 17:09:09, Peter Beverloo wrote:
> nit: I know you're just passing by, but mind updating these typedefs to the
> newer `using` syntax?

Done.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_client.h:180: // run on the UI thread.
On 2017/02/08 17:09:09, Peter Beverloo wrote:
> [The callback can...the UI thread]
> 
> That seems wrong. The GCM Client lives on the IO thread, and since it uses
> Send() it *must* be invoked on the IO thread.

Yup, I updated the code without updating the comment. I'll fix that.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:60: const int kReceiptTTLInSeconds =
10;
On 2017/02/08 17:09:09, Peter Beverloo wrote:
> micro nit: unify with the constant block around lines 93-105

Done.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1428:
GCMMessageStatus::GCM_INVALID_SUBTYPE);
On 2017/02/08 17:09:09, Peter Beverloo wrote:
> INVALID_SUBTYPE is not really accurate - INVALID_REGISTRATION would be better,
> since that's what |registered| actually represents. Alternatively you need to
> consider the pair of [was_subtype, registered, registration type (v3/IID)] to
> choose the appropriate value.

Thanks for reminding me, I was planning to rewrite this and add a lot more
values, but I completely forgot. Updated now.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1498: DCHECK(!app_id.empty());
On 2017/02/08 17:09:09, Peter Beverloo wrote:
> Could |app_id| (or |message_id|, which should get the same treatment) be the
> empty string at this point because of a server error? If so, we should
silently
> bail out (maybe w/ LOG(ERROR)) instead of crashing the browser.

There are checks in some of the GCMDriver code, but those are all places where
the call was coming from outside the component, so it's reasonable to check. In
this case, it's always coming from GCM, so it shouldn't ever be empty. I've
removed the check.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1500: // Prepare a message to send to
GCM which will log the status of the received
On 2017/02/08 17:09:09, Peter Beverloo wrote:
> What do you think about guarding this behind a finch roll-out? We can have
three
> settings -
> 
>   1) No receipts.
>   2) Only unsuccessful receipts.
>   3) All receipts.
> 
> That'll allow us to roll the feature out progressively and give us a kill
switch
> at the same time.

Sounds like a good idea. I'll upload the patch set with all the other changes
and then upload another one with Finch changes.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1505:
base::Int64ToString(base::Time::NowFromSystemTime().ToInternalValue());
On 2017/02/08 17:09:09, Peter Beverloo wrote:
> How was this decided? base::Time stores milliseconds, so collisions are
entirely
> possible here. (Unlike other scheduled sub-systems such as heartbeat.)
> 
> Should we use a more unique value? base::GenerateGUID() can be your friend.

I'm using the time because that's the way GCMClientImpl::Send does it, and I
thought that it would be most important to be unique between the types of
messages that are being sent to GCM.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.h (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.h:159: virtual void
SendMessageReceipt(const std::string& message_id,
On 2017/02/08 17:09:09, Peter Beverloo wrote:
> This shouldn't be declared as `virtual` - nothing overrides it.

Done.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_driver.cc:16: #include
"components/gcm_driver/gcm_message_status.h"
On 2017/02/08 17:09:09, Peter Beverloo wrote:
> nit: this is already included in the header

Done.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_driver.cc:311: DoSendMessageReceipt(callback,
GCMMessageStatus::GCM_ENCRYPTION_FAILURE);
On 2017/02/08 at 17:09:09, Peter Beverloo wrote:
> I'd argue that this should just be SendMessageReceipt() that lives on the UI
thread.

Now it just invokes the callback.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_driver.h:325: GCMMessageStatus status) = 0;
On 2017/02/08 at 17:09:09, Peter Beverloo wrote:
> Instead of defining this in the base class (GCMDriver), let's just define it
twice in the implementation classes (GCMDriver{Android,Desktop}) with the
signatures they need?

Done.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_driver_android.cc:127: NOTIMPLEMENTED();
On 2017/02/08 17:09:09, Peter Beverloo wrote:
> nit: drop the NOTIMPLEMENTED() -- that'll just spam the console

Done.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_driver_android.h (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_driver_android.h:102: GCMMessageSatus status);
On 2017/02/08 at 17:09:09, Peter Beverloo wrote:
> nit: this won't compile (missing `override`). I hope we can merge these two
methods in either case.

Weird that this one isn't included in the components_unitttests target.

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/2578583002/diff/120001/components/gcm_driver/...
components/gcm_driver/gcm_driver_desktop.cc:1238: DispatchMessage(app_id,
message, callback);
On 2017/02/08 17:09:09, Peter Beverloo wrote:
> The |callback| passed to DispatchMessage any beyond *must* be invoked on the
UI
> thread. The one that's being passed now *must* be invoked on the IO thread.
> 
> The simplest path here would be to -
> 
> GCMDriverDesktop
>     void SendMessageReceipt(
>         const MessageReceiptCallback& io_receipt_callback,
>         GCMMessageStatus status);
> 
> --> DispatchMessage(..., base::Bind(&SendMessageReceipt, callback))
> 
> GCMDriverAndroid
>     void SendMessageReceipt(
>         const std::string& message_id,
>         const std::string& app_id,
>         GCMMessageStatus status);
> 
> --> DispatchMessage(..., base::Bind(&SendMessageReceipt, message_id,
>                                     app_id))
> 
> That way you don't need DoSendMessageReceipt either, since SendMessageReceipt
> can take care of posting the task if needs be (desktop).

Good point, I hadn't considered that there was a step in the GCMDriver(Desktop)
where we could intercept that before it got into Dispatch Message.

Moved the re-bind to that level.

[harkness, 2017-02-13 12:08:39]

Finch trial is available. I believe the code is complete at this point.

[Peter Beverloo, 2017-02-13 13:47:08]

Largely LG, two larger items:

  1) Field trial status tuple (see gcm_client_impl.cc)
  2) Remove the dependency on the variations component.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client.h:183: const MessageReceiptCallback&
optional_receipt_callback) = 0;
nit: standardize on (optional_)?receipt_callback naming. Distinguishing between
threads sgtm, but let's use the same otherwise.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:106: const char kGcmMessageTypeKey[] =
"type";
Q: Why id this different from kMessageTypeKey?

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:115: "GCMMessageReceiptsFeature",
base::FEATURE_ENABLED_BY_DEFAULT};
Rather than having a trial that's always enabled and has three values {none,
failures, all}, have you considered having a trial that can be disabled {none}
and enabled w/ a value of {failures, all}?

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1410: << " had a reservation but the
sender_id " << sender
s/reservation/registration/

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1520: int message_types_to_send =
variations::GetVariationParamByFeatureAsInt(
Use base::GetFieldTrialParamByFeatureAsInt() instead, the one you're using has
been marked as deprecated. That also avoids the DEPS change.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1530: return;
nit: we tend to prefer switch() statements so that it's clearer to deal with odd
expected and unexpected values. Otherwise, use {} since this counts as a
multi-line statement. (Due to the condition.)

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.h (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.h:161: GCMMessageStatus status);
nit: types before methods - there's an appropriate method section around line
258 already

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl_unittest.cc:1814: EXPECT_TRUE(0);
FAIL() << "Unexpected key: " << pair.key();

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl_unittest.cc:1893: EXPECT_TRUE(0);
dito

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_driver_desktop.cc:1233: const MessageReceiptCallback&
receipt_callback) {
nit: it'd be a lot clearer if this were called io_receipt_callback to
distinguish from the other receipt_callbacks.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_driver_desktop.cc:1371:
base::Unretained(io_worker_.get()), receipt_callback, status));
You can skip the indirection and just invoke |receipt_callback| directly:

  io_thread_->PostTask(FROM_HERE, base::Bind(receipt_callback, status));

That way you don't need GCMDriverDesktop::IOWorker::SendMessageReceipt either.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_driver_desktop.h (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_driver_desktop.h:188: GCMMessageStatus status);
Please add this around line 157.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_message_status.h (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_message_status.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
2017

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_message_status.h:21: GCM_ENCRYPTION_FAILURE = 5,
nit: *DE*CRYPTION_FAILURE

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_message_status.h:23: GCM_NO_APP_HANDLER = 7,
GCM_INVALID_APP so that it can be generalized to Android too?

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_message_status.h:26: using MessageReceiptCallback =
base::Callback<void(GCMMessageStatus)>;
include //base/callback.h

[harkness, 2017-02-14 19:22:29]

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client.h:183: const MessageReceiptCallback&
optional_receipt_callback) = 0;
On 2017/02/13 13:47:06, Peter Beverloo wrote:
> nit: standardize on (optional_)?receipt_callback naming. Distinguishing
between
> threads sgtm, but let's use the same otherwise.

I decided that I really like "optional" for standardization, even when there are
threading questions. I didn't want to switch back and forth from optional_ to
io_ or something like that.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:106: const char kGcmMessageTypeKey[] =
"type";
On 2017/02/13 13:47:07, Peter Beverloo wrote:
> Q: Why id this different from kMessageTypeKey?

Actually, I don't remember where I got this key from, but you're right they
should be the same.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:115: "GCMMessageReceiptsFeature",
base::FEATURE_ENABLED_BY_DEFAULT};
On 2017/02/13 13:47:06, Peter Beverloo wrote:
> Rather than having a trial that's always enabled and has three values {none,
> failures, all}, have you considered having a trial that can be disabled {none}
> and enabled w/ a value of {failures, all}?

I did consider that, and functionally they're identical. This one just make a
bit more sense to me, but it's purely an aesthetic choice. I can change it if
you prefer.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1410: << " had a reservation but the
sender_id " << sender
On 2017/02/13 13:47:07, Peter Beverloo wrote:
> s/reservation/registration/

Done.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1520: int message_types_to_send =
variations::GetVariationParamByFeatureAsInt(
On 2017/02/13 13:47:06, Peter Beverloo wrote:
> Use base::GetFieldTrialParamByFeatureAsInt() instead, the one you're using has
> been marked as deprecated. That also avoids the DEPS change.

Ah, I wasn't aware of that one. I'll update it.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1530: return;
On 2017/02/13 13:47:07, Peter Beverloo wrote:
> nit: we tend to prefer switch() statements so that it's clearer to deal with
odd
> expected and unexpected values. Otherwise, use {} since this counts as a
> multi-line statement. (Due to the condition.)

I agree in 95% of cases that a switch is better, but because the second if also
checks the status, and because both just return if they're true, I think it
reads better as if statements.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.h (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.h:161: GCMMessageStatus status);
On 2017/02/13 13:47:07, Peter Beverloo wrote:
> nit: types before methods - there's an appropriate method section around line
> 258 already

Done.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl_unittest.cc:1814: EXPECT_TRUE(0);
On 2017/02/13 13:47:07, Peter Beverloo wrote:
> FAIL() << "Unexpected key: " << pair.key();

Done.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl_unittest.cc:1893: EXPECT_TRUE(0);
On 2017/02/13 13:47:07, Peter Beverloo wrote:
> dito

Done.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_driver_desktop.cc:1233: const MessageReceiptCallback&
receipt_callback) {
On 2017/02/13 13:47:07, Peter Beverloo wrote:
> nit: it'd be a lot clearer if this were called io_receipt_callback to
> distinguish from the other receipt_callbacks.

I agree, but I also think everything is better with optional, and I didn't want
to mix things here.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_driver_desktop.cc:1371:
base::Unretained(io_worker_.get()), receipt_callback, status));
On 2017/02/13 13:47:07, Peter Beverloo wrote:
> You can skip the indirection and just invoke |receipt_callback| directly:
> 
>   io_thread_->PostTask(FROM_HERE, base::Bind(receipt_callback, status));
> 
> That way you don't need GCMDriverDesktop::IOWorker::SendMessageReceipt either.

Good point, I'd forgotten that it wasn't being called from anywhere else.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_driver_desktop.h (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_driver_desktop.h:188: GCMMessageStatus status);
On 2017/02/13 13:47:07, Peter Beverloo wrote:
> Please add this around line 157.

Done.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_message_status.h (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_message_status.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2017/02/13 13:47:07, Peter Beverloo wrote:
> 2017

oops

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_message_status.h:21: GCM_ENCRYPTION_FAILURE = 5,
On 2017/02/13 13:47:07, Peter Beverloo wrote:
> nit: *DE*CRYPTION_FAILURE

Done.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_message_status.h:23: GCM_NO_APP_HANDLER = 7,
On 2017/02/13 13:47:07, Peter Beverloo wrote:
> GCM_INVALID_APP so that it can be generalized to Android too?

Done.

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_message_status.h:26: using MessageReceiptCallback =
base::Callback<void(GCMMessageStatus)>;
On 2017/02/13 13:47:07, Peter Beverloo wrote:
> include //base/callback.h

Done.

[harkness, 2017-02-14 19:23:28]

The last patch set also included a rebase, because I needed to pick up the field
trial changes. So some of the recent patches from johnme also show up with
changes in the files.

[Peter Beverloo, 2017-02-15 15:17:16]

lgtm % testing to make sure that the disabled field trial actually bails out

I'd also like to ask you to get a third opinion from the metrics team (isherman@
perhaps?) regarding the field trial's option distribution.

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
File components/gcm_driver/gcm_account_mapper.h (right):

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_account_mapper.h:33: typedef
std::vector<AccountMapping> AccountMappings;
nit: `using` for consistency, line 66 too

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1546: Send(app_id,
kReceiptGCMDestinationID, message);
Does the server ack this? (I.e. do we bail out in
GCMDriverDesktop::SendAcknowledged?)

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl_unittest.cc:488:
scoped_feature_list_.InitWithFeatureList(std::move(feature_list));
Can we have a test for making sure that *disabling* the field trial also works?

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_driver.h:294: // |message| will be dispatched
immediately to the handler for |app_id|.
  // The |optional_receipt_callback| must be invoked on the UI thread.

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
File components/gcm_driver/gcm_message_status.h (right):

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_message_status.h:15: GCM_UNRESOLVED = 0,
Should we DCHECK that this value will never be send to the server?

[harkness, 2017-02-22 17:19:30]

harkness@chromium.org changed reviewers:
+ isherman@chromium.org

[harkness, 2017-02-22 17:19:32]

isherman@ Could you please take a look at the field trial I've established and
give your opinion on whether None, All, and Failures are a reasonable
distribution for the trial?

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
File components/gcm_driver/gcm_account_mapper.h (right):

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_account_mapper.h:33: typedef
std::vector<AccountMapping> AccountMappings;
On 2017/02/15 15:17:15, Peter Beverloo wrote:
> nit: `using` for consistency, line 66 too

Done.

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1546: Send(app_id,
kReceiptGCMDestinationID, message);
On 2017/02/15 15:17:15, Peter Beverloo wrote:
> Does the server ack this? (I.e. do we bail out in
> GCMDriverDesktop::SendAcknowledged?)

At the moment, this will be acknowledged and the AppHandler will be notified of
the SendAcknowledged() call. In general, I think that's a good thing, because in
the case of GCM errors, it lets the AppHandler know that the GCM is trying to
send messages that aren't getting through (for example, because of decryption
errors).

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl_unittest.cc:488:
scoped_feature_list_.InitWithFeatureList(std::move(feature_list));
On 2017/02/15 15:17:15, Peter Beverloo wrote:
> Can we have a test for making sure that *disabling* the field trial also
works?

Done.

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_driver.h:294: // |message| will be dispatched
immediately to the handler for |app_id|.
On 2017/02/15 15:17:15, Peter Beverloo wrote:
>   // The |optional_receipt_callback| must be invoked on the UI thread.

Done.

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
File components/gcm_driver/gcm_message_status.h (right):

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_message_status.h:15: GCM_UNRESOLVED = 0,
On 2017/02/15 15:17:15, Peter Beverloo wrote:
> Should we DCHECK that this value will never be send to the server?

Good idea. Done.

[Peter Beverloo, 2017-02-22 17:24:24]

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2578583002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1546: Send(app_id,
kReceiptGCMDestinationID, message);
On 2017/02/22 17:19:32, harkness wrote:
> On 2017/02/15 15:17:15, Peter Beverloo wrote:
> > Does the server ack this? (I.e. do we bail out in
> > GCMDriverDesktop::SendAcknowledged?)
> 
> At the moment, this will be acknowledged and the AppHandler will be notified
of
> the SendAcknowledged() call. In general, I think that's a good thing, because
in
> the case of GCM errors, it lets the AppHandler know that the GCM is trying to
> send messages that aren't getting through (for example, because of decryption
> errors).
> 

That is not OK per the contract of OnSendAcknowledged. Please figure out a
solution here --

PushMessagingServiceImpl will DCHECK(false).
CryptAuthGCMManagerImpl will DCHECK(false).
ExtensionGCMAppHandler will ignore.
GCMAccountMapper will drop the message because there'll be an ID mismatch.
Fragile, though.
GCMInvalidationBridge will DCHECK(false).

[harkness, 2017-02-22 17:28:48]

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 17:29:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Beverloo, 2017-02-22 17:32:09]

On 2017/02/22 17:24:24, Peter Beverloo wrote:
> That is not OK per the contract of OnSendAcknowledged. Please figure out a
> solution here --
> 
> PushMessagingServiceImpl will DCHECK(false).
> CryptAuthGCMManagerImpl will DCHECK(false).
> ExtensionGCMAppHandler will ignore.
> GCMAccountMapper will drop the message because there'll be an ID mismatch.
> Fragile, though.
> GCMInvalidationBridge will DCHECK(false).

Two ideas:
  1) Is there really no flag that'll tell the GCM server to not ack? :(
  2) Emphasize that we really need a flag - hack around it by using some ignore
prefix/suffix in the message ID until there is. (Which then should be ASAP.)

[commit-bot: I haz the power, 2017-02-22 17:45:07]

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

[commit-bot: I haz the power, 2017-02-22 17:45:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[harkness, 2017-02-22 18:01:12]

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 18:01:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[harkness, 2017-02-22 18:02:10]

On 2017/02/22 17:32:09, Peter Beverloo wrote:
> On 2017/02/22 17:24:24, Peter Beverloo wrote:
> > That is not OK per the contract of OnSendAcknowledged. Please figure out a
> > solution here --
> > 
> > PushMessagingServiceImpl will DCHECK(false).
> > CryptAuthGCMManagerImpl will DCHECK(false).
> > ExtensionGCMAppHandler will ignore.
> > GCMAccountMapper will drop the message because there'll be an ID mismatch.
> > Fragile, though.
> > GCMInvalidationBridge will DCHECK(false).
> 
> Two ideas:
>   1) Is there really no flag that'll tell the GCM server to not ack? :(
>   2) Emphasize that we really need a flag - hack around it by using some
ignore
> prefix/suffix in the message ID until there is. (Which then should be ASAP.)

I'll investigate and discuss it with Pinar tomorrow.

[commit-bot: I haz the power, 2017-02-22 18:22:34]

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

[commit-bot: I haz the power, 2017-02-22 18:22:35]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Ilya Sherman, 2017-02-22 23:17:02]

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2578583002/diff/180001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:115: "GCMMessageReceiptsFeature",
base::FEATURE_ENABLED_BY_DEFAULT};
On 2017/02/14 19:22:28, harkness wrote:
> On 2017/02/13 13:47:06, Peter Beverloo wrote:
> > Rather than having a trial that's always enabled and has three values {none,
> > failures, all}, have you considered having a trial that can be disabled
{none}
> > and enabled w/ a value of {failures, all}?
> 
> I did consider that, and functionally they're identical. This one just make a
> bit more sense to me, but it's purely an aesthetic choice. I can change it if
> you prefer.

Either approach is okay from a functionality perspective, so I'll defer to your
team on which setup you prefer.  I do agree with Peter that if I were writing
this code, I'd probably treat "none" as a "disabled" state rather than as an
"enabled" state.