Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1599013002:
Follow-up of Improve the ProfileInfoCache API (Closed)
DescriptionFollow-up of Improve the ProfileInfoCache API
This is a follow-up of https://codereview.chromium.org/1214483002/, which adds
functions in profile_manager.cc and testing_profile_manager.cc that returns
ProfileAttributesStorage. We should obtain ProfileAttributesEntry using
either GetProfileAttributesWithPath() or GetAllProfilesAttributes() in
ProfileAttributesStorage.
A sample use of the ProfileAttributesStorage is found in profile_statistics.cc
(Note: profile_statistics.cc and also profile_statistics_unittest.cc
needs a few naming changes to reflect the use of the new interface. However,
to make things simple, these changes are not included in this CL.)
BUG=305720
Committed: https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb
Cr-Commit-Position: refs/heads/master@{#370831}
Patch Set 1 #
Total comments: 7
Messages
Total messages: 28 (14 generated)
Description was changed from ========== Added functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage, which is the intended way to get ProfileAttributesEntry. A sample use of the interface is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the interace. However, to make things simple, these changes are not included in this CL.) BUG=305720 ========== to ========== Added functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage, which is the intended way to get ProfileAttributesEntry. A sample use of the interface is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the interace. However, to make things simple, these changes are not included in this CL.) BUG=305720 ==========
Description was changed from ========== Added functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage, which is the intended way to get ProfileAttributesEntry. A sample use of the interface is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the interace. However, to make things simple, these changes are not included in this CL.) BUG=305720 ========== to ========== 123456789012345678901234567890123456789012345678901234567890123456789012 This is a follow-up of CL 1214483002, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the interace. However, to make things simple, these changes are not included in this CL.) BUG=305720 ==========
lwchkg@gmail.com changed reviewers: + anthonyvd@chromium.org, mlerman@chromium.org
Description was changed from ========== 123456789012345678901234567890123456789012345678901234567890123456789012 This is a follow-up of CL 1214483002, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the interace. However, to make things simple, these changes are not included in this CL.) BUG=305720 ========== to ========== This is a follow-up of CL 1214483002, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the interace. However, to make things simple, these changes are not included in this CL.) BUG=305720 ==========
Description was changed from ========== This is a follow-up of CL 1214483002, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the interace. However, to make things simple, these changes are not included in this CL.) BUG=305720 ========== to ========== This is a follow-up of CL 1214483002, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using either GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the interace. However, to make things simple, these changes are not included in this CL.) BUG=305720 ==========
Description was changed from ========== This is a follow-up of CL 1214483002, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using either GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the interace. However, to make things simple, these changes are not included in this CL.) BUG=305720 ========== to ========== This is a follow-up of CL 1214483002, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using either GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the new interface. However, to make things simple, these changes are not included in this CL.) BUG=305720 ==========
Dear Anthony and Mike, PTAL. Regards, WC Leung. https://codereview.chromium.org/1599013002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1599013002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics.cc:354: ProfileCategoryStats GetProfileStatisticsFromCache( This function will be renamed to GetProfileStatisticsFromAttributesStorage in a follow-up CL. WDYT about the function name? https://codereview.chromium.org/1599013002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics.cc:387: void SetProfileStatisticsInCache(const base::FilePath& profile_path, Similarly, this function will be renamed SetProfileStatisticsInAttributesStorage.
Thanks for this, I only have a single comment. https://codereview.chromium.org/1599013002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1599013002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics.cc:354: ProfileCategoryStats GetProfileStatisticsFromCache( On 2016/01/17 12:33:28, lwchkg wrote: > This function will be renamed to GetProfileStatisticsFromAttributesStorage in a > follow-up CL. WDYT about the function name? I think this functionality (as well as the other one you left a comment on) should probably be moved to the ProfileAttributesStorage/Entry since ultimately it's just forwarding info it gets from there.
Here's my reply. https://codereview.chromium.org/1599013002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1599013002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics.cc:354: ProfileCategoryStats GetProfileStatisticsFromCache( On 2016/01/18 15:22:31, anthonyvd wrote: > On 2016/01/17 12:33:28, lwchkg wrote: > > This function will be renamed to GetProfileStatisticsFromAttributesStorage in > a > > follow-up CL. WDYT about the function name? > > I think this functionality (as well as the other one you left a comment on) > should probably be moved to the ProfileAttributesStorage/Entry since ultimately > it's just forwarding info it gets from there. The decision of putting these two functions here was made in the following CL : https://codereview.chromium.org/1415223002/diff/190001/chrome/browser/profile... Summary: My first implementation did something you suggested here, but Mike was against returning a std::map from ProfileInfoCache. His reason is: it isn't ProfileInfoCache's role to return a structure. Finally I agreed with him so the conversions from those bool and int into structures (i.e. ProfileCategoryStats and base::DictionaryValue) are done here and also in user_manager_screen_handler.cc. Note: the CL about the second file is found in https://codereview.chromium.org/1428973003/ . No more related discussion is made there though.
This seems good to me, but I'll let anthony do final l-g-t-m. https://codereview.chromium.org/1599013002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1599013002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics.cc:387: void SetProfileStatisticsInCache(const base::FilePath& profile_path, On 2016/01/17 12:33:28, lwchkg wrote: > Similarly, this function will be renamed > SetProfileStatisticsInAttributesStorage. Or just SetProfileStatisticsInStorage. (don't need Atttributes any more than we didn't need Info before)
On 2016/01/19 16:57:46, Mike Lerman wrote: > This seems good to me, but I'll let anthony do final l-g-t-m. > > https://codereview.chromium.org/1599013002/diff/1/chrome/browser/profiles/pro... > File chrome/browser/profiles/profile_statistics.cc (right): > > https://codereview.chromium.org/1599013002/diff/1/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_statistics.cc:387: void > SetProfileStatisticsInCache(const base::FilePath& profile_path, > On 2016/01/17 12:33:28, lwchkg wrote: > > Similarly, this function will be renamed > > SetProfileStatisticsInAttributesStorage. > > Or just SetProfileStatisticsInStorage. (don't need Atttributes any more than we > didn't need Info before) That name is fine by me. Thanks for explaining the rationale lwchkg@. lgtm
The CQ bit was checked by lwchkg@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1599013002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1599013002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lwchkg@gmail.com changed reviewers: + sky@chromium.org
Dear Scott, PTAL. I need your review on chrome/test/base/*. Thanks. Regards, WC Leung.
https://codereview.chromium.org/1599013002/diff/1/chrome/test/base/testing_pr... File chrome/test/base/testing_profile_manager.cc (right): https://codereview.chromium.org/1599013002/diff/1/chrome/test/base/testing_pr... chrome/test/base/testing_profile_manager.cc:240: return profile_info_cache(); Why do you need this function? Can't callers use profile_info_cache() directly? I don't see any test code calling this function. Am I missing something?
Scott: Please see my reply below. https://codereview.chromium.org/1599013002/diff/1/chrome/test/base/testing_pr... File chrome/test/base/testing_profile_manager.cc (right): https://codereview.chromium.org/1599013002/diff/1/chrome/test/base/testing_pr... chrome/test/base/testing_profile_manager.cc:240: return profile_info_cache(); On 2016/01/20 16:19:55, sky wrote: > Why do you need this function? Can't callers use profile_info_cache() directly? > I don't see any test code calling this function. Am I missing something? 1. We plan to replace profile_info_cache() by this function later. But at this stage it is only a thin wrapper around profile_info_cache(). 2. AFAIK no code is calling this function now. But we do plan to replace all test code calling profile_info_cache() by this function right after this CL lands. Anyway, this CL is one of a long series of CLs in the refactoring of ProfileInfoCache. The plan is written in the last page of the following document: https://docs.google.com/a/google.com/document/d/12V8ahA9lEsXWyT8OqasEz3HR4J5j... (I'm not sure if this is written by Anthony, or by an old c/b/profile owner.) FYI, here is a past CL that was committed: https://codereview.chromium.org/1214483002/ (by Anthony) And several CLs that has never exited draft. This CL is entirely extracted from the first CL in the list. https://codereview.chromium.org/1242793005/ (by Anthony) https://codereview.chromium.org/33753002/ (by noms (Monica), 2yrs ago) https://codereview.chromium.org/8539043/ (by Sailesh, 4yrs ago)
Ok, LGTM
The CQ bit was checked by lwchkg@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1599013002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1599013002/1
Description was changed from ========== This is a follow-up of CL 1214483002, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using either GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the new interface. However, to make things simple, these changes are not included in this CL.) BUG=305720 ========== to ========== Follow-up of Improve the ProfileInfoCache API This is a follow-up of CL 1214483002, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using either GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the new interface. However, to make things simple, these changes are not included in this CL.) BUG=305720 ==========
Description was changed from ========== Follow-up of Improve the ProfileInfoCache API This is a follow-up of CL 1214483002, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using either GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the new interface. However, to make things simple, these changes are not included in this CL.) BUG=305720 ========== to ========== Follow-up of Improve the ProfileInfoCache API This is a follow-up of https://codereview.chromium.org/1214483002/, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using either GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the new interface. However, to make things simple, these changes are not included in this CL.) BUG=305720 ==========
Message was sent while issue was closed.
Description was changed from ========== Follow-up of Improve the ProfileInfoCache API This is a follow-up of https://codereview.chromium.org/1214483002/, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using either GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the new interface. However, to make things simple, these changes are not included in this CL.) BUG=305720 ========== to ========== Follow-up of Improve the ProfileInfoCache API This is a follow-up of https://codereview.chromium.org/1214483002/, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using either GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the new interface. However, to make things simple, these changes are not included in this CL.) BUG=305720 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Follow-up of Improve the ProfileInfoCache API This is a follow-up of https://codereview.chromium.org/1214483002/, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using either GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the new interface. However, to make things simple, these changes are not included in this CL.) BUG=305720 ========== to ========== Follow-up of Improve the ProfileInfoCache API This is a follow-up of https://codereview.chromium.org/1214483002/, which adds functions in profile_manager.cc and testing_profile_manager.cc that returns ProfileAttributesStorage. We should obtain ProfileAttributesEntry using either GetProfileAttributesWithPath() or GetAllProfilesAttributes() in ProfileAttributesStorage. A sample use of the ProfileAttributesStorage is found in profile_statistics.cc (Note: profile_statistics.cc and also profile_statistics_unittest.cc needs a few naming changes to reflect the use of the new interface. However, to make things simple, these changes are not included in this CL.) BUG=305720 Committed: https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb Cr-Commit-Position: refs/heads/master@{#370831} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb Cr-Commit-Position: refs/heads/master@{#370831} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
