Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2319973003:
[USS] Implement SharedModelTypeProcessor::GetAllNodes for USS (Closed)
Description[USS] Implement SharedModelTypeProcessor::GetAllNodes for USS
mplement SharedModelTypeProcessor::GetAllNodes for USS, so
"Sync Node Browser" tab in chrome://sync-internals/ will show
USS data.
BTW, modelTypeWorker already ignore the root folder from
server.
BUG=645303
Committed: https://crrev.com/85949830038a3b8cb2f34d6cd92801c5c4b921f8
Cr-Commit-Position: refs/heads/master@{#418735}
Patch Set 1 : add comments #
Total comments: 46
Patch Set 2 : update base on comments #
Total comments: 2
Patch Set 3 : update to MakeUnique<> #Patch Set 4 : change structure of the info a little bit #
Total comments: 6
Patch Set 5 : add blank #
Messages
Total messages: 53 (40 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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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.
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: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
Description was changed from ========== callback deps include ordering first checkin check in for work on directory side first try BUG= ========== to ========== [USS] Implement SharedModelTypeProcessor::GetAllNodes for USS mplement SharedModelTypeProcessor::GetAllNodes for USS, so "Sync Node Browser" tab in chrome://sync-internals/ will show USS data. BUG=645303 ==========
gangwu@chromium.org changed reviewers: + maxbogue@chromium.org, pavely@chromium.org, skym@chromium.org
Description was changed from ========== [USS] Implement SharedModelTypeProcessor::GetAllNodes for USS mplement SharedModelTypeProcessor::GetAllNodes for USS, so "Sync Node Browser" tab in chrome://sync-internals/ will show USS data. BUG=645303 ========== to ========== [USS] Implement SharedModelTypeProcessor::GetAllNodes for USS mplement SharedModelTypeProcessor::GetAllNodes for USS, so "Sync Node Browser" tab in chrome://sync-internals/ will show USS data. BTW, modelTypeWorker already ignore the root folder from server. BUG=645303 ==========
PTAL BTW:The trybot falure is trybot issue.
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.
https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... File components/sync/api/entity_data.cc (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:17: namespace { I don't think namespaces affect macros https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:18: #define ADD_TO_DICT(dict, name, value) \ Why are you using a macro and not a function here? https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:19: dict->SetString(base::ToUpperASCII(#name), value); What does the # in "#name" do? https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:20: } // namespace https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... File components/sync/api/entity_data.h (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.h:79: static std::unique_ptr<base::DictionaryValue> ToValue( Kinda odd that ToValue returns a DictionaryValue, right? https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... File components/sync/core/shared_model_type_processor.cc (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:731: // entity could be null if there are some unapplied changes. Capitalize first letter https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:735: node->MergeDictionary(metadata.get()); 1) Why do we merge these two? Why not use a single copy and ignore the other one? 2) Who wins conflicts? https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:737: node->SetString("modelType", ModelTypeToString(type_)); I know this wasn't you, but its really odd how some of these keys are camelCase and some are SHOUTY_CASE. https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:741: // Create a permanent folder for this data type Comment sentence should have a period at the end. Also, can you explain why we do this? From the looks of this, it looks like you you're assuming we're never going to migrate parent/root folders and that they shouldn't be in our model representation any longer. Is that correct? https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:742: std::unique_ptr<base::DictionaryValue> rootnode(new base::DictionaryValue()); MakeUnique? https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:743: rootnode->SetString("PARENT_ID", "r"); r? https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:746: rootnode->SetString("NON_UNIQUE_NAME", ModelTypeToString(type_)); You call ModelTypeToString a lot in this function... Just saying. https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... File components/sync/core/shared_model_type_processor.h (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.h:54: void GetAllNodes( Comments https://codereview.chromium.org/2319973003/diff/60001/components/sync/driver/... File components/sync/driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/driver/... components/sync/driver/non_blocking_data_type_controller.cc:85: (syncer_v2::SharedModelTypeProcessor*)service->change_processor(); This cast should have a TODO https://codereview.chromium.org/2319973003/diff/60001/components/sync/protoco... File components/sync/protocol/proto_value_conversions.cc (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/protoco... components/sync/protocol/proto_value_conversions.cc:32: #include "components/sync/protocol/entity_metadata.pb.h" This is dangerous. I'm not sure if this file syncs/needs to be synced with the server copy of these proto files, but EntityMetadata is currently (but should not be) synced with the source files the server uses. Adding this dependency is a potential hazard if one day we clean things up and stop sharing EntityMetadata. Can you figure out if proto_value_conversions.cc should be shared and if so, leave comments making this hazard clear?
https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... File components/sync/api/entity_data.cc (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:19: dict->SetString(base::ToUpperASCII(#name), value); On 2016/09/12 16:40:38, skym wrote: > What does the # in "#name" do? I think it extracts the text of the variable name...? https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:50: ADD_TO_DICT(dict, id, entity_data.id); Can you not just do dict->SetString("ID", entity_data.id)? That would be lot clearer than this macro approach.
https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... File components/sync/api/entity_data.cc (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:18: #define ADD_TO_DICT(dict, name, value) \ When you use one-off macros like this it is better to define it right before the function it is used in and then #undef it right after. https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:50: ADD_TO_DICT(dict, id, entity_data.id); You have two patterns here: simple string fields and fields that require nontrivial conversion. For string fields you can create macro SET_STRING_FIELD(dict, field). It would look something like this: #define SET_STRING_FIELD(dict, field) \ dict->SetString(#field, field); For other fields you can either call dict->SetString(...) directory or create macro SET_FIELD_WITH_CONVERSION(dict, field, conv_fn) and call conversion function from within. There is a conversion function from base::Time to string: https://cs.chromium.org/chromium/src/components/sync/base/time.h?q=syncer::Ge... https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... File components/sync/api/entity_data.h (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.h:79: static std::unique_ptr<base::DictionaryValue> ToValue( ToValue shouldn't be static. It is always called with valid reference to EntityData and has the same access to fields as any member function. Could you change it to non-static function? https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... File components/sync/core/shared_model_type_processor.h (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.h:57: std::unique_ptr<base::ListValue>)>& callback); Could you use DataTypeController::AllNodesCallback instead. And fix other places where callback signature is expanded.
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...
updated, PTAL https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... File components/sync/api/entity_data.cc (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:17: namespace { On 2016/09/12 16:40:38, skym wrote: > I don't think namespaces affect macros Done. https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:18: #define ADD_TO_DICT(dict, name, value) \ On 2016/09/12 16:40:38, skym wrote: > Why are you using a macro and not a function here? # can translate parameter to string, more general than write a function to translate parameter to string. https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:18: #define ADD_TO_DICT(dict, name, value) \ On 2016/09/12 20:43:04, pavely wrote: > When you use one-off macros like this it is better to define it right before the > function it is used in and then #undef it right after. Done. https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:19: dict->SetString(base::ToUpperASCII(#name), value); On 2016/09/12 16:40:38, skym wrote: > What does the # in "#name" do? #name can translate parameter name to string "name" https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:19: dict->SetString(base::ToUpperASCII(#name), value); On 2016/09/12 17:44:26, maxbogue wrote: > On 2016/09/12 16:40:38, skym wrote: > > What does the # in "#name" do? > > I think it extracts the text of the variable name...? yes. https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:20: } On 2016/09/12 16:40:38, skym wrote: > // namespace Done. https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:50: ADD_TO_DICT(dict, id, entity_data.id); On 2016/09/12 17:44:26, maxbogue wrote: > Can you not just do dict->SetString("ID", entity_data.id)? That would be lot > clearer than this macro approach. I copy this approach from proto_value_conversions.cc. I do agree this macro approach is not clear, but for consistence, I used proto_value_conversions approach, and even the naming standard(TOValue not ToDictionaryValue). https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.cc:50: ADD_TO_DICT(dict, id, entity_data.id); On 2016/09/12 20:43:04, pavely wrote: > You have two patterns here: simple string fields and fields that require > nontrivial conversion. > For string fields you can create macro SET_STRING_FIELD(dict, field). It would > look something like this: > #define SET_STRING_FIELD(dict, field) \ > dict->SetString(#field, field); > > For other fields you can either call dict->SetString(...) directory or create > macro SET_FIELD_WITH_CONVERSION(dict, field, conv_fn) and call conversion > function from within. > > There is a conversion function from base::Time to string: > https://cs.chromium.org/chromium/src/components/sync/base/time.h?q=syncer::Ge... Done. https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... File components/sync/api/entity_data.h (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.h:79: static std::unique_ptr<base::DictionaryValue> ToValue( On 2016/09/12 16:40:38, skym wrote: > Kinda odd that ToValue returns a DictionaryValue, right? got this name from proto_value_conversions.cc, most of function named ...ToValue and returns a DictionaryValue. how about change to ToDictionaryValue() ? https://codereview.chromium.org/2319973003/diff/60001/components/sync/api/ent... components/sync/api/entity_data.h:79: static std::unique_ptr<base::DictionaryValue> ToValue( On 2016/09/12 20:43:04, pavely wrote: > ToValue shouldn't be static. It is always called with valid reference to > EntityData and has the same access to fields as any member function. Could you > change it to non-static function? Done. https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... File components/sync/core/shared_model_type_processor.cc (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:731: // entity could be null if there are some unapplied changes. On 2016/09/12 16:40:39, skym wrote: > Capitalize first letter Done. https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:735: node->MergeDictionary(metadata.get()); On 2016/09/12 16:40:38, skym wrote: > 1) Why do we merge these two? Why not use a single copy and ignore the other > one? > 2) Who wins conflicts? |node| is real data, |metadata| is metadata, so mergeDictionary at here shouldn't have any conflicts, hopefully. https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:737: node->SetString("modelType", ModelTypeToString(type_)); On 2016/09/12 16:40:39, skym wrote: > I know this wasn't you, but its really odd how some of these keys are camelCase > and some are SHOUTY_CASE. We can discuss this in USS meeting, since this will involve a lot of keys. https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:741: // Create a permanent folder for this data type On 2016/09/12 16:40:39, skym wrote: > Comment sentence should have a period at the end. Also, can you explain why we > do this? From the looks of this, it looks like you you're assuming we're never > going to migrate parent/root folders and that they shouldn't be in our model > representation any longer. Is that correct? yes, that is correct. Comments added. https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:742: std::unique_ptr<base::DictionaryValue> rootnode(new base::DictionaryValue()); On 2016/09/12 16:40:39, skym wrote: > MakeUnique? any different? https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:743: rootnode->SetString("PARENT_ID", "r"); On 2016/09/12 16:40:39, skym wrote: > r? I guess "r" means root, isTypeRootNode in sync_node_browser.js use this to determinate which node is root folder. I will add more comments for this section. https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:746: rootnode->SetString("NON_UNIQUE_NAME", ModelTypeToString(type_)); On 2016/09/12 16:40:38, skym wrote: > You call ModelTypeToString a lot in this function... Just saying. Done. https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... File components/sync/core/shared_model_type_processor.h (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.h:54: void GetAllNodes( On 2016/09/12 16:40:39, skym wrote: > Comments Done. https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.h:57: std::unique_ptr<base::ListValue>)>& callback); On 2016/09/12 20:43:04, pavely wrote: > Could you use DataTypeController::AllNodesCallback instead. And fix other places > where callback signature is expanded. I tried, but got circle dependence error during "git cl upload". https://codereview.chromium.org/2319973003/diff/60001/components/sync/driver/... File components/sync/driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/driver/... components/sync/driver/non_blocking_data_type_controller.cc:85: (syncer_v2::SharedModelTypeProcessor*)service->change_processor(); On 2016/09/12 16:40:39, skym wrote: > This cast should have a TODO Done. https://codereview.chromium.org/2319973003/diff/60001/components/sync/protoco... File components/sync/protocol/proto_value_conversions.cc (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/protoco... components/sync/protocol/proto_value_conversions.cc:32: #include "components/sync/protocol/entity_metadata.pb.h" On 2016/09/12 16:40:39, skym wrote: > This is dangerous. I'm not sure if this file syncs/needs to be synced with the > server copy of these proto files, but EntityMetadata is currently (but should > not be) synced with the source files the server uses. Adding this dependency is > a potential hazard if one day we clean things up and stop sharing > EntityMetadata. Can you figure out if proto_value_conversions.cc should be > shared and if so, leave comments making this hazard clear? Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... File components/sync/core/shared_model_type_processor.cc (right): https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:735: node->MergeDictionary(metadata.get()); On 2016/09/12 22:38:03, Gang Wu wrote: > On 2016/09/12 16:40:38, skym wrote: > > 1) Why do we merge these two? Why not use a single copy and ignore the other > > one? > > 2) Who wins conflicts? > > |node| is real data, |metadata| is metadata, so mergeDictionary at here > shouldn't have any conflicts, hopefully. Really? Looking at EntityData::ToDictionaryValue() and EntityMetadataToValue() they both set client_tag_hash, creation_time, modification_time. https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh... components/sync/core/shared_model_type_processor.cc:742: std::unique_ptr<base::DictionaryValue> rootnode(new base::DictionaryValue()); On 2016/09/12 22:38:03, Gang Wu wrote: > On 2016/09/12 16:40:39, skym wrote: > > MakeUnique? > > any different? https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/iQgMedVA8-k https://codereview.chromium.org/2319973003/diff/80001/components/sync/driver/... File components/sync/driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/2319973003/diff/80001/components/sync/driver/... components/sync/driver/non_blocking_data_type_controller.cc:84: // TODO(gangwu): Casting should happen “near” where the processor factory has These don't look like ascii " marks
I changed info from
{
"CLIENT_TAG_HASH": "",
"CREATION_TIME": "Wednesday, December 31, 1969 at 4:00:00 PM",
.
.
.
"client_tag_hash": "sPXiBfl2DkXgToi+9C5foDQWlZ8=",
"creation_time": "1473205031259",
.
.
.
}
to
{
"CLIENT_TAG_HASH": "",
"CREATION_TIME": "Wednesday, December 31, 1969 at 4:00:00 PM",
.
.
.
"metadata": {
"client_tag_hash": "sPXiBfl2DkXgToi+9C5foDQWlZ8=",
"creation_time": "1473205031259",
},
}
do you guys think this is clearer?
https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh...
File components/sync/core/shared_model_type_processor.cc (right):
https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh...
components/sync/core/shared_model_type_processor.cc:735:
node->MergeDictionary(metadata.get());
On 2016/09/13 16:45:17, skym wrote:
> On 2016/09/12 22:38:03, Gang Wu wrote:
> > On 2016/09/12 16:40:38, skym wrote:
> > > 1) Why do we merge these two? Why not use a single copy and ignore the
other
> > > one?
> > > 2) Who wins conflicts?
> >
> > |node| is real data, |metadata| is metadata, so mergeDictionary at here
> > shouldn't have any conflicts, hopefully.
>
> Really? Looking at EntityData::ToDictionaryValue() and EntityMetadataToValue()
> they both set client_tag_hash, creation_time, modification_time.
Thanks for point that out!
But in here, all the keys in node is uppercase, and all the keys in metadata is
lowercase, so still wont have any conflicts.
That also said, there will have two client_tag_hash, creation_time and
modification_time.
But since this is debug info, I think show both of them is better, what do you
think?
https://codereview.chromium.org/2319973003/diff/60001/components/sync/core/sh...
components/sync/core/shared_model_type_processor.cc:742:
std::unique_ptr<base::DictionaryValue> rootnode(new base::DictionaryValue());
On 2016/09/13 16:45:17, skym wrote:
> On 2016/09/12 22:38:03, Gang Wu wrote:
> > On 2016/09/12 16:40:39, skym wrote:
> > > MakeUnique?
> >
> > any different?
>
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/iQgMedVA8-k
Done.
https://codereview.chromium.org/2319973003/diff/80001/components/sync/driver/...
File components/sync/driver/non_blocking_data_type_controller.cc (right):
https://codereview.chromium.org/2319973003/diff/80001/components/sync/driver/...
components/sync/driver/non_blocking_data_type_controller.cc:84: // TODO(gangwu):
Casting should happen “near” where the processor factory has
On 2016/09/13 16:45:17, skym wrote:
> These don't look like ascii " marks
Done.
lgtm https://codereview.chromium.org/2319973003/diff/120001/components/sync/api/en... File components/sync/api/entity_data.cc (right): https://codereview.chromium.org/2319973003/diff/120001/components/sync/api/en... components/sync/api/entity_data.cc:18: namespace { nit: new line after "namespace syncer_v2 {" and before "} // namespace"
lgtm; I left a comment about the macro but at this point it's just my opinion. Since Pavel has lgtm'd, feel free to ignore me. https://codereview.chromium.org/2319973003/diff/120001/components/sync/api/en... File components/sync/api/entity_data.cc (right): https://codereview.chromium.org/2319973003/diff/120001/components/sync/api/en... components/sync/api/entity_data.cc:18: namespace { On 2016/09/14 17:15:30, pavely wrote: > nit: new line after "namespace syncer_v2 {" and before "} // namespace" Since there's already a newline after namespace syncer_v2, I assume pavel means after "namespace {" https://codereview.chromium.org/2319973003/diff/120001/components/sync/api/en... components/sync/api/entity_data.cc:48: #define ADD_TO_DICT(dict, value, transform) \ Despite this being how proto_value_conversions does it, I still think it would be more readable without the macro.
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.
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/2319973003/#ps140001 (title: "add blank")
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] Implement SharedModelTypeProcessor::GetAllNodes for USS mplement SharedModelTypeProcessor::GetAllNodes for USS, so "Sync Node Browser" tab in chrome://sync-internals/ will show USS data. BTW, modelTypeWorker already ignore the root folder from server. BUG=645303 ========== to ========== [USS] Implement SharedModelTypeProcessor::GetAllNodes for USS mplement SharedModelTypeProcessor::GetAllNodes for USS, so "Sync Node Browser" tab in chrome://sync-internals/ will show USS data. BTW, modelTypeWorker already ignore the root folder from server. BUG=645303 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [USS] Implement SharedModelTypeProcessor::GetAllNodes for USS mplement SharedModelTypeProcessor::GetAllNodes for USS, so "Sync Node Browser" tab in chrome://sync-internals/ will show USS data. BTW, modelTypeWorker already ignore the root folder from server. BUG=645303 ========== to ========== [USS] Implement SharedModelTypeProcessor::GetAllNodes for USS mplement SharedModelTypeProcessor::GetAllNodes for USS, so "Sync Node Browser" tab in chrome://sync-internals/ will show USS data. BTW, modelTypeWorker already ignore the root folder from server. BUG=645303 Committed: https://crrev.com/85949830038a3b8cb2f34d6cd92801c5c4b921f8 Cr-Commit-Position: refs/heads/master@{#418735} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/85949830038a3b8cb2f34d6cd92801c5c4b921f8 Cr-Commit-Position: refs/heads/master@{#418735}
Message was sent while issue was closed.
https://codereview.chromium.org/2319973003/diff/120001/components/sync/api/en... File components/sync/api/entity_data.cc (right): https://codereview.chromium.org/2319973003/diff/120001/components/sync/api/en... components/sync/api/entity_data.cc:18: namespace { On 2016/09/14 17:15:30, pavely wrote: > nit: new line after "namespace syncer_v2 {" and before "} // namespace" Done. https://codereview.chromium.org/2319973003/diff/120001/components/sync/api/en... components/sync/api/entity_data.cc:18: namespace { On 2016/09/14 17:58:02, maxbogue wrote: > On 2016/09/14 17:15:30, pavely wrote: > > nit: new line after "namespace syncer_v2 {" and before "} // namespace" > > Since there's already a newline after namespace syncer_v2, I assume pavel means > after "namespace {" Done. https://codereview.chromium.org/2319973003/diff/120001/components/sync/api/en... components/sync/api/entity_data.cc:48: #define ADD_TO_DICT(dict, value, transform) \ On 2016/09/14 17:58:02, maxbogue wrote: > Despite this being how proto_value_conversions does it, I still think it would > be more readable without the macro. Acknowledged. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
