Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2374913002:
[USS] Show USS counters in about:sync page (Closed)
Description[USS] Show USS counters in about:sync page
BUG=328606
Committed: https://crrev.com/d26ef311a38ab8e88b4fa82a6a6753786a873e36
Cr-Commit-Position: refs/heads/master@{#423808}
Patch Set 1 #Patch Set 2 : For IOS #
Total comments: 8
Patch Set 3 : update base on comments, and rebase #
Total comments: 3
Patch Set 4 : remove unique_ptr and rebase #
Total comments: 14
Patch Set 5 : get processor on model thread and rebase #
Total comments: 6
Patch Set 6 : re-comments #Messages
Total messages: 77 (62 generated)
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== check in for sync-internals counter BUG= ========== to ========== check in for sync-internals counter BUG= ==========
gangwu@chromium.org changed reviewers: + pavely@chromium.org
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Patchset #3 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
Description was changed from ========== check in for sync-internals counter BUG= ========== to ========== [USS] Show USS counters in about page BUG=328606 ==========
Description was changed from ========== [USS] Show USS counters in about page BUG=328606 ========== to ========== [USS] Show USS counters in about:sync page BUG=328606 ==========
PTAL https://codereview.chromium.org/2374913002/diff/80001/components/browser_sync... File components/browser_sync/profile_sync_service.h (left): https://codereview.chromium.org/2374913002/diff/80001/components/browser_sync... components/browser_sync/profile_sync_service.h:324: base::Value* GetTypeStatusMap() const override; remove this is because ProfileSyncService::OnDirectoryTypeStatusCounterUpdated do not have const, and cannot add const to OnDirectoryTypeStatusCounterUpdated since OnDirectoryTypeStatusCounterUpdated uses FOR_EACH_OBSERVER, and FOR_EACH_OBSERVER uses iterator not const_interator.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
What is the reason you function signatures passing counters from const reference to unique_ptr? https://codereview.chromium.org/2374913002/diff/80001/components/browser_sync... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2374913002/diff/80001/components/browser_sync... components/browser_sync/profile_sync_service.cc:563: OnStatusCountersUpdated(type, std::move(counters))); This macro expands into a loop with function invocation included in it. With multiple observers I think only one will get ownership of counters. Why do you need to transfer ownership? https://codereview.chromium.org/2374913002/diff/80001/components/browser_sync... File components/browser_sync/profile_sync_service.h (left): https://codereview.chromium.org/2374913002/diff/80001/components/browser_sync... components/browser_sync/profile_sync_service.h:324: base::Value* GetTypeStatusMap() const override; On 2016/09/28 00:54:59, Gang Wu wrote: > remove this is because ProfileSyncService::OnDirectoryTypeStatusCounterUpdated > do not have const, and cannot add const to OnDirectoryTypeStatusCounterUpdated > since OnDirectoryTypeStatusCounterUpdated uses FOR_EACH_OBSERVER, and > FOR_EACH_OBSERVER uses iterator not const_interator. Acknowledged. https://codereview.chromium.org/2374913002/diff/80001/components/sync/driver/... File components/sync/driver/sync_frontend.h (right): https://codereview.chromium.org/2374913002/diff/80001/components/sync/driver/... components/sync/driver/sync_frontend.h:82: // Called when we receive an updated status counter for a datatype which is I don't think this comment needs to elaborate kinds of datatypes. Simply "Called when we receive an updated status counter for a datatype." should be enough. https://codereview.chromium.org/2374913002/diff/80001/components/sync/engine_... File components/sync/engine_impl/cycle/directory_type_debug_info_emitter.cc (right): https://codereview.chromium.org/2374913002/diff/80001/components/sync/engine_... components/sync/engine_impl/cycle/directory_type_debug_info_emitter.cc:88: OnStatusCountersUpdated(type_, std::move(counters))); Same argument as in ProfileSyncService. Who will get ownership of counters when there is more than one observer?
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:120001) has been deleted
Use unique_ptr instead of const reference, because get StatusCounters need to jump to model thread, and then model thread will pass StatusCounters back to UI thread, so const reference will cause Access Violation. https://codereview.chromium.org/2374913002/diff/80001/components/browser_sync... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2374913002/diff/80001/components/browser_sync... components/browser_sync/profile_sync_service.cc:563: OnStatusCountersUpdated(type, std::move(counters))); On 2016/09/30 00:01:51, pavely wrote: > This macro expands into a loop with function invocation included in it. With > multiple observers I think only one will get ownership of counters. > > Why do you need to transfer ownership? my bad, will update. https://codereview.chromium.org/2374913002/diff/80001/components/sync/driver/... File components/sync/driver/sync_frontend.h (right): https://codereview.chromium.org/2374913002/diff/80001/components/sync/driver/... components/sync/driver/sync_frontend.h:82: // Called when we receive an updated status counter for a datatype which is On 2016/09/30 00:01:51, pavely wrote: > I don't think this comment needs to elaborate kinds of datatypes. > Simply "Called when we receive an updated status counter for a datatype." should > be enough. Done. https://codereview.chromium.org/2374913002/diff/80001/components/sync/engine_... File components/sync/engine_impl/cycle/directory_type_debug_info_emitter.cc (right): https://codereview.chromium.org/2374913002/diff/80001/components/sync/engine_... components/sync/engine_impl/cycle/directory_type_debug_info_emitter.cc:88: OnStatusCountersUpdated(type_, std::move(counters))); On 2016/09/30 00:01:51, pavely wrote: > Same argument as in ProfileSyncService. Who will get ownership of counters when > there is more than one observer? Done. https://codereview.chromium.org/2374913002/diff/140001/components/sync/driver... File components/sync/driver/directory_data_type_controller.cc (right): https://codereview.chromium.org/2374913002/diff/140001/components/sync/driver... components/sync/driver/directory_data_type_controller.cc:50: base::ThreadTaskRunnerHandle::Get()->PostTask( do post here even already on UI thread, because need to send real counters for each data type after GetTypeStatusMap pass empty dictionary to UI. Otherwise UI will receive real counters first, then empty data.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/10/01 07:14:09, Gang Wu wrote: > Use unique_ptr instead of const reference, because get StatusCounters need to > jump to model thread, and then model thread will pass StatusCounters back to UI > thread, so const reference will cause Access Violation. Got it. Actually status counters are already posted as const ref across threads: https://cs.chromium.org/chromium/src/components/sync/driver/glue/sync_backend... This is safe because Bind removes reference and const, see usages of std::decay in bind_internal.h. Also check documentation at https://cs.chromium.org/chromium/src/docs/callback.md?q=callback.md&sq=packag...
https://codereview.chromium.org/2374913002/diff/140001/components/sync/driver... File components/sync/driver/directory_data_type_controller.cc (right): https://codereview.chromium.org/2374913002/diff/140001/components/sync/driver... components/sync/driver/directory_data_type_controller.cc:50: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/10/01 07:14:09, Gang Wu wrote: > do post here even already on UI thread, because need to send real counters for > each data type after GetTypeStatusMap pass empty dictionary to UI. Otherwise UI > will receive real counters first, then empty data. Could you put this comment in the code. It would be useful for future readers to understand the reason.
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #5 (id:180001) has been deleted
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Patchset #4 (id:160001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Patchset #4 (id:200001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
unique_ptr removed https://codereview.chromium.org/2374913002/diff/140001/components/sync/driver... File components/sync/driver/directory_data_type_controller.cc (right): https://codereview.chromium.org/2374913002/diff/140001/components/sync/driver... components/sync/driver/directory_data_type_controller.cc:50: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/10/03 03:10:55, pavely wrote: > On 2016/10/01 07:14:09, Gang Wu wrote: > > do post here even already on UI thread, because need to send real counters for > > each data type after GetTypeStatusMap pass empty dictionary to UI. Otherwise > UI > > will receive real counters first, then empty data. > > Could you put this comment in the code. It would be useful for future readers to > understand the reason. Done.
https://codereview.chromium.org/2374913002/diff/220001/components/sync/core/s... File components/sync/core/shared_model_type_processor.cc (right): https://codereview.chromium.org/2374913002/diff/220001/components/sync/core/s... components/sync/core/shared_model_type_processor.cc:212: ++counters.num_entries_and_tombstones; Please move num_entries_and_tombstones calculation outside the loop and change it to "counters.num_entries_and_tombstones = entities_.size();" https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... File components/sync/driver/data_type_controller.h (right): https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... components/sync/driver/data_type_controller.h:158: // Returns StatusCounters representing the counters for this data type to Alternative for first sentence: Collects StatusCounters for this datatype and passes them to |callback|. https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... File components/sync/driver/directory_data_type_controller.cc (right): https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... components/sync/driver/directory_data_type_controller.cc:51: // do post here even already on UI thread, because need to send real counters I attempted to rewrite this comment: We post callback to current thread instead of calling it directly to ensure that results of ProfileSyncService::GetTypeStatusMap are applied to UI before sync counters collected here. Otherwise results of ProfileSyncService::GetTypeStatusMap will overwrite counters with empty data. https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... File components/sync/driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... components/sync/driver/non_blocking_data_type_controller.cc:106: base::Unretained(processor), Why is it safe to pass processor as Unretained pointer? What if meanwhile model type calls clear_change_processor() before this callback gets a chance to run? Let's discuss it on tomorrow's standup meeting. https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... File components/sync/driver/resources/about.js (right): https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... components/sync/driver/resources/about.js:31: function onAboutInfoCountersUpdated(e) { Could you move this function below onAboutInfoUpdatedEvent. onAboutInfoUpdatedEvent and refreshAboutInfo belong together as former calls latter. https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... components/sync/driver/resources/about.js:37: var type_status_array = chrome.sync.aboutInfo.type_status; onCountersUpdated can be called for different counter types. Do you need to handle it here? Maybe it doesn't matter.
maxbogue@chromium.org changed reviewers: + maxbogue@chromium.org
https://codereview.chromium.org/2374913002/diff/220001/components/browser_syn... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2374913002/diff/220001/components/browser_syn... components/browser_sync/profile_sync_service.cc:1896: base::Bind(&ProfileSyncService::OnDatatypeStatusCounterUpdated, If this Bind call were wrapped with syncer::BindToCurrentThread, then you would be enforcing the asynchronous requirement from within this method, and you would not need to PostTask in DirectoryDataTypeController or pass the task_runner to post with to the SharedModelTypeProcessor. You would definitely want to document this in the comment for DataTypeController::GetStatusCounters though.
Patchset #5 (id:240001) has been deleted
Patchset #5 (id:260001) has been deleted
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
update to get processor on model thread, also use syncer::BindToCurrentThread to avoid pass task runner to processor. https://codereview.chromium.org/2374913002/diff/220001/components/browser_syn... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2374913002/diff/220001/components/browser_syn... components/browser_sync/profile_sync_service.cc:1896: base::Bind(&ProfileSyncService::OnDatatypeStatusCounterUpdated, On 2016/10/04 00:02:00, maxbogue wrote: > If this Bind call were wrapped with syncer::BindToCurrentThread, then you would > be enforcing the asynchronous requirement from within this method, and you would > not need to PostTask in DirectoryDataTypeController or pass the task_runner to > post with to the SharedModelTypeProcessor. > > You would definitely want to document this in the comment for > DataTypeController::GetStatusCounters though. Done. https://codereview.chromium.org/2374913002/diff/220001/components/sync/core/s... File components/sync/core/shared_model_type_processor.cc (right): https://codereview.chromium.org/2374913002/diff/220001/components/sync/core/s... components/sync/core/shared_model_type_processor.cc:212: ++counters.num_entries_and_tombstones; On 2016/10/03 21:40:14, pavely wrote: > Please move num_entries_and_tombstones calculation outside the loop and change > it to "counters.num_entries_and_tombstones = entities_.size();" Done. https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... File components/sync/driver/data_type_controller.h (right): https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... components/sync/driver/data_type_controller.h:158: // Returns StatusCounters representing the counters for this data type to On 2016/10/03 21:40:14, pavely wrote: > Alternative for first sentence: > Collects StatusCounters for this datatype and passes them to |callback|. Done. https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... File components/sync/driver/directory_data_type_controller.cc (right): https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... components/sync/driver/directory_data_type_controller.cc:51: // do post here even already on UI thread, because need to send real counters On 2016/10/03 21:40:14, pavely wrote: > I attempted to rewrite this comment: > We post callback to current thread instead of calling it directly to ensure that > results of ProfileSyncService::GetTypeStatusMap are applied to UI before sync > counters collected here. Otherwise results of > ProfileSyncService::GetTypeStatusMap will overwrite counters with empty data. Done. https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... File components/sync/driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... components/sync/driver/non_blocking_data_type_controller.cc:106: base::Unretained(processor), On 2016/10/03 21:40:14, pavely wrote: > Why is it safe to pass processor as Unretained pointer? What if meanwhile model > type calls clear_change_processor() before this callback gets a chance to run? > Let's discuss it on tomorrow's standup meeting. Done. https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... File components/sync/driver/resources/about.js (right): https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... components/sync/driver/resources/about.js:31: function onAboutInfoCountersUpdated(e) { On 2016/10/03 21:40:14, pavely wrote: > Could you move this function below onAboutInfoUpdatedEvent. > onAboutInfoUpdatedEvent and refreshAboutInfo belong together as former calls > latter. Done. https://codereview.chromium.org/2374913002/diff/220001/components/sync/driver... components/sync/driver/resources/about.js:37: var type_status_array = chrome.sync.aboutInfo.type_status; On 2016/10/03 21:40:14, pavely wrote: > onCountersUpdated can be called for different counter types. Do you need to > handle it here? Maybe it doesn't matter. I think it doesn't matter, just means update more.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
non-JS lgtm (didn't look at the JS) w/ a nit and some comment clarification suggestions. https://codereview.chromium.org/2374913002/diff/280001/components/browser_syn... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2374913002/diff/280001/components/browser_syn... components/browser_sync/profile_sync_service.cc:1898: // OnDatatypeStatusCounterUpdated that posts back to the UI thread. "OnDatatypeStatusCounterUpdated that posts back to the UI thread so that real results can't get overwritten by the empty counters set at the end of this method." https://codereview.chromium.org/2374913002/diff/280001/components/sync/driver... File components/sync/driver/data_type_controller.h (right): https://codereview.chromium.org/2374913002/diff/280001/components/sync/driver... components/sync/driver/data_type_controller.h:153: // Collects StatusCounters for this datatype and passes them to |callback|. "Collects StatusCounters for this datatype and passes them to |callback|, which should be wrapped with syncer::BindToCurrentThread already. Used to display entity counts in chrome://sync-internals." https://codereview.chromium.org/2374913002/diff/280001/components/sync/driver... File components/sync/driver/model_type_controller.cc (right): https://codereview.chromium.org/2374913002/diff/280001/components/sync/driver... components/sync/driver/model_type_controller.cc:29: You could probably add a GetProcessorFromService() function up here that does lines 34-46. Then in e.g. CallProcessorGetAllNodes you'd just have: SharedModelTypeProcessor* processor = GetProcessorFromService(service); if (processor) { processor->GetAllNodes(callback); }
maxbogue@chromium.org changed required reviewers: + pavely@chromium.org
lgtm % Max's comments.
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
https://codereview.chromium.org/2374913002/diff/280001/components/browser_syn... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2374913002/diff/280001/components/browser_syn... components/browser_sync/profile_sync_service.cc:1898: // OnDatatypeStatusCounterUpdated that posts back to the UI thread. On 2016/10/06 17:16:32, maxbogue wrote: > "OnDatatypeStatusCounterUpdated that posts back to the UI thread so that real > results can't get overwritten by the empty counters set at the end of this > method." Done. https://codereview.chromium.org/2374913002/diff/280001/components/sync/driver... File components/sync/driver/data_type_controller.h (right): https://codereview.chromium.org/2374913002/diff/280001/components/sync/driver... components/sync/driver/data_type_controller.h:153: // Collects StatusCounters for this datatype and passes them to |callback|. On 2016/10/06 17:16:32, maxbogue wrote: > "Collects StatusCounters for this datatype and passes them to |callback|, which > should be wrapped with syncer::BindToCurrentThread already. Used to display > entity counts in chrome://sync-internals." Done. https://codereview.chromium.org/2374913002/diff/280001/components/sync/driver... File components/sync/driver/model_type_controller.cc (right): https://codereview.chromium.org/2374913002/diff/280001/components/sync/driver... components/sync/driver/model_type_controller.cc:29: On 2016/10/06 17:16:32, maxbogue wrote: > You could probably add a GetProcessorFromService() function up here that does > lines 34-46. Then in e.g. CallProcessorGetAllNodes you'd just have: > > SharedModelTypeProcessor* processor = GetProcessorFromService(service); > if (processor) { > processor->GetAllNodes(callback); > } Done.
The CQ bit was checked by gangwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maxbogue@chromium.org, pavely@chromium.org Link to the patchset: https://codereview.chromium.org/2374913002/#ps300001 (title: "re-comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [USS] Show USS counters in about:sync page BUG=328606 ========== to ========== [USS] Show USS counters in about:sync page BUG=328606 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== [USS] Show USS counters in about:sync page BUG=328606 ========== to ========== [USS] Show USS counters in about:sync page BUG=328606 Committed: https://crrev.com/d26ef311a38ab8e88b4fa82a6a6753786a873e36 Cr-Commit-Position: refs/heads/master@{#423808} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d26ef311a38ab8e88b4fa82a6a6753786a873e36 Cr-Commit-Position: refs/heads/master@{#423808} |
