GCP2.0 Device: Instant AccessToken update on AuthFailed.

GCP2.0 Device: Instant AccessToken update on AuthFailed.

Also, registration timeout implemented.

BUG=

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

[maksymb, 2013-08-05 18:04:25]

54 lines

[gene, 2013-08-05 20:56:28]

https://chromiumcodereview.appspot.com/22184007/diff/1/cloud_print/gcp20/prot...
File cloud_print/gcp20/prototype/cloud_print_requester.cc (right):

https://chromiumcodereview.appspot.com/22184007/diff/1/cloud_print/gcp20/prot...
cloud_print/gcp20/prototype/cloud_print_requester.cc:178: // TODO(maksymb):
|server_api| and other
still need this TODO?

https://chromiumcodereview.appspot.com/22184007/diff/1/cloud_print/gcp20/prot...
File cloud_print/gcp20/prototype/printer.cc (right):

https://chromiumcodereview.appspot.com/22184007/diff/1/cloud_print/gcp20/prot...
cloud_print/gcp20/prototype/printer.cc:761:
base::MessageLoop::current()->PostDelayedTask(
Since this is a post task, it is possible you can get /privet/register call
between OnAuthError() and TryConnect(). Since requester_ will be NULL, it will
crash in this case.

[maksymb, 2013-08-06 21:45:54]

https://codereview.chromium.org/22184007/diff/1/cloud_print/gcp20/prototype/c...
File cloud_print/gcp20/prototype/cloud_print_requester.cc (right):

https://codereview.chromium.org/22184007/diff/1/cloud_print/gcp20/prototype/c...
cloud_print/gcp20/prototype/cloud_print_requester.cc:178: // TODO(maksymb):
|server_api| and other
On 2013/08/05 20:56:28, gene wrote:
> still need this TODO?

Yes. NOTIMPLEMENTED was deleted because I already have one in
cloud_print_request.cc

https://codereview.chromium.org/22184007/diff/1/cloud_print/gcp20/prototype/p...
File cloud_print/gcp20/prototype/printer.cc (right):

https://codereview.chromium.org/22184007/diff/1/cloud_print/gcp20/prototype/p...
cloud_print/gcp20/prototype/printer.cc:761:
base::MessageLoop::current()->PostDelayedTask(
On 2013/08/05 20:56:28, gene wrote:
> Since this is a post task, it is possible you can get /privet/register call
> between OnAuthError() and TryConnect(). Since requester_ will be NULL, it will
> crash in this case.

Done.

[gene, 2013-08-08 02:01:55]

https://codereview.chromium.org/22184007/diff/1/cloud_print/gcp20/prototype/c...
File cloud_print/gcp20/prototype/cloud_print_requester.cc (right):

https://codereview.chromium.org/22184007/diff/1/cloud_print/gcp20/prototype/c...
cloud_print/gcp20/prototype/cloud_print_requester.cc:178: // TODO(maksymb):
|server_api| and other
What does "|server_api| and other" mean?
Could you please explain a little bit what do you want to do here?

On 2013/08/06 21:45:54, maksymb wrote:
> On 2013/08/05 20:56:28, gene wrote:
> > still need this TODO?
> 
> Yes. NOTIMPLEMENTED was deleted because I already have one in
> cloud_print_request.cc

https://codereview.chromium.org/22184007/diff/1/cloud_print/gcp20/prototype/p...
File cloud_print/gcp20/prototype/printer.cc (right):

https://codereview.chromium.org/22184007/diff/1/cloud_print/gcp20/prototype/p...
cloud_print/gcp20/prototype/printer.cc:761:
base::MessageLoop::current()->PostDelayedTask(
Could you please explain how this has been fixed?
What if you get /privet/printer/submitdoc between OnAuthError() and
TryConnect()?

https://codereview.chromium.org/22184007/diff/18001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printer.cc (right):

https://codereview.chromium.org/22184007/diff/18001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:810: SetRegistrationError("Cannot access
server during registration process");
I am not sure I understand in what cases this error is returned.

https://codereview.chromium.org/22184007/diff/18001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/privet_http_server.h (right):

https://codereview.chromium.org/22184007/diff/18001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/privet_http_server.h:90: virtual bool IsRegistered()
= 0;
why IsRegistered() is not const any more?

[maksymb, 2013-08-08 18:35:45]

https://codereview.chromium.org/22184007/diff/1/cloud_print/gcp20/prototype/c...
File cloud_print/gcp20/prototype/cloud_print_requester.cc (right):

https://codereview.chromium.org/22184007/diff/1/cloud_print/gcp20/prototype/c...
cloud_print/gcp20/prototype/cloud_print_requester.cc:178: // TODO(maksymb):
|server_api| and other
On 2013/08/08 02:01:55, gene wrote:
> What does "|server_api| and other" mean?
> Could you please explain a little bit what do you want to do here?
> 
> On 2013/08/06 21:45:54, maksymb wrote:
> > On 2013/08/05 20:56:28, gene wrote:
> > > still need this TODO?
> > 
> > Yes. NOTIMPLEMENTED was deleted because I already have one in
> > cloud_print_request.cc
> 

Done.

https://codereview.chromium.org/22184007/diff/1/cloud_print/gcp20/prototype/p...
File cloud_print/gcp20/prototype/printer.cc (right):

https://codereview.chromium.org/22184007/diff/1/cloud_print/gcp20/prototype/p...
cloud_print/gcp20/prototype/printer.cc:761:
base::MessageLoop::current()->PostDelayedTask(
On 2013/08/08 02:01:55, gene wrote:
> Could you please explain how this has been fixed?
> What if you get /privet/printer/submitdoc between OnAuthError() and
> TryConnect()?

/privet/accesstoken can be processed only in OnIdle

https://codereview.chromium.org/22184007/diff/18001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printer.cc (right):

https://codereview.chromium.org/22184007/diff/18001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:810: SetRegistrationError("Cannot access
server during registration process");
On 2013/08/08 02:01:55, gene wrote:
> I am not sure I understand in what cases this error is returned.

Once something bad happened during registration, this method will be called and
we will block registration process.
In this case user can start registration again with
/privet/register?action=start

https://codereview.chromium.org/22184007/diff/18001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/privet_http_server.h (right):

https://codereview.chromium.org/22184007/diff/18001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/privet_http_server.h:90: virtual bool IsRegistered()
= 0;
On 2013/08/08 02:01:55, gene wrote:
> why IsRegistered() is not const any more?

Hm... Really, I don't need to call CheckRegistrationExpiration in this method. I
can left it still const. Done.

[maksymb, 2013-08-15 17:15:54]

Please take a look at my CL

[Vitaly Buka (NO REVIEWS), 2013-08-15 17:48:06]

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printer.cc (right):

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:187: access_token_update_ =
base::Time::UnixEpoch();
Why not now?

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:812: void Printer::FallOffline(bool
instant_reconnect) {
I guess you can remove argument and use TryConnect() where FallOffline(true) is
used

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:829:
base::TimeDelta::FromSeconds(kReconnectTimeout));
Fix alignment

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printer.h (right):

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.h:171: //
??

[maksymb, 2013-08-15 18:39:13]

PTAL

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printer.cc (right):

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:187: access_token_update_ =
base::Time::UnixEpoch();
On 2013/08/15 17:48:06, Vitaly Buka wrote:
> Why not now?

UnixEpoch() is definitely in the past, Now() is only now and when we check
whether we need to update |access_token| in TryConnect():
    if (access_token_update_ < base::Time::Now()) {
      requester_->UpdateAccesstoken(reg_info_.refresh_token);
it may be called /*in theory*/ in the same microsecond and won't be updated.
Another solution is to change comparison from < to <=. Is this solution more
readable? UnixEpoch() variant is stable enough and reader doesn't have to know
exact implementation of comparison (< or <=) to be sure it will work.

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:812: void Printer::FallOffline(bool
instant_reconnect) {
On 2013/08/15 17:48:06, Vitaly Buka wrote:
> I guess you can remove argument and use TryConnect() where FallOffline(true)
is
> used

1. In this case ChangeState(OFFLINE) won't be called and   |requester_| and
|xmpp_listener_| won't be reset.
2. Also, I want to block registration and set ERROR if I receive server or
network error.

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printer.h (right):

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.h:171: //
On 2013/08/15 17:48:06, Vitaly Buka wrote:
> ??

Unnecessary declaration deleted.

[maksymb, 2013-08-15 20:15:30]

Fix:
  UnixEpoch() -> Time()

[Vitaly Buka (NO REVIEWS), 2013-08-15 20:17:06]

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printer.cc (right):

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:187: access_token_update_ =
base::Time::UnixEpoch();
= base::Time::Time();

On 2013/08/15 18:39:13, maksymb wrote:
> On 2013/08/15 17:48:06, Vitaly Buka wrote:
> > Why not now?
> 
> UnixEpoch() is definitely in the past, Now() is only now and when we check
> whether we need to update |access_token| in TryConnect():
>     if (access_token_update_ < base::Time::Now()) {
>       requester_->UpdateAccesstoken(reg_info_.refresh_token);
> it may be called /*in theory*/ in the same microsecond and won't be updated.
> Another solution is to change comparison from < to <=. Is this solution more
> readable? UnixEpoch() variant is stable enough and reader doesn't have to know
> exact implementation of comparison (< or <=) to be sure it will work.

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:812: void Printer::FallOffline(bool
instant_reconnect) {
It won't be called anyway
DCHECK is debug only.

On 2013/08/15 18:39:13, maksymb wrote:
> On 2013/08/15 17:48:06, Vitaly Buka wrote:
> > I guess you can remove argument and use TryConnect() where FallOffline(true)
> is
> > used
> 
> 1. In this case ChangeState(OFFLINE) won't be called and   |requester_| and
> |xmpp_listener_| won't be reset.
> 2. Also, I want to block registration and set ERROR if I receive server or
> network error.

[maksymb, 2013-08-15 20:23:00]

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
File cloud_print/gcp20/prototype/printer.cc (right):

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:187: access_token_update_ =
base::Time::UnixEpoch();
On 2013/08/15 20:17:06, Vitaly Buka wrote:
> = base::Time::Time();
> 
> On 2013/08/15 18:39:13, maksymb wrote:
> > On 2013/08/15 17:48:06, Vitaly Buka wrote:
> > > Why not now?
> > 
> > UnixEpoch() is definitely in the past, Now() is only now and when we check
> > whether we need to update |access_token| in TryConnect():
> >     if (access_token_update_ < base::Time::Now()) {
> >       requester_->UpdateAccesstoken(reg_info_.refresh_token);
> > it may be called /*in theory*/ in the same microsecond and won't be updated.
> > Another solution is to change comparison from < to <=. Is this solution more
> > readable? UnixEpoch() variant is stable enough and reader doesn't have to
know
> > exact implementation of comparison (< or <=) to be sure it will work.
> 

Done.

https://codereview.chromium.org/22184007/diff/35001/cloud_print/gcp20/prototy...
cloud_print/gcp20/prototype/printer.cc:812: void Printer::FallOffline(bool
instant_reconnect) {
On 2013/08/15 20:17:06, Vitaly Buka wrote:
> It won't be called anyway
> DCHECK is debug only.
> 
> On 2013/08/15 18:39:13, maksymb wrote:
> > On 2013/08/15 17:48:06, Vitaly Buka wrote:
> > > I guess you can remove argument and use TryConnect() where
FallOffline(true)
> > is
> > > used
> > 
> > 1. In this case ChangeState(OFFLINE) won't be called and   |requester_| and
> > |xmpp_listener_| won't be reset.
> > 2. Also, I want to block registration and set ERROR if I receive server or
> > network error.
> 

Moved outside of DCHECK.

[Vitaly Buka (NO REVIEWS), 2013-08-15 20:25:06]

lgtm

[commit-bot: I haz the power, 2013-08-15 20:32:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maksymb@chromium.org/22184007/65001

[commit-bot: I haz the power, 2013-08-16 00:33:58]

Change committed as 217880