Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2239653002:
Add robustness against OAuth2 Token requests getting canceled. (Closed)
|
Created:
4 years, 4 months ago by jkrcal Modified:
4 years, 4 months ago Reviewers:
Marc Treib CC:
chromium-reviews, ntp-dev+reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd robustness against OAuth2 Token requests getting canceled.
OAuth2 token requests get canceled whenever new refresh tokens are
loaded. On Android this happens soon after startup and (on slow devices)
quite reliably cancels the requests for content suggestions.
This CL adds one retry in case the token request gets canceled.
BUG=632740
Committed: https://crrev.com/36f23cf9e0aae1b88b19afab849ac4a7c80d5e80
Cr-Commit-Position: refs/heads/master@{#411325}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Marc's comments #
Total comments: 2
Patch Set 3 : Marc's comments #2 #
Messages
Total messages: 18 (10 generated)
jkrcal@chromium.org changed reviewers: + treib@chromium.org
Marc, PTAL!
Description was changed from ========== Add robustness against OAuth2 Token requests getting canceled. OAuth2 token requests get canceled whenever new refresh tokens are loaded. On Android this happens soon after startup and (on slow devices) quite reliably cancels the requests for content suggestions. This CL adds one retry in case the token request gets cancelled. BUG=632740 ========== to ========== Add robustness against OAuth2 Token requests getting canceled. OAuth2 token requests get canceled whenever new refresh tokens are loaded. On Android this happens soon after startup and (on slow devices) quite reliably cancels the requests for content suggestions. This CL adds one retry in case the token request gets canceled. BUG=632740 ==========
We don't have any tests for authenticated fetching yet, right? We really should add some... https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (left): https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:440: << " - fetching the snippets without authentication."; Outdated documentation ftl :D https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:235: oauth_token_retried_ = false; You should also initialize this in the ctor https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.h:233: // When a token request get's cancelled - we want to retry for _one_ time. nit: gets, not apostrophe. Also comma instead of dash :)
Thanks! PTAL, again. https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (left): https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:440: << " - fetching the snippets without authentication."; On 2016/08/11 09:35:17, Marc Treib wrote: > Outdated documentation ftl :D :) https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:235: oauth_token_retried_ = false; On 2016/08/11 09:35:17, Marc Treib wrote: > You should also initialize this in the ctor Done. I was wondering about it. In the current code, I always write before read. What is the rule? Should I always initialize _everything_ to some sane value? https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.h:233: // When a token request get's cancelled - we want to retry for _one_ time. On 2016/08/11 09:35:17, Marc Treib wrote: > nit: gets, not apostrophe. Also comma instead of dash :) Done.
lgtm https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:235: oauth_token_retried_ = false; On 2016/08/11 10:03:56, jkrcal wrote: > On 2016/08/11 09:35:17, Marc Treib wrote: > > You should also initialize this in the ctor > > Done. > > I was wondering about it. In the current code, I always write before read. What > is the rule? Should I always initialize _everything_ to some sane value? Yes. I'm not sure if the style guide has a rule about this, but IMO it's a good practice to never have uninitialized values - those are just asking for trouble. https://codereview.chromium.org/2239653002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2239653002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:233: // When a token request gets canceled, we want to retry for _one_ time. nit: s/for one time/once/
Thanks, Marc! https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2239653002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:235: oauth_token_retried_ = false; On 2016/08/11 10:11:03, Marc Treib wrote: > On 2016/08/11 10:03:56, jkrcal wrote: > > On 2016/08/11 09:35:17, Marc Treib wrote: > > > You should also initialize this in the ctor > > > > Done. > > > > I was wondering about it. In the current code, I always write before read. > What > > is the rule? Should I always initialize _everything_ to some sane value? > > Yes. I'm not sure if the style guide has a rule about this, but IMO it's a good > practice to never have uninitialized values - those are just asking for trouble. Makes sense. Thanks. https://codereview.chromium.org/2239653002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2239653002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:233: // When a token request gets canceled, we want to retry for _one_ time. On 2016/08/11 10:11:03, Marc Treib wrote: > nit: s/for one time/once/ Done.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2239653002/#ps40001 (title: "Marc's comments #2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add robustness against OAuth2 Token requests getting canceled. OAuth2 token requests get canceled whenever new refresh tokens are loaded. On Android this happens soon after startup and (on slow devices) quite reliably cancels the requests for content suggestions. This CL adds one retry in case the token request gets canceled. BUG=632740 ========== to ========== Add robustness against OAuth2 Token requests getting canceled. OAuth2 token requests get canceled whenever new refresh tokens are loaded. On Android this happens soon after startup and (on slow devices) quite reliably cancels the requests for content suggestions. This CL adds one retry in case the token request gets canceled. BUG=632740 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add robustness against OAuth2 Token requests getting canceled. OAuth2 token requests get canceled whenever new refresh tokens are loaded. On Android this happens soon after startup and (on slow devices) quite reliably cancels the requests for content suggestions. This CL adds one retry in case the token request gets canceled. BUG=632740 ========== to ========== Add robustness against OAuth2 Token requests getting canceled. OAuth2 token requests get canceled whenever new refresh tokens are loaded. On Android this happens soon after startup and (on slow devices) quite reliably cancels the requests for content suggestions. This CL adds one retry in case the token request gets canceled. BUG=632740 Committed: https://crrev.com/36f23cf9e0aae1b88b19afab849ac4a7c80d5e80 Cr-Commit-Position: refs/heads/master@{#411325} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/36f23cf9e0aae1b88b19afab849ac4a7c80d5e80 Cr-Commit-Position: refs/heads/master@{#411325} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
