Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
284763004:
Create AccountServiceFlagFetcher which downloads an account's Gaia service flags. (Closed)
|
Created:
6 years, 7 months ago by Marc Treib Modified:
6 years, 6 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCreate AccountServiceFlagFetcher which downloads an account's Gaia service flags, using https://accounts.google.com/GetUserInfo.
BUG=372381
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274541
Patch Set 1 #Patch Set 2 : Add missing OVERRIDEs #
Total comments: 17
Patch Set 3 : atwilson comments #Patch Set 4 : cleanup #
Total comments: 7
Patch Set 5 : bauerb comments #
Total comments: 10
Patch Set 6 : atwilson comments #Patch Set 7 : Unit test #Patch Set 8 : Cleanup #
Total comments: 17
Patch Set 9 : Review comments #
Total comments: 2
Patch Set 10 : bauerb comments #
Total comments: 4
Patch Set 11 : Rebase #
Messages
Total messages: 29 (0 generated)
PTAL! Try failures unrelated.
https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... File chrome/browser/signin/account_service_flag_fetcher.h (right): https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.h:20: class AccountServiceFlagFetcher : public GaiaAuthConsumer, Gaia account maybe? This is very specific to Gaia.
I don't think this API is quite right yet. See my notes. https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... File chrome/browser/signin/account_service_flag_fetcher.cc (right): https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.cc:51: service->RemoveObserver(this); Is it OK to call RemoveObserver() if you've never called AddObserver()? https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.cc:67: if (account_id != signin_manager->GetAuthenticatedAccountId()) Is it possible that you'll never get a token loaded for the authenticated account? Should you listen for OnRefreshTokensLoaded() to catch this case? https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.cc:111: callback_.Reset(); Hmmm. So, the paradigm is that you call Start(), and if your request fails you basically are never notified? That seems like a hard API to use - how can you tell if your request failed, or if it's just taking a really long time? Consider reporting errors somehow. https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... File chrome/browser/signin/account_service_flag_fetcher.h (right): https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.h:26: typedef base::Callback<void(const std::vector<std::string>& /* flags */)> What do you do in case of error? https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.h:29: explicit AccountServiceFlagFetcher(Profile* profile); Please put this in components/signin. To do this, you'll need to change this class to not pass in a Profile but to instead to pass in your RequestContext, PO2TS and SigninManager directly. https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.h:38: void Cancel(); Do you have to call Cancel() before destroying the object? If not, then maybe we shouldn't even expose Cancel() but instead just have callers destroy the object if they want to cancel calls? Then you could have a static API that both creates an AccountServiceFlagFetcher and also Start()s it - this makes the class single-use-only, and avoids this icky situation of callers having to worry about whether there's already a pending call.
Thanks for the feedback. PTAL again! https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... File chrome/browser/signin/account_service_flag_fetcher.cc (right): https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.cc:51: service->RemoveObserver(this); On 2014/05/16 08:44:44, Andrew T Wilson wrote: > Is it OK to call RemoveObserver() if you've never called AddObserver()? Yes. https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.cc:67: if (account_id != signin_manager->GetAuthenticatedAccountId()) On 2014/05/16 08:44:44, Andrew T Wilson wrote: > Is it possible that you'll never get a token loaded for the authenticated > account? Should you listen for OnRefreshTokensLoaded() to catch this case? I suppose that can happen - so you're saying I should report an error when I get OnRefreshTokensLoaded before OnRefreshTokenAvailable? But couldn't it also happen that I still get a refresh token after OnRefreshTokensLoaded, e.g. when UpdateCredentials is called directly? https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.cc:111: callback_.Reset(); On 2014/05/16 08:44:44, Andrew T Wilson wrote: > Hmmm. So, the paradigm is that you call Start(), and if your request fails you > basically are never notified? That seems like a hard API to use - how can you > tell if your request failed, or if it's just taking a really long time? Consider > reporting errors somehow. I have added a status code to the callback to report errors. Previously, you would have had to explicitly call IsPending to check if your request was still there. https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... File chrome/browser/signin/account_service_flag_fetcher.h (right): https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.h:20: class AccountServiceFlagFetcher : public GaiaAuthConsumer, On 2014/05/16 08:19:38, Bernhard Bauer wrote: > Gaia account maybe? This is very specific to Gaia. Drew, WDYT? "Account" is used as a synonym for "Gaia account" in other places as well, e.g. components/signin/core/browser/account_reconcilor. https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.h:26: typedef base::Callback<void(const std::vector<std::string>& /* flags */)> On 2014/05/16 08:44:44, Andrew T Wilson wrote: > What do you do in case of error? I've added a status enum to the callback to report errors. This seems sufficient here, and it's simpler than a separate callback or a listener interface. https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.h:29: explicit AccountServiceFlagFetcher(Profile* profile); On 2014/05/16 08:44:44, Andrew T Wilson wrote: > Please put this in components/signin. To do this, you'll need to change this > class to not pass in a Profile but to instead to pass in your RequestContext, > PO2TS and SigninManager directly. Done. The SigninManager actually isn't required, it's enough to pass in the account id. https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.h:38: void Cancel(); On 2014/05/16 08:44:44, Andrew T Wilson wrote: > Do you have to call Cancel() before destroying the object? If not, then maybe we > shouldn't even expose Cancel() but instead just have callers destroy the object > if they want to cancel calls? > > Then you could have a static API that both creates an AccountServiceFlagFetcher > and also Start()s it - this makes the class single-use-only, and avoids this > icky situation of callers having to worry about whether there's already a > pending call. Okay, now the constructor immediately starts the request - I like to keep the ctor-dtor symmetry, as opposed to a static CreateAndStart function, and then having a private ctor and public dtor.
https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... File chrome/browser/signin/account_service_flag_fetcher.h (right): https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.h:20: class AccountServiceFlagFetcher : public GaiaAuthConsumer, On 2014/05/20 10:46:40, treib wrote: > On 2014/05/16 08:19:38, Bernhard Bauer wrote: > > Gaia account maybe? This is very specific to Gaia. > > Drew, WDYT? "Account" is used as a synonym for "Gaia account" in other places as > well, e.g. components/signin/core/browser/account_reconcilor. Oh, right. OK, if account in here means Gaia account, it's fine with me. https://codereview.chromium.org/284763004/diff/50001/components/signin/core/b... File components/signin/core/browser/account_service_flag_fetcher.cc (right): https://codereview.chromium.org/284763004/diff/50001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.cc:16: : OAuth2TokenService::Consumer("account_service_flag_fetcher"), This should be indented four spaces, then the next lines aligned with OAuth2TokenService::Consumer (i.e. indented six spaces). https://codereview.chromium.org/284763004/diff/50001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.cc:89: callback_.Run(SERVICE_ERROR, std::vector<std::string>()); Hm, if we now support returning errors and have a dedicated enum for them, we could distinguish between the different types of errors. https://codereview.chromium.org/284763004/diff/50001/components/signin/core/b... File components/signin/core/browser/account_service_flag_fetcher.h (right): https://codereview.chromium.org/284763004/diff/50001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.h:31: SERVICE_ERROR // Service returned an error or malformed reply. You can add a trailing comma here, to reduce edit churn when adding more values.
https://codereview.chromium.org/284763004/diff/50001/components/signin/core/b... File components/signin/core/browser/account_service_flag_fetcher.cc (right): https://codereview.chromium.org/284763004/diff/50001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.cc:16: : OAuth2TokenService::Consumer("account_service_flag_fetcher"), On 2014/05/20 12:35:27, Bernhard Bauer wrote: > This should be indented four spaces, then the next lines aligned with > OAuth2TokenService::Consumer (i.e. indented six spaces). Done. https://codereview.chromium.org/284763004/diff/50001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.cc:89: callback_.Run(SERVICE_ERROR, std::vector<std::string>()); On 2014/05/20 12:35:27, Bernhard Bauer wrote: > Hm, if we now support returning errors and have a dedicated enum for them, we > could distinguish between the different types of errors. True, we could distinguish between more types of errors. The question is, how much detail is interesting to a client? Will anyone care which of the calls failed? Also, the set of possible errors will probably change when (if) we get a new API for this. https://codereview.chromium.org/284763004/diff/50001/components/signin/core/b... File components/signin/core/browser/account_service_flag_fetcher.h (right): https://codereview.chromium.org/284763004/diff/50001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.h:31: SERVICE_ERROR // Service returned an error or malformed reply. On 2014/05/20 12:35:27, Bernhard Bauer wrote: > You can add a trailing comma here, to reduce edit churn when adding more values. Done.
https://codereview.chromium.org/284763004/diff/50001/components/signin/core/b... File components/signin/core/browser/account_service_flag_fetcher.cc (right): https://codereview.chromium.org/284763004/diff/50001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.cc:89: callback_.Run(SERVICE_ERROR, std::vector<std::string>()); On 2014/05/20 13:24:28, treib wrote: > On 2014/05/20 12:35:27, Bernhard Bauer wrote: > > Hm, if we now support returning errors and have a dedicated enum for them, we > > could distinguish between the different types of errors. > > True, we could distinguish between more types of errors. The question is, how > much detail is interesting to a client? Will anyone care which of the calls > failed? Well, at a minimum, a client could log the errors (if we got rid of the logging statements in here, you could e.g. unit test this class without producing log output). > Also, the set of possible errors will probably change when (if) we get a new API > for this.
Thanks for the changes - this is much closer. Still needs a unit test, however. https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... File chrome/browser/signin/account_service_flag_fetcher.cc (right): https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.cc:67: if (account_id != signin_manager->GetAuthenticatedAccountId()) On 2014/05/20 10:46:40, treib wrote: > On 2014/05/16 08:44:44, Andrew T Wilson wrote: > > Is it possible that you'll never get a token loaded for the authenticated > > account? Should you listen for OnRefreshTokensLoaded() to catch this case? > > I suppose that can happen - so you're saying I should report an error when I get > OnRefreshTokensLoaded before OnRefreshTokenAvailable? Technically, yes. > But couldn't it also happen that I still get a refresh token after > OnRefreshTokensLoaded, e.g. when UpdateCredentials is called directly? That only happens if the user does a re-auth, which could happen hours or days later. I think you want your callback invoked with an error much earlier than that, rather than have it hang around, pending, for days waiting for the user to notice an auth error and re-sign-in. https://codereview.chromium.org/284763004/diff/70001/components/signin/core/b... File components/signin/core/browser/account_service_flag_fetcher.cc (right): https://codereview.chromium.org/284763004/diff/70001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.cc:101: << "GetUserInfo response didn't include allServices field."; Should this be a NOTREACHED instead of a DLOG? https://codereview.chromium.org/284763004/diff/70001/components/signin/core/b... File components/signin/core/browser/account_service_flag_fetcher.h (right): https://codereview.chromium.org/284763004/diff/70001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.h:23: // Downloads an account's list of Gaia service flags. Can you add a bit of information about how to use this class? You have documentation below on the constructor/destructor that talks about how they start/stop the fetch, but it'd be nice to talk about that up here at the class level. https://codereview.chromium.org/284763004/diff/70001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.h:33: // If the flag download is successful, this will return the list of service nit: blank line above https://codereview.chromium.org/284763004/diff/70001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.h:37: Callback; This is OK, I guess, except you're now hiding base::Callback. Can you name this something else (like ServiceFlagResultCallback or something like that)? https://codereview.chromium.org/284763004/diff/70001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.h:44: // Destructing the object before the callback is called cancels the request. nit: blank line above
Thanks for the feedback! I think I have addressed all the comments; the unit test is coming up next... https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... File chrome/browser/signin/account_service_flag_fetcher.cc (right): https://codereview.chromium.org/284763004/diff/10001/chrome/browser/signin/ac... chrome/browser/signin/account_service_flag_fetcher.cc:67: if (account_id != signin_manager->GetAuthenticatedAccountId()) On 2014/05/23 14:51:59, Andrew T Wilson wrote: > On 2014/05/20 10:46:40, treib wrote: > > On 2014/05/16 08:44:44, Andrew T Wilson wrote: > > > Is it possible that you'll never get a token loaded for the authenticated > > > account? Should you listen for OnRefreshTokensLoaded() to catch this case? > > > > I suppose that can happen - so you're saying I should report an error when I > get > > OnRefreshTokensLoaded before OnRefreshTokenAvailable? > > Technically, yes. Done. However, if we call this after the tokens have been loaded with an account ID for which we don't have a token, it will still hang around indefinitely. I don't see a way to reliably detect this case. Maybe it's enough to document the behavior in this case? > > But couldn't it also happen that I still get a refresh token after > > OnRefreshTokensLoaded, e.g. when UpdateCredentials is called directly? > > That only happens if the user does a re-auth, which could happen hours or days > later. I think you want your callback invoked with an error much earlier than > that, rather than have it hang around, pending, for days waiting for the user to > notice an auth error and re-sign-in. https://codereview.chromium.org/284763004/diff/70001/components/signin/core/b... File components/signin/core/browser/account_service_flag_fetcher.cc (right): https://codereview.chromium.org/284763004/diff/70001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.cc:101: << "GetUserInfo response didn't include allServices field."; On 2014/05/23 14:51:59, Andrew T Wilson wrote: > Should this be a NOTREACHED instead of a DLOG? Bernhard convinced me that it should be a DLOG ;) The argument was that NOTREACHED implies a bug in Chrome, which isn't really the case here, it's just an invalid reply from an external service. https://codereview.chromium.org/284763004/diff/70001/components/signin/core/b... File components/signin/core/browser/account_service_flag_fetcher.h (right): https://codereview.chromium.org/284763004/diff/70001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.h:23: // Downloads an account's list of Gaia service flags. On 2014/05/23 14:51:59, Andrew T Wilson wrote: > Can you add a bit of information about how to use this class? You have > documentation below on the constructor/destructor that talks about how they > start/stop the fetch, but it'd be nice to talk about that up here at the class > level. Done. https://codereview.chromium.org/284763004/diff/70001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.h:33: // If the flag download is successful, this will return the list of service On 2014/05/23 14:51:59, Andrew T Wilson wrote: > nit: blank line above Done. https://codereview.chromium.org/284763004/diff/70001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.h:37: Callback; On 2014/05/23 14:51:59, Andrew T Wilson wrote: > This is OK, I guess, except you're now hiding base::Callback. Can you name this > something else (like ServiceFlagResultCallback or something like that)? I called it "ResultCallback"; I think the "ServiceFlag" is implicit since it's a member of AccountServiceFlagFetcher. https://codereview.chromium.org/284763004/diff/70001/components/signin/core/b... components/signin/core/browser/account_service_flag_fetcher.h:44: // Destructing the object before the callback is called cancels the request. On 2014/05/23 14:51:59, Andrew T Wilson wrote: > nit: blank line above Done.
One suite of unit tests, coming right up!
https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... File chrome/browser/signin/account_service_flag_fetcher_unittest.cc (right): https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:5: #include "components/signin/core/browser/account_service_flag_fetcher.h" This isn't really necessary. The point of this is that for every header file, there should be one file that includes it first, so that the compiler will complain about errors in the header for that file instead of an unrelated file that includes it. We already have such a file though, so this include doesn't need any special treatment. https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:28: // move as well. Yeah, the whole class hierarchy is a bit weird there... :-( https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:58: "", Use std::string() instead of the implicit constructor. https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:122: message_loop_.RunUntilIdle(); Running a message loop directly is somewhat discouraged, as it can lead to situations where the message loop is quit too early (or the wrong one is quit, in the case of nested message loops), and Problems Ensue(tm). You should use base::RunLoop().RunUntilIdle() here, or even better, instead of mocking out AccountServiceFlagFetcherTest::OnFlagsFetched, implement it to take a callback and run it, then bind that to the RunLoop::QuitClosure() above (which requires you to construct the RunLoop further up). That way the run loop will run exactly until OnFlagsFetched is called. https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:168: EXPECT_EQ(token_service_->GetPendingRequests().size(), 0U); Put the expected value first, for nicer error messages. https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:261: } Would it also make sense to add a test that checks that the callback is not called when the AccountServiceFlagFetcher is destroyed? You can ping me if you want tips on how to test that.
lgtm https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... File chrome/browser/signin/account_service_flag_fetcher_unittest.cc (right): https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:35: url_fetcher_factory_.reset(new net::FakeURLFetcherFactory(NULL)); nit: Does this need to be dynamically allocated? Consider making token_service_ and url_fetcher_factory_ be statically defined as class members instead of scoped_ptr<>. In fact, I think all of these could be regular class members.
https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... File chrome/browser/signin/account_service_flag_fetcher_unittest.cc (right): https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:5: #include "components/signin/core/browser/account_service_flag_fetcher.h" On 2014/05/28 16:32:43, Bernhard Bauer wrote: > This isn't really necessary. The point of this is that for every header file, > there should be one file that includes it first, so that the compiler will > complain about errors in the header for that file instead of an unrelated file > that includes it. We already have such a file though, so this include doesn't > need any special treatment. True. I did this for consistency, since most other unit tests seem to do the same thing even when it's not strictly necessary. But sure, I've moved the include down into the list. https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:35: url_fetcher_factory_.reset(new net::FakeURLFetcherFactory(NULL)); On 2014/05/30 06:51:44, Andrew T Wilson wrote: > nit: Does this need to be dynamically allocated? Consider making token_service_ > and url_fetcher_factory_ be statically defined as class members instead of > scoped_ptr<>. In fact, I think all of these could be regular class members. The TestURLRequestContextGetter can't, because it has a protected dtor. The others could, but I'd still want to realloc them in SetUp to make sure I get a fresh instance for each test, and then I'd have a pointless extra initialization in the ctor. So I'd prefer to keep it as is. https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:58: "", On 2014/05/28 16:32:43, Bernhard Bauer wrote: > Use std::string() instead of the implicit constructor. Done. https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:122: message_loop_.RunUntilIdle(); On 2014/05/28 16:32:43, Bernhard Bauer wrote: > Running a message loop directly is somewhat discouraged, as it can lead to > situations where the message loop is quit too early (or the wrong one is quit, > in the case of nested message loops), and Problems Ensue(tm). You should use > base::RunLoop().RunUntilIdle() here, or even better, instead of mocking out > AccountServiceFlagFetcherTest::OnFlagsFetched, implement it to take a callback > and run it, then bind that to the RunLoop::QuitClosure() above (which requires > you to construct the RunLoop further up). That way the run loop will run exactly > until OnFlagsFetched is called. Okay, I'm using RunLoop::RunUntilIdle now. I don't think the extra complexity to run exactly until the OnFlagsFetched call is worth it here. And I'd still need a separate mock method so I can EXPECT_CALL. https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:168: EXPECT_EQ(token_service_->GetPendingRequests().size(), 0U); On 2014/05/28 16:32:43, Bernhard Bauer wrote: > Put the expected value first, for nicer error messages. Done. https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:261: } On 2014/05/28 16:32:43, Bernhard Bauer wrote: > Would it also make sense to add a test that checks that the callback is not > called when the AccountServiceFlagFetcher is destroyed? You can ping me if you > want tips on how to test that. I've added a test for that. Is there more to it than a .Times(0)?
https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... File chrome/browser/signin/account_service_flag_fetcher_unittest.cc (right): https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:35: url_fetcher_factory_.reset(new net::FakeURLFetcherFactory(NULL)); On 2014/06/02 09:14:41, treib wrote: > On 2014/05/30 06:51:44, Andrew T Wilson wrote: > > nit: Does this need to be dynamically allocated? Consider making > token_service_ > > and url_fetcher_factory_ be statically defined as class members instead of > > scoped_ptr<>. In fact, I think all of these could be regular class members. > > The TestURLRequestContextGetter can't, because it has a protected dtor. (Because it's refcounted). > The > others could, but I'd still want to realloc them in SetUp to make sure I get a > fresh instance for each test, and then I'd have a pointless extra initialization > in the ctor. So I'd prefer to keep it as is. The test fixture is destroyed after every test as well, so you'll get a fresh instance in any case. https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:261: } On 2014/06/02 09:14:41, treib wrote: > On 2014/05/28 16:32:43, Bernhard Bauer wrote: > > Would it also make sense to add a test that checks that the callback is not > > called when the AccountServiceFlagFetcher is destroyed? You can ping me if you > > want tips on how to test that. > > I've added a test for that. Is there more to it than a .Times(0)? Yes and no. *In principle* you can do it with .Times(0) and RunUntilIdle(). The problem is that it's surprisingly subtle to get RunUntilIdle right. For example, in the latest patch set, you set up all the responses, but then destroy the AccountServiceFlagFetcher before running the run loop, which means there is a task in the message loop that would end up calling OnFlagsFetched. If something in the implementation would start calling callbacks synchronously instead of posting them to the message loop, the test would start failing. A more robust solution would be to wait with e.g. the GetUserInfo response until the AccountServiceFlagFetcher has been destroyed. In more complicated tests, it could happen that RunUntilIdle will quit too soon, or that it won't quit at all, which will cause flakiness and timeouts. Getting these things right then requires knowing implementation details of the class you're testing, which will in turn make the tests brittle. If you use RunLoop::Run() with a quit closure, you can create a closure that will fail the test if it is run, and causes the run loop to quit if it is destroyed (i.e. the last reference to it is dropped) :) https://codereview.chromium.org/284763004/diff/150001/chrome/browser/signin/a... File chrome/browser/signin/account_service_flag_fetcher_unittest.cc (right): https://codereview.chromium.org/284763004/diff/150001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:221: base::RunLoop run_loop; If you are using RunUntilIdle, for consistency you should probably declare the run loop below right before you use it. Another thing you could do is to just declare it inline: base::RunLoop().RunUntilIdle()
https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... File chrome/browser/signin/account_service_flag_fetcher_unittest.cc (right): https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:35: url_fetcher_factory_.reset(new net::FakeURLFetcherFactory(NULL)); On 2014/06/02 10:25:42, Bernhard Bauer wrote: > On 2014/06/02 09:14:41, treib wrote: > > On 2014/05/30 06:51:44, Andrew T Wilson wrote: > > > nit: Does this need to be dynamically allocated? Consider making > > token_service_ > > > and url_fetcher_factory_ be statically defined as class members instead of > > > scoped_ptr<>. In fact, I think all of these could be regular class members. > > > > The TestURLRequestContextGetter can't, because it has a protected dtor. > > (Because it's refcounted). > > > The > > others could, but I'd still want to realloc them in SetUp to make sure I get a > > fresh instance for each test, and then I'd have a pointless extra > initialization > > in the ctor. So I'd prefer to keep it as is. > > The test fixture is destroyed after every test as well, so you'll get a fresh > instance in any case. Huh, so it is. Yep, in that case I can move everything into a ctor instead of SetUp, and make the members non-pointers. Done. https://codereview.chromium.org/284763004/diff/130001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:261: } On 2014/06/02 10:25:42, Bernhard Bauer wrote: > On 2014/06/02 09:14:41, treib wrote: > > On 2014/05/28 16:32:43, Bernhard Bauer wrote: > > > Would it also make sense to add a test that checks that the callback is not > > > called when the AccountServiceFlagFetcher is destroyed? You can ping me if > you > > > want tips on how to test that. > > > > I've added a test for that. Is there more to it than a .Times(0)? > > Yes and no. *In principle* you can do it with .Times(0) and RunUntilIdle(). The > problem is that it's surprisingly subtle to get RunUntilIdle right. For example, > in the latest patch set, you set up all the responses, but then destroy the > AccountServiceFlagFetcher before running the run loop, which means there is a > task in the message loop that would end up calling OnFlagsFetched. If something > in the implementation would start calling callbacks synchronously instead of > posting them to the message loop, the test would start failing. A more robust > solution would be to wait with e.g. the GetUserInfo response until the > AccountServiceFlagFetcher has been destroyed. > > In more complicated tests, it could happen that RunUntilIdle will quit too soon, > or that it won't quit at all, which will cause flakiness and timeouts. Getting > these things right then requires knowing implementation details of the class > you're testing, which will in turn make the tests brittle. > > If you use RunLoop::Run() with a quit closure, you can create a closure that > will fail the test if it is run, and causes the run loop to quit if it is > destroyed (i.e. the last reference to it is dropped) :) As discussed offline: The URLFetcher is now stubbed out differently, so I can control when the results are returned. https://codereview.chromium.org/284763004/diff/150001/chrome/browser/signin/a... File chrome/browser/signin/account_service_flag_fetcher_unittest.cc (right): https://codereview.chromium.org/284763004/diff/150001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:221: base::RunLoop run_loop; On 2014/06/02 10:25:42, Bernhard Bauer wrote: > If you are using RunUntilIdle, for consistency you should probably declare the > run loop below right before you use it. > > Another thing you could do is to just declare it inline: > base::RunLoop().RunUntilIdle() With TestURLFetcherFactory instead of FakeURLFetcherFactory, explicitly spinning the msgloop is not required anymore. :)
Nice, LGTM! https://codereview.chromium.org/284763004/diff/170001/chrome/browser/signin/a... File chrome/browser/signin/account_service_flag_fetcher_unittest.cc (right): https://codereview.chromium.org/284763004/diff/170001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:34: base::MessageLoopProxy::current())) { I'm wondering whether passing NULL here would work (because we don't send any network requests, so we might not need a network task runner). That way we could be sure that no tasks land in the message loop now that we don't run it anymore. If it doesn't work, don't worry about it though.
https://codereview.chromium.org/284763004/diff/170001/chrome/browser/signin/a... File chrome/browser/signin/account_service_flag_fetcher_unittest.cc (right): https://codereview.chromium.org/284763004/diff/170001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:34: base::MessageLoopProxy::current())) { On 2014/06/02 16:36:25, Bernhard Bauer wrote: > I'm wondering whether passing NULL here would work (because we don't send any > network requests, so we might not need a network task runner). That way we could > be sure that no tasks land in the message loop now that we don't run it anymore. > > If it doesn't work, don't worry about it though. The TestURLRequestContextGetter DCHECKs that it got a non-NULL task runner. Not sure that it actually needs it in our case (maybe someone else is spinning it?), but removing those DCHECKs seems like a bad idea, so I'll leave it as is. Thanks for all the feedback!
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/284763004/170001
https://codereview.chromium.org/284763004/diff/170001/chrome/browser/signin/a... File chrome/browser/signin/account_service_flag_fetcher_unittest.cc (right): https://codereview.chromium.org/284763004/diff/170001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:34: base::MessageLoopProxy::current())) { On 2014/06/03 07:51:26, treib wrote: > On 2014/06/02 16:36:25, Bernhard Bauer wrote: > > I'm wondering whether passing NULL here would work (because we don't send any > > network requests, so we might not need a network task runner). That way we > could > > be sure that no tasks land in the message loop now that we don't run it > anymore. > > > > If it doesn't work, don't worry about it though. > > The TestURLRequestContextGetter DCHECKs that it got a non-NULL task runner. I find DCHECKS for non-NULL pointers a bit pointless (no pun intended), because dereferencing the NULL pointer will crash in any case (and if it's an abstract interface like SingleThreadTaskRunner, it will be a clean crash). But okay, let's not change that here. > Not > sure that it actually needs it in our case (maybe someone else is spinning it?), > but removing those DCHECKs seems like a bad idea, so I'll leave it as is. > Thanks for all the feedback!
https://codereview.chromium.org/284763004/diff/170001/chrome/browser/signin/a... File chrome/browser/signin/account_service_flag_fetcher_unittest.cc (right): https://codereview.chromium.org/284763004/diff/170001/chrome/browser/signin/a... chrome/browser/signin/account_service_flag_fetcher_unittest.cc:34: base::MessageLoopProxy::current())) { On 2014/06/03 08:09:51, Bernhard Bauer wrote: > On 2014/06/03 07:51:26, treib wrote: > > On 2014/06/02 16:36:25, Bernhard Bauer wrote: > > > I'm wondering whether passing NULL here would work (because we don't send > any > > > network requests, so we might not need a network task runner). That way we > > could > > > be sure that no tasks land in the message loop now that we don't run it > > anymore. > > > > > > If it doesn't work, don't worry about it though. > > > > The TestURLRequestContextGetter DCHECKs that it got a non-NULL task runner. > > I find DCHECKS for non-NULL pointers a bit pointless (no pun intended), because > dereferencing the NULL pointer will crash in any case (and if it's an abstract > interface like SingleThreadTaskRunner, it will be a clean crash). But okay, > let's not change that here. Well, at least they tell you somewhat earlier that something's wrong. And the TestURLRequestContextGetter doesn't actually use the task runner itself, but it does expose it via a public getter. > > Not > > sure that it actually needs it in our case (maybe someone else is spinning > it?), > > but removing those DCHECKs seems like a bad idea, so I'll leave it as is. > > Thanks for all the feedback! >
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/284763004/190001
Message was sent while issue was closed.
Change committed as 274541 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
