Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1530763004:
Add Statistics for SupervisedUserSettingService during merging and syncing data. (Closed)
|
Created:
5 years ago by Deepak Modified:
4 years, 11 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, pam+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding Statistics like number of items added, modified for
SupervisedUserSettingService for SUPERVISED_USER_SHARED_SETTINGS while merging
and syncing data starts.
BUG=572586
Committed: https://crrev.com/1974ef3da3b8a4c68a0d83899d0150a489b0f5be
Cr-Commit-Position: refs/heads/master@{#368060}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : Changes as per review comments. #Patch Set 4 : Added before and after association statistic. #
Total comments: 5
Patch Set 5 : Added Test Case. #
Total comments: 8
Patch Set 6 : Changes as per review comments. #
Total comments: 8
Patch Set 7 : Changes as per review comments. #
Total comments: 8
Patch Set 8 : Changes as per review comments. #Patch Set 9 : #
Total comments: 2
Patch Set 10 : Addressing review comments. #
Messages
Total messages: 27 (8 generated)
Description was changed from ========== Adding Statistics for SupervisedUserSettingService for SUPERVISED_USER_SETTINGS and SUPERVISED_USER_SHARED_SETTINGS while merging and syncing data starts. BUG= ========== to ========== Adding Statistics for SupervisedUserSettingService for SUPERVISED_USER_SETTINGS and SUPERVISED_USER_SHARED_SETTINGS while merging and syncing data starts. BUG= ==========
Description was changed from ========== Adding Statistics for SupervisedUserSettingService for SUPERVISED_USER_SETTINGS and SUPERVISED_USER_SHARED_SETTINGS while merging and syncing data starts. BUG= ========== to ========== Adding Statistics for SupervisedUserSettingService for SUPERVISED_USER_SETTINGS and SUPERVISED_USER_SHARED_SETTINGS while merging and syncing data starts. BUG=572586 ==========
deepak.m1@samsung.com changed reviewers: + bauerb@chromium.org
PTAL
https://codereview.chromium.org/1530763004/diff/20001/chrome/browser/supervis... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (left): https://codereview.chromium.org/1530763004/diff/20001/chrome/browser/supervis... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:262: // TODO(bauerb): Statistics? Sorry, that comment should have been more detailed 😃 The statistics mentioned here are statistics about the merge result, i.e. things like the number of items that were added, deleted, updated, etc. (see the API of SyncMergeResult for details), not UMA histograms.
On 2016/01/04 12:53:30, bauerb catching up on reviews wrote: > https://codereview.chromium.org/1530763004/diff/20001/chrome/browser/supervis... > File > chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc > (left): > > https://codereview.chromium.org/1530763004/diff/20001/chrome/browser/supervis... > chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:262: > // TODO(bauerb): Statistics? > Sorry, that comment should have been more detailed 😃 The statistics mentioned > here are statistics about the merge result, i.e. things like the number of items > that were added, deleted, updated, etc. (see the API of SyncMergeResult for > details), not UMA histograms. Ahh.. I misunderstood.. I will do changes again and submit. Thanks for pointing out and giving directions :) .
Description was changed from ========== Adding Statistics for SupervisedUserSettingService for SUPERVISED_USER_SETTINGS and SUPERVISED_USER_SHARED_SETTINGS while merging and syncing data starts. BUG=572586 ========== to ========== Adding Statistics like number of items added, modified for SupervisedUserSettingService for SUPERVISED_USER_SETTINGS and SUPERVISED_USER_SHARED_SETTINGS while merging and syncing data starts. BUG=572586 ==========
PTAL https://codereview.chromium.org/1530763004/diff/20001/chrome/browser/supervis... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (left): https://codereview.chromium.org/1530763004/diff/20001/chrome/browser/supervis... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:262: // TODO(bauerb): Statistics? On 2016/01/04 12:53:30, bauerb catching up on reviews wrote: > Sorry, that comment should have been more detailed 😃 The statistics mentioned > here are statistics about the merge result, i.e. things like the number of items > that were added, deleted, updated, etc. (see the API of SyncMergeResult for > details), not UMA histograms. Done.
Thanks! Could you also update the unit tests for these classes to verify the numbers? That would also help with sanity-checking the logic in here. https://codereview.chromium.org/1530763004/diff/60001/chrome/browser/supervis... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (right): https://codereview.chromium.org/1530763004/diff/60001/chrome/browser/supervis... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:264: result.set_num_items_added(change_list.size()); Wait, is this right? IIUC, num_items_added is the number of items added to the _local_ set of data, whereas this is the number of items added remotely. https://codereview.chromium.org/1530763004/diff/60001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/1530763004/diff/60001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_settings_service.cc:220: int num_before_association = queued_items->size(); This also doesn't seem quite right... This is only the number of items that were queued for upload (but couldn't be uploaded before, because Sync wasn't ready yet). The number of items should be the total though, i.e. including all the atomic and split settings.
Description was changed from ========== Adding Statistics like number of items added, modified for SupervisedUserSettingService for SUPERVISED_USER_SETTINGS and SUPERVISED_USER_SHARED_SETTINGS while merging and syncing data starts. BUG=572586 ========== to ========== Adding Statistics like number of items added, modified for SupervisedUserSettingService for SUPERVISED_USER_SHARED_SETTINGS while merging and syncing data starts. BUG=572586 ==========
PTAL https://codereview.chromium.org/1530763004/diff/60001/chrome/browser/supervis... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (right): https://codereview.chromium.org/1530763004/diff/60001/chrome/browser/supervis... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:264: result.set_num_items_added(change_list.size()); On 2016/01/05 09:37:30, bauerb catching up on reviews wrote: > Wait, is this right? IIUC, num_items_added is the number of items added to the > _local_ set of data, whereas this is the number of items added remotely. Done. https://codereview.chromium.org/1530763004/diff/60001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/1530763004/diff/60001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_settings_service.cc:220: int num_before_association = queued_items->size(); On 2016/01/05 09:37:30, bauerb catching up on reviews wrote: > This also doesn't seem quite right... This is only the number of items that were > queued for upload (but couldn't be uploaded before, because Sync wasn't ready > yet). The number of items should be the total though, i.e. including all the > atomic and split settings. I would like to do the changes of this file in separate CL,once the changes of supervised_user_shared_settings_service.cc goes properly.
https://codereview.chromium.org/1530763004/diff/60001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/1530763004/diff/60001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_settings_service.cc:220: int num_before_association = queued_items->size(); On 2016/01/05 13:29:19, Deepak wrote: > On 2016/01/05 09:37:30, bauerb catching up on reviews wrote: > > This also doesn't seem quite right... This is only the number of items that > were > > queued for upload (but couldn't be uploaded before, because Sync wasn't ready > > yet). The number of items should be the total though, i.e. including all the > > atomic and split settings. > > I would like to do the changes of this file in separate CL,once the changes of > supervised_user_shared_settings_service.cc goes properly. Acknowledged. https://codereview.chromium.org/1530763004/diff/80001/chrome/browser/supervis... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (right): https://codereview.chromium.org/1530763004/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:268: result.set_num_items_modified(num_modified); Add the number of items before and after association as well? (And I would actually not do anything clever like inferring the number of items afterwards from the number of items beforehand plus the number of added items, because the correctness of that is what we want to verify.) https://codereview.chromium.org/1530763004/diff/80001/chrome/browser/supervis... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service_unittest.cc (right): https://codereview.chromium.org/1530763004/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service_unittest.cc:236: TEST_F(SupervisedUserSharedSettingsServiceTest, MergeResult) { Can you just merge this into the Merge test (heh)? https://codereview.chromium.org/1530763004/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service_unittest.cc:242: EXPECT_EQ(0, result.num_items_modified()); Also verify the number of deleted items, just for completeness? https://codereview.chromium.org/1530763004/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service_unittest.cc:265: sync_data.push_back( I don't think we need to test this, as this shouldn't happen in practice (the ID should be unique for server-side data).
PTAL https://codereview.chromium.org/1530763004/diff/80001/chrome/browser/supervis... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (right): https://codereview.chromium.org/1530763004/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:268: result.set_num_items_modified(num_modified); On 2016/01/05 14:10:54, Bernhard Bauer wrote: > Add the number of items before and after association as well? > > (And I would actually not do anything clever like inferring the number of items > afterwards from the number of items beforehand plus the number of added items, > because the correctness of that is what we want to verify.) Done. https://codereview.chromium.org/1530763004/diff/80001/chrome/browser/supervis... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service_unittest.cc (right): https://codereview.chromium.org/1530763004/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service_unittest.cc:236: TEST_F(SupervisedUserSharedSettingsServiceTest, MergeResult) { On 2016/01/05 14:10:54, Bernhard Bauer wrote: > Can you just merge this into the Merge test (heh)? Done. https://codereview.chromium.org/1530763004/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service_unittest.cc:242: EXPECT_EQ(0, result.num_items_modified()); On 2016/01/05 14:10:54, Bernhard Bauer wrote: > Also verify the number of deleted items, just for completeness? Done. https://codereview.chromium.org/1530763004/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service_unittest.cc:265: sync_data.push_back( On 2016/01/05 14:10:54, Bernhard Bauer wrote: > I don't think we need to test this, as this shouldn't happen in practice (the ID > should be unique for server-side data). Done.
https://codereview.chromium.org/1530763004/diff/100001/chrome/browser/supervi... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (right): https://codereview.chromium.org/1530763004/diff/100001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:208: DictionaryPrefUpdate update_prefs(prefs_, If you don't need to modify anything, you can just use prefs_->GetDictionary(). https://codereview.chromium.org/1530763004/diff/100001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:211: result.set_num_items_before_association(pref_dict->size()); Can you put this into a local variable like |num_added| and |num_modified|, then construct the result below like you did previously? Also, isn't this a two-level dictionary, so we'd need to iterate over it and add the sizes of its child dictionaries? https://codereview.chromium.org/1530763004/diff/100001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:275: result.set_num_items_deleted(0); It's fine not to set this value (0 is the default anyway), I just wanted you to verify it. https://codereview.chromium.org/1530763004/diff/100001/chrome/browser/supervi... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service_unittest.cc (right): https://codereview.chromium.org/1530763004/diff/100001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service_unittest.cc:229: EXPECT_EQ(3, result.num_items_after_association()); Is this correct? I would have expected 3 values before association (lines 207-209), 1 modification (the name for kid A changes from Jack to Jill), and 1 addition ("baz" for kid C), for a total of 4 values after association. ...and after looking at this for a while, I can see why you get these results, but it's because the logic you've added is indeed wrong (so, good thing we added unit tests!) You count the number of users before and after association instead of the total number of settings for all users (see my comment above), and what you calculate for the number of items added and modified is... something altogether different. This also goes to show why writing tests doesn't mean you should just run your code and then define whatever the output is to be correct. So, I've told you above what the output should be; now please update the expectations correspondingly and then fix the code so the test passes.
Thanks for review and giving directions. Changes done as suggested. PTAL. https://codereview.chromium.org/1530763004/diff/100001/chrome/browser/supervi... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (right): https://codereview.chromium.org/1530763004/diff/100001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:208: DictionaryPrefUpdate update_prefs(prefs_, On 2016/01/06 16:41:50, Bernhard Bauer wrote: > If you don't need to modify anything, you can just use prefs_->GetDictionary(). Done. https://codereview.chromium.org/1530763004/diff/100001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:211: result.set_num_items_before_association(pref_dict->size()); On 2016/01/06 16:41:50, Bernhard Bauer wrote: > Can you put this into a local variable like |num_added| and |num_modified|, then > construct the result below like you did previously? > > Also, isn't this a two-level dictionary, so we'd need to iterate over it and add > the sizes of its child dictionaries? Done. https://codereview.chromium.org/1530763004/diff/100001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:275: result.set_num_items_deleted(0); On 2016/01/06 16:41:50, Bernhard Bauer wrote: > It's fine not to set this value (0 is the default anyway), I just wanted you to > verify it. Done. https://codereview.chromium.org/1530763004/diff/100001/chrome/browser/supervi... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service_unittest.cc (right): https://codereview.chromium.org/1530763004/diff/100001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service_unittest.cc:229: EXPECT_EQ(3, result.num_items_after_association()); On 2016/01/06 16:41:50, Bernhard Bauer wrote: > Is this correct? I would have expected 3 values before association (lines > 207-209), 1 modification (the name for kid A changes from Jack to Jill), and 1 > addition ("baz" for kid C), for a total of 4 values after association. > > ...and after looking at this for a while, I can see why you get these results, > but it's because the logic you've added is indeed wrong (so, good thing we added > unit tests!) You count the number of users before and after association instead > of the total number of settings for all users (see my comment above), and what > you calculate for the number of items added and modified is... something > altogether different. > > This also goes to show why writing tests doesn't mean you should just run your > code and then define whatever the output is to be correct. So, I've told you > above what the output should be; now please update the expectations > correspondingly and then fix the code so the test passes. Done.
https://codereview.chromium.org/1530763004/diff/120001/chrome/browser/supervi... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (right): https://codereview.chromium.org/1530763004/diff/120001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:208: const DictionaryValue* dict = NULL; Use nullptr instead of NULL. https://codereview.chromium.org/1530763004/diff/120001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:218: std::map<std::string, std::set<std::string> > seen_keys; Rename this to |sync_seen_keys| to distinguish it from |pref_seen_keys|? https://codereview.chromium.org/1530763004/diff/120001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:257: const DictionaryValue* dict = NULL; Could you update this to nullptr as well? Thanks! https://codereview.chromium.org/1530763004/diff/120001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:285: result.set_num_items_after_association(num_before_association + num_added); Let's not do this, please. If we have a bug in the code above, manually counting the number of items afterwards might expose it, whereas if we calculate it like this, we don't get anything from verifying that num_after_association == num_before_association + num_added. We already have a loop over the preferences after association (starting at line 255), so we can do the count there.
PTAL https://codereview.chromium.org/1530763004/diff/120001/chrome/browser/supervi... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (right): https://codereview.chromium.org/1530763004/diff/120001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:208: const DictionaryValue* dict = NULL; On 2016/01/07 10:04:29, Bernhard Bauer wrote: > Use nullptr instead of NULL. Done. https://codereview.chromium.org/1530763004/diff/120001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:218: std::map<std::string, std::set<std::string> > seen_keys; On 2016/01/07 10:04:29, Bernhard Bauer wrote: > Rename this to |sync_seen_keys| to distinguish it from |pref_seen_keys|? Done. https://codereview.chromium.org/1530763004/diff/120001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:257: const DictionaryValue* dict = NULL; On 2016/01/07 10:04:29, Bernhard Bauer wrote: > Could you update this to nullptr as well? Thanks! Done. https://codereview.chromium.org/1530763004/diff/120001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:285: result.set_num_items_after_association(num_before_association + num_added); On 2016/01/07 10:04:29, Bernhard Bauer wrote: > Let's not do this, please. If we have a bug in the code above, manually counting > the number of items afterwards might expose it, whereas if we calculate it like > this, we don't get anything from verifying that num_after_association == > num_before_association + num_added. > > We already have a loop over the preferences after association (starting at line > 255), so we can do the count there. I got your point. Thanks
https://codereview.chromium.org/1530763004/diff/160001/chrome/browser/supervi... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (right): https://codereview.chromium.org/1530763004/diff/160001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:201: int num_added = 0; Actually, can you move |num_added| and |num_modified| to right before the loop currently in line 223, and |num_after_association| to right before the loop in line 254? That way they are declared as late as possible.
PTAL Thanks https://codereview.chromium.org/1530763004/diff/160001/chrome/browser/supervi... File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (right): https://codereview.chromium.org/1530763004/diff/160001/chrome/browser/supervi... chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:201: int num_added = 0; On 2016/01/07 10:54:46, Bernhard Bauer wrote: > Actually, can you move |num_added| and |num_modified| to right before the loop > currently in line 223, and |num_after_association| to right before the loop in > line 254? That way they are declared as late as possible. Done.
LGTM, thanks!
The CQ bit was checked by deepak.m1@samsung.com
On 2016/01/07 12:00:07, Bernhard Bauer wrote: > LGTM, thanks! Thankyou Bernhard.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1530763004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1530763004/180001
Message was sent while issue was closed.
Description was changed from ========== Adding Statistics like number of items added, modified for SupervisedUserSettingService for SUPERVISED_USER_SHARED_SETTINGS while merging and syncing data starts. BUG=572586 ========== to ========== Adding Statistics like number of items added, modified for SupervisedUserSettingService for SUPERVISED_USER_SHARED_SETTINGS while merging and syncing data starts. BUG=572586 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Adding Statistics like number of items added, modified for SupervisedUserSettingService for SUPERVISED_USER_SHARED_SETTINGS while merging and syncing data starts. BUG=572586 ========== to ========== Adding Statistics like number of items added, modified for SupervisedUserSettingService for SUPERVISED_USER_SHARED_SETTINGS while merging and syncing data starts. BUG=572586 Committed: https://crrev.com/1974ef3da3b8a4c68a0d83899d0150a489b0f5be Cr-Commit-Position: refs/heads/master@{#368060} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/1974ef3da3b8a4c68a0d83899d0150a489b0f5be Cr-Commit-Position: refs/heads/master@{#368060} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
