Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1239)

Issue 1563833002: [Part-2]Add Statistics for SupervisedUserSettingService during merging and syncing data. (Closed)

Created:
4 years, 11 months ago by Deepak
Modified:
4 years, 9 months ago
Reviewers:
Bernhard Bauer
CC:
chromium-reviews, pam+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Adding Statistics like number of items added, modified for SupervisedUserSettingService for SUPERVISED_USER_SETTINGS while merging and syncing data starts in supervised_user_settings_service.cc. BUG=572586 Committed: https://crrev.com/d33a1e9898ed8ae0623b5259c0dadd8bfc8381ae Cr-Commit-Position: refs/heads/master@{#378198}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 16

Patch Set 4 : Changes as per review comments. #

Patch Set 5 : Changes as per review comments. #

Total comments: 6

Patch Set 6 : #

Patch Set 7 : #

Total comments: 6

Patch Set 8 : #

Total comments: 2

Patch Set 9 : #

Total comments: 4

Patch Set 10 : #

Total comments: 4

Patch Set 11 : Changes as per review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+150 lines, -19 lines) Patch
M chrome/browser/supervised_user/supervised_user_settings_service.cc View 1 2 3 4 5 6 7 8 9 10 12 chunks +70 lines, -10 lines 0 comments Download
M chrome/browser/supervised_user/supervised_user_settings_service_unittest.cc View 1 2 3 4 5 6 7 8 9 10 6 chunks +80 lines, -9 lines 0 comments Download

Messages

Total messages: 27 (7 generated)
Deepak
*Here all the atomic and split settings have been cleared and then getting created based ...
4 years, 11 months ago (2016-01-08 12:27:57 UTC) #5
Bernhard Bauer
https://codereview.chromium.org/1563833002/diff/40001/chrome/browser/supervised_user/supervised_user_settings_service.cc File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/1563833002/diff/40001/chrome/browser/supervised_user/supervised_user_settings_service.cc#newcode212 chrome/browser/supervised_user/supervised_user_settings_service.cc:212: base::JSONWriter::Write(jt.value(), &json_value); You serialize the values and then compare ...
4 years, 11 months ago (2016-01-08 16:04:47 UTC) #6
Deepak
PTAL https://codereview.chromium.org/1563833002/diff/40001/chrome/browser/supervised_user/supervised_user_settings_service.cc File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/1563833002/diff/40001/chrome/browser/supervised_user/supervised_user_settings_service.cc#newcode212 chrome/browser/supervised_user/supervised_user_settings_service.cc:212: base::JSONWriter::Write(jt.value(), &json_value); On 2016/01/08 16:04:46, Bernhard Bauer wrote: ...
4 years, 11 months ago (2016-01-11 10:38:36 UTC) #7
Deepak
ping :)
4 years, 11 months ago (2016-01-18 04:59:43 UTC) #8
Bernhard Bauer
Sorry for the delay! https://codereview.chromium.org/1563833002/diff/40001/chrome/browser/supervised_user/supervised_user_settings_service.cc File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/1563833002/diff/40001/chrome/browser/supervised_user/supervised_user_settings_service.cc#newcode251 chrome/browser/supervised_user/supervised_user_settings_service.cc:251: // depending on whether they ...
4 years, 11 months ago (2016-01-18 17:56:56 UTC) #9
Deepak
Thanks for the review. PTAL https://codereview.chromium.org/1563833002/diff/80001/chrome/browser/supervised_user/supervised_user_settings_service.cc File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/1563833002/diff/80001/chrome/browser/supervised_user/supervised_user_settings_service.cc#newcode227 chrome/browser/supervised_user/supervised_user_settings_service.cc:227: std::map<std::string, const base::Value*>::iterator it ...
4 years, 11 months ago (2016-01-19 09:54:03 UTC) #10
Bernhard Bauer
https://codereview.chromium.org/1563833002/diff/120001/chrome/browser/supervised_user/supervised_user_settings_service.cc File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/1563833002/diff/120001/chrome/browser/supervised_user/supervised_user_settings_service.cc#newcode228 chrome/browser/supervised_user/supervised_user_settings_service.cc:228: const auto& it = seen_keys.find(name_key); Just auto (the return ...
4 years, 11 months ago (2016-01-20 17:03:04 UTC) #11
Deepak
PTAL https://codereview.chromium.org/1563833002/diff/120001/chrome/browser/supervised_user/supervised_user_settings_service.cc File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/1563833002/diff/120001/chrome/browser/supervised_user/supervised_user_settings_service.cc#newcode228 chrome/browser/supervised_user/supervised_user_settings_service.cc:228: const auto& it = seen_keys.find(name_key); On 2016/01/20 17:03:04, ...
4 years, 11 months ago (2016-01-21 06:15:59 UTC) #12
Bernhard Bauer
https://codereview.chromium.org/1563833002/diff/140001/chrome/browser/supervised_user/supervised_user_settings_service.cc File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/1563833002/diff/140001/chrome/browser/supervised_user/supervised_user_settings_service.cc#newcode196 chrome/browser/supervised_user/supervised_user_settings_service.cc:196: std::map<std::string, const base::Value*> seen_keys; This could now be a ...
4 years, 11 months ago (2016-01-22 18:54:03 UTC) #13
Deepak
A small doubt: When we are adding 2 items via initial_sync_data, so I have num_added ...
4 years, 11 months ago (2016-01-24 07:45:12 UTC) #14
Deepak
4 years, 10 months ago (2016-01-28 05:38:33 UTC) #15
Bernhard Bauer
Sorry again for the delay! I've been kinda busy, and this turned out to be ...
4 years, 10 months ago (2016-01-28 13:56:33 UTC) #16
Deepak
@bernhard Thanks for enlighten me, and giving guidance. Changes done as suggested. PTAL https://codereview.chromium.org/1563833002/diff/160001/chrome/browser/supervised_user/supervised_user_settings_service.cc File ...
4 years, 10 months ago (2016-01-29 07:00:29 UTC) #17
Bernhard Bauer
I think we're almost there :) https://codereview.chromium.org/1563833002/diff/180001/chrome/browser/supervised_user/supervised_user_settings_service.cc File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/1563833002/diff/180001/chrome/browser/supervised_user/supervised_user_settings_service.cc#newcode219 chrome/browser/supervised_user/supervised_user_settings_service.cc:219: if (queued_items) I ...
4 years, 10 months ago (2016-02-03 11:38:13 UTC) #18
Deepak
Sorry for the late reply due to health issues. Final changes done as suggested in ...
4 years, 9 months ago (2016-02-29 07:14:10 UTC) #19
Bernhard Bauer
No worries, hope you're feeling better! LGTM
4 years, 9 months ago (2016-02-29 11:35:16 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1563833002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1563833002/200001
4 years, 9 months ago (2016-02-29 12:05:33 UTC) #22
Deepak
On 2016/02/29 11:35:16, Bernhard Bauer wrote: > No worries, hope you're feeling better! > > ...
4 years, 9 months ago (2016-02-29 12:06:05 UTC) #23
commit-bot: I haz the power
Committed patchset #11 (id:200001)
4 years, 9 months ago (2016-02-29 12:11:10 UTC) #25
commit-bot: I haz the power
4 years, 9 months ago (2016-02-29 12:12:45 UTC) #27
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/d33a1e9898ed8ae0623b5259c0dadd8bfc8381ae
Cr-Commit-Position: refs/heads/master@{#378198}

Powered by Google App Engine
This is Rietveld 408576698