Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
276463002:
non-new-profile-management creates a "no-op" style account_reconcilor, (Closed)
|
Created:
6 years, 7 months ago by Mike Lerman Modified:
6 years, 6 months ago CC:
chromium-reviews, rginda+watch_chromium.org, yoshiki+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionnon-new-profile-management creates a "no-op" style account_reconcilor,
useful for tracking stats but won't have any real effects.
Modify the AccountReconcilor_unittest to execute with the new_profile_management flag on.
BUG=357693
TEST=Account Reconciler should function normally when
new_profile_management flag is on. Should not have effects when the
flag is off, but UMA stats (histograms) and logging (for
--vmodule=account_reconcilor=1) should still trace the execution path.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272131
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272826
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273205
Patch Set 1 #
Total comments: 9
Patch Set 2 : Refactor account_reconcilor naming. Make unit tests work with the flag. #Patch Set 3 : Rebase #Patch Set 4 : Rebase #Patch Set 5 : Fix gaia tests #Patch Set 6 : Rebase #Patch Set 7 : TestSigninClient for AccountReconcilorTests #
Total comments: 12
Patch Set 8 : Nits #Messages
Total messages: 50 (0 generated)
The switch controlling the execution of the AccountReconcilor has been pushed from the ProfileManager into the class itself; it can now perform the logic and trace the execution without performing actual operations. Monica, can you look at the ProfileManager, Roger, can you look at the AccountReconcilor? Obrigado!
profiles lgtm, with nits about variable names :) https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... File components/signin/core/browser/account_reconcilor.h (right): https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... components/signin/core/browser/account_reconcilor.h:234: // Set noop_only mode of the account reconciler should do all the tracking I don't think I understand this comment very well. Would something like "If |noop_only_mode_| is true, then the account reconciler will do all the tracking..." ? I also think the variable name is a little confusing; no-op without the dash looks weird, and no-op-only sounds like a double negation :( How about something like "skip_reconciliation" or "no_reconciliation_needed" instead?
On 2014/05/07 14:57:24, Monica Dinculescu wrote: > profiles lgtm, with nits about variable names :) > > https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... > File components/signin/core/browser/account_reconcilor.h (right): > > https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... > components/signin/core/browser/account_reconcilor.h:234: // Set noop_only mode > of the account reconciler should do all the tracking > I don't think I understand this comment very well. Would something like "If > |noop_only_mode_| is true, then the account reconciler will do all the > tracking..." ? > > I also think the variable name is a little confusing; no-op without the dash > looks weird, and no-op-only sounds like a double negation :( > How about something like "skip_reconciliation" or "no_reconciliation_needed" > instead? How about stats_only_reconciliation? I'll wait and let Roger weigh in.
Looks good, a few comments below. Seems like I lied to you :-) There are some functions that do not have "Perform" in the name that also have side effects. Suggested below to rename those functions/add new functions to make this more explicit. Note that this will affect the mock object overrides in the unit tests. Make sense? https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... File components/signin/core/browser/account_reconcilor.cc (right): https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... components/signin/core/browser/account_reconcilor.cc:344: VLOG(1) << "AccountReconcilor::OnCookieChanged: cookie change is ingored" Could you fix this typo: ingored -> ignored https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... components/signin/core/browser/account_reconcilor.cc:409: merge_session_helper_.LogOut(account_id, accounts_only); Most calls to |merge_session_helper_| have side effects that should not happen in no-op mode. This is one of them. We should make a check at the top of this function. We can also add a check in StartRemoveAction() to avoid an unnecessary round trip to gaia in the call to GetAccountsFromCookie(). Also, to increase consistency, I wonder if we should rename these two function so that "Perform" is in the name. That way we know which methods have side effects: StartRemoveAction --> StartPerformRemoveAction FinishRemoveAction --> FinishPerformRemoveAction https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... components/signin/core/browser/account_reconcilor.cc:738: if (!refresh_token.empty()) { Should probably also add a check to this if block too, since it could change the state of the token service. In theory we should never get here since the check in PerformAddToChromeAction() blocks the series of actions that would eventually call HandleRefreshTokenFetched(), but might be safe. Alternatively, it might also be more consistent with the rest of the code to move the one line 739 into a new function like PerformAddAccountToTokenService(account_id, refresh_token) and add the check for the noop flag in there. What do you think? https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... File components/signin/core/browser/account_reconcilor.h (right): https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... components/signin/core/browser/account_reconcilor.h:234: // Set noop_only mode of the account reconciler should do all the tracking On 2014/05/07 14:57:24, Monica Dinculescu wrote: > I don't think I understand this comment very well. Would something like "If > |noop_only_mode_| is true, then the account reconciler will do all the > tracking..." ? > > I also think the variable name is a little confusing; no-op without the dash > looks weird, and no-op-only sounds like a double negation :( > How about something like "skip_reconciliation" or "no_reconciliation_needed" > instead? The mirror promo may change the value returned by IsNewProfileManagement() at runtime. So better not to cache this at all and just call the function each time we need it. Note that the value can only change from false->true. True->false requires a browser restart, so I think we are covered in all cases.
https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... File components/signin/core/browser/account_reconcilor.cc (right): https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... components/signin/core/browser/account_reconcilor.cc:344: VLOG(1) << "AccountReconcilor::OnCookieChanged: cookie change is ingored" On 2014/05/07 16:14:45, Roger Tawa wrote: > Could you fix this typo: ingored -> ignored Done. https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... components/signin/core/browser/account_reconcilor.cc:409: merge_session_helper_.LogOut(account_id, accounts_only); On 2014/05/07 16:14:45, Roger Tawa wrote: > Most calls to |merge_session_helper_| have side effects that should not happen > in no-op mode. This is one of them. We should make a check at the top of this > function. > > We can also add a check in StartRemoveAction() to avoid an unnecessary round > trip to gaia in the call to GetAccountsFromCookie(). > > Also, to increase consistency, I wonder if we should rename these two function > so that "Perform" is in the name. That way we know which methods have side > effects: > > StartRemoveAction --> StartPerformRemoveAction > FinishRemoveAction --> FinishPerformRemoveAction > Functions renamed. Add a check on top of PerformFinishRemoveAction. As discussed, did not add a check to PerformStartRemoveAction because logging information about the accounts fetch is interesting and desirable. https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... components/signin/core/browser/account_reconcilor.cc:738: if (!refresh_token.empty()) { On 2014/05/07 16:14:45, Roger Tawa wrote: > Should probably also add a check to this if block too, since it could change the > state of the token service. In theory we should never get here since the check > in PerformAddToChromeAction() blocks the series of actions that would eventually > call HandleRefreshTokenFetched(), but might be safe. > > Alternatively, it might also be more consistent with the rest of the code to > move the one line 739 into a new function like > PerformAddAccountToTokenService(account_id, refresh_token) and add the check for > the noop flag in there. What do you think? Added the check, you're right it doesn't hurt (much). This was implemented in a new PerformAddAccountToTokenService method to maintain the pattern of Perform* methods checking the flag. https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... File components/signin/core/browser/account_reconcilor.h (right): https://codereview.chromium.org/276463002/diff/1/components/signin/core/brows... components/signin/core/browser/account_reconcilor.h:234: // Set noop_only mode of the account reconciler should do all the tracking On 2014/05/07 16:14:45, Roger Tawa wrote: > On 2014/05/07 14:57:24, Monica Dinculescu wrote: > > I don't think I understand this comment very well. Would something like "If > > |noop_only_mode_| is true, then the account reconciler will do all the > > tracking..." ? > > > > I also think the variable name is a little confusing; no-op without the dash > > looks weird, and no-op-only sounds like a double negation :( > > How about something like "skip_reconciliation" or "no_reconciliation_needed" > > instead? > > The mirror promo may change the value returned by IsNewProfileManagement() at > runtime. So better not to cache this at all and just call the function each > time we need it. > > Note that the value can only change from false->true. True->false requires a > browser restart, so I think we are covered in all cases. Ok, so since the value will not be cached this variable is removed.
lgtm Thanks Mike.
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/276463002/20001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
On 2014/05/08 21:21:46, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) This CL should wait until the unknown issues affecting the account reconcilor in pre-mirror are resolved.
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/276463002/20001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/276463002/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/276463002/60001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
Roger, Can you please take a new look, for the change I made to fake_gaia? Thanks!
lgtm
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/276463002/80001
Message was sent while issue was closed.
Change committed as 272131
On 2014/05/22 07:59:03, I haz the power (commit-bot) wrote: > Change committed as 272131 Re-opened. Waiting on CL: https://codereview.chromium.org/296703011/
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/276463002/80001
Message was sent while issue was closed.
Change committed as 272826
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/297303003/ by jochen@chromium.org. The reason for reverting is: unit tests fail on mac.
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/299313008/ by mlerman@chromium.org. The reason for reverting is: Fails on Mac unit tests..
Message was sent while issue was closed.
On 2014/05/26 18:30:53, Mike Lerman wrote: > A revert of this CL has been created in > https://codereview.chromium.org/299313008/ by mailto:mlerman@chromium.org. > > The reason for reverting is: Fails on Mac unit tests.. Sorry, ignore my message.
Roger, I added a fair number of changes for the TestSigninClient. Can you please re-review? Obrigado!
lgtm with nits below. https://codereview.chromium.org/276463002/diff/110001/chrome/browser/signin/a... File chrome/browser/signin/account_reconcilor_unittest.cc (right): https://codereview.chromium.org/276463002/diff/110001/chrome/browser/signin/a... chrome/browser/signin/account_reconcilor_unittest.cc:115: TestingProfileManager* testing_profile_manager_; Use a scoped_ptr<TestingProfileManager> instead of raw pointer? https://codereview.chromium.org/276463002/diff/110001/chrome/browser/signin/a... chrome/browser/signin/account_reconcilor_unittest.cc:458: Remove extra line. https://codereview.chromium.org/276463002/diff/110001/chrome/browser/signin/c... File chrome/browser/signin/chrome_signin_client_factory.cc (right): https://codereview.chromium.org/276463002/diff/110001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_client_factory.cc:9: #include "components/signin/core/browser/test_signin_client.h" Is this include needed here? https://codereview.chromium.org/276463002/diff/110001/components/signin/core/... File components/signin/core/browser/test_signin_client.cc (right): https://codereview.chromium.org/276463002/diff/110001/components/signin/core/... components/signin/core/browser/test_signin_client.cc:24: base::MessageLoopProxy::current())) { Set |pref_service_| member to NULL. https://codereview.chromium.org/276463002/diff/110001/components/signin/core/... File components/signin/core/browser/test_signin_client.h (right): https://codereview.chromium.org/276463002/diff/110001/components/signin/core/... components/signin/core/browser/test_signin_client.h:24: } Do we need these forward declares? I'm not sure there should be in //component code. Should forward declare PrefService? https://codereview.chromium.org/276463002/diff/110001/google_apis/gaia/fake_g... File google_apis/gaia/fake_gaia.cc (right): https://codereview.chromium.org/276463002/diff/110001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.cc:542: kPeopleGetResponseFormat, "name")); Is it worth using StringPrintf if we are hardcoding %s to "name" all the time?
Thanks Roger. https://codereview.chromium.org/276463002/diff/110001/chrome/browser/signin/a... File chrome/browser/signin/account_reconcilor_unittest.cc (right): https://codereview.chromium.org/276463002/diff/110001/chrome/browser/signin/a... chrome/browser/signin/account_reconcilor_unittest.cc:115: TestingProfileManager* testing_profile_manager_; On 2014/05/27 19:15:38, Roger Tawa wrote: > Use a scoped_ptr<TestingProfileManager> instead of raw pointer? Done. https://codereview.chromium.org/276463002/diff/110001/chrome/browser/signin/a... chrome/browser/signin/account_reconcilor_unittest.cc:458: On 2014/05/27 19:15:38, Roger Tawa wrote: > Remove extra line. Done. https://codereview.chromium.org/276463002/diff/110001/chrome/browser/signin/c... File chrome/browser/signin/chrome_signin_client_factory.cc (right): https://codereview.chromium.org/276463002/diff/110001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_client_factory.cc:9: #include "components/signin/core/browser/test_signin_client.h" On 2014/05/27 19:15:38, Roger Tawa wrote: > Is this include needed here? Done. https://codereview.chromium.org/276463002/diff/110001/components/signin/core/... File components/signin/core/browser/test_signin_client.cc (right): https://codereview.chromium.org/276463002/diff/110001/components/signin/core/... components/signin/core/browser/test_signin_client.cc:24: base::MessageLoopProxy::current())) { On 2014/05/27 19:15:38, Roger Tawa wrote: > Set |pref_service_| member to NULL. Done. https://codereview.chromium.org/276463002/diff/110001/components/signin/core/... File components/signin/core/browser/test_signin_client.h (right): https://codereview.chromium.org/276463002/diff/110001/components/signin/core/... components/signin/core/browser/test_signin_client.h:24: } On 2014/05/27 19:15:38, Roger Tawa wrote: > Do we need these forward declares? I'm not sure there should be in //component > code. > > Should forward declare PrefService? Done. https://codereview.chromium.org/276463002/diff/110001/google_apis/gaia/fake_g... File google_apis/gaia/fake_gaia.cc (right): https://codereview.chromium.org/276463002/diff/110001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.cc:542: kPeopleGetResponseFormat, "name")); On 2014/05/27 19:15:38, Roger Tawa wrote: > Is it worth using StringPrintf if we are hardcoding %s to "name" all the time? Done.
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/276463002/130001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/276463002/130001
Message was sent while issue was closed.
Change committed as 273205 |
