|
|
DescriptionRefactor of ProfileInfoCache: hide methods which return ProfileInfoCache
Currently almost all code using ProfileInfoCache is already migrated to
the new interface. This CL hides methods that provide the old interface
(i.e. ProfileManager::GetProfileInfoCache() and
TestingProfileManager::profile_info_cache() from the public (except for
existing code), so new callers cannot use the old interface by mistake.
BUG=305720
Committed: https://crrev.com/070dc6ff1861a3ce286e9b6d6de66a2c6d4f1adb
Cr-Commit-Position: refs/heads/master@{#396755}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Respond to anthonyvd@'s comment #
Total comments: 4
Patch Set 3 : Use friend class in TestingProfileManager instead of a complicated but more fine-grained mechanism #
Messages
Total messages: 37 (15 generated)
Description was changed from ========== Refactor of ProfileInfoCache: hide methods which return ProfileInfoCache Currently almost all code using ProfileInfoCache is already migrated to the new interface. Now method returning the old interface (i.e. ProfileManager::GetProfileInfoCache() and TestingProfileManager::profile_info_cache() are made invisible to the public (except for existing code), so new callers cannot use the old interface by mistake. BUG=305720 ========== to ========== Refactor of ProfileInfoCache: hide methods which return ProfileInfoCache Currently almost all code using ProfileInfoCache is already migrated to the new interface. This CL hides methods that provide the old interface (i.e. ProfileManager::GetProfileInfoCache() and TestingProfileManager::profile_info_cache() from the public (except for existing code), so new callers cannot use the old interface by mistake. BUG=305720 ==========
lwchkg@gmail.com changed reviewers: + anthonyvd@chromium.org, mlerman@chromium.org
Dear Anthony and Mike, PTAL and also start a CQ dry run (I need to know whether I've missed any non-Windows code). Once the code looks good I'll add owners of chrome/test (probably sky@). Regards, WC Leung.
The CQ bit was checked by mlerman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1926663003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1926663003/1
On 2016/04/27 18:04:54, lwchkg wrote: > Dear Anthony and Mike, > > PTAL and also start a CQ dry run (I need to know whether I've missed any > non-Windows code). > > Once the code looks good I'll add owners of chrome/test (probably sky@). > > Regards, > WC Leung. Do you not have trybots access? You should be able to kick off the bots yourself; I thought I got you this access a while ago.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1926663003/diff/1/chrome/test/base/testing_pr... File chrome/test/base/testing_profile_manager.h (right): https://codereview.chromium.org/1926663003/diff/1/chrome/test/base/testing_pr... chrome/test/base/testing_profile_manager.h:32: class TestingProfileInfoCacheHelper { Why not modify the tests right away in the same CL instead of introducing this new mechanism? https://codereview.chromium.org/1926663003/diff/1/chrome/test/base/testing_pr... chrome/test/base/testing_profile_manager.h:34: // Do NOT expand the friend list. Explain why in the comment.
The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1926663003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1926663003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
New patch uploaded. PTAL. Mike: Just tested and I still don't have trybot access. Anyway, nobody told me that I have trybot access before, and I didn't initiate any trybot access related communication. So I didn't expect I do suddenly have trybot access. P.S. I did sent a email last week to both of you asking for committer status and referral. Not sure whether I qualify for any of them, but I guess it is appropriate to ask again after a few months of work. (Anyway, didn't receive any replies of that email, so I guess the email didn't reach you.) https://codereview.chromium.org/1926663003/diff/1/chrome/test/base/testing_pr... File chrome/test/base/testing_profile_manager.h (right): https://codereview.chromium.org/1926663003/diff/1/chrome/test/base/testing_pr... chrome/test/base/testing_profile_manager.h:32: class TestingProfileInfoCacheHelper { On 2016/04/28 15:31:50, anthonyvd wrote: > Why not modify the tests right away in the same CL instead of introducing this > new mechanism? Before the actual removal of ProfileInfoCache, we need to allow a few tests (most of them are tests of ProfileInfoCache itself) to access profile_info_cache(). In the three friend classes, ProfileInfoCacheTest and ProfileNameVerifierObserver are test classes in profile_info_cache_unittest.cc, and their need to use profile_info_cache() is quite obvious. For ProfileAttributesStorageTest, I'm unable to refactor ProfileAttributesStorageTest.AccessFromElsewhere. It is using both ProfileInfoCache and ProfileAttributesStorage at the same time with a good reason. (Currently gaia_info_update_service_unittest.cc is also using ProfileInfoCache by some inheritance from ProfileInfoCacheTest. It is however unrelated to this mechanism.) P.S. actually I wanted to use the same mechanism in ProfileManager, but found that no one needs the mechanism at all! https://codereview.chromium.org/1926663003/diff/1/chrome/test/base/testing_pr... chrome/test/base/testing_profile_manager.h:34: // Do NOT expand the friend list. On 2016/04/28 15:31:50, anthonyvd wrote: > Explain why in the comment. Done.
lgtm. You should have trybot access now (and should have gotten an email about it recently). Let me know if there's any issue (and congrats :) ).
lwchkg@gmail.com changed reviewers: + sky@chromium.org
Dear Scott, PTAL as owner of chrome/test/. Thanks. Regards, WC Leung.
https://codereview.chromium.org/1926663003/diff/20001/chrome/test/base/testin... File chrome/test/base/testing_profile_manager.h (right): https://codereview.chromium.org/1926663003/diff/20001/chrome/test/base/testin... chrome/test/base/testing_profile_manager.h:31: class TestingProfileInfoCacheHelper { Why do you need a new class? Why not friend the classes in TestingProfileManager?
https://codereview.chromium.org/1926663003/diff/20001/chrome/test/base/testin... File chrome/test/base/testing_profile_manager.h (right): https://codereview.chromium.org/1926663003/diff/20001/chrome/test/base/testin... chrome/test/base/testing_profile_manager.h:31: class TestingProfileInfoCacheHelper { On 2016/05/18 00:05:42, sky wrote: > Why do you need a new class? Why not friend the classes in > TestingProfileManager? If we just friend the classes, then those classes will be able to access all private methods and members of TestingProfileManager, which is not exactly what we want. So we either accept this tradeoff to get simpler code, or have better encapsulation by using an extra class. I'm okay with either. Scott: WDYT?
https://codereview.chromium.org/1926663003/diff/20001/chrome/test/base/testin... File chrome/test/base/testing_profile_manager.h (right): https://codereview.chromium.org/1926663003/diff/20001/chrome/test/base/testin... chrome/test/base/testing_profile_manager.h:31: class TestingProfileInfoCacheHelper { On 2016/05/18 10:35:11, lwchkg wrote: > On 2016/05/18 00:05:42, sky wrote: > > Why do you need a new class? Why not friend the classes in > > TestingProfileManager? > > If we just friend the classes, then those classes will be able to access all > private methods and members of TestingProfileManager, which is not exactly what > we want. > > So we either accept this tradeoff to get simpler code, or have better > encapsulation by using an extra class. I'm okay with either. > > Scott: WDYT? This is temporary right? Assuming it is I would rather not introduce the new class like this. Too bizarre.
Patchset #3 (id:40001) has been deleted
Dear sky@ and anthonyvd@, New patch uploaded. PTAL. Regards, WC Leung. (anthonyvd@: I need your review despite the lgtm you've given for a previous patch.) https://codereview.chromium.org/1926663003/diff/20001/chrome/test/base/testin... File chrome/test/base/testing_profile_manager.h (right): https://codereview.chromium.org/1926663003/diff/20001/chrome/test/base/testin... chrome/test/base/testing_profile_manager.h:31: class TestingProfileInfoCacheHelper { On 2016/05/18 16:27:46, sky wrote: > On 2016/05/18 10:35:11, lwchkg wrote: > > On 2016/05/18 00:05:42, sky wrote: > > > Why do you need a new class? Why not friend the classes in > > > TestingProfileManager? > > > > If we just friend the classes, then those classes will be able to access all > > private methods and members of TestingProfileManager, which is not exactly > what > > we want. > > > > So we either accept this tradeoff to get simpler code, or have better > > encapsulation by using an extra class. I'm okay with either. > > > > Scott: WDYT? > > This is temporary right? Assuming it is I would rather not introduce the new > class like this. Too bizarre. I see. Done.
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1926663003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1926663003/60001
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lwchkg@chromium.org changed reviewers: - mlerman@chromium.org
lwchkg@chromium.org changed reviewers: - lwchkg@chromium.org
ping Anthony: need your final review before committing (despite your L-G-T-M in an old patch).
lgtm
The CQ bit was checked by lwchkg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1926663003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1926663003/60001
Message was sent while issue was closed.
Description was changed from ========== Refactor of ProfileInfoCache: hide methods which return ProfileInfoCache Currently almost all code using ProfileInfoCache is already migrated to the new interface. This CL hides methods that provide the old interface (i.e. ProfileManager::GetProfileInfoCache() and TestingProfileManager::profile_info_cache() from the public (except for existing code), so new callers cannot use the old interface by mistake. BUG=305720 ========== to ========== Refactor of ProfileInfoCache: hide methods which return ProfileInfoCache Currently almost all code using ProfileInfoCache is already migrated to the new interface. This CL hides methods that provide the old interface (i.e. ProfileManager::GetProfileInfoCache() and TestingProfileManager::profile_info_cache() from the public (except for existing code), so new callers cannot use the old interface by mistake. BUG=305720 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Refactor of ProfileInfoCache: hide methods which return ProfileInfoCache Currently almost all code using ProfileInfoCache is already migrated to the new interface. This CL hides methods that provide the old interface (i.e. ProfileManager::GetProfileInfoCache() and TestingProfileManager::profile_info_cache() from the public (except for existing code), so new callers cannot use the old interface by mistake. BUG=305720 ========== to ========== Refactor of ProfileInfoCache: hide methods which return ProfileInfoCache Currently almost all code using ProfileInfoCache is already migrated to the new interface. This CL hides methods that provide the old interface (i.e. ProfileManager::GetProfileInfoCache() and TestingProfileManager::profile_info_cache() from the public (except for existing code), so new callers cannot use the old interface by mistake. BUG=305720 Committed: https://crrev.com/070dc6ff1861a3ce286e9b6d6de66a2c6d4f1adb Cr-Commit-Position: refs/heads/master@{#396755} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/070dc6ff1861a3ce286e9b6d6de66a2c6d4f1adb Cr-Commit-Position: refs/heads/master@{#396755} |