Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1413233003:
Organizing model type, notification type, root tag and model type string in map. (Closed)
|
Created:
5 years, 2 months ago by Deepak Modified:
5 years, 1 month ago CC:
chromium-reviews, tim+watch_chromium.org, pvalenzuela+watch_chromium.org, maxbogue+watch_chromium.org, plaree+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOrganizing model type, notification type, root tag and model type string in map
so that all value at one place to avoid clattering of the code and reducing code size
and easy maintainability.
BUG=545968
Committed: https://crrev.com/fa1f6182af9df39ba618a3214a9f529e23796bb4
Cr-Commit-Position: refs/heads/master@{#357524}
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : Changes as per review comments. #
Total comments: 8
Patch Set 4 : #Patch Set 5 : #
Total comments: 17
Patch Set 6 : Added Test cases. #Patch Set 7 : #
Total comments: 2
Patch Set 8 : Changes as per review comments. #Patch Set 9 : #
Total comments: 10
Patch Set 10 : Changes as per review comments. #
Total comments: 2
Patch Set 11 : Addressing nit. #
Messages
Total messages: 27 (6 generated)
Description was changed from ========== Organizing model type, notification type, root tag and model type string in map. BUG=545968 ========== to ========== Organizing model type, notification type, root tag and model type string in map so that all value at one place to avoid clattering of the code and reducing code size and easy maintainability. BUG=545968 ==========
Description was changed from ========== Organizing model type, notification type, root tag and model type string in map so that all value at one place to avoid clattering of the code and reducing code size and easy maintainability. BUG=545968 ========== to ========== Organizing model type, notification type, root tag and model type string in map so that all value at one place to avoid clattering of the code and reducing code size and easy maintainability. BUG=545968 ==========
Description was changed from ========== Organizing model type, notification type, root tag and model type string in map so that all value at one place to avoid clattering of the code and reducing code size and easy maintainability. BUG=545968 ========== to ========== Organizing model type, notification type, root tag and model type string in map so that all value at one place to avoid clattering of the code and reducing code size and easy maintainability. BUG=545968 ==========
deepak.m1@samsung.com changed reviewers: + ajith.v@chromium.org
PTAL
Please check inline comments. https://codereview.chromium.org/1413233003/diff/20001/sync/syncable/model_typ... File sync/syncable/model_type.cc (right): https://codereview.chromium.org/1413233003/diff/20001/sync/syncable/model_typ... sync/syncable/model_type.cc:37: {BOOKMARKS, "BOOKMARK", "bookmarks", "Bookmarks", 2}, Why 0 and 1 for model_type_histogram_val, UNSPECIFIED and TOP_LEVEL_FOLDER is not included here ? https://codereview.chromium.org/1413233003/diff/20001/sync/syncable/model_typ... sync/syncable/model_type.cc:737: if (model_type != PROXY_TABS) { Why explicitly checking for PROXY_TABS ? https://codereview.chromium.org/1413233003/diff/20001/sync/syncable/model_typ... sync/syncable/model_type.cc:752: if (notification_type != "") { empty() method we can use and do early return by setting unspecified type.
PTAL https://codereview.chromium.org/1413233003/diff/20001/sync/syncable/model_typ... File sync/syncable/model_type.cc (right): https://codereview.chromium.org/1413233003/diff/20001/sync/syncable/model_typ... sync/syncable/model_type.cc:37: {BOOKMARKS, "BOOKMARK", "bookmarks", "Bookmarks", 2}, On 2015/10/26 10:32:00, AKV wrote: > Why 0 and 1 for model_type_histogram_val, UNSPECIFIED and TOP_LEVEL_FOLDER is > not included here ? Done. https://codereview.chromium.org/1413233003/diff/20001/sync/syncable/model_typ... sync/syncable/model_type.cc:737: if (model_type != PROXY_TABS) { On 2015/10/26 10:32:00, AKV wrote: > Why explicitly checking for PROXY_TABS ? Because for PROXY_TABS,UNSPECIFIED and TOP_LEVEL_FOLDER their is no notification type. https://codereview.chromium.org/1413233003/diff/20001/sync/syncable/model_typ... sync/syncable/model_type.cc:752: if (notification_type != "") { On 2015/10/26 10:32:00, AKV wrote: > empty() method we can use and do early return by setting unspecified type. Done.
Please check inline comments https://codereview.chromium.org/1413233003/diff/40001/sync/syncable/model_typ... File sync/syncable/model_type.cc (right): https://codereview.chromium.org/1413233003/diff/40001/sync/syncable/model_typ... sync/syncable/model_type.cc:647: if (model_type_string != "Unspecified" && early return pls https://codereview.chromium.org/1413233003/diff/40001/sync/syncable/model_typ... sync/syncable/model_type.cc:719: if (type != UNSPECIFIED && type != TOP_LEVEL_FOLDER) { early return pls. https://codereview.chromium.org/1413233003/diff/40001/sync/syncable/model_typ... sync/syncable/model_type.cc:732: if (model_type != UNSPECIFIED && model_type != TOP_LEVEL_FOLDER && early return pls https://codereview.chromium.org/1413233003/diff/40001/sync/syncable/model_typ... sync/syncable/model_type.cc:748: if (notification_type != std::string()) { Early return if (notification_type.empty()) { *model_type = UNSPECIFIED; return false; }
Changes done. PTAL https://codereview.chromium.org/1413233003/diff/40001/sync/syncable/model_typ... File sync/syncable/model_type.cc (right): https://codereview.chromium.org/1413233003/diff/40001/sync/syncable/model_typ... sync/syncable/model_type.cc:647: if (model_type_string != "Unspecified" && On 2015/10/27 10:01:33, AKV wrote: > early return pls Done. https://codereview.chromium.org/1413233003/diff/40001/sync/syncable/model_typ... sync/syncable/model_type.cc:719: if (type != UNSPECIFIED && type != TOP_LEVEL_FOLDER) { On 2015/10/27 10:01:32, AKV wrote: > early return pls. Done. https://codereview.chromium.org/1413233003/diff/40001/sync/syncable/model_typ... sync/syncable/model_type.cc:732: if (model_type != UNSPECIFIED && model_type != TOP_LEVEL_FOLDER && On 2015/10/27 10:01:33, AKV wrote: > early return pls Done. https://codereview.chromium.org/1413233003/diff/40001/sync/syncable/model_typ... sync/syncable/model_type.cc:748: if (notification_type != std::string()) { On 2015/10/27 10:01:33, AKV wrote: > Early return > if (notification_type.empty()) { > *model_type = UNSPECIFIED; > return false; > } Done.
peer review looks good to me! Thanks!
deepak.m1@samsung.com changed reviewers: + zea@chromium.org
@zea PTAL
I like the idea of moving this all into one place, and making it data rather than code. But I'd like to be extra careful to make sure that the behavior stays the same. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... File sync/syncable/model_type.cc (right): https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:28: struct ModelTypeToNotificationMap { Given that this holds more than notifications info, I'd prefer calling this something different. How about naming the struct ModelTypeInfo and calling the map kModelTypeInfoMap. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:29: ModelType model_type; make all of these const as well? (and make all of the string const char* const). It theoretically doesn't matter if the array is const, but it's better style. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:36: ModelTypeToNotificationMap kModelTypeToNotificationMap[] = { make array const https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:50: {APPS, "APPs", "apps", "Apps", 12}, "APPs" is not the right notification type, it should be APP. Could you, as part of this change, write unit tests for each category (notification type, root tag, model type string, model type histogram val), and use that to ensure that none of these are changing as a result of your patch? We should also add an assertion about the length of this map to ensure that if someone defines a new model type, they update this too. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:597: for (const ModelTypeToNotificationMap& entry : kModelTypeToNotificationMap) { Why not just index directly into the array, here and everywhere else? e.g. return kModelTypeToNotificationMap[model_type].model_type_string; https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:648: model_type_string == "Top Level Folder") { Make this a NOTREACHED instead of an if statement. Also use IsRealDataType, instead of hardcoding these two datatypes. Same goes for everywhere else we special case these two types, and where we previously would have hit a NOTREACHED. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:717: if (type == PROXY_TABS) Use IsProxyType, here and elsewhere. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:731: if (model_type == UNSPECIFIED || model_type == TOP_LEVEL_FOLDER || You can use ProtocolTypes to refer to those types sent over the wire (and therefore excluding proxy types and such).
@Zea Test case added for notification type,root tag,model type string.Test case for Histogram is already their. PTAL Thanks. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... File sync/syncable/model_type.cc (right): https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:28: struct ModelTypeToNotificationMap { On 2015/10/27 20:37:19, Nicolas Zea wrote: > Given that this holds more than notifications info, I'd prefer calling this > something different. How about naming the struct ModelTypeInfo and calling the > map kModelTypeInfoMap. Done. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:29: ModelType model_type; On 2015/10/27 20:37:19, Nicolas Zea wrote: > make all of these const as well? (and make all of the string const char* const). > It theoretically doesn't matter if the array is const, but it's better style. Done. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:36: ModelTypeToNotificationMap kModelTypeToNotificationMap[] = { On 2015/10/27 20:37:19, Nicolas Zea wrote: > make array const Done. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:50: {APPS, "APPs", "apps", "Apps", 12}, On 2015/10/27 20:37:19, Nicolas Zea wrote: > "APPs" is not the right notification type, it should be APP. > > Could you, as part of this change, write unit tests for each category > (notification type, root tag, model type string, model type histogram val), and > use that to ensure that none of these are changing as a result of your patch? > > We should also add an assertion about the length of this map to ensure that if > someone defines a new model type, they update this too. Done. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:597: for (const ModelTypeToNotificationMap& entry : kModelTypeToNotificationMap) { On 2015/10/27 20:37:19, Nicolas Zea wrote: > Why not just index directly into the array, here and everywhere else? > e.g. return kModelTypeToNotificationMap[model_type].model_type_string; There are 2 reasons for this: 1) enum values are not in the same order as the histogram index value, or the order in the struct, for example : NIGORI have index value and histogram value as 17, but its ModelType enum value is 34. 2) We are making sure that we are handling correct ModelType by checking "if(entry.model_type == model_type)", so we are not based on index and returning proper model_type_string based on model_type. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:648: model_type_string == "Top Level Folder") { On 2015/10/27 20:37:19, Nicolas Zea wrote: > Make this a NOTREACHED instead of an if statement. Also use IsRealDataType, > instead of hardcoding these two datatypes. Same goes for everywhere else we > special case these two types, and where we previously would have hit a > NOTREACHED. As we don't have the ModelType in input so we can not use IsRealDataType(). Changes done so that it will hit the NOTREACHED when we have "Unspecified" or "Top Level Folder" as model_type_string like earlier. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:717: if (type == PROXY_TABS) On 2015/10/27 20:37:19, Nicolas Zea wrote: > Use IsProxyType, here and elsewhere. Done. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:731: if (model_type == UNSPECIFIED || model_type == TOP_LEVEL_FOLDER || On 2015/10/27 20:37:19, Nicolas Zea wrote: > You can use ProtocolTypes to refer to those types sent over the wire (and > therefore excluding proxy types and such). Done.
Thanks for adding the tests! Couple more comments. https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... File sync/syncable/model_type.cc (right): https://codereview.chromium.org/1413233003/diff/80001/sync/syncable/model_typ... sync/syncable/model_type.cc:597: for (const ModelTypeToNotificationMap& entry : kModelTypeToNotificationMap) { On 2015/10/28 07:21:57, Deepak wrote: > On 2015/10/27 20:37:19, Nicolas Zea wrote: > > Why not just index directly into the array, here and everywhere else? > > e.g. return kModelTypeToNotificationMap[model_type].model_type_string; > > There are 2 reasons for this: > 1) enum values are not in the same order as the histogram index value, or the > order in the struct, for example : NIGORI have index value and histogram value > as 17, but its ModelType enum value is 34. > 2) We are making sure that we are handling correct ModelType by checking > "if(entry.model_type == model_type)", so we are not based on index and returning > proper model_type_string based on model_type. In that case, I wonder if it would be better to have the struct order match the enum order, rather than the histogram order. This would also make it easier to find the model types within the struct, as the enum order is what most people look at. In addition, being able to directly look up an entity improves the performance, as these lookups can be performed quite frequently. https://codereview.chromium.org/1413233003/diff/120001/sync/syncable/model_ty... File sync/syncable/model_type.cc (right): https://codereview.chromium.org/1413233003/diff/120001/sync/syncable/model_ty... sync/syncable/model_type.cc:28: struct ModelTypeInfo { It would be good to comment each of these fields. In particular, call out that the model type histogram val needs to be unique for the model type. Also, none of these values should ever be modified, as that would break backwards compatibility.
@Zea Changes done as suggested. PTAL https://codereview.chromium.org/1413233003/diff/120001/sync/syncable/model_ty... File sync/syncable/model_type.cc (right): https://codereview.chromium.org/1413233003/diff/120001/sync/syncable/model_ty... sync/syncable/model_type.cc:28: struct ModelTypeInfo { On 2015/10/28 19:54:02, Nicolas Zea wrote: > It would be good to comment each of these fields. In particular, call out that > the model type histogram val needs to be unique for the model type. Also, none > of these values should ever be modified, as that would break backwards > compatibility. Done.
Couple more comments https://codereview.chromium.org/1413233003/diff/150001/sync/syncable/model_ty... File sync/syncable/model_type.cc (right): https://codereview.chromium.org/1413233003/diff/150001/sync/syncable/model_ty... sync/syncable/model_type.cc:34: // of these values should ever be modified, without updating "SyncModelTypes" To be clear, existing histogram values should never be modified period. You're correct to point out that new ones should be added to the SyncModelTypes histogram enum. Update the comment to clarify? https://codereview.chromium.org/1413233003/diff/150001/sync/syncable/model_ty... sync/syncable/model_type.cc:39: const ModelTypeInfo kModelTypeInfoMap[] = { Add a comment explaining that these must match the ModelType enum order. It would also be good to update model_type.h to call out that any reordering of the enums must also be reflected here. https://codereview.chromium.org/1413233003/diff/150001/sync/syncable/model_ty... sync/syncable/model_type.cc:237: int GetSpecificsFieldNumberFromModelType(ModelType model_type) { Could these field numbers also be part of the struct as well? https://codereview.chromium.org/1413233003/diff/150001/sync/syncable/model_ty... sync/syncable/model_type.cc:728: if (!ProtocolTypes().Has(model_type)) { You can remove this condition right? And make the one below be if (IsProtocolType(...)). https://codereview.chromium.org/1413233003/diff/150001/sync/syncable/model_ty... File sync/syncable/model_type_unittest.cc (right): https://codereview.chromium.org/1413233003/diff/150001/sync/syncable/model_ty... sync/syncable/model_type_unittest.cc:164: TEST_F(ModelTypeTest, ModelTypeStingMapping) { nit: ModelTypeStingMapping -> ModelTypeStringMapping
PTAL https://codereview.chromium.org/1413233003/diff/150001/sync/syncable/model_ty... File sync/syncable/model_type.cc (right): https://codereview.chromium.org/1413233003/diff/150001/sync/syncable/model_ty... sync/syncable/model_type.cc:34: // of these values should ever be modified, without updating "SyncModelTypes" On 2015/10/29 22:50:43, Nicolas Zea wrote: > To be clear, existing histogram values should never be modified period. You're > correct to point out that new ones should be added to the SyncModelTypes > histogram enum. Update the comment to clarify? I have updated this, Is it fine now !. https://codereview.chromium.org/1413233003/diff/150001/sync/syncable/model_ty... sync/syncable/model_type.cc:39: const ModelTypeInfo kModelTypeInfoMap[] = { On 2015/10/29 22:50:43, Nicolas Zea wrote: > Add a comment explaining that these must match the ModelType enum order. > > It would also be good to update model_type.h to call out that any reordering of > the enums must also be reflected here. Done. https://codereview.chromium.org/1413233003/diff/150001/sync/syncable/model_ty... sync/syncable/model_type.cc:237: int GetSpecificsFieldNumberFromModelType(ModelType model_type) { On 2015/10/29 22:50:43, Nicolas Zea wrote: > Could these field numbers also be part of the struct as well? Changes done, Have to check "ProtocolTypes().Has(model_type)" in DCHECK as well as in if condition as DCHECK does not work in release build. https://codereview.chromium.org/1413233003/diff/150001/sync/syncable/model_ty... sync/syncable/model_type.cc:728: if (!ProtocolTypes().Has(model_type)) { On 2015/10/29 22:50:43, Nicolas Zea wrote: > You can remove this condition right? And make the one below be if > (IsProtocolType(...)). Done. https://codereview.chromium.org/1413233003/diff/150001/sync/syncable/model_ty... File sync/syncable/model_type_unittest.cc (right): https://codereview.chromium.org/1413233003/diff/150001/sync/syncable/model_ty... sync/syncable/model_type_unittest.cc:164: TEST_F(ModelTypeTest, ModelTypeStingMapping) { On 2015/10/29 22:50:43, Nicolas Zea wrote: > nit: ModelTypeStingMapping -> ModelTypeStringMapping Done.
LG, only thing missing I think is that, given the specifics field number are part of the map now, could you also add a test for that as well? https://codereview.chromium.org/1413233003/diff/170001/sync/internal_api/publ... File sync/internal_api/public/base/model_type.h (right): https://codereview.chromium.org/1413233003/diff/170001/sync/internal_api/publ... sync/internal_api/public/base/model_type.h:40: // |kModelTypeInfoMap| struct entries are in the same order as their defination nit: "defination" -> "definition".
@Zea Thanks for review. Test case for "specifics field number" are already present and working well. PTAL. https://codereview.chromium.org/1413233003/diff/170001/sync/internal_api/publ... File sync/internal_api/public/base/model_type.h (right): https://codereview.chromium.org/1413233003/diff/170001/sync/internal_api/publ... sync/internal_api/public/base/model_type.h:40: // |kModelTypeInfoMap| struct entries are in the same order as their defination On 2015/10/30 22:06:48, Nicolas Zea wrote: > nit: "defination" -> "definition". Done.
@Zea Thanks for review. Test cases for "specifics field number" TEST_F(ModelTypeTest, ModelTypeToFromSpecificsFieldNumber) TEST_F(ModelTypeTest, ModelTypeOfInvalidSpecificsFieldNumber) is already present and working well. PTAL.
Ah, you're right. LGTM then, and thanks!
On 2015/11/02 17:52:40, Nicolas Zea wrote: > Ah, you're right. LGTM then, and thanks! Thank You Zea.
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413233003/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413233003/190001
Message was sent while issue was closed.
Committed patchset #11 (id:190001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/fa1f6182af9df39ba618a3214a9f529e23796bb4 Cr-Commit-Position: refs/heads/master@{#357524} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
