Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1214483002:
Improve the ProfileInfoCache API. (Closed)
DescriptionImprove the ProfileInfoCache API.
This change introduces a new API to the ProfileInfoCache and implements
it in terms of the current ProfileInfoCache, along with unit tests.
This is the first step in addressing the API issues the ProfileInfoCache
exhibits, with upcoming follow up CLs replacing the current uses with
this new interface and ultimately improving the implementation as well.
A document detailing the design and discussion around this effort can be
found here:
https://docs.google.com/a/google.com/document/d/12V8ahA9lEsXWyT8OqasEz3HR4J5jK9hwbRTeU0hUm6Y/edit?usp=sharing
BUG=305720
Committed: https://crrev.com/b50a7cbab0df3cc66f1881dcb08e4cef18f96835
Cr-Commit-Position: refs/heads/master@{#338513}
Patch Set 1 #
Total comments: 41
Patch Set 2 : Rebase #Patch Set 3 : Address review feedback #
Total comments: 19
Patch Set 4 : Rebase #Patch Set 5 : Address feedback and slightly change the interface. #
Total comments: 19
Patch Set 6 : Address feedback. #Patch Set 7 : Comments and deprecation #Patch Set 8 : Rebase #Patch Set 9 : Tentative trybot build fix. #Patch Set 10 : Replace std::map. #Patch Set 11 : Fix use of base::StringPrintf in unit test. #Patch Set 12 : Add TestBrowserThreadBundle to unit tests #Patch Set 13 : Rebase #Messages
Total messages: 15 (3 generated)
anthonyvd@chromium.org changed reviewers: + mlerman@chromium.org, noms@chromium.org
Hi Monica and Michael, Welcome to ProfileInfoCache refactor, the Third! Can you guys please take a look at this CL? It introduces the new interface and its related unit tests. It doesn't involve modifying any current callers or the internals of the ProfileInfoCache yet but I figured approaching this in bite-sized CLs would make it more manageable :) Thanks a ton in advance for your time and feedback, maybe third time's the charm...? https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_metadata_storage.h (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:85: // SetIsOmittedProfileAtIndex() to clear the flag when the profile is ready to Oops, need to update that comment.
Wooohoo! This is looking great. I left some initial comments (and hopefully none of them are giant yak shaves) Could you run some bots, please? Who knows what's going on in there :) https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:1242: // ProfileMetadataStorage implementation nit: . at the end? https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:1258: ProfileInfoCache::GetAllProfilesMetadata() { Yeah, here metadata is even worse (see the other comment about why I don't like the word). Here, I'd expect this to return some metadata common to all profiles, not all of the data for all of the profiles. How about just `GetAllProfiles()` ? While you're here, you're probably also going to need a `GetSortedProfiles()`, for things like the avatar menu. https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:1270: const base::FilePath& path, ProfileMetadataEntry* entry) { Idea: Why not return the `entry` (or null if it doesn't exist)? The caller has to check that `entry` isn't null anyway, so this isn't worse. https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:1280: entry->profile_index_ = index; Weeeeeeeell if it's a private member, you probably should be using a setter here. :) https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:1286: base::string16 ProfileMetadataStorage::ProfileMetadataEntry::GetName() const { I think this should be in a different file, maybe. Some Impl of sorts? This is giant, and the profile info cache is already giant. 1500 lines of getters and setters make pandas super sad https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_metadata_storage.h (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. The 2014-and-after style says there's no (c) needed anymore: https://www.chromium.org/developers/coding-style https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:13: class ProfileMetadataEntry { I don't love `metadata` (to me, metadata is like the headers of a table, but this actually contains legit information, doesn't just describe it). How about: ProfileAttributes or ProfileInfoEntry? https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:15: base::string16 GetName() const; I'm so confused. This just has getters, but doesn't define the members? That's kind of weird. :/ (I know they're in the profile info cache, but they probably should live here, with their happy interface family. No? Maybe? Who knows, I write JavaScript now) https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:70: // These members are an implementation detail meant to smooth the migration Probably safe to add a TODO here :) https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_metadata_storage_unittest.cc (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage_unittest.cc:2: // Use of this source code is governed by a BSD-style license that can be nit: same comment about the copyright
I am excited about this! https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:1242: // ProfileMetadataStorage implementation On 2015/06/29 17:10:39, noms (slow. ping if too slow) wrote: > nit: . at the end? I didn't think we generally had leading comments like this, that state what these few functions will do; we should just follow the implementation order in the header file. https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:1253: void ProfileInfoCache::DeleteProfile(const base::FilePath& profile_path) { nit: newline https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:1270: const base::FilePath& path, ProfileMetadataEntry* entry) { On 2015/06/29 17:10:39, noms (slow. ping if too slow) wrote: > Idea: Why not return the `entry` (or null if it doesn't exist)? The caller has > to check that `entry` isn't null anyway, so this isn't worse. > I think the advantage of Anthony's implementation is that it's much more likely for future developers to write code that actually checks the return of GetProfileMetadataWithPath. if the function returns the entry, a lot of people will just write GetProfileMetataWithProfile(profile-GetPath())->GetAThing(). https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:1286: base::string16 ProfileMetadataStorage::ProfileMetadataEntry::GetName() const { On 2015/06/29 17:10:39, noms (slow. ping if too slow) wrote: > I think this should be in a different file, maybe. Some Impl of sorts? This is > giant, and the profile info cache is already giant. 1500 lines of getters and > setters make pandas super sad I agree - this seems like the wrong place for this. All of the ProfileInfoCache methods you're calling are public. https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_info_cache.h (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.h:166: // ProfileMetadataStorage implementation nit: I think the style standard is // ProfileMetadataStorage: https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.h:175: GetAllProfilesMetadata() override; nit: indent 4 spaces please. https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_metadata_storage.h (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:13: class ProfileMetadataEntry { On 2015/06/29 17:10:40, noms (slow. ping if too slow) wrote: > I don't love `metadata` (to me, metadata is like the headers of a table, but > this actually contains legit information, doesn't just describe it). How about: > ProfileAttributes or ProfileInfoEntry? I was okay with ProfileMetadata, though I'm also okay with ProfileAttributes. Also - does this have to be an internal/nested class? it's going to be referenced a lot, is part of the public api, and will likely be modified independently of the Storage. Perhaps put it into its own file? https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:76: ProfileInfoCache* profile_info_cache_; I imagine you will one day have member variables for all of those pieces of information that are listed above? Perhaps write a TODO to that effect? https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:105: const base::FilePath& path, ProfileMetadataEntry* entry) = 0; Perhaps make the second parameter a *& rather than a *? Down the road you may want to cache the ProfileMetadataEntry objects rather than re-creating them all the time from prefs, and a *& will let you just return the cached object. For now, you can just assert that the *& (or **) passed actually points to something. https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:107: // Returns the amount of known profiles. s/amount/count? quantity? numeracy?/ https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_metadata_storage_unittest.cc (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage_unittest.cc:96: TestingProfileManager testing_profile_manager_; nit: private:
Thanks a ton to both of you for your feedback! I think the latest patchset should address a lot of your comments and I left some replies to try and clarify some things. I'm also running some bots which should hopefully not raise any issues since no one but unit tests currently consume the new code. Let me know what you think and thanks again for your time and thorough help :) https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:1258: ProfileInfoCache::GetAllProfilesMetadata() { On 2015/06/29 17:10:40, noms (slow. ping if too slow) wrote: > Yeah, here metadata is even worse (see the other comment about why I don't like > the word). Here, I'd expect this to return some metadata common to all profiles, > not all of the data for all of the profiles. > > How about just `GetAllProfiles()` ? > > While you're here, you're probably also going to need a `GetSortedProfiles()`, > for things like the avatar menu. You're absolutely right. I changed "Metadata" to "Attributes" and this method to "GetAllProfilesAttributes()" since "GetAllProfiles()" could be confusing (Profile is another type). As for GetSortedProfiles(), I was planning on leaving the responsibility of sorting to the callers that need it. Since only very few callers require that, we'll be able to avoid keeping the entries sorted in the ProfileInfoCache. It also allows callers to sort the profiles in the order they want instead of having just that one option. In short, I think this is the responsibility of the caller that needs it, not the storage object. Happy to hear your thoughts :) https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:1270: const base::FilePath& path, ProfileMetadataEntry* entry) { On 2015/06/29 19:14:37, Mike Lerman wrote: > On 2015/06/29 17:10:39, noms (slow. ping if too slow) wrote: > > Idea: Why not return the `entry` (or null if it doesn't exist)? The caller has > > to check that `entry` isn't null anyway, so this isn't worse. > > > > I think the advantage of Anthony's implementation is that it's much more likely > for future developers to write code that actually checks the return of > GetProfileMetadataWithPath. if the function returns the entry, a lot of people > will just write GetProfileMetataWithProfile(profile-GetPath())->GetAThing(). Exactly. The main reason for crashes right now is that it's not clear enough that getting profile info for a path can fail. Callers should be checking that GetIndexForProfileWithPath doesn't return string::npos but they don't :( I think returning a bool and using an out parameter makes it more obvious that the operation can fail and must be checked. https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:1280: entry->profile_index_ = index; On 2015/06/29 17:10:40, noms (slow. ping if too slow) wrote: > Weeeeeeeell if it's a private member, you probably should be using a setter > here. :) I wanted those to be private with the ProfileInfoCache as a friend since they're an implementation detail and that as-is, the ProfileInfoCache is the only one that has business creating Entries. The latest patchset changes this to use a private constructor, still with ProfileInfoCache as a friend. This is still meh but a little better maybe? Let me know if you can think of a better way :) https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:1286: base::string16 ProfileMetadataStorage::ProfileMetadataEntry::GetName() const { On 2015/06/29 19:14:37, Mike Lerman wrote: > On 2015/06/29 17:10:39, noms (slow. ping if too slow) wrote: > > I think this should be in a different file, maybe. Some Impl of sorts? This is > > giant, and the profile info cache is already giant. 1500 lines of getters and > > setters make pandas super sad > > I agree - this seems like the wrong place for this. All of the ProfileInfoCache > methods you're calling are public. Good point, which kind of ties in to the other comment about moving the Entry class to a separate file. I went ahead and did that, let me know what you guys think :) https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_info_cache.h (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.h:166: // ProfileMetadataStorage implementation On 2015/06/29 19:14:37, Mike Lerman wrote: > nit: I think the style standard is > // ProfileMetadataStorage: Done. https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.h:175: GetAllProfilesMetadata() override; On 2015/06/29 19:14:37, Mike Lerman wrote: > nit: indent 4 spaces please. Done. https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_metadata_storage.h (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/06/29 17:10:40, noms (slow. ping if too slow) wrote: > The 2014-and-after style says there's no (c) needed anymore: > https://www.chromium.org/developers/coding-style Done. https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:13: class ProfileMetadataEntry { On 2015/06/29 19:14:37, Mike Lerman wrote: > On 2015/06/29 17:10:40, noms (slow. ping if too slow) wrote: > > I don't love `metadata` (to me, metadata is like the headers of a table, but > > this actually contains legit information, doesn't just describe it). How > about: > > ProfileAttributes or ProfileInfoEntry? > > I was okay with ProfileMetadata, though I'm also okay with ProfileAttributes. > > Also - does this have to be an internal/nested class? it's going to be > referenced a lot, is part of the public api, and will likely be modified > independently of the Storage. Perhaps put it into its own file? Done and done. I preferred moving away from ProfileInfoSomething since a ProfileInfo is already a thing. Also, good points about "metadata". Is everyone OK with Attributes like this? https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:15: base::string16 GetName() const; On 2015/06/29 17:10:40, noms (slow. ping if too slow) wrote: > I'm so confused. This just has getters, but doesn't define the members? That's > kind of weird. :/ > > (I know they're in the profile info cache, but they probably should live here, > with their happy interface family. No? Maybe? Who knows, I write JavaScript now) I completely agree that it's kind of weird. The idea is to have it this way to ease the transition away from the ProfileInfoCache's interface by implementing this super thin layer that just speaks with the ProfileInfoCache. Once everyone is moved to the new interface, we'll move the logic that currently lives in Get*ForProfileAtIndex methods to this class. At that point its only member will be a base::DictionaryValue* from which specific fields will be read. This first very thin layer is just a good way to start refactoring the callers before the actual implementation work is done, which I felt was more manageable. It also avoids the clunky situation where some callers are migrated but not others, requiring duplicate logic in both the ProfileInfoCache and this class. Hopefully all of this makes sense :) https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:70: // These members are an implementation detail meant to smooth the migration On 2015/06/29 17:10:40, noms (slow. ping if too slow) wrote: > Probably safe to add a TODO here :) Done. https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:76: ProfileInfoCache* profile_info_cache_; On 2015/06/29 19:14:37, Mike Lerman wrote: > I imagine you will one day have member variables for all of those pieces of > information that are listed above? Perhaps write a TODO to that effect? Do you think the own above w.r.t. Monica's comment covers this as well? https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:85: // SetIsOmittedProfileAtIndex() to clear the flag when the profile is ready to On 2015/06/25 21:38:37, anthonyvd wrote: > Oops, need to update that comment. Done. https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:105: const base::FilePath& path, ProfileMetadataEntry* entry) = 0; On 2015/06/29 19:14:37, Mike Lerman wrote: > Perhaps make the second parameter a *& rather than a *? Down the road you may > want to cache the ProfileMetadataEntry objects rather than re-creating them all > the time from prefs, and a *& will let you just return the cached object. For > now, you can just assert that the *& (or **) passed actually points to > something. I'm not too sure about that. The pointer-to-pointer version kind of seems like the storage owns the object, which is not the case. The reference-to-pointer on the other hand doesn't make it obvious enough at the call site that the pointer's value can change (in my opinion). Also, I think there are 2 ways the future of this class can go: either it stores a (not owned) pointer to the underlying base::DictionaryValue or it extracts and stores the data at construction time. The first case (which is what I at least planned to start with) wouldn't prevent us from copying the cached object for cheap. The second version would change the semantics of the interface to make it so the storage owns the objects, which might be reasonable but I think the change to a pointer-to-pointer should happen at that point (this way the current interface doesn't have conflicting semantics). Happy to discuss this more though, it's a great point! :) https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:107: // Returns the amount of known profiles. On 2015/06/29 19:14:37, Mike Lerman wrote: > s/amount/count? quantity? numeracy?/ Done. https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_metadata_storage_unittest.cc (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage_unittest.cc:2: // Use of this source code is governed by a BSD-style license that can be On 2015/06/29 17:10:40, noms (slow. ping if too slow) wrote: > nit: same comment about the copyright Done. https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage_unittest.cc:96: TestingProfileManager testing_profile_manager_; On 2015/06/29 19:14:37, Mike Lerman wrote: > nit: private: Done.
Thanks for those changes! Some more thoughts :) https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:1258: ProfileInfoCache::GetAllProfilesMetadata() { On 2015/06/30 21:24:52, anthonyvd wrote: > On 2015/06/29 17:10:40, noms (slow. ping if too slow) wrote: > > Yeah, here metadata is even worse (see the other comment about why I don't > like > > the word). Here, I'd expect this to return some metadata common to all > profiles, > > not all of the data for all of the profiles. > > > > How about just `GetAllProfiles()` ? > > > > While you're here, you're probably also going to need a `GetSortedProfiles()`, > > for things like the avatar menu. > > You're absolutely right. I changed "Metadata" to "Attributes" and this method to > "GetAllProfilesAttributes()" since "GetAllProfiles()" could be confusing > (Profile is another type). > > As for GetSortedProfiles(), I was planning on leaving the responsibility of > sorting to the callers that need it. Since only very few callers require that, > we'll be able to avoid keeping the entries sorted in the ProfileInfoCache. It > also allows callers to sort the profiles in the order they want instead of > having just that one option. In short, I think this is the responsibility of the > caller that needs it, not the storage object. Happy to hear your thoughts :) Ok, it makes sense to leave sorting to the callers. Avatar_menu can I'm sure do that nicely :) https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_metadata_storage.h (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:105: const base::FilePath& path, ProfileMetadataEntry* entry) = 0; On 2015/06/30 21:24:53, anthonyvd wrote: > On 2015/06/29 19:14:37, Mike Lerman wrote: > > Perhaps make the second parameter a *& rather than a *? Down the road you may > > want to cache the ProfileMetadataEntry objects rather than re-creating them > all > > the time from prefs, and a *& will let you just return the cached object. For > > now, you can just assert that the *& (or **) passed actually points to > > something. > > I'm not too sure about that. The pointer-to-pointer version kind of seems like > the storage owns the object, which is not the case. The reference-to-pointer on > the other hand doesn't make it obvious enough at the call site that the > pointer's value can change (in my opinion). > > Also, I think there are 2 ways the future of this class can go: either it stores > a (not owned) pointer to the underlying base::DictionaryValue or it extracts and > stores the data at construction time. The first case (which is what I at least > planned to start with) wouldn't prevent us from copying the cached object for > cheap. The second version would change the semantics of the interface to make it > so the storage owns the objects, which might be reasonable but I think the > change to a pointer-to-pointer should happen at that point (this way the current > interface doesn't have conflicting semantics). > > Happy to discuss this more though, it's a great point! :) I agree with your analysis of the two options, and I lean towards preferring the second eventually since it means that there won't be out-of-date objects people may accidentally hold on to and also minimizes memory requirements. I see what you mean about not wanting to change form 1) to 2) without changing the interface. What do you think about implementing 2) from the get-go, in this CL? https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:22: ProfileAttributesEntry(); virtual ~ProfileAttributesEntry(); https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_storage.h (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_storage.h:30: virtual std::vector<ProfileAttributesEntry> GetAllProfilesAttributes() = 0; As in the other comment - if it's not sorted, perhaps use std::set instead of std::vector https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_storage_unittest.cc (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_storage_unittest.cc:263: } Can you add some tests around: - I get a ProfileAttributes for A, delete a Profile B, then use my ProfileAttributes from A - it still works? - I get a ProfileAttributes for A, I rename B (which changes the indices), use the ProfileAttributes from A - it still works? - I get a ProfileAttributes for A, delete the Profile for A, try to use the ProfileAttributes - does the system respond as it should? - Other crazy scenarios... https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:1242: // ProfileMetadataStorage implementation I think you can remove this comment. https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:1279: *entry = ProfileAttributesEntry(this, index); you're explicitly making use of operator=(). You should probably either define operator=() explicitly or put a comment as to why you're not putting DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:1279: *entry = ProfileAttributesEntry(this, index); This is fragile because the index of a Profile can change. I can call this method on a Profile like /MyProfile which has index 2, call SetName(), and now /MyProfile has index of 1 and some other profile has index of 2, and now further accessing methods on the ProfileAttributesEntry are working on the wrong object. I think you have to pass the whole profile_path into the ProfileAttributesEntry constructor and do the lookup each time. Please add a UnitTest that triggers this scenario, to make sure we won't trip it up later! https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.h (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.h:174: std::vector<ProfileAttributesEntry> GetAllProfilesAttributes() override; This is now an unordered collection. Perhaps return a set instead of a vector to convey that order isn't fixed/guaranteed/important?
Thanks for the comments and sorry for the delay. Here's a new shiny patchset that should address your feedback :) https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_metadata_storage.h (right): https://codereview.chromium.org/1214483002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metadata_storage.h:105: const base::FilePath& path, ProfileMetadataEntry* entry) = 0; On 2015/07/02 18:01:30, Mike Lerman wrote: > On 2015/06/30 21:24:53, anthonyvd wrote: > > On 2015/06/29 19:14:37, Mike Lerman wrote: > > > Perhaps make the second parameter a *& rather than a *? Down the road you > may > > > want to cache the ProfileMetadataEntry objects rather than re-creating them > > all > > > the time from prefs, and a *& will let you just return the cached object. > For > > > now, you can just assert that the *& (or **) passed actually points to > > > something. > > > > I'm not too sure about that. The pointer-to-pointer version kind of seems like > > the storage owns the object, which is not the case. The reference-to-pointer > on > > the other hand doesn't make it obvious enough at the call site that the > > pointer's value can change (in my opinion). > > > > Also, I think there are 2 ways the future of this class can go: either it > stores > > a (not owned) pointer to the underlying base::DictionaryValue or it extracts > and > > stores the data at construction time. The first case (which is what I at least > > planned to start with) wouldn't prevent us from copying the cached object for > > cheap. The second version would change the semantics of the interface to make > it > > so the storage owns the objects, which might be reasonable but I think the > > change to a pointer-to-pointer should happen at that point (this way the > current > > interface doesn't have conflicting semantics). > > > > Happy to discuss this more though, it's a great point! :) > > I agree with your analysis of the two options, and I lean towards preferring the > second eventually since it means that there won't be out-of-date objects people > may accidentally hold on to and also minimizes memory requirements. > > I see what you mean about not wanting to change form 1) to 2) without changing > the interface. What do you think about implementing 2) from the get-go, in this > CL? I went ahead and implemented 2) without the actual internal fields. I think this is a good middle ground where we get the stable interface right away without writing the code to keep both storages in sync that would be removed after the migration to strictly using the new interface. What do you think? https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:22: ProfileAttributesEntry(); On 2015/07/02 18:01:30, Mike Lerman wrote: > virtual ~ProfileAttributesEntry(); Done. Is it a coding guideline thing to have a virtual dtor on a base class like this? https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_storage.h (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_storage.h:30: virtual std::vector<ProfileAttributesEntry> GetAllProfilesAttributes() = 0; On 2015/07/02 18:01:30, Mike Lerman wrote: > As in the other comment - if it's not sorted, perhaps use std::set instead of > std::vector Replied to the other comment :) https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_storage_unittest.cc (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_storage_unittest.cc:263: } On 2015/07/02 18:01:30, Mike Lerman wrote: > Can you add some tests around: > - I get a ProfileAttributes for A, delete a Profile B, then use my > ProfileAttributes from A - it still works? Done. Also testing deleting from the ProfileInfoCache's own method. > - I get a ProfileAttributes for A, I rename B (which changes the indices), use > the ProfileAttributes from A - it still works? Done. > - I get a ProfileAttributes for A, delete the Profile for A, try to use the > ProfileAttributes - does the system respond as it should? Well the contract with the ProfileAttributesStorage (as with the ProfileInfoCache) is that this is undefined, for the same reasons caching the index or the entry pointer is undefined. If you have any suggestions w.r.t. improving this I'd be happy to implement them but I can't think of a way other than doing the whole bool GetFoo(FooType* out); pattern for every accessor. I think that's a little overkill. > - Other crazy scenarios... I added tests around modification from other entry pointers and from the ProfileInfoCache itself as well. Anything else in mind? https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:1242: // ProfileMetadataStorage implementation On 2015/07/02 18:01:30, Mike Lerman wrote: > I think you can remove this comment. Done. https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:1279: *entry = ProfileAttributesEntry(this, index); On 2015/07/02 18:01:30, Mike Lerman wrote: > you're explicitly making use of operator=(). You should probably either define > operator=() explicitly or put a comment as to why you're not putting > DISALLOW_COPY_AND_ASSIGN. Having the storage own the entries makes DISALLOW_COPY_AND_ASSIGN easier so done. https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:1279: *entry = ProfileAttributesEntry(this, index); On 2015/07/02 18:01:30, Mike Lerman wrote: > This is fragile because the index of a Profile can change. I can call this > method on a Profile like /MyProfile which has index 2, call SetName(), and now > /MyProfile has index of 1 and some other profile has index of 2, and now further > accessing methods on the ProfileAttributesEntry are working on the wrong object. > I think you have to pass the whole profile_path into the ProfileAttributesEntry > constructor and do the lookup each time. > > Please add a UnitTest that triggers this scenario, to make sure we won't trip it > up later! You're absolutely right, done. In the future though, the entry should hopefully never be invalidated this way (unless it's deleted I guess) since we won't be resorting the internal storage. At that point however, this path/index dance won't exist anymore. Until then, this is better! https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.h (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.h:174: std::vector<ProfileAttributesEntry> GetAllProfilesAttributes() override; On 2015/07/02 18:01:31, Mike Lerman wrote: > This is now an unordered collection. Perhaps return a set instead of a vector to > convey that order isn't fixed/guaranteed/important? I believe this would make using this function hard to use. The callers that need the elements sorted would have to transfer them to a sortable collection. Also, std::vector doesn't guarantee anything about order as opposed to std::set (sorted set) and std::unordered_set faces the problem described above. In that sense, I personally feel like it relates the "probably not sorted" situation well while letting callers easily sort it if they need to.
Just down to comments and nits - everything major about the API looks good! https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:22: ProfileAttributesEntry(); On 2015/07/07 21:05:32, anthonyvd wrote: > On 2015/07/02 18:01:30, Mike Lerman wrote: > > virtual ~ProfileAttributesEntry(); > > Done. Is it a coding guideline thing to have a virtual dtor on a base class like > this? I think so - I've gotten that as feedback many times. https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_storage_unittest.cc (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_storage_unittest.cc:263: } On 2015/07/07 21:05:32, anthonyvd wrote: > On 2015/07/02 18:01:30, Mike Lerman wrote: > > Can you add some tests around: > > - I get a ProfileAttributes for A, delete a Profile B, then use my > > ProfileAttributes from A - it still works? > > Done. Also testing deleting from the ProfileInfoCache's own method. > > > - I get a ProfileAttributes for A, I rename B (which changes the indices), use > > the ProfileAttributes from A - it still works? > > Done. > > > - I get a ProfileAttributes for A, delete the Profile for A, try to use the > > ProfileAttributes - does the system respond as it should? > > Well the contract with the ProfileAttributesStorage (as with the > ProfileInfoCache) is that this is undefined, for the same reasons caching the > index or the entry pointer is undefined. > > If you have any suggestions w.r.t. improving this I'd be happy to implement them > but I can't think of a way other than doing the whole bool GetFoo(FooType* out); > pattern for every accessor. I think that's a little overkill. > > > - Other crazy scenarios... > > I added tests around modification from other entry pointers and from the > ProfileInfoCache itself as well. Anything else in mind? Undefined contract - understood. Makes sense. I agree about using an out parameter for every accessor is overkill. Is it worth adding a DCHECK? In the old code, it was impossible to get anything from the ProfileInfoCache if the Profile had been deleted, because it gets removed. Now, it is possible, but very very unpredictable. Nothing else in mind :) https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.h (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.h:174: std::vector<ProfileAttributesEntry> GetAllProfilesAttributes() override; On 2015/07/07 21:05:32, anthonyvd wrote: > On 2015/07/02 18:01:31, Mike Lerman wrote: > > This is now an unordered collection. Perhaps return a set instead of a vector > to > > convey that order isn't fixed/guaranteed/important? > > I believe this would make using this function hard to use. The callers that need > the elements sorted would have to transfer them to a sortable collection. > > Also, std::vector doesn't guarantee anything about order as opposed to std::set > (sorted set) and std::unordered_set faces the problem described above. In that > sense, I personally feel like it relates the "probably not sorted" situation > well while letting callers easily sort it if they need to. Easily sort - because you can do an in-place sort. Right? Please leave a comment then, highlighting this understanding. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.cc (right): https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.cc:14: profile_info_cache_ = cache; verify the members are empty (haven't already been set) and that what's being passed in is non-empty. e.g. DCHECK(!profile_info_cache_); DCHECK(cache); https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:25: base::string16 GetName() const; It would be ideal to actually have comments for all of these. I know we didn't in the ProfileInfoCache, but it would be nice. Feel free to do that in another CL, though. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_storage.h (right): https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_storage.h:26: virtual void DeleteProfile(const base::FilePath& profile_path) = 0; Please add a comment for this method. e.g. it doesn't delete profile data from disk, merely boots the associated profile out of this Storage. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_storage.h:35: // |entry| should not be cached as it will not reflect subsequent changes to s/will/may. It actually DOES reflect subsequent changes in the current implementation. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_storage_unittest.cc (right): https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_storage_unittest.cc:289: entry->SetName(base::ASCIIToUTF16("zulu_name")); // Trigger a resort. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_storage_unittest.cc:339: EXPECT_EQ(base::ASCIIToUTF16("NewName"), second_entry->GetName()); We can also test pointer equality. We should have second_entry == first_entry. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:1272: if (GetNumberOfProfiles() == 0) { nit: braces unneeded here - the condition's really short. (and just below) https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:1276: size_t index = GetIndexOfProfileWithPath(path); inline GetIndexOfProfileWithPath into the condition below; we don't use index anywhere else anymore. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.h (right): https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.h:167: // ProfileMetadataStorage: Perhaps note that these methods are experimental, and not to be used "yet"? It would be great to have a way to enforce that, too.
As usual, thanks for the feedback! Here's a new patchset and some comments :) https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_storage_unittest.cc (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_storage_unittest.cc:263: } On 2015/07/08 18:26:34, Mike Lerman wrote: > On 2015/07/07 21:05:32, anthonyvd wrote: > > On 2015/07/02 18:01:30, Mike Lerman wrote: > > > Can you add some tests around: > > > - I get a ProfileAttributes for A, delete a Profile B, then use my > > > ProfileAttributes from A - it still works? > > > > Done. Also testing deleting from the ProfileInfoCache's own method. > > > > > - I get a ProfileAttributes for A, I rename B (which changes the indices), > use > > > the ProfileAttributes from A - it still works? > > > > Done. > > > > > - I get a ProfileAttributes for A, delete the Profile for A, try to use the > > > ProfileAttributes - does the system respond as it should? > > > > Well the contract with the ProfileAttributesStorage (as with the > > ProfileInfoCache) is that this is undefined, for the same reasons caching the > > index or the entry pointer is undefined. > > > > If you have any suggestions w.r.t. improving this I'd be happy to implement > them > > but I can't think of a way other than doing the whole bool GetFoo(FooType* > out); > > pattern for every accessor. I think that's a little overkill. > > > > > - Other crazy scenarios... > > > > I added tests around modification from other entry pointers and from the > > ProfileInfoCache itself as well. Anything else in mind? > > Undefined contract - understood. Makes sense. > > I agree about using an out parameter for every accessor is overkill. Is it worth > adding a DCHECK? In the old code, it was impossible to get anything from the > ProfileInfoCache if the Profile had been deleted, because it gets removed. Now, > it is possible, but very very unpredictable. > > Nothing else in mind :) Good point, I added a DCHECK in ProfileInfoAttributesEntry::profile_index(). It's called in every other method and it would return std::string::npos if the profile had been deleted. FWIW, I don't think the situation is less predictable than before the refactor though. The only way to access a deleted profile now is to hold onto the Entry, delete the profile, then access it again. If that happens, the Entry would be a pointer to an invalidated (deleted) element of ProfileInfoCache::profile_attributes_entries_ and dereferencing it would be undefined. Before, it would be less predictable. In the same situation, a caller would be holding onto the index, deleting a profile, then trying to access it through the index. This would either be out of range if the deleted profile was previously the last one (again, undefined) or the index would be pointing to another profile, potentially reading/modifying the wrong profile info. If anything, it's probably safer after the change you suggested w.r.t. the storage owning the entries and storing the path instead of the index. https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.h (right): https://codereview.chromium.org/1214483002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.h:174: std::vector<ProfileAttributesEntry> GetAllProfilesAttributes() override; On 2015/07/08 18:26:34, Mike Lerman wrote: > On 2015/07/07 21:05:32, anthonyvd wrote: > > On 2015/07/02 18:01:31, Mike Lerman wrote: > > > This is now an unordered collection. Perhaps return a set instead of a > vector > > to > > > convey that order isn't fixed/guaranteed/important? > > > > I believe this would make using this function hard to use. The callers that > need > > the elements sorted would have to transfer them to a sortable collection. > > > > Also, std::vector doesn't guarantee anything about order as opposed to > std::set > > (sorted set) and std::unordered_set faces the problem described above. In that > > sense, I personally feel like it relates the "probably not sorted" situation > > well while letting callers easily sort it if they need to. > > Easily sort - because you can do an in-place sort. Right? > > Please leave a comment then, highlighting this understanding. Exactly! Done. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.cc (right): https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.cc:14: profile_info_cache_ = cache; On 2015/07/08 18:26:35, Mike Lerman wrote: > verify the members are empty (haven't already been set) and that what's being > passed in is non-empty. e.g. DCHECK(!profile_info_cache_); DCHECK(cache); Done. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:25: base::string16 GetName() const; On 2015/07/08 18:26:35, Mike Lerman wrote: > It would be ideal to actually have comments for all of these. I know we didn't > in the ProfileInfoCache, but it would be nice. Feel free to do that in another > CL, though. I'll do it in this CL. The latest patchset has some of them and I'll have another patchset with the rest shortly. (This way you can see/reply to the other discussions we started while I'm commenting all the things :) ) https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_storage.h (right): https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_storage.h:26: virtual void DeleteProfile(const base::FilePath& profile_path) = 0; On 2015/07/08 18:26:35, Mike Lerman wrote: > Please add a comment for this method. e.g. it doesn't delete profile data from > disk, merely boots the associated profile out of this Storage. Added the comment and renamed the method to RemoveProfile() since that probably reflects the actual operation and aligns with AddProfile better. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_storage.h:35: // |entry| should not be cached as it will not reflect subsequent changes to On 2015/07/08 18:26:35, Mike Lerman wrote: > s/will/may. It actually DOES reflect subsequent changes in the current > implementation. Good point, done. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_storage_unittest.cc (right): https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_storage_unittest.cc:289: entry->SetName(base::ASCIIToUTF16("zulu_name")); On 2015/07/08 18:26:35, Mike Lerman wrote: > // Trigger a resort. Done. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_storage_unittest.cc:339: EXPECT_EQ(base::ASCIIToUTF16("NewName"), second_entry->GetName()); On 2015/07/08 18:26:35, Mike Lerman wrote: > We can also test pointer equality. We should have second_entry == first_entry. Done. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:1272: if (GetNumberOfProfiles() == 0) { On 2015/07/08 18:26:35, Mike Lerman wrote: > nit: braces unneeded here - the condition's really short. > (and just below) Done. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:1276: size_t index = GetIndexOfProfileWithPath(path); On 2015/07/08 18:26:35, Mike Lerman wrote: > inline GetIndexOfProfileWithPath into the condition below; we don't use index > anywhere else anymore. Done. https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.h (right): https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.h:167: // ProfileMetadataStorage: On 2015/07/08 18:26:35, Mike Lerman wrote: > Perhaps note that these methods are experimental, and not to be used "yet"? It > would be great to have a way to enforce that, too. Well I'd argue the opposite. If someone were to be writing new code that calls into the ProfileInfoCache I'd much rather have them use the new interface. If anything I'd love to deprecate the other ones! Thoughts?
Great! Woo! Fantastic! LGTM! https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.h (right): https://codereview.chromium.org/1214483002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.h:167: // ProfileMetadataStorage: On 2015/07/09 17:02:14, anthonyvd wrote: > On 2015/07/08 18:26:35, Mike Lerman wrote: > > Perhaps note that these methods are experimental, and not to be used "yet"? It > > would be great to have a way to enforce that, too. > > Well I'd argue the opposite. If someone were to be writing new code that calls > into the ProfileInfoCache I'd much rather have them use the new interface. If > anything I'd love to deprecate the other ones! Thoughts? SGTM.
The CQ bit was checked by anthonyvd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlerman@chromium.org Link to the patchset: https://codereview.chromium.org/1214483002/#ps230001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214483002/230001
Message was sent while issue was closed.
Committed patchset #13 (id:230001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/b50a7cbab0df3cc66f1881dcb08e4cef18f96835 Cr-Commit-Position: refs/heads/master@{#338513} |
