[GCM] Unregistration request

[GCM] Unregistration request

Wrapper over URL fetcher allowing for making request to remove registration of apps from the GCM. It pretends to be an API call instead of GCM client infrastructure code on purpose, to omit certain server side logic.

Patch includes:
* Implementation of the request using a URL fetcher with delete request
* Parsing of the response, with error handling
* triggering of the retry logic based on the provided backoff policy and when the last error allows to retry
* UMA logging of the request result.

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

[fgorski, 2014-02-13 22:57:34]

asvitkine@chromium.org: Please review changes in 
histograms

zea@chromium.org: Please review changes in 
everything else

juyik@chromium.org: Please take a look at UMA related code.

[juyik, 2014-02-14 03:26:04]

On 2014/02/13 22:57:34, Filip Gorski wrote:
> asvitkine@chromium.org: Please review changes in 
> histograms
> 
> zea@chromium.org: Please review changes in 
> everything else
> 
> juyik@chromium.org: Please take a look at UMA related code.

These 2 status entries: INTERNAL_SERVER_ERROR and HTTP_NOT_OK, have different
ordering in unregistration_request.cc compared to histograms.xml.

[fgorski, 2014-02-14 03:57:47]

On 2014/02/14 03:26:04, juyik wrote:
> On 2014/02/13 22:57:34, Filip Gorski wrote:
> > asvitkine@chromium.org: Please review changes in 
> > histograms
> > 
> > zea@chromium.org: Please review changes in 
> > everything else
> > 
> > juyik@chromium.org: Please take a look at UMA related code.
> 
> These 2 status entries: INTERNAL_SERVER_ERROR and HTTP_NOT_OK, have different
> ordering in unregistration_request.cc compared to histograms.xml.

Thanks. That was a last minute change.

[juyik, 2014-02-14 03:59:12]

On 2014/02/14 03:57:47, Filip Gorski wrote:
> On 2014/02/14 03:26:04, juyik wrote:
> > On 2014/02/13 22:57:34, Filip Gorski wrote:
> > > asvitkine@chromium.org: Please review changes in 
> > > histograms
> > > 
> > > zea@chromium.org: Please review changes in 
> > > everything else
> > > 
> > > juyik@chromium.org: Please take a look at UMA related code.
> > 
> > These 2 status entries: INTERNAL_SERVER_ERROR and HTTP_NOT_OK, have
different
> > ordering in unregistration_request.cc compared to histograms.xml.
> 
> Thanks. That was a last minute change.

lgtm on the uma code.

[Alexei Svitkine (slow), 2014-02-14 14:45:32]

histograms lgtm

[jianli, 2014-02-14 18:36:18]

It seems that UnregistrationRequest is very similar to RegistrationRequest. Is
it possible to merge these two? You might introduce something like
RegistrationRequest::Delegate for the specific logic to registration and
unregistration:
  BuildRequest(...)
  ParseResponse(...)
You don't need to do it now. Could you please add a TODO for it?

Also, it seems that unregistration request is not hooked up with the system,
right?

https://codereview.chromium.org/147763008/diff/400001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/147763008/diff/400001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request.cc:76: std::string response;
nit: better to move the definition of local variable right before its usage

https://codereview.chromium.org/147763008/diff/400001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request.h (right):

https://codereview.chromium.org/147763008/diff/400001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request.h:9: #include <vector>
Are these 2 includes needed?

[fgorski, 2014-02-14 18:48:41]

Updated.

https://codereview.chromium.org/147763008/diff/400001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/147763008/diff/400001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request.cc:76: std::string response;
On 2014/02/14 18:36:18, jianli wrote:
> nit: better to move the definition of local variable right before its usage

Done.

https://codereview.chromium.org/147763008/diff/400001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request.h (right):

https://codereview.chromium.org/147763008/diff/400001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request.h:9: #include <vector>
On 2014/02/14 18:36:18, jianli wrote:
> Are these 2 includes needed?

Done.

[jianli, 2014-02-14 18:54:13]

lgtm

[Nicolas Zea, 2014-02-14 19:05:02]

LGTM

https://codereview.chromium.org/147763008/diff/400001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request.h (right):

https://codereview.chromium.org/147763008/diff/400001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request.h:11: #include "base/basictypes.h"
keep basic types (defines uint64 and the like)

https://codereview.chromium.org/147763008/diff/520001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/147763008/diff/520001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request.cc:45: // Outcomens of the
response parsing. Note that these enums are consumed by a
Outcome

[fgorski, 2014-02-14 19:56:34]

Updated all.
Thanks for review.

https://codereview.chromium.org/147763008/diff/400001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request.h (right):

https://codereview.chromium.org/147763008/diff/400001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request.h:11: #include "base/basictypes.h"
On 2014/02/14 19:05:05, Nicolas Zea wrote:
> keep basic types (defines uint64 and the like)

Done.

https://codereview.chromium.org/147763008/diff/520001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/147763008/diff/520001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request.cc:45: // Outcomens of the
response parsing. Note that these enums are consumed by a
On 2014/02/14 19:05:05, Nicolas Zea wrote:
> Outcome

Done.

[fgorski, 2014-02-14 19:56:37]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-02-14 20:00:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fgorski@chromium.org/147763008/640001

[commit-bot: I haz the power, 2014-02-15 03:39:56]

Change committed as 251496