Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1091363002:
Change ProfileDownloader to use AccountTrackerService. (Closed)
DescriptionChange ProfileDownloader to use AccountTrackerService.
BUG=466799
Committed: https://crrev.com/56e93a34eefd5e1c8b54cc958aca3ad04d3d8329
Cr-Commit-Position: refs/heads/master@{#330566}
Patch Set 1 #
Total comments: 45
Patch Set 2 : Address review feedback. #
Total comments: 22
Patch Set 3 : Clean up and use proper testing facilities for URLFetchers. #Patch Set 4 : Remove file committed by mistake. #
Total comments: 2
Patch Set 5 : Clean up unit test. #
Total comments: 2
Patch Set 6 : Formatting. #Patch Set 7 : Rebase #Patch Set 8 : Remove erroneous change. #Patch Set 9 : Fix bad merge #Patch Set 10 : Fix chromeos and android builds. #Patch Set 11 : Fix ChromeOS tests and cleanup. #
Total comments: 4
Patch Set 12 : Nits. #Messages
Total messages: 37 (15 generated)
anthonyvd@chromium.org changed reviewers: + mlerman@chromium.org, rogerta@chromium.org
Hello to both of you! Could you please take a look at this change that makes the ProfileDownloader call into the AccountTrackerService? mlerman@chromium.org: chrome/browser/profiles/* rogerta@chromium.org: chrome/browser/signin/* and components/signin/core/* I plan on adding some more unit tests to the ProfileDownloader but this seemed like a good place to get the ball rolling on this CL. Thanks a lot for your time!
I'm really excited to see this coming together! Great work. Meta-question: Should the ProfileDownloader become a PKS? Now that it has a dependency on a PKS (two really, since it already depends on the PO2TS), and has such a strong relationship with the ATS, it might make since for it to have a similar lifecycle. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_downloader.cc (right): https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:28: #include "google_apis/gaia/gaia_constants.h" I imagine these google_apis/gaia includes aren't needed anymore. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:162: !BrowserThread::IsMessageLoopValid(BrowserThread::UI)); I don't understand why you need IsMessageLoopValid. Generally we just use CurrentlyOn and everything tests fine? https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:186: if (account_tracker_service) Since we now have a reference to a PKS, I wonder if this should be a PKS. Also - we can set this in the constructor using the delegate. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:194: return base::UTF8ToUTF16(account_info_.hosted_domain); I'm not convinced we still need these methods at all. The store of truth for these pieces of data is supposed to be the ATS. We could either remove them completely and make all things that references them reference the ATS instead (though that requires consumers of Profile information to know quite a bit about Signin classes), or have these methods get the value from the ATS. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:219: return account_info_.picture_url; This used to be the result of GetImageURLWithSize. Now it's the raw url from the ATS - these aren't the same. I expect this would have side effects. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:230: nit: Delete this empty line. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:236: account_tracker_service_->AddObserver(this); I think the idea here is that the ATS is still fetching the AccountInfo but will soon, so we wait for that fetch to complete. Two concerns: 1) How do we know that the ATS is actually working? 2) How do we know that the result we will observe will be for the Primary account? We should start observing the ATS as soon as we get a reference to it, Remove ourselves as an observer when appropriate (on deletion of this object? Perhaps the ATS will have to be responsible, I'm not sure if there's any consistent guidelines around the lifetime of the ProfileDownloader, unless we make it a PKS). That way the ProfileDownloader stays up to date with the ATS's updates. Also, this would imply the observer methods need to verify we're being notified for the Primary Account, which is a good thing. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:292: if (!profile_image_fetcher_) { Why not reset and Create a new fetcher? That way, if the ImageURL or other data gets changed for some reason, we'll fetch the Newest and Latest image rather than ignore the update and sticking with the old image. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:314: if (network_error || source->GetResponseCode() != 200) { We should include automatic retry with exponential backoff for transient errors. Also, if our Access Token was stale/invalid (such as if OnAccountUpdated was called and we didn't have a good access token at the time) we should fetch a new token and try again with it. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:327: profile_image_fetcher_.reset(); Do we need to reset() the profile_image_fetcher_ before calling OnProfileDownloadFailure(), in case the delegate's reaction is to Synchronously retry the fetch? https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:388: FetchImageData(account_info_.picture_url); How do we know the Access Token is valid when FetchImageData is called? Either fetch a new one here, or ensure OnURLFetchComplete is robust to failures. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_downloader.h (right): https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.h:16: #include "google_apis/gaia/gaia_oauth_client.h" Probably don't need this include anymore. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.h:57: AccountTrackerService* account_tracker_service = nullptr); Pretty sure Chromium style guide doesn't permit default parameters. Also - we can access the Profile* through the ProfileDownloaderDelegate and then use the AccountTrackerServiceFactory to get the appropriate AccountTrackerService*. Why add it here? https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.h:136: AccountTrackerService::AccountInfo account_info_; A profile has multiple AccountInfo objects associated to it. This tracks, I believe, information from only the primary account? If so - can we rename this to primary_account_info_? https://codereview.chromium.org/1091363002/diff/1/components/signin/core/brow... File components/signin/core/browser/account_tracker_service.cc (right): https://codereview.chromium.org/1091363002/diff/1/components/signin/core/brow... components/signin/core/browser/account_tracker_service.cc:489: if (dict->GetString(kAccountPictureURLPath, &value)) Roger: Does every account HAVE a picture? What if the user never set one and still uses the default blue head? https://codereview.chromium.org/1091363002/diff/1/components/signin/core/brow... File components/signin/core/browser/account_tracker_service.h (right): https://codereview.chromium.org/1091363002/diff/1/components/signin/core/brow... components/signin/core/browser/account_tracker_service.h:57: // TODO(rogerta): eventually this structure will include other information Please update this TODO with the items that actually still need to be added (eg services)
Some initial comments. Need to check to how the profile downloader is used in clank, since it may be used *before* the profile is signed in to show the sign in promo carousel. If this is the case, then it might necessary for the downloader to use the gaiaauthfether directly if the refresh token is not available, and ATS when it is. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_downloader.cc (right): https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:162: !BrowserThread::IsMessageLoopValid(BrowserThread::UI)); On 2015/04/20 13:25:23, Mike Lerman wrote: > I don't understand why you need IsMessageLoopValid. Generally we just use > CurrentlyOn and everything tests fine? Use a TestBrowserThreadBundle in the test. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:186: if (account_tracker_service) On 2015/04/20 13:25:22, Mike Lerman wrote: > Since we now have a reference to a PKS, I wonder if this should be a PKS. > > Also - we can set this in the constructor using the delegate. I think it might be worth making this a PKS, since I have seen crashes in the past during shutdown due to this class. I don't think this CL makes that more problematic, since this class today depends directly on the Profile pointer. Depending on how big this CL gets, I think making it into a PKS could be done in a follow up CL. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:194: return base::UTF8ToUTF16(account_info_.hosted_domain); On 2015/04/20 13:25:23, Mike Lerman wrote: > I'm not convinced we still need these methods at all. The store of truth for > these pieces of data is supposed to be the ATS. We could either remove them > completely and make all things that references them reference the ATS instead > (though that requires consumers of Profile information to know quite a bit about > Signin classes), or have these methods get the value from the ATS. Agreed we don't need these methods. As is, the code is already getting the values from ATS, since they come from the account_info_ member. Again, depending on hos big this CL gets, this cleanup could be done in a follow up CL. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:236: account_tracker_service_->AddObserver(this); On 2015/04/20 13:25:23, Mike Lerman wrote: > I think the idea here is that the ATS is still fetching the AccountInfo but will > soon, so we wait for that fetch to complete. Two concerns: > 1) How do we know that the ATS is actually working? > 2) How do we know that the result we will observe will be for the Primary > account? > > We should start observing the ATS as soon as we get a reference to it, Remove > ourselves as an observer when appropriate (on deletion of this object? Perhaps > the ATS will have to be responsible, I'm not sure if there's any consistent > guidelines around the lifetime of the ProfileDownloader, unless we make it a > PKS). That way the ProfileDownloader stays up to date with the ATS's updates. > Also, this would imply the observer methods need to verify we're being notified > for the Primary Account, which is a good thing. Right, would be better to get pointer to ATS in ctor and start observing right away. I don't think we want to add knowledge specific about the primary account. Instead the observer should check that the notification is for the account |account_id_|. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:263: void ProfileDownloader::FetchImageData(std::string image_url) { This should probably be a no-arg function. All callers pass |account_info_.picture_url| as arg. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:388: FetchImageData(account_info_.picture_url); On 2015/04/20 13:25:23, Mike Lerman wrote: > How do we know the Access Token is valid when FetchImageData is called? Either > fetch a new one here, or ensure OnURLFetchComplete is robust to failures. I think this CL needs to turn the order of operations around. Looking at the old code, it did this: . when StartForAccount() is called, look for a refresh token. If available, use it. Otherwise wait an indefinite amount of time for the token. . when the token is available, call userinfo. . when userinfo returns, fetch the image. The new code does this: . when StartForAccount() is called, look for a refresh token. If available, use it. Otherwise wait an indefinite amount of time for the token. . when the the token is available, see if the ATS has valid info. If not, wait for an indefinite amount of time. . once the info is available, fetch the image. It might be better to do the following https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_downloader.h (right): https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.h:136: AccountTrackerService::AccountInfo account_info_; On 2015/04/20 13:25:23, Mike Lerman wrote: > A profile has multiple AccountInfo objects associated to it. This tracks, I > believe, information from only the primary account? If so - can we rename this > to primary_account_info_? Actually, StartForAccount() can be called with an arbitrary account_id. so I think the original name is fine. https://codereview.chromium.org/1091363002/diff/1/components/signin/core/brow... File components/signin/core/browser/account_tracker_service.cc (right): https://codereview.chromium.org/1091363002/diff/1/components/signin/core/brow... components/signin/core/browser/account_tracker_service.cc:489: if (dict->GetString(kAccountPictureURLPath, &value)) On 2015/04/20 13:25:23, Mike Lerman wrote: > Roger: Does every account HAVE a picture? What if the user never set one and > still uses the default blue head? That's a good question, don't have the answer. If an account without a user defined picture returns nothing for this path, then we need to have a special string like "NO_PICTURE" or something. Similar to what we do with hosted domain.
Thanks for your comments, most of them are addressed below. Re: making this a PKS: I'd be inclined to wait for a future CL although I agree it makes sense. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_downloader.cc (right): https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:28: #include "google_apis/gaia/gaia_constants.h" On 2015/04/20 13:25:22, Mike Lerman wrote: > I imagine these google_apis/gaia includes aren't needed anymore. gaia_constants still is but gaia_urls isn't anymore. Thanks! https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:162: !BrowserThread::IsMessageLoopValid(BrowserThread::UI)); On 2015/04/20 13:25:23, Mike Lerman wrote: > I don't understand why you need IsMessageLoopValid. Generally we just use > CurrentlyOn and everything tests fine? Done. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:219: return account_info_.picture_url; On 2015/04/20 13:25:23, Mike Lerman wrote: > This used to be the result of GetImageURLWithSize. Now it's the raw url from the > ATS - these aren't the same. I expect this would have side effects. You're right, fixed! https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:230: On 2015/04/20 13:25:23, Mike Lerman wrote: > nit: Delete this empty line. Done. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:236: account_tracker_service_->AddObserver(this); On 2015/04/21 14:00:33, Roger Tawa wrote: > On 2015/04/20 13:25:23, Mike Lerman wrote: > > I think the idea here is that the ATS is still fetching the AccountInfo but > will > > soon, so we wait for that fetch to complete. Two concerns: > > 1) How do we know that the ATS is actually working? > > 2) How do we know that the result we will observe will be for the Primary > > account? > > > > We should start observing the ATS as soon as we get a reference to it, Remove > > ourselves as an observer when appropriate (on deletion of this object? Perhaps > > the ATS will have to be responsible, I'm not sure if there's any consistent > > guidelines around the lifetime of the ProfileDownloader, unless we make it a > > PKS). That way the ProfileDownloader stays up to date with the ATS's updates. > > Also, this would imply the observer methods need to verify we're being > notified > > for the Primary Account, which is a good thing. > > Right, would be better to get pointer to ATS in ctor and start observing right > away. > > I don't think we want to add knowledge specific about the primary account. > Instead the observer should check that the notification is for the account > |account_id_|. Makes sense, done. The new version starts observing right away and keeps account_info_ up to date considering the account id the downloader was started with. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:263: void ProfileDownloader::FetchImageData(std::string image_url) { On 2015/04/21 14:00:33, Roger Tawa wrote: > This should probably be a no-arg function. All callers pass > |account_info_.picture_url| as arg. Done. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:292: if (!profile_image_fetcher_) { On 2015/04/20 13:25:23, Mike Lerman wrote: > Why not reset and Create a new fetcher? That way, if the ImageURL or other data > gets changed for some reason, we'll fetch the Newest and Latest image rather > than ignore the update and sticking with the old image. That makes a lot of sense. I wasn't resetting the fetcher inside this function to be able to mock it in unit tests (see ProfileDownloader::StartFetchingImageForTesting above) but I've taken a different approach now. Let me know what you think! https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:314: if (network_error || source->GetResponseCode() != 200) { On 2015/04/20 13:25:22, Mike Lerman wrote: > We should include automatic retry with exponential backoff for transient errors. > > Also, if our Access Token was stale/invalid (such as if OnAccountUpdated was > called and we didn't have a good access token at the time) we should fetch a new > token and try again with it. RE: automatic retry: What's the desired behavior on failure? Should we retry transparently or should we still call OnProfileDownloadFailure and then retry? I imagine the former is the way to go but just to be sure. As for exponential backoff, which time values should be used? https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:327: profile_image_fetcher_.reset(); On 2015/04/20 13:25:23, Mike Lerman wrote: > Do we need to reset() the profile_image_fetcher_ before calling > OnProfileDownloadFailure(), in case the delegate's reaction is to Synchronously > retry the fetch? Can fetchers be reused? Let's say the delegate decided to retry, would calling Start() a second time on the same fetcher have the expected behavior? In this case the right thing is probably to reset() before calling OnProfileDownloadFailure in case the image url changed anyways but I'm just wondering. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:388: FetchImageData(account_info_.picture_url); On 2015/04/21 14:00:34, Roger Tawa wrote: > On 2015/04/20 13:25:23, Mike Lerman wrote: > > How do we know the Access Token is valid when FetchImageData is called? Either > > fetch a new one here, or ensure OnURLFetchComplete is robust to failures. > > I think this CL needs to turn the order of operations around. Looking at the > old code, it did this: > > . when StartForAccount() is called, look for a refresh token. If available, use > it. Otherwise wait an indefinite amount of time for the token. > . when the token is available, call userinfo. > . when userinfo returns, fetch the image. > > The new code does this: > > . when StartForAccount() is called, look for a refresh token. If available, use > it. Otherwise wait an indefinite amount of time for the token. > . when the the token is available, see if the ATS has valid info. If not, wait > for an indefinite amount of time. > . once the info is available, fetch the image. > > It might be better to do the following The last part of that comment seems to have been cut off, can you clarify? https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_downloader.h (right): https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.h:16: #include "google_apis/gaia/gaia_oauth_client.h" On 2015/04/20 13:25:23, Mike Lerman wrote: > Probably don't need this include anymore. Done. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.h:57: AccountTrackerService* account_tracker_service = nullptr); On 2015/04/20 13:25:23, Mike Lerman wrote: > Pretty sure Chromium style guide doesn't permit default parameters. > > Also - we can access the Profile* through the ProfileDownloaderDelegate and then > use the AccountTrackerServiceFactory to get the appropriate > AccountTrackerService*. Why add it here? You're right, it's a remain of my trying a few things to make the tests. It's not needed anymore so it's removed now. Thanks! https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.h:136: AccountTrackerService::AccountInfo account_info_; On 2015/04/21 14:00:34, Roger Tawa wrote: > On 2015/04/20 13:25:23, Mike Lerman wrote: > > A profile has multiple AccountInfo objects associated to it. This tracks, I > > believe, information from only the primary account? If so - can we rename this > > to primary_account_info_? > > Actually, StartForAccount() can be called with an arbitrary account_id. so I > think the original name is fine. Acknowledged. https://codereview.chromium.org/1091363002/diff/1/components/signin/core/brow... File components/signin/core/browser/account_tracker_service.cc (right): https://codereview.chromium.org/1091363002/diff/1/components/signin/core/brow... components/signin/core/browser/account_tracker_service.cc:489: if (dict->GetString(kAccountPictureURLPath, &value)) On 2015/04/21 14:00:34, Roger Tawa wrote: > On 2015/04/20 13:25:23, Mike Lerman wrote: > > Roger: Does every account HAVE a picture? What if the user never set one and > > still uses the default blue head? > > That's a good question, don't have the answer. If an account without a user > defined picture returns nothing for this path, then we need to have a special > string like "NO_PICTURE" or something. Similar to what we do with hosted > domain. Done. https://codereview.chromium.org/1091363002/diff/1/components/signin/core/brow... File components/signin/core/browser/account_tracker_service.h (right): https://codereview.chromium.org/1091363002/diff/1/components/signin/core/brow... components/signin/core/browser/account_tracker_service.h:57: // TODO(rogerta): eventually this structure will include other information On 2015/04/20 13:25:23, Mike Lerman wrote: > Please update this TODO with the items that actually still need to be added (eg > services) Done.
Nice progress! I think you did a rebase between the first patchset and this one? It's easier for reviewers if you git cl upload before and after a rebase, so that you can isolate the changes that came from the rebase. Thanks for the work on this. Mike https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_downloader.cc (right): https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:292: if (!profile_image_fetcher_) { On 2015/04/29 15:15:08, anthonyvd wrote: > On 2015/04/20 13:25:23, Mike Lerman wrote: > > Why not reset and Create a new fetcher? That way, if the ImageURL or other > data > > gets changed for some reason, we'll fetch the Newest and Latest image rather > > than ignore the update and sticking with the old image. > > That makes a lot of sense. I wasn't resetting the fetcher inside this function > to be able to mock it in unit tests (see > ProfileDownloader::StartFetchingImageForTesting above) but I've taken a > different approach now. Let me know what you think! commented in the header - use the TestSigninClient in tests, and use the SigninClient's URLRequestContext in your URLFetcher rather than the Profile's. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:314: if (network_error || source->GetResponseCode() != 200) { On 2015/04/29 15:15:08, anthonyvd wrote: > On 2015/04/20 13:25:22, Mike Lerman wrote: > > We should include automatic retry with exponential backoff for transient > errors. > > > > Also, if our Access Token was stale/invalid (such as if OnAccountUpdated was > > called and we didn't have a good access token at the time) we should fetch a > new > > token and try again with it. > > RE: automatic retry: What's the desired behavior on failure? Should we retry > transparently or should we still call OnProfileDownloadFailure and then retry? I > imagine the former is the way to go but just to be sure. > > As for exponential backoff, which time values should be used? Don't call OnProfileDownloadFailure - that means any dependent service would think this had failed fully, which is not the case. I'd agree with the former. Time values? There are some options in GaiaCookieManagerService or in ProfileSyncService in the net::Backoff constant. https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:327: profile_image_fetcher_.reset(); On 2015/04/29 15:15:08, anthonyvd wrote: > On 2015/04/20 13:25:23, Mike Lerman wrote: > > Do we need to reset() the profile_image_fetcher_ before calling > > OnProfileDownloadFailure(), in case the delegate's reaction is to > Synchronously > > retry the fetch? > > Can fetchers be reused? Let's say the delegate decided to retry, would calling > Start() a second time on the same fetcher have the expected behavior? > > In this case the right thing is probably to reset() before calling > OnProfileDownloadFailure in case the image url changed anyways but I'm just > wondering. You're now always reset()ing at line 295, so now it's not an issue, I think. https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_downloader.cc (right): https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:160: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Can we add a DCHECK that account_id_.empty()? https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:178: if (service->RefreshTokenIsAvailable(account_id_)) { nit: 1-line condition and statements mean we don't need braces. https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:229: if (account_info_.IsValid()) { nit: don't need braces for the if/else when each statement/condition is 1-line. https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:262: GURL url; Can you DCHECK(account_info_.IsValid())? https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_downloader.h (right): https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.h:143: TestURLFetcherProvider* test_url_fetcher_provider_; The TestURLFetcherProvider shouldn't be here. The better way to do this is to set the RequestContext on the URLFetcher to that of the SigninClient's GetURLRequestContext() method. For tests, this will be implemented by the TestSigninClient and work the way you need it to. Also, you can instatiate a net::TestURLFetcherFactory in your test to look at how to determine what the results of fetches will be. Examples are in the account_reconcilor_unittest.cc and gaia_cookie_manager_service_unittest.cc. https://codereview.chromium.org/1091363002/diff/20001/components/signin/core/... File components/signin/core/browser/account_tracker_service.cc (right): https://codereview.chromium.org/1091363002/diff/20001/components/signin/core/... components/signin/core/browser/account_tracker_service.cc:466: // TODO(rogerta): figure out when to retry. Any plans to add some automated retry logic for transient failures? Another CL? https://codereview.chromium.org/1091363002/diff/20001/components/signin/core/... File components/signin/core/browser/account_tracker_service.h (right): https://codereview.chromium.org/1091363002/diff/20001/components/signin/core/... components/signin/core/browser/account_tracker_service.h:44: // Value representing no picture URL associated with and account. nit: s/and/an/
lgtm for chrome/browser/signin and components/signin From looking at the android code, I think it will still work even when the profile is not signed in yet. On android, the AO2TS still responds for valid device accounts in this state. Therefore I don't think we do need to order reversal thing as I started to explain and then dropped off the face of the earth :-) https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_downloader.cc (right): https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:160: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2015/04/30 19:09:07, Mike Lerman wrote: > Can we add a DCHECK that account_id_.empty()? Why? An empty value means use primary account. A non-empty value means use the one specified. https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:212: if (!GetImageURLWithSize(GURL(account_info_.picture_url), Should there be a ! here? https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:229: if (account_info_.IsValid()) { On 2015/04/30 19:09:07, Mike Lerman wrote: > nit: don't need braces for the if/else when each statement/condition is 1-line. I believe you do if there is an else clause. https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:265: &url)) { Can we call GetProfilePictureURL() here? Maybe only if account_info_.IsValid()?
Comments addressed. Sorry for patch set #3, I had another commit in there by mistake. Thanks both for your time and feedback :) https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_downloader.cc (right): https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:292: if (!profile_image_fetcher_) { On 2015/04/30 19:09:06, Mike Lerman wrote: > On 2015/04/29 15:15:08, anthonyvd wrote: > > On 2015/04/20 13:25:23, Mike Lerman wrote: > > > Why not reset and Create a new fetcher? That way, if the ImageURL or other > > data > > > gets changed for some reason, we'll fetch the Newest and Latest image rather > > > than ignore the update and sticking with the old image. > > > > That makes a lot of sense. I wasn't resetting the fetcher inside this function > > to be able to mock it in unit tests (see > > ProfileDownloader::StartFetchingImageForTesting above) but I've taken a > > different approach now. Let me know what you think! > > commented in the header - use the TestSigninClient in tests, and use the > SigninClient's URLRequestContext in your URLFetcher rather than the Profile's. See header, done! https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:314: if (network_error || source->GetResponseCode() != 200) { On 2015/04/30 19:09:07, Mike Lerman wrote: > On 2015/04/29 15:15:08, anthonyvd wrote: > > On 2015/04/20 13:25:22, Mike Lerman wrote: > > > We should include automatic retry with exponential backoff for transient > > errors. > > > > > > Also, if our Access Token was stale/invalid (such as if OnAccountUpdated was > > > called and we didn't have a good access token at the time) we should fetch a > > new > > > token and try again with it. > > > > RE: automatic retry: What's the desired behavior on failure? Should we retry > > transparently or should we still call OnProfileDownloadFailure and then retry? > I > > imagine the former is the way to go but just to be sure. > > > > As for exponential backoff, which time values should be used? > > Don't call OnProfileDownloadFailure - that means any dependent service would > think this had failed fully, which is not the case. I'd agree with the former. > > Time values? There are some options in GaiaCookieManagerService or in > ProfileSyncService in the net::Backoff constant. Awesome. I suggest doing it in a follow up CL since this one already changes a lot of things and there are multiple sites were there needs to be retries. Makes sense? https://codereview.chromium.org/1091363002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_downloader.cc:327: profile_image_fetcher_.reset(); On 2015/04/30 19:09:07, Mike Lerman wrote: > On 2015/04/29 15:15:08, anthonyvd wrote: > > On 2015/04/20 13:25:23, Mike Lerman wrote: > > > Do we need to reset() the profile_image_fetcher_ before calling > > > OnProfileDownloadFailure(), in case the delegate's reaction is to > > Synchronously > > > retry the fetch? > > > > Can fetchers be reused? Let's say the delegate decided to retry, would calling > > Start() a second time on the same fetcher have the expected behavior? > > > > In this case the right thing is probably to reset() before calling > > OnProfileDownloadFailure in case the image url changed anyways but I'm just > > wondering. > > You're now always reset()ing at line 295, so now it's not an issue, I think. Right, it was just a generally useful thing to know :) https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_downloader.cc (right): https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:160: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2015/05/01 15:51:26, Roger Tawa wrote: > On 2015/04/30 19:09:07, Mike Lerman wrote: > > Can we add a DCHECK that account_id_.empty()? > > Why? An empty value means use primary account. A non-empty value means use the > one specified. Acknowledged. https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:178: if (service->RefreshTokenIsAvailable(account_id_)) { On 2015/04/30 19:09:07, Mike Lerman wrote: > nit: 1-line condition and statements mean we don't need braces. Done. https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:212: if (!GetImageURLWithSize(GURL(account_info_.picture_url), On 2015/05/01 15:51:26, Roger Tawa wrote: > Should there be a ! here? Nope, good catch. Thanks! https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:229: if (account_info_.IsValid()) { On 2015/04/30 19:09:07, Mike Lerman wrote: > nit: don't need braces for the if/else when each statement/condition is 1-line. Done. https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:229: if (account_info_.IsValid()) { On 2015/05/01 15:51:26, Roger Tawa wrote: > On 2015/04/30 19:09:07, Mike Lerman wrote: > > nit: don't need braces for the if/else when each statement/condition is > 1-line. > > I believe you do if there is an else clause. The style guide only says: "In general, curly braces are not required for single-line statements, but they are allowed if you like them" and "However, if one part of an if-else statement uses curly braces, the other part must too". I personally prefer always having braces but I'll remove them for consistency here. https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:262: GURL url; On 2015/04/30 19:09:07, Mike Lerman wrote: > Can you DCHECK(account_info_.IsValid())? Done. https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.cc:265: &url)) { On 2015/05/01 15:51:26, Roger Tawa wrote: > Can we call GetProfilePictureURL() here? Maybe only if account_info_.IsValid()? Makes sense, done! https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_downloader.h (right): https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.h:143: TestURLFetcherProvider* test_url_fetcher_provider_; On 2015/04/30 19:09:07, Mike Lerman wrote: > The TestURLFetcherProvider shouldn't be here. The better way to do this is to > set the RequestContext on the URLFetcher to that of the SigninClient's > GetURLRequestContext() method. For tests, this will be implemented by the > TestSigninClient and work the way you need it to. > > Also, you can instatiate a net::TestURLFetcherFactory in your test to look at > how to determine what the results of fetches will be. Examples are in the > account_reconcilor_unittest.cc and gaia_cookie_manager_service_unittest.cc. I *think* I'm doing it correctly now :) https://codereview.chromium.org/1091363002/diff/20001/components/signin/core/... File components/signin/core/browser/account_tracker_service.cc (right): https://codereview.chromium.org/1091363002/diff/20001/components/signin/core/... components/signin/core/browser/account_tracker_service.cc:466: // TODO(rogerta): figure out when to retry. On 2015/04/30 19:09:07, Mike Lerman wrote: > Any plans to add some automated retry logic for transient failures? Another CL? I'd do it in a follow up. See other comment. https://codereview.chromium.org/1091363002/diff/20001/components/signin/core/... File components/signin/core/browser/account_tracker_service.h (right): https://codereview.chromium.org/1091363002/diff/20001/components/signin/core/... components/signin/core/browser/account_tracker_service.h:44: // Value representing no picture URL associated with and account. On 2015/04/30 19:09:07, Mike Lerman wrote: > nit: s/and/an/ Done.
One last little thing, then LGTM :) Thanks! https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_downloader.h (right): https://codereview.chromium.org/1091363002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader.h:143: TestURLFetcherProvider* test_url_fetcher_provider_; On 2015/05/01 19:16:34, anthonyvd wrote: > On 2015/04/30 19:09:07, Mike Lerman wrote: > > The TestURLFetcherProvider shouldn't be here. The better way to do this is to > > set the RequestContext on the URLFetcher to that of the SigninClient's > > GetURLRequestContext() method. For tests, this will be implemented by the > > TestSigninClient and work the way you need it to. > > > > Also, you can instatiate a net::TestURLFetcherFactory in your test to look at > > how to determine what the results of fetches will be. Examples are in the > > account_reconcilor_unittest.cc and gaia_cookie_manager_service_unittest.cc. > > I *think* I'm doing it correctly now :) I think so, too! https://codereview.chromium.org/1091363002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_downloader_unittest.cc (right): https://codereview.chromium.org/1091363002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader_unittest.cc:53: signin_client_.reset(new TestSigninClient); This should be the SigninClient from the Factory. Since line 47 sets the ChromeSigninClientFactory to build TestSigninClients, you should here use the Factory to get a reference to the SigninClient associated with the profile.
anthonyvd@chromium.org changed reviewers: + treib@chromium.org
Hi treib@, could you please take a quick look at chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.cc? Thanks! https://codereview.chromium.org/1091363002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_downloader_unittest.cc (right): https://codereview.chromium.org/1091363002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_downloader_unittest.cc:53: signin_client_.reset(new TestSigninClient); On 2015/05/04 17:17:42, Mike Lerman wrote: > This should be the SigninClient from the Factory. Since line 47 sets the > ChromeSigninClientFactory to build TestSigninClients, you should here use the > Factory to get a reference to the SigninClient associated with the profile. Done.
lgtm for supervised_user https://codereview.chromium.org/1091363002/diff/80001/chrome/browser/supervis... File chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.cc (right): https://codereview.chromium.org/1091363002/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.cc:46: profile_downloader_.reset( nit: Is this a valid way to break the line? I'd write it as profile_downloader_.reset(new ProfileDownloader( this, ChromeSigninClientFactory::GetForProfile(custodian_profile_)));
The CQ bit was checked by anthonyvd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org, mlerman@chromium.org, treib@chromium.org Link to the patchset: https://codereview.chromium.org/1091363002/#ps160001 (title: "Fix bad merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1091363002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by anthonyvd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, mlerman@chromium.org, rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/1091363002/#ps180001 (title: "Fix chromeos and android builds.")
The CQ bit was unchecked by anthonyvd@chromium.org
The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1091363002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
anthonyvd@chromium.org changed reviewers: + nkostylev@chromium.org
Hello, nkostylev@ could you please take a look at the changes in user_image_manager_browsertest.cc that I made to adapt to the ProfileDownloader changes in this CL? Thanks a lot for your time! https://codereview.chromium.org/1091363002/diff/80001/chrome/browser/supervis... File chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.cc (right): https://codereview.chromium.org/1091363002/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.cc:46: profile_downloader_.reset( On 2015/05/06 21:30:05, Marc Treib wrote: > nit: Is this a valid way to break the line? I'd write it as > profile_downloader_.reset(new ProfileDownloader( > this, ChromeSigninClientFactory::GetForProfile(custodian_profile_))); Done.
lgtm https://codereview.chromium.org/1091363002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/avatar/user_image_manager_browsertest.cc (right): https://codereview.chromium.org/1091363002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/avatar/user_image_manager_browsertest.cc:225: info.account_id = ""; nit: std::string() https://codereview.chromium.org/1091363002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/avatar/user_image_manager_browsertest.cc:230: info.hosted_domain = ""; nit: same here
The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, mlerman@chromium.org, rogerta@chromium.org, nkostylev@chromium.org Link to the patchset: https://codereview.chromium.org/1091363002/#ps220001 (title: "Nits.")
https://codereview.chromium.org/1091363002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/avatar/user_image_manager_browsertest.cc (right): https://codereview.chromium.org/1091363002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/avatar/user_image_manager_browsertest.cc:225: info.account_id = ""; On 2015/05/14 18:44:22, Nikita wrote: > nit: std::string() Done. https://codereview.chromium.org/1091363002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/avatar/user_image_manager_browsertest.cc:230: info.hosted_domain = ""; On 2015/05/14 18:44:22, Nikita wrote: > nit: same here Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1091363002/220001
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 anthonyvd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1091363002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/56e93a34eefd5e1c8b54cc958aca3ad04d3d8329 Cr-Commit-Position: refs/heads/master@{#330566} |
