Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2771713003:
RemoteSuggestionsFetcher: Use common AccessTokenFetcher helper class (Closed)
|
Created:
3 years, 9 months ago by Marc Treib Modified:
3 years, 9 months ago Reviewers:
jkrcal CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoteSuggestionsFetcher: Use common AccessTokenFetcher helper class
This simplifies the RemoteSuggestionsFetcher quite a bit.
BUG=609084
Review-Url: https://codereview.chromium.org/2771713003
Cr-Commit-Position: refs/heads/master@{#459731}
Committed: https://chromium.googlesource.com/chromium/src/+/c31ec95e72b921cc625e8a48e7064721fc85de09
Patch Set 1 #
Total comments: 9
Patch Set 2 : review #Patch Set 3 : comment #
Messages
Total messages: 19 (12 generated)
Description was changed from ========== RemoteSuggestionsFetcher: Use common AccessTokenFetcher helper class This simplifies the RemoteSuggestionsFetcher a lot! BUG=609084 ========== to ========== RemoteSuggestionsFetcher: Use common AccessTokenFetcher helper class This simplifies the RemoteSuggestionsFetcher quite a bit. BUG=609084 ==========
The CQ bit was checked by treib@chromium.org to run a CQ dry run
treib@chromium.org changed reviewers: + jkrcal@chromium.org
PTAL! https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (left): https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_fetcher.cc:290: } else if (signin_manager_->AuthInProgress()) { Do you remember if we had actual proof that this case ever happened? Per the discussion/investigation on https://codereview.chromium.org/2582573002, it should never return true on Android (and even if it did, we should maybe treat it as not-signed-in). Please double-check :) If we do want to handle it, we could just put an "|| AuthInProgress" into the if above; the AccessTokenFetcher would then wait for the sign-in to complete.
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.
lgtm, thanks! https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (left): https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_fetcher.cc:290: } else if (signin_manager_->AuthInProgress()) { On 2017/03/23 19:23:19, Marc Treib wrote: > Do you remember if we had actual proof that this case ever happened? Per the > discussion/investigation on https://codereview.chromium.org/2582573002, it > should never return true on Android (and even if it did, we should maybe treat > it as not-signed-in). Please double-check :) > > If we do want to handle it, we could just put an "|| AuthInProgress" into the if > above; the AccessTokenFetcher would then wait for the sign-in to complete. The question is: how does it behave on iOS? Unless we know it also does not happen on iOS, I'd rather keep add the "|| .." clause to the previous if and keep the TODO in the unit-test :| https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (right): https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_fetcher.cc:342: void RemoteSuggestionsFetcher::AccessTokenAvailable( The naming is misleading for me as this function also includes the case of an error. Maybe AccessTokenFetchFinished? https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_fetcher.cc:345: // Delete the fetcher after we leave this method. nit: I do not see the token_fetcher_ being used later in this method. Why not token_fetcher_.reset(nullptr)? I do not think this would need the comment.
https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (left): https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_fetcher.cc:290: } else if (signin_manager_->AuthInProgress()) { On 2017/03/24 09:09:26, jkrcal wrote: > On 2017/03/23 19:23:19, Marc Treib wrote: > > Do you remember if we had actual proof that this case ever happened? Per the > > discussion/investigation on https://codereview.chromium.org/2582573002, it > > should never return true on Android (and even if it did, we should maybe treat > > it as not-signed-in). Please double-check :) > > > > If we do want to handle it, we could just put an "|| AuthInProgress" into the > if > > above; the AccessTokenFetcher would then wait for the sign-in to complete. > > The question is: how does it behave on iOS? > Unless we know it also does not happen on iOS, I'd rather keep add the "|| .." > clause to the previous if and keep the TODO in the unit-test :| Alright, makes sense, at least until we've investigated further. I've added the AuthInProgress check again, and extended the test comment a bit. https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (right): https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_fetcher.cc:342: void RemoteSuggestionsFetcher::AccessTokenAvailable( On 2017/03/24 09:09:26, jkrcal wrote: > The naming is misleading for me as this function also includes the case of an > error. Maybe AccessTokenFetchFinished? Done. https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_fetcher.cc:345: // Delete the fetcher after we leave this method. On 2017/03/24 09:09:26, jkrcal wrote: > nit: I do not see the token_fetcher_ being used later in this method. Why not > token_fetcher_.reset(nullptr)? I do not think this would need the comment. This method is called from the fetcher, so deleting it earlier is dangerous. E.g. our method parameters (passed by reference) might stop to exist.
The CQ bit was checked by treib@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...
https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (right): https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_fetcher.cc:345: // Delete the fetcher after we leave this method. On 2017/03/27 08:08:20, Marc Treib wrote: > On 2017/03/24 09:09:26, jkrcal wrote: > > nit: I do not see the token_fetcher_ being used later in this method. Why not > > token_fetcher_.reset(nullptr)? I do not think this would need the comment. > > This method is called from the fetcher, so deleting it earlier is dangerous. > E.g. our method parameters (passed by reference) might stop to exist. Understood! nit: Can you expand the comment a bit? e.g. // Delete the fetcher only after we leave this method (which is called from the fetcher itself).
https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (right): https://codereview.chromium.org/2771713003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_fetcher.cc:345: // Delete the fetcher after we leave this method. On 2017/03/27 08:35:59, jkrcal wrote: > On 2017/03/27 08:08:20, Marc Treib wrote: > > On 2017/03/24 09:09:26, jkrcal wrote: > > > nit: I do not see the token_fetcher_ being used later in this method. Why > not > > > token_fetcher_.reset(nullptr)? I do not think this would need the comment. > > > > This method is called from the fetcher, so deleting it earlier is dangerous. > > E.g. our method parameters (passed by reference) might stop to exist. > > Understood! > > nit: Can you expand the comment a bit? e.g. > // Delete the fetcher only after we leave this method (which is called from the > fetcher itself). Sure, done!
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org Link to the patchset: https://codereview.chromium.org/2771713003/#ps40001 (title: "comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490603885951530,
"parent_rev": "abb3bc7778455201bce0facbf6cbb30bef6a3c49", "commit_rev":
"c31ec95e72b921cc625e8a48e7064721fc85de09"}
Message was sent while issue was closed.
Description was changed from ========== RemoteSuggestionsFetcher: Use common AccessTokenFetcher helper class This simplifies the RemoteSuggestionsFetcher quite a bit. BUG=609084 ========== to ========== RemoteSuggestionsFetcher: Use common AccessTokenFetcher helper class This simplifies the RemoteSuggestionsFetcher quite a bit. BUG=609084 Review-Url: https://codereview.chromium.org/2771713003 Cr-Commit-Position: refs/heads/master@{#459731} Committed: https://chromium.googlesource.com/chromium/src/+/c31ec95e72b921cc625e8a48e706... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c31ec95e72b921cc625e8a48e706... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
