Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12380006:
Build a new TokenCacheService so I can stop using TokenService for something it wasn't designed for. (Closed)
|
Created:
7 years, 9 months ago by Pete Williamson Modified:
7 years, 9 months ago Reviewers:
Michael Courage, sky, Andrew T Wilson (Slow), dcheng, asargent_no_longer_on_chrome, tim (not reviewing) CC:
chromium-reviews, akalin, Raghu Simha, sail+watch_chromium.org, Aaron Boodman, chromium-apps-reviews_chromium.org, haitaol1, tim (not reviewing) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionBuild a new TokenCacheService so I can stop using TokenService.
When building code to cache the Obfuscated Gaia Id that we receive from the server, we had used the TokenService to cache it. However, the Token service wasn't designed to cache arbitrary tokens, it was designed to fetch tokens, and cached the fetched token as a side effect.
This patch delivers a new TokenCache service that does nothing more than cache tokens on a per-profile basis. The Cache will be cleared out when the user signs out and when chromium exits.
This patch also removes the code I added to TokenService to allow manually adding a token, since we no longer need that code.
BUG=177125
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188130
Patch Set 1 #
Total comments: 55
Patch Set 2 : Token Cache - CR changes per DCheng and ATWilson #
Total comments: 30
Patch Set 3 : Token Cache - feedback per DCheng #Patch Set 4 : Token #Patch Set 5 : Token Cache Service - update namespace #
Total comments: 36
Patch Set 6 : Token Cache Service - more CR fixes per DCheng #
Total comments: 19
Patch Set 7 : Token Cache Service - CR fixes per DCheng and ATWilson #
Total comments: 14
Patch Set 8 : Token Cache Service - fix nits #
Total comments: 2
Patch Set 9 : Token Cache Service - fix gypi #
Total comments: 8
Patch Set 10 : Token Cache Service - post LGTM nits and merging with master #Messages
Total messages: 30 (0 generated)
Which reviewers should review what: sky@: OWNERS review of the new directory I'm adding to chrome/browser, and all its contents, c/b/profiles changes, and the .gypi file ATWilson@: Please make sure this addresses your original concern, and OWNERS review of c/b/signin changes Tim@: OWNERS review for google_apis/gaia changes please DCheng@: Please check everything, OWNERS review of push_messaging_api.cc Courage@: You are here for FYI, but feel free to bring up any issues you may have.
https://codereview.chromium.org/12380006/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:22: #include "chrome/browser/signin/token_service_factory.h" Seems like we don't need these includes anymore. https://codereview.chromium.org/12380006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:133: if (token_cache != NULL) { Why would this be NULL? https://codereview.chromium.org/12380006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:185: if (token_cache) { Ditto. Why would this be NULL? https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... File chrome/browser/token_cache/token_cache.cc (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.cc:7: #include <algorithm> Are you actually using this header? https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.cc:10: #include "base/time.h" Typically, people only #include a file in the header or the .cc, not both. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.cc:37: TimeDelta delta = TimeDelta::FromMilliseconds(expiration_timeout); I think second granularity is sufficient. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.cc:39: } else if (expiration_timeout < 0) { I think it is nonsensical to pass in a negative time delta. We should just CHECK or DCHECK this. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.cc:71: token_cache_.clear(); So we don't persist this to disk at all anymore? I guess that's OK. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.cc:92: Time zero; A simpler way would be to use time.is_null(). https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... File chrome/browser/token_cache/token_cache.h (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:5: // This class caches tokens for the current user. It will flush tokens out "flush" is an ambiguous term, since it could mean flushing the entries out to disk. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:12: #include <algorithm> Why is this in the header? https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:35: content::NotificationObserver { Indent. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:46: const int64 expiration_timeout); Use base::TimeDelta instead. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:66: std::vector<TokenCacheData> token_cache_; Why not just use a map since that's logically what it is anyway. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:68: Profile* profile_; Profile* const. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:70: friend class TokenCacheTest; DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... File chrome/browser/token_cache/token_cache_factory.cc (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache_factory.cc:25: DependsOn(extensions::ExtensionSystemFactory::GetInstance()); Shouldn't this dependency edge be the other way around? The extensions system depends on this. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... File chrome/browser/token_cache/token_cache_factory.h (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache_factory.h:5: // This class caches tokens for the curren user. It will flush tokens out Remove this comment. This is just a factory class, and so file-level comment would be more appropriate in token_cache.h (where it already is).
I'd either put this service under extensions/api (since you're the only user) or, if you wanted to change OAuth2TokenService to use this cache to store its tokens, then you might put it under browser/signin. I don't think we should create a new toplevel directory for it. https://codereview.chromium.org/12380006/diff/1/chrome/browser/signin/token_s... File chrome/browser/signin/token_service.cc (left): https://codereview.chromium.org/12380006/diff/1/chrome/browser/signin/token_s... chrome/browser/signin/token_service.cc:107: void TokenService::AddAuthTokenManually(const std::string& service, I think we should keep this code as it's the primary way for people to feed tokens into TokenService (like the sync token from SyncBackendHost) https://codereview.chromium.org/12380006/diff/1/chrome/browser/sync/glue/sync... File chrome/browser/sync/glue/sync_backend_host.cc (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/sync/glue/sync... chrome/browser/sync/glue/sync_backend_host.cc:1589: token_cache->StoreToken(GaiaConstants::kObfuscatedGaiaId, details.token(), Should we really be writing the sync token as the kObfuscatedGaiaId? this seems incorrect - I thought this code was writing the chromiumsync token, so it should be calling AddAuthTokenManually. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... File chrome/browser/token_cache/token_cache_factory.cc (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache_factory.cc:25: DependsOn(extensions::ExtensionSystemFactory::GetInstance()); Agreed, this shouldn't depend on ExtensionSystemFactory. In fact, I don't think ExtensionSystemFactory should depend on this either, unless you need to access this service from ExtensionSystemFactory::Initialize(). https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache_factory.cc:36: bool TokenCacheServiceFactory::ServiceRedirectedInIncognito() const { I don't think you want this. I think you want null in incognito, because incognito windows are supposed to act as if they are not signed in. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache_factory.cc:41: return true; Why do we need to create this with the profile? Typically, you just have the service get created when it's needed (lazy-init). https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache_factory.cc:45: return true; Why are we doing this? There's no reason not to have this for a TestingProfile.
Addressed a round of CR feedback, ATWilson and DCheng, please take another look https://codereview.chromium.org/12380006/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:22: #include "chrome/browser/signin/token_service_factory.h" On 2013/02/28 19:18:04, dcheng wrote: > Seems like we don't need these includes anymore. Nope, these are still used elsewhere (StartGaiaIdFetch, etc.) https://codereview.chromium.org/12380006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:133: if (token_cache != NULL) { On 2013/02/28 19:18:04, dcheng wrote: > Why would this be NULL? I was thinking out of memory in the token cache service. However, if the out of memory handler catches the exception, I don't need to test for null here. Test removed. https://codereview.chromium.org/12380006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:185: if (token_cache) { On 2013/02/28 19:18:04, dcheng wrote: > Ditto. Why would this be NULL? Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/signin/token_s... File chrome/browser/signin/token_service.cc (left): https://codereview.chromium.org/12380006/diff/1/chrome/browser/signin/token_s... chrome/browser/signin/token_service.cc:107: void TokenService::AddAuthTokenManually(const std::string& service, On 2013/03/01 17:50:54, Andrew T Wilson wrote: > I think we should keep this code as it's the primary way for people to feed > tokens into TokenService (like the sync token from SyncBackendHost) Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/sync/glue/sync... File chrome/browser/sync/glue/sync_backend_host.cc (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/sync/glue/sync... chrome/browser/sync/glue/sync_backend_host.cc:1589: token_cache->StoreToken(GaiaConstants::kObfuscatedGaiaId, details.token(), On 2013/03/01 17:50:54, Andrew T Wilson wrote: > Should we really be writing the sync token as the kObfuscatedGaiaId? this seems > incorrect - I thought this code was writing the chromiumsync token, so it should > be calling AddAuthTokenManually. Good catch, reverted. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... File chrome/browser/token_cache/token_cache.cc (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.cc:7: #include <algorithm> On 2013/02/28 19:18:04, dcheng wrote: > Are you actually using this header? Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.cc:10: #include "base/time.h" On 2013/02/28 19:18:04, dcheng wrote: > Typically, people only #include a file in the header or the .cc, not both. Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.cc:37: TimeDelta delta = TimeDelta::FromMilliseconds(expiration_timeout); On 2013/02/28 19:18:04, dcheng wrote: > I think second granularity is sufficient. I'm using time delta now, so it doesn't matter. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.cc:39: } else if (expiration_timeout < 0) { On 2013/02/28 19:18:04, dcheng wrote: > I think it is nonsensical to pass in a negative time delta. We should just CHECK > or DCHECK this. using time delta now, no need to check https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.cc:71: token_cache_.clear(); On 2013/02/28 19:18:04, dcheng wrote: > So we don't persist this to disk at all anymore? I guess that's OK. Right, this is not persisted to disk. You could still hit the rate limit if you keep starting and shutting down chrome quickly https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.cc:92: Time zero; On 2013/02/28 19:18:04, dcheng wrote: > A simpler way would be to use time.is_null(). Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... File chrome/browser/token_cache/token_cache.h (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:5: // This class caches tokens for the current user. It will flush tokens out On 2013/02/28 19:18:04, dcheng wrote: > "flush" is an ambiguous term, since it could mean flushing the entries out to > disk. Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:12: #include <algorithm> On 2013/02/28 19:18:04, dcheng wrote: > Why is this in the header? This was leftover from when I tried the find-if version. I'll remove it for now and put it back if I refactor to find-if. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:35: content::NotificationObserver { On 2013/02/28 19:18:04, dcheng wrote: > Indent. Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:46: const int64 expiration_timeout); On 2013/02/28 19:18:04, dcheng wrote: > Use base::TimeDelta instead. Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:66: std::vector<TokenCacheData> token_cache_; On 2013/02/28 19:18:04, dcheng wrote: > Why not just use a map since that's logically what it is anyway. My concern is efficiency. Maps generate a lot more code, and are only faster when you have lots of entries due to locality of reference. Vectors are much faster until the search time dominates. Currently only one token will be here, so I don't want to pay the overhead of a map. I don't expect breakeven until we store more than 100 tokens. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:68: Profile* profile_; On 2013/02/28 19:18:04, dcheng wrote: > Profile* const. changed to "const Profile* profile_;" https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:70: friend class TokenCacheTest; On 2013/02/28 19:18:04, dcheng wrote: > DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... File chrome/browser/token_cache/token_cache_factory.cc (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache_factory.cc:25: DependsOn(extensions::ExtensionSystemFactory::GetInstance()); On 2013/03/01 17:50:54, Andrew T Wilson wrote: > Agreed, this shouldn't depend on ExtensionSystemFactory. In fact, I don't think > ExtensionSystemFactory should depend on this either, unless you need to access > this service from ExtensionSystemFactory::Initialize(). Dependency removed (It was cut and pasted from an example, I should have understood that it was not required here.) https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache_factory.cc:36: bool TokenCacheServiceFactory::ServiceRedirectedInIncognito() const { On 2013/03/01 17:50:54, Andrew T Wilson wrote: > I don't think you want this. I think you want null in incognito, because > incognito windows are supposed to act as if they are not signed in. Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache_factory.cc:41: return true; On 2013/03/01 17:50:54, Andrew T Wilson wrote: > Why do we need to create this with the profile? Typically, you just have the > service get created when it's needed (lazy-init). Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache_factory.cc:45: return true; On 2013/03/01 17:50:54, Andrew T Wilson wrote: > Why are we doing this? There's no reason not to have this for a TestingProfile. Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... File chrome/browser/token_cache/token_cache_factory.h (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache_factory.h:5: // This class caches tokens for the curren user. It will flush tokens out On 2013/02/28 19:18:04, dcheng wrote: > Remove this comment. This is just a factory class, and so file-level comment > would be more appropriate in token_cache.h (where it already is). Done.
https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... File chrome/browser/token_cache/token_cache.cc (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.cc:39: } else if (expiration_timeout < 0) { On 2013/03/04 18:32:53, Pete Williamson wrote: > On 2013/02/28 19:18:04, dcheng wrote: > > I think it is nonsensical to pass in a negative time delta. We should just > CHECK > > or DCHECK this. > > using time delta now, no need to check Well, TimeDelta can still be negative. It's probably still worth it to DCHECK on this situation since doing so is a logical error. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... File chrome/browser/token_cache/token_cache.h (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:35: content::NotificationObserver { On 2013/03/04 18:32:53, Pete Williamson wrote: > On 2013/02/28 19:18:04, dcheng wrote: > > Indent. > > Done. Sorry, I should have clarified (and I also missed something). This should be public ProfiledKeyedService, public content::NotificationObserver, and the "public" keywords should be aligned under each other. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:66: std::vector<TokenCacheData> token_cache_; On 2013/03/04 18:32:53, Pete Williamson wrote: > On 2013/02/28 19:18:04, dcheng wrote: > > Why not just use a map since that's logically what it is anyway. > > My concern is efficiency. Maps generate a lot more code, and are only faster > when you have lots of entries due to locality of reference. Vectors are much > faster until the search time dominates. Currently only one token will be here, > so I don't want to pay the overhead of a map. I don't expect breakeven until we > store more than 100 tokens. But then you just have to reimplement the lookup logic for a map which almost certainly won't be shared. I think it makes more sense to just use the concepts that the code is expressing. Using a vector doesn't gain much other than making the code more complicated. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:68: Profile* profile_; On 2013/03/04 18:32:53, Pete Williamson wrote: > On 2013/02/28 19:18:04, dcheng wrote: > > Profile* const. > > changed to "const Profile* profile_;" In that case, const Profile* const profile_; please. Note the positioning of the original const. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:143: Remove newline? https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/token_cache/token_cache.cc (right): https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache.cc:31: TimeDelta zeroDelta; Naming. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache.cc:66: token_cache_.clear(); Do we need this method? Can't we just let it happen in the destructor? https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/token_cache/token_cache.h (right): https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache.h:21: #include "content/public/browser/notification_service.h" I think this include isn't needed. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache.h:31: // TODO: Should I put this in a namespace? Which one? I'm not sure that "api/token_cache" is an appropriate place for this file, since it's not implementing an extension API for token_cache. For now, you could try putting it c/b/extensions. In that case, it should be in the extensions namespace. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache.h:35: Extra newline. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache.h:45: const base::TimeDelta time_to_live); No const. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/token_cache/token_cache_factory.cc (right): https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache_factory.cc:35: bool TokenCacheServiceFactory::ServiceRedirectedInIncognito() const { Please remove methods that simply use the default behavior (this is all baseline behavior defined in ProfileKeyedBaseFactory). https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/token_cache/token_cache_unittest.cc (right): https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache_unittest.cc:16: base::TimeDelta zero; Indent is incorrect. Also, I think this will create a static initializer, which is not allowed. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache_unittest.cc:27: cache_.token_cache_.clear(); Why are we doing this? https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache_unittest.cc:39: Extra newline... https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache_unittest.cc:61: Extra newline... https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache_unittest.cc:70: // autoconverting? Remove this TODO (I would argue the answer is no). https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache_unittest.cc:91: EXPECT_EQ(CacheSize(), (size_t) 1); Don't use C-style casts. In this case, you can just say 1U though. https://codereview.chromium.org/12380006/diff/9001/google_apis/gaia/gaia_cons... File google_apis/gaia/gaia_constants.cc (right): https://codereview.chromium.org/12380006/diff/9001/google_apis/gaia/gaia_cons... google_apis/gaia/gaia_constants.cc:50: const base::TimeDelta kObfuscatedGaiaIdTimeout = base::TimeDelta::FromDays(30); This creates a static initializer, which we really want to avoid. Also, if we're not writing the cache to disk... what's the likelihood that we're actually going to reach the 30 day limit?
This should address all outstanding feedback from DCheng. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... File chrome/browser/token_cache/token_cache.cc (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.cc:39: } else if (expiration_timeout < 0) { On 2013/03/04 22:40:59, dcheng wrote: > On 2013/03/04 18:32:53, Pete Williamson wrote: > > On 2013/02/28 19:18:04, dcheng wrote: > > > I think it is nonsensical to pass in a negative time delta. We should just > > CHECK > > > or DCHECK this. > > > > using time delta now, no need to check > > Well, TimeDelta can still be negative. It's probably still worth it to DCHECK on > this situation since doing so is a logical error. Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... File chrome/browser/token_cache/token_cache.h (right): https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:35: content::NotificationObserver { On 2013/03/04 22:40:59, dcheng wrote: > On 2013/03/04 18:32:53, Pete Williamson wrote: > > On 2013/02/28 19:18:04, dcheng wrote: > > > Indent. > > > > Done. > > Sorry, I should have clarified (and I also missed something). This should be > public ProfiledKeyedService, public content::NotificationObserver, and the > "public" keywords should be aligned under each other. Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:66: std::vector<TokenCacheData> token_cache_; On 2013/02/28 19:18:04, dcheng wrote: > Why not just use a map since that's logically what it is anyway. Done. https://codereview.chromium.org/12380006/diff/1/chrome/browser/token_cache/to... chrome/browser/token_cache/token_cache.h:68: Profile* profile_; On 2013/03/04 22:40:59, dcheng wrote: > On 2013/03/04 18:32:53, Pete Williamson wrote: > > On 2013/02/28 19:18:04, dcheng wrote: > > > Profile* const. > > > > changed to "const Profile* profile_;" > > In that case, const Profile* const profile_; please. Note the positioning of the > original const. Done. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:143: On 2013/03/04 22:40:59, dcheng wrote: > Remove newline? Done. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/token_cache/token_cache.cc (right): https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache.cc:31: TimeDelta zeroDelta; On 2013/03/04 22:40:59, dcheng wrote: > Naming. Done. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache.cc:66: token_cache_.clear(); On 2013/03/04 22:40:59, dcheng wrote: > Do we need this method? Can't we just let it happen in the destructor? Done. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/token_cache/token_cache.h (right): https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache.h:21: #include "content/public/browser/notification_service.h" On 2013/03/04 22:40:59, dcheng wrote: > I think this include isn't needed. Nope, we need it for content::Source<Profile> https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache.h:31: // TODO: Should I put this in a namespace? Which one? On 2013/03/04 22:40:59, dcheng wrote: > I'm not sure that "api/token_cache" is an appropriate place for this file, since > it's not implementing an extension API for token_cache. For now, you could try > putting it c/b/extensions. In that case, it should be in the extensions > namespace. Done. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache.h:35: On 2013/03/04 22:40:59, dcheng wrote: > Extra newline. Done. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache.h:45: const base::TimeDelta time_to_live); On 2013/03/04 22:40:59, dcheng wrote: > No const. Done. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/token_cache/token_cache_factory.cc (right): https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache_factory.cc:35: bool TokenCacheServiceFactory::ServiceRedirectedInIncognito() const { On 2013/03/04 22:40:59, dcheng wrote: > Please remove methods that simply use the default behavior (this is all baseline > behavior defined in ProfileKeyedBaseFactory). Done. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/token_cache/token_cache_unittest.cc (right): https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache_unittest.cc:16: base::TimeDelta zero; On 2013/03/04 22:40:59, dcheng wrote: > Indent is incorrect. Also, I think this will create a static initializer, which > is not allowed. Done. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache_unittest.cc:27: cache_.token_cache_.clear(); On 2013/03/04 22:40:59, dcheng wrote: > Why are we doing this? To make sure the cache is cleared out for the next run, we use the same cache_ for multiple tests. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache_unittest.cc:39: On 2013/03/04 22:40:59, dcheng wrote: > Extra newline... Done. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache_unittest.cc:61: On 2013/03/04 22:40:59, dcheng wrote: > Extra newline... Done. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache_unittest.cc:70: // autoconverting? On 2013/03/04 22:40:59, dcheng wrote: > Remove this TODO (I would argue the answer is no). Done. https://codereview.chromium.org/12380006/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/token_cache/token_cache_unittest.cc:91: EXPECT_EQ(CacheSize(), (size_t) 1); On 2013/03/04 22:40:59, dcheng wrote: > Don't use C-style casts. In this case, you can just say 1U though. Done. https://codereview.chromium.org/12380006/diff/9001/google_apis/gaia/gaia_cons... File google_apis/gaia/gaia_constants.cc (right): https://codereview.chromium.org/12380006/diff/9001/google_apis/gaia/gaia_cons... google_apis/gaia/gaia_constants.cc:50: const base::TimeDelta kObfuscatedGaiaIdTimeout = base::TimeDelta::FromDays(30); On 2013/03/04 22:40:59, dcheng wrote: > This creates a static initializer, which we really want to avoid. > > Also, if we're not writing the cache to disk... what's the likelihood that we're > actually going to reach the 30 day limit? Done.
https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:182: int timeoutDays = GaiaConstants::kObfuscatedGaiaIdTimeoutInDays; Naming is incorrect. Also, I'd suggest just doing this instead: base::TimeDelta timeout = base::TimeDelta::FromDays(GaiaConstants::kObfuscatedGaiaIdTimeoutInDays); https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache.cc (right): https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:45: std::map<std::string, TokenCacheData>::iterator it = FindMatch(token_name); Why do we need to use FindMatch() for this? Isn't it the same thing either way? https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:67: void TokenCacheService::Shutdown() { You can remove this, since it simply matches the default implementation now. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:79: std::map<std::string, TokenCacheData>::iterator TokenCacheService::FindMatch( Just move this logic into RetrieveToken, since this logic isn't needed in StoreToken anyway. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:81: Extra newline. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:84: it = token_cache_.find(token_name); Combine declaration and initialization. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:88: Time zero; Unused. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache.h (right): https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.h:21: #include "content/public/browser/notification_service.h" 1) Forward declare content::NotificationSource. 2) #include notification_source.h in your .cc (not notification_service.h). https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.h:33: content::NotificationObserver { Align inherited classes, add missing "public" keyword. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.h:34: Extra newline? https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_unittest.cc (right): https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_unittest.cc:32: bool HasMatch(std::string token_name) { const std::string& https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_unittest.cc:36: void InsertExpiredToken(std::string token_name, std::string token_value) { const std::string&, etc. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_unittest.cc:53: Extra newlines. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_unittest.cc:115: Remove extra newlines please. https://codereview.chromium.org/12380006/diff/18001/google_apis/gaia/gaia_con... File google_apis/gaia/gaia_constants.cc (right): https://codereview.chromium.org/12380006/diff/18001/google_apis/gaia/gaia_con... google_apis/gaia/gaia_constants.cc:50: const int kObfuscatedGaiaIdTimeoutInDays = 30; Just use one space. Lining it up like this will require lots of changes if someone ever uses a type that has a name longer than 4 characters. https://codereview.chromium.org/12380006/diff/18001/google_apis/gaia/gaia_con... File google_apis/gaia/gaia_constants.h (right): https://codereview.chromium.org/12380006/diff/18001/google_apis/gaia/gaia_con... google_apis/gaia/gaia_constants.h:10: #include "base/time.h" Remove this include.
Addressing more comments from DCheng https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:182: int timeoutDays = GaiaConstants::kObfuscatedGaiaIdTimeoutInDays; On 2013/03/05 22:38:44, dcheng wrote: > Naming is incorrect. Also, I'd suggest just doing this instead: > base::TimeDelta timeout = > base::TimeDelta::FromDays(GaiaConstants::kObfuscatedGaiaIdTimeoutInDays); Done. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache.cc (right): https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:45: std::map<std::string, TokenCacheData>::iterator it = FindMatch(token_name); On 2013/03/05 22:38:44, dcheng wrote: > Why do we need to use FindMatch() for this? Isn't it the same thing either way? FindMatch has the desirable side effect of lazily removing timed out entries. Let me know if you can think of a better name which denotes this. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:67: void TokenCacheService::Shutdown() { On 2013/03/05 22:38:44, dcheng wrote: > You can remove this, since it simply matches the default implementation now. Done. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:79: std::map<std::string, TokenCacheData>::iterator TokenCacheService::FindMatch( On 2013/03/05 22:38:44, dcheng wrote: > Just move this logic into RetrieveToken, since this logic isn't needed in > StoreToken anyway. I use this from the unit test also, so I'd like to leave it here. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:81: On 2013/03/05 22:38:44, dcheng wrote: > Extra newline. Done. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:84: it = token_cache_.find(token_name); On 2013/03/05 22:38:44, dcheng wrote: > Combine declaration and initialization. Done. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:88: Time zero; On 2013/03/05 22:38:44, dcheng wrote: > Unused. Done. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache.h (right): https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.h:21: #include "content/public/browser/notification_service.h" On 2013/03/05 22:38:44, dcheng wrote: > 1) Forward declare content::NotificationSource. > 2) #include notification_source.h in your .cc (not notification_service.h). Done. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.h:33: content::NotificationObserver { On 2013/03/05 22:38:44, dcheng wrote: > Align inherited classes, add missing "public" keyword. Done. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.h:34: On 2013/03/05 22:38:44, dcheng wrote: > Extra newline? Done. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_unittest.cc (right): https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_unittest.cc:32: bool HasMatch(std::string token_name) { On 2013/03/05 22:38:44, dcheng wrote: > const std::string& Done. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_unittest.cc:36: void InsertExpiredToken(std::string token_name, std::string token_value) { On 2013/03/05 22:38:44, dcheng wrote: > const std::string&, etc. Done. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_unittest.cc:53: On 2013/03/05 22:38:44, dcheng wrote: > Extra newlines. Done. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_unittest.cc:115: On 2013/03/05 22:38:44, dcheng wrote: > Remove extra newlines please. Done. https://codereview.chromium.org/12380006/diff/18001/google_apis/gaia/gaia_con... File google_apis/gaia/gaia_constants.cc (right): https://codereview.chromium.org/12380006/diff/18001/google_apis/gaia/gaia_con... google_apis/gaia/gaia_constants.cc:50: const int kObfuscatedGaiaIdTimeoutInDays = 30; On 2013/03/05 22:38:44, dcheng wrote: > Just use one space. Lining it up like this will require lots of changes if > someone ever uses a type that has a name longer than 4 characters. Done.
https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache.cc (right): https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:45: std::map<std::string, TokenCacheData>::iterator it = FindMatch(token_name); On 2013/03/06 00:09:40, Pete Williamson wrote: > On 2013/03/05 22:38:44, dcheng wrote: > > Why do we need to use FindMatch() for this? Isn't it the same thing either > way? > > FindMatch has the desirable side effect of lazily removing timed out entries. > Let me know if you can think of a better name which denotes this. Even if you remove it, aren't you going to immediately populate it again? I'm not seeing what calling FindMatch() accomplishes here. https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:79: std::map<std::string, TokenCacheData>::iterator TokenCacheService::FindMatch( On 2013/03/06 00:09:40, Pete Williamson wrote: > On 2013/03/05 22:38:44, dcheng wrote: > > Just move this logic into RetrieveToken, since this logic isn't needed in > > StoreToken anyway. > > I use this from the unit test also, so I'd like to leave it here. Looking at the unit test, it seems like it'd be better to use RetrieveToken() in all instances, since that's the logic we actually want to test. https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_factory.cc (right): https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_factory.cc:11: // static No indent. https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_factory.h (right): https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_factory.h:27: // Inherited from ProfileKeyedServiceFactory: Fix indent (this looks like 3 spaces) https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_unittest.cc (right): https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_unittest.cc:27: cache_.token_cache_.clear(); Do we really need to do this? If not, just remove SetUp() / TearDown() methods. https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_unittest.cc:62: EXPECT_EQ(CacheSize(), (size_t) 1); 1U. https://codereview.chromium.org/12380006/diff/26001/google_apis/gaia/gaia_con... File google_apis/gaia/gaia_constants.h (right): https://codereview.chromium.org/12380006/diff/26001/google_apis/gaia/gaia_con... google_apis/gaia/gaia_constants.h:38: // How long to keep an obfuscated Gaia Id in an internal cache, in milliseconds. Update comment.
Answers for DCheng (the changes are minor, so I will batch them with the next round) https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache.cc (right): https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:45: std::map<std::string, TokenCacheData>::iterator it = FindMatch(token_name); On 2013/03/06 00:26:35, dcheng wrote: > On 2013/03/06 00:09:40, Pete Williamson wrote: > > On 2013/03/05 22:38:44, dcheng wrote: > > > Why do we need to use FindMatch() for this? Isn't it the same thing either > > way? > > > > FindMatch has the desirable side effect of lazily removing timed out entries. > > Let me know if you can think of a better name which denotes this. > > Even if you remove it, aren't you going to immediately populate it again? I'm > not seeing what calling FindMatch() accomplishes here. No, we don't always populate it again. In the retrieve token case, we need to make sure that the token is deleted, and we are not going to reset it immediately here (though the calling code may decide to get a new token and reset it). https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:79: std::map<std::string, TokenCacheData>::iterator TokenCacheService::FindMatch( On 2013/03/06 00:26:35, dcheng wrote: > On 2013/03/06 00:09:40, Pete Williamson wrote: > > On 2013/03/05 22:38:44, dcheng wrote: > > > Just move this logic into RetrieveToken, since this logic isn't needed in > > > StoreToken anyway. > > > > I use this from the unit test also, so I'd like to leave it here. > > Looking at the unit test, it seems like it'd be better to use RetrieveToken() in > all instances, since that's the logic we actually want to test. The advantage of FindMatch in the unit test is that it can return the actual item in the test instead of just the string. RetrieveToken returns a null string in both the case where there is a null string stored as the token and no token is found. Granted, my code ended up not needing to know the difference, and I don't feel strongly about it, so I changed the unit test as you suggest. https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_factory.cc (right): https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_factory.cc:11: // static On 2013/03/06 00:26:35, dcheng wrote: > No indent. Done. https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_unittest.cc (right): https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_unittest.cc:27: cache_.token_cache_.clear(); On 2013/03/06 00:26:35, dcheng wrote: > Do we really need to do this? If not, just remove SetUp() / TearDown() methods. Done. https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_unittest.cc:62: EXPECT_EQ(CacheSize(), (size_t) 1); On 2013/03/06 00:26:35, dcheng wrote: > 1U. Done. https://codereview.chromium.org/12380006/diff/26001/google_apis/gaia/gaia_con... File google_apis/gaia/gaia_constants.h (right): https://codereview.chromium.org/12380006/diff/26001/google_apis/gaia/gaia_con... google_apis/gaia/gaia_constants.h:38: // How long to keep an obfuscated Gaia Id in an internal cache, in milliseconds. On 2013/03/06 00:26:35, dcheng wrote: > Update comment. Done.
https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache.cc (right): https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:45: std::map<std::string, TokenCacheData>::iterator it = FindMatch(token_name); On 2013/03/06 00:50:05, Pete Williamson wrote: > On 2013/03/06 00:26:35, dcheng wrote: > > On 2013/03/06 00:09:40, Pete Williamson wrote: > > > On 2013/03/05 22:38:44, dcheng wrote: > > > > Why do we need to use FindMatch() for this? Isn't it the same thing either > > > way? > > > > > > FindMatch has the desirable side effect of lazily removing timed out > entries. > > > Let me know if you can think of a better name which denotes this. > > > > Even if you remove it, aren't you going to immediately populate it again? I'm > > not seeing what calling FindMatch() accomplishes here. > > No, we don't always populate it again. In the retrieve token case, we need to > make sure that the token is deleted, and we are not going to reset it > immediately here (though the calling code may decide to get a new token and > reset it). I still don't understand. I'm talking specifically about StoreToken(). What is the point of calling FindMatch() in StoreToken()? Either way, we end up calling token_cache_[token_name] and don't use the returned iterator in any way. Why can't lines 44 to 50 simply be removed?
Mostly looks good, although I think both Daniel and I are somewhat uncomfortable with FindMatch(). https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache.cc (right): https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:47: if (it != token_cache_.end()) { Not sure why we are doing this - why not skip the call to FindMatch() entirely and just stuff it into the map? That would let you fold FindMatch() into RetrieveToken() and simplify your API. https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:75: std::map<std::string, TokenCacheData>::iterator TokenCacheService::FindMatch( BTW, you could probably name this more explicitly like FindMatchAndRemoveIfExpired(), if for some reason you don't just fold it into RetrieveToken(). https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:94: nit: remove superfluous blank line https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache.h (right): https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.h:35: class TokenCacheService : public ProfileKeyedService, BTW, technically I think this file should be named token_cache_service.* - it's non-standard (but not *technically* against the style guide) to put TokenCacheService inside token_cache.* So it's your call if you want to leave it like this, but I'd suggest renaming the files. https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.h:52: std::map<std::string, TokenCacheData>::iterator FindMatch( This seems very specific to be exposing as part of your public API. If you want to expose this for tests, then I'd recommend adding some kind of friend class statement. You should probably make this private.
On 2013/03/06 02:30:25, dcheng wrote: > https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... > File chrome/browser/extensions/token_cache/token_cache.cc (right): > > https://codereview.chromium.org/12380006/diff/18001/chrome/browser/extensions... > chrome/browser/extensions/token_cache/token_cache.cc:45: std::map<std::string, > TokenCacheData>::iterator it = FindMatch(token_name); > On 2013/03/06 00:50:05, Pete Williamson wrote: > > On 2013/03/06 00:26:35, dcheng wrote: > > > On 2013/03/06 00:09:40, Pete Williamson wrote: > > > > On 2013/03/05 22:38:44, dcheng wrote: > > > > > Why do we need to use FindMatch() for this? Isn't it the same thing > either > > > > way? > > > > > > > > FindMatch has the desirable side effect of lazily removing timed out > > entries. > > > > Let me know if you can think of a better name which denotes this. > > > > > > Even if you remove it, aren't you going to immediately populate it again? > I'm > > > not seeing what calling FindMatch() accomplishes here. > > > > No, we don't always populate it again. In the retrieve token case, we need to > > make sure that the token is deleted, and we are not going to reset it > > immediately here (though the calling code may decide to get a new token and > > reset it). > > I still don't understand. I'm talking specifically about StoreToken(). What is > the point of calling FindMatch() in StoreToken()? Either way, we end up calling > token_cache_[token_name] and don't use the returned iterator in any way. Why > can't lines 44 to 50 simply be removed? Oh, I see. Yep, we can definitely do that. Done.
Renamed files to match classes, other minor CR fixes per DCheng and ATWilson https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache.cc (right): https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:47: if (it != token_cache_.end()) { On 2013/03/06 16:55:28, Andrew T Wilson wrote: > Not sure why we are doing this - why not skip the call to FindMatch() entirely > and just stuff it into the map? > > That would let you fold FindMatch() into RetrieveToken() and simplify your API. Done. https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:75: std::map<std::string, TokenCacheData>::iterator TokenCacheService::FindMatch( On 2013/03/06 16:55:28, Andrew T Wilson wrote: > BTW, you could probably name this more explicitly like > FindMatchAndRemoveIfExpired(), if for some reason you don't just fold it into > RetrieveToken(). I just removed it. https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.cc:94: On 2013/03/06 16:55:28, Andrew T Wilson wrote: > nit: remove superfluous blank line Done. https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache.h (right): https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache.h:52: std::map<std::string, TokenCacheData>::iterator FindMatch( On 2013/03/06 16:55:28, Andrew T Wilson wrote: > This seems very specific to be exposing as part of your public API. If you want > to expose this for tests, then I'd recommend adding some kind of friend class > statement. You should probably make this private. Removed https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_factory.h (right): https://codereview.chromium.org/12380006/diff/26001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_factory.h:27: // Inherited from ProfileKeyedServiceFactory: On 2013/03/06 00:26:35, dcheng wrote: > Fix indent (this looks like 3 spaces) Done.
LGTM with nits addressed. https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_service_factory.cc (right): https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service_factory.cc:14: return static_cast<extensions::TokenCacheService*>( Indent is weird. https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_service_unittest.cc (right): https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service_unittest.cc:56: EXPECT_EQ(CacheSize(), 1U); Nit: EXPECT_EQ(<actual>, <expected>) is the preferred order, so this (and other instances of EXPECT_EQ) should be EXPECT_EQ(1U, CacheSize()). https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service_unittest.cc:89: content::Source<TokenCacheService> stub_source(&cache_); Hmm. This is supposed to be a Profile pointer right? I'd say just create a content::Source<Profile> instead and populate it with NULL.
Also, please remember to update the CL description before committing this. Remember that it ends up becoming the commit message so the "Questions for reviewers" and "who should review what" section aren't relevant. In the future, you can just send those as part of the initial mail so they don't get accidentally included in a commit that lands.
All nits fixed (I'll include them with the next batch of actual code review changes since I don't think anyone will find the change interesting). https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_service_factory.cc (right): https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service_factory.cc:14: return static_cast<extensions::TokenCacheService*>( On 2013/03/06 19:53:52, dcheng wrote: > Indent is weird. Done. https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_service_unittest.cc (right): https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service_unittest.cc:56: EXPECT_EQ(CacheSize(), 1U); On 2013/03/06 19:53:52, dcheng wrote: > Nit: EXPECT_EQ(<actual>, <expected>) is the preferred order, so this (and other > instances of EXPECT_EQ) should be EXPECT_EQ(1U, CacheSize()). Done. https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service_unittest.cc:89: content::Source<TokenCacheService> stub_source(&cache_); On 2013/03/06 19:53:52, dcheng wrote: > Hmm. This is supposed to be a Profile pointer right? I'd say just create a > content::Source<Profile> instead and populate it with NULL. Done.
LGTM with a few nits. https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_service.cc (right): https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service.cc:74: if (type == chrome::NOTIFICATION_GOOGLE_SIGNED_OUT) I'd make this: DCHECK_EQ(type, chrome::NOTIFICATION_GOOGLE_SIGNED_OUT) ...since this code should never be called with any other type. https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_service.h (right): https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service.h:7: // the instance of chrome shuts down. I would probably move this comment down to where the class is defined, as it seems to be more of a class-level comment. https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service.h:30: struct TokenCacheData { I think this should be moved inside TokenCacheService as a private inner struct. https://codereview.chromium.org/12380006/diff/41001/google_apis/gaia/gaia_con... File google_apis/gaia/gaia_constants.h (right): https://codereview.chromium.org/12380006/diff/41001/google_apis/gaia/gaia_con... google_apis/gaia/gaia_constants.h:39: extern const int kObfuscatedGaiaIdTimeoutInDays; BTW, this seems like an odd place to keep this. Are you sure you don't want to just move this to be an internal constant in push_messaging_api?
ping sky@: OWNERS review of the new directory I'm adding to chrome/browser, and all its contents, c/b/profiles changes, and the .gypi file. DCheng and ATWilson have already looked at this. ping Tim@: OWNERS review for google_apis/gaia changes please All nits from DCheng and ATWilson fixed. https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_service.cc (right): https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service.cc:74: if (type == chrome::NOTIFICATION_GOOGLE_SIGNED_OUT) On 2013/03/07 11:48:09, Andrew T Wilson wrote: > I'd make this: > > DCHECK_EQ(type, chrome::NOTIFICATION_GOOGLE_SIGNED_OUT) > > ...since this code should never be called with any other type. Done. https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_service.h (right): https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service.h:7: // the instance of chrome shuts down. On 2013/03/07 11:48:09, Andrew T Wilson wrote: > I would probably move this comment down to where the class is defined, as it > seems to be more of a class-level comment. Done. https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service.h:30: struct TokenCacheData { On 2013/03/07 11:48:09, Andrew T Wilson wrote: > I think this should be moved inside TokenCacheService as a private inner struct. Done. https://codereview.chromium.org/12380006/diff/41001/google_apis/gaia/gaia_con... File google_apis/gaia/gaia_constants.h (right): https://codereview.chromium.org/12380006/diff/41001/google_apis/gaia/gaia_con... google_apis/gaia/gaia_constants.h:39: extern const int kObfuscatedGaiaIdTimeoutInDays; On 2013/03/07 11:48:09, Andrew T Wilson wrote: > BTW, this seems like an odd place to keep this. Are you sure you don't want to > just move this to be an internal constant in push_messaging_api? Done.
On Thu, Mar 7, 2013 at 12:02 PM, <petewil@chromium.org> wrote: > ping sky@: OWNERS review of the new directory I'm adding to > chrome/browser, and > all its contents, c/b/profiles changes, and the .gypi file. DCheng and > ATWilson > have already looked at this. > You mean chrome/browser/extensions/token_cache? You should get an owner in extensions to review that. Similarly get an owner from c/b/profiles for the profiles changes. > ping Tim@: OWNERS review for google_apis/gaia changes please > > All nits from DCheng and ATWilson fixed. > > > > https://codereview.chromium.**org/12380006/diff/41001/** > chrome/browser/extensions/**token_cache/token_cache_**service.cc<https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions/token_cache/token_cache_service.cc> > File chrome/browser/extensions/**token_cache/token_cache_**service.cc > (right): > > https://codereview.chromium.**org/12380006/diff/41001/** > chrome/browser/extensions/**token_cache/token_cache_**service.cc#newcode74<https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions/token_cache/token_cache_service.cc#newcode74> > chrome/browser/extensions/**token_cache/token_cache_**service.cc:74: if > (type == chrome::NOTIFICATION_GOOGLE_**SIGNED_OUT) > On 2013/03/07 11:48:09, Andrew T Wilson wrote: > >> I'd make this: >> > > DCHECK_EQ(type, chrome::NOTIFICATION_GOOGLE_**SIGNED_OUT) >> > > ...since this code should never be called with any other type. >> > > Done. > > > https://codereview.chromium.**org/12380006/diff/41001/** > chrome/browser/extensions/**token_cache/token_cache_**service.h<https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions/token_cache/token_cache_service.h> > File chrome/browser/extensions/**token_cache/token_cache_**service.h > (right): > > https://codereview.chromium.**org/12380006/diff/41001/** > chrome/browser/extensions/**token_cache/token_cache_**service.h#newcode7<https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions/token_cache/token_cache_service.h#newcode7> > chrome/browser/extensions/**token_cache/token_cache_**service.h:7: // the > instance of chrome shuts down. > On 2013/03/07 11:48:09, Andrew T Wilson wrote: > >> I would probably move this comment down to where the class is defined, >> > as it > >> seems to be more of a class-level comment. >> > > Done. > > > https://codereview.chromium.**org/12380006/diff/41001/** > chrome/browser/extensions/**token_cache/token_cache_**service.h#newcode30<https://codereview.chromium.org/12380006/diff/41001/chrome/browser/extensions/token_cache/token_cache_service.h#newcode30> > chrome/browser/extensions/**token_cache/token_cache_**service.h:30: struct > TokenCacheData { > On 2013/03/07 11:48:09, Andrew T Wilson wrote: > >> I think this should be moved inside TokenCacheService as a private >> > inner struct. > > Done. > > > https://codereview.chromium.**org/12380006/diff/41001/** > google_apis/gaia/gaia_**constants.h<https://codereview.chromium.org/12380006/diff/41001/google_apis/gaia/gaia_constants.h> > File google_apis/gaia/gaia_**constants.h (right): > > https://codereview.chromium.**org/12380006/diff/41001/** > google_apis/gaia/gaia_**constants.h#newcode39<https://codereview.chromium.org/12380006/diff/41001/google_apis/gaia/gaia_constants.h#newcode39> > google_apis/gaia/gaia_**constants.h:39: extern const int > kObfuscatedGaiaIdTimeoutInDays**; > On 2013/03/07 11:48:09, Andrew T Wilson wrote: > >> BTW, this seems like an odd place to keep this. Are you sure you don't >> > want to > >> just move this to be an internal constant in push_messaging_api? >> > > Done. > > https://codereview.chromium.**org/12380006/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/12380006/diff/47001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/12380006/diff/47001/chrome/chrome_browser.gyp... chrome/chrome_browser.gypi:594: 'browser/token_cache/token_cache_factory.h', I don't see this file in your patch and you're missing token_cache_service_factory.h.
Fix the gypi problem Sky noticed. https://codereview.chromium.org/12380006/diff/47001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/12380006/diff/47001/chrome/chrome_browser.gyp... chrome/chrome_browser.gypi:594: 'browser/token_cache/token_cache_factory.h', On 2013/03/07 21:08:41, sky wrote: > I don't see this file in your patch and you're missing > token_cache_service_factory.h. Done.
*.gypi LGTM
+ASargent - Anthony, please do an OWNERS review of the new files in extensions/notifications/sync_notifier. DCheng and Zea have already reviewed them.
LGTM https://codereview.chromium.org/12380006/diff/52001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_service.h (right): https://codereview.chromium.org/12380006/diff/52001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service.h:28: // the instance of chrome shuts down. We deciding to clear out the token cache when chrome shuts down, did you consider the case of users of chrome apps who have the app launcher but don't use chrome as their main browser? There could be workflows where they launch app A, close it, and then immediately launch app B, and this token will have expired after A shuts down. I don't have a great sense for how common this would be or what the cost of having to re-fetch the token is, but I just wanted to raise the issue for you to think about. It could very well be the case that it isn't worth the complexity of saving these to disk across browser session lifetimes. https://codereview.chromium.org/12380006/diff/52001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service.h:46: // inherited from NotificationObserver nit: capitalize Inherited, and finish sentence with a period. https://codereview.chromium.org/12380006/diff/52001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service.h:57: std::map<std::string, TokenCacheData> token_cache_; Would it be worth mentioning in a comment here what the string key is? (I'm guessing it's the token name?) https://codereview.chromium.org/12380006/diff/52001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service.h:61: friend class TokenCacheTest; nit: I don't think this is a hard rule in the style guide, but I think a lot of chrome code, at least in the extensions system, tends to put friend declarations just after the private: label instead of at the bottom.
Replies for ASargent (I'll upload the fixes with the next batch) https://codereview.chromium.org/12380006/diff/52001/chrome/browser/extensions... File chrome/browser/extensions/token_cache/token_cache_service.h (right): https://codereview.chromium.org/12380006/diff/52001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service.h:28: // the instance of chrome shuts down. On 2013/03/12 00:11:31, Antony Sargent wrote: > We deciding to clear out the token cache when chrome shuts down, did you > consider the case of users of chrome apps who have the app launcher but don't > use chrome as their main browser? There could be workflows where they launch app > A, close it, and then immediately launch app B, and this token will have expired > after A shuts down. > > I don't have a great sense for how common this would be or what the cost of > having to re-fetch the token is, but I just wanted to raise the issue for you to > think about. It could very well be the case that it isn't worth the complexity > of saving these to disk across browser session lifetimes. The token is only used by Push Messaging. If there is any app installed with Push Messaging permission, it will keep chrome up even when the user exits. Also, Michael (on my team) is considering using this cache for the Identity API, and if he does, he will make it persist to disk. https://codereview.chromium.org/12380006/diff/52001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service.h:46: // inherited from NotificationObserver On 2013/03/12 00:11:31, Antony Sargent wrote: > nit: capitalize Inherited, and finish sentence with a period. Done. https://codereview.chromium.org/12380006/diff/52001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service.h:57: std::map<std::string, TokenCacheData> token_cache_; On 2013/03/12 00:11:31, Antony Sargent wrote: > Would it be worth mentioning in a comment here what the string key is? (I'm > guessing it's the token name?) > Done. https://codereview.chromium.org/12380006/diff/52001/chrome/browser/extensions... chrome/browser/extensions/token_cache/token_cache_service.h:61: friend class TokenCacheTest; On 2013/03/12 00:11:31, Antony Sargent wrote: > nit: I don't think this is a hard rule in the style guide, but I think a lot of > chrome code, at least in the extensions system, tends to put friend declarations > just after the private: label instead of at the bottom. Done.
On 2013/03/12 00:11:31, Antony Sargent wrote: > LGTM > > https://codereview.chromium.org/12380006/diff/52001/chrome/browser/extensions... > File chrome/browser/extensions/token_cache/token_cache_service.h (right): > > https://codereview.chromium.org/12380006/diff/52001/chrome/browser/extensions... > chrome/browser/extensions/token_cache/token_cache_service.h:28: // the instance > of chrome shuts down. > We deciding to clear out the token cache when chrome shuts down, did you > consider the case of users of chrome apps who have the app launcher but don't > use chrome as their main browser? There could be workflows where they launch app > A, close it, and then immediately launch app B, and this token will have expired > after A shuts down. > Two notes here: 1) Ben Wells is working on a proposal for keeping the browser process (and hence, the profile and any cached data) alive even when no app is active, so I think that's a better, more general solution to the latency issue. 2) In general, we're moving to a world where other tokens (OAuth) aren't persisted to disk, but instead are re-fetched as needed. Given that these tokens are only needed for network requests, having to re-fetch them the first time you need them in a given session is not terribly onerous.
Ok, good to hear - it sounds like what's in this CL is the right design then. On Tue, Mar 12, 2013 at 1:58 AM, <atwilson@chromium.org> wrote: > On 2013/03/12 00:11:31, Antony Sargent wrote: > >> LGTM >> > > > https://codereview.chromium.**org/12380006/diff/52001/** > chrome/browser/extensions/**token_cache/token_cache_**service.h<https://codereview.chromium.org/12380006/diff/52001/chrome/browser/extensions/token_cache/token_cache_service.h> > >> File chrome/browser/extensions/**token_cache/token_cache_**service.h >> (right): >> > > > https://codereview.chromium.**org/12380006/diff/52001/** > chrome/browser/extensions/**token_cache/token_cache_**service.h#newcode28<https://codereview.chromium.org/12380006/diff/52001/chrome/browser/extensions/token_cache/token_cache_service.h#newcode28> > >> chrome/browser/extensions/**token_cache/token_cache_**service.h:28: // >> the >> > instance > >> of chrome shuts down. >> >> We deciding to clear out the token cache when chrome shuts down, did you >> consider the case of users of chrome apps who have the app launcher but >> don't >> use chrome as their main browser? There could be workflows where they >> launch >> > app > >> A, close it, and then immediately launch app B, and this token will have >> > expired > >> after A shuts down. >> > > Two notes here: > > 1) Ben Wells is working on a proposal for keeping the browser process (and > hence, the profile and any cached data) alive even when no app is active, > so I > think that's a better, more general solution to the latency issue. > > 2) In general, we're moving to a world where other tokens (OAuth) aren't > persisted to disk, but instead are re-fetched as needed. Given that these > tokens > are only needed for network requests, having to re-fetch them the first > time you > need them in a given session is not terribly onerous. > > > https://codereview.chromium.**org/12380006/<https://codereview.chromium.org/1... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petewil@chromium.org/12380006/41002
Message was sent while issue was closed.
Change committed as 188130 |
