Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1247853007:
[Sync] Add auto-generated ModelType in Java. (Closed)
|
Created:
5 years, 5 months ago by maxbogue Modified:
5 years, 4 months ago CC:
chromium-reviews, tim+watch_chromium.org, zea+watch_chromium.org, maxbogue+watch_chromium.org, pvalenzuela+watch_chromium.org, plaree+watch_chromium.org, maniscalco+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] Add auto-generated ModelType in Java.
The new ModelType is just represented by integer constants.
ModelTypeHelper uses JNI to convert the constants to strings
for the invalidation code and will replace the logic in the old ModelType.
BUG=509788
Committed: https://crrev.com/a909f56705b64f3cd067fdba76225b330f8bca48
Cr-Commit-Position: refs/heads/master@{#342260}
Patch Set 1 #Patch Set 2 : Fix more tests and other issues. #Patch Set 3 : Fix more tests. #Patch Set 4 : Fix InvalidationController test and rebase. #Patch Set 5 : Fix GN build and a test. #Patch Set 6 : Attempt to fix GN build. #Patch Set 7 : Add SYNC_EXPORT and rebase. #
Total comments: 12
Patch Set 8 : Trim down to only the additions. #
Total comments: 23
Patch Set 9 : Address comments, rebase, fix bots. #
Total comments: 10
Patch Set 10 : Address comments. #Patch Set 11 : toString -> toNotificationType #
Total comments: 6
Patch Set 12 : More consistent use of notification type. #Patch Set 13 : Remove experiment code. #Patch Set 14 : Don't use synchronized methods. #
Total comments: 2
Patch Set 15 : Fix indentation. #
Total comments: 6
Patch Set 16 : Update comment. #
Messages
Total messages: 36 (6 generated)
maxbogue@chromium.org changed reviewers: + nyquist@chromium.org, zea@chromium.org
maxbogue@chromium.org changed required reviewers: + nyquist@chromium.org, zea@chromium.org
Hey Tommy and Nicolas, I think I finally got this thing all working. PTAL!
pvalenzuela@chromium.org changed reviewers: + pvalenzuela@chromium.org
I know I'm not on the R= line here, but is there any way to break this change into smaller pieces? This is a beast of a CL. That said, this CL will have a positive impact on Sync+Android code quality (both production code and tests it looks like)! Thanks for taking this on.
On 2015/07/29 18:54:00, pvalenzuela wrote: > I know I'm not on the R= line here, but is there any way to break this change > into smaller pieces? This is a beast of a CL. > > That said, this CL will have a positive impact on Sync+Android code quality > (both production code and tests it looks like)! Thanks for taking this on. Honestly, if you look at it without the test and build files, it's not a very big change. I could break it up into adding the new ModelType/JNI and then removing the old/converting everything, but that would might just make it less clear how things are being transitioned (e.g., that ModelTypeHelper is replacing the logic in the old ModelType). Tommy or Nicolas, if either of you would like me to do that I will.
https://codereview.chromium.org/1247853007/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java (right): https://codereview.chromium.org/1247853007/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:73: private static final int[] ALL_SELECTABLE_TYPES = new int[] { Where was this previously? https://codereview.chromium.org/1247853007/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:387: int[] modelTypeArray = new int[modelTypeSet.size()]; Is the order of the elements int he array defined? It doesn't necessarily seem that the order matter...could that lead to problems (I wonder about two different components creating effectively equivalent arrays, but with different order, which then don't compare as equivalent)? https://codereview.chromium.org/1247853007/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java (right): https://codereview.chromium.org/1247853007/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java:74: LibraryLoader.get(LibraryProcessType.PROCESS_BROWSER).ensureInitialized(mContext); What is this for? https://codereview.chromium.org/1247853007/diff/120001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/1247853007/diff/120001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:165: "//sync", Looks like sync is already a dep here? Do we need to add it below? https://codereview.chromium.org/1247853007/diff/120001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1247853007/diff/120001/chrome/chrome_browser.... chrome/chrome_browser.gypi:3302: '../sync/sync.gyp:sync', this is already included above? https://codereview.chromium.org/1247853007/diff/120001/components/invalidatio... File components/invalidation.gypi (right): https://codereview.chromium.org/1247853007/diff/120001/components/invalidatio... components/invalidation.gypi:227: '../content/content_shell_and_tests.gyp:content_java_test_support', I don't think you can depend on content/ from components/
Also I do tend to agree that this could be split up more. Splitting off one change to introduce the new logic/tests, and another to do the actual conversion, would help.
https://codereview.chromium.org/1247853007/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java (right): https://codereview.chromium.org/1247853007/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:73: private static final int[] ALL_SELECTABLE_TYPES = new int[] { On 2015/07/30 01:02:04, Nicolas Zea wrote: > Where was this previously? It was implicit in the implementation of setPreferredDataTypes(). https://codereview.chromium.org/1247853007/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:387: int[] modelTypeArray = new int[modelTypeSet.size()]; On 2015/07/30 01:02:04, Nicolas Zea wrote: > Is the order of the elements int he array defined? It doesn't necessarily seem > that the order matter...could that lead to problems (I wonder about two > different components creating effectively equivalent arrays, but with different > order, which then don't compare as equivalent)? The array only exists for the JNI conversion and isn't used for any equivalence checks. It's used as a set in both java and native. https://codereview.chromium.org/1247853007/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java (right): https://codereview.chromium.org/1247853007/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java:74: LibraryLoader.get(LibraryProcessType.PROCESS_BROWSER).ensureInitialized(mContext); On 2015/07/30 01:02:04, Nicolas Zea wrote: > What is this for? It loads native, which is now necessary for sync invalidation code so it can do the ModelType -> string conversion. https://codereview.chromium.org/1247853007/diff/120001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/1247853007/diff/120001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:165: "//sync", On 2015/07/30 01:02:04, Nicolas Zea wrote: > Looks like sync is already a dep here? Do we need to add it below? You're probably right. I'll remove it. https://codereview.chromium.org/1247853007/diff/120001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1247853007/diff/120001/chrome/chrome_browser.... chrome/chrome_browser.gypi:3302: '../sync/sync.gyp:sync', On 2015/07/30 01:02:04, Nicolas Zea wrote: > this is already included above? So it is. https://codereview.chromium.org/1247853007/diff/120001/components/invalidatio... File components/invalidation.gypi (right): https://codereview.chromium.org/1247853007/diff/120001/components/invalidatio... components/invalidation.gypi:227: '../content/content_shell_and_tests.gyp:content_java_test_support', On 2015/07/30 01:02:04, Nicolas Zea wrote: > I don't think you can depend on content/ from components/ I think it's fine for tests: https://code.google.com/p/chromium/codesearch#chromium/src/components/test/DEPS
Thanks for splitting up the patch! That makes it much easier to review. https://codereview.chromium.org/1247853007/diff/140001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1247853007/diff/140001/chrome/chrome_browser.... chrome/chrome_browser.gypi:3302: '../sync/sync.gyp:sync', Did you mean to remove this (see line 3216 above)? https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:30: private static Set<String> sNonInvalidationTypes = null; Would be good to comment what these fields are for (e.g. why is PROXY_TABS treated differently). https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:31: private static Set<String> sTypesDisabledForExperiment = null; nit: they're not being disabled per se, just not being registered for invalidations. Perhaps name this sNonInvalidationTypesExperiment? https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:33: private static void initNonInvalidationTypes() { In general I prefer being generous with commenting methods for anything that's not ultra trivial. As such, I think it would be good too comment initNonInvalidationTypes and isInvalidationType. https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:49: return !FieldTrialList.findFullName("AndroidSessionNotifications").equals("Disabled"); Seems like this is a bit of a layering invalidation. Either the notion of Sessions/Favicons being part of an experiment should be tightly coupled to the field trial here, or the field trial should be coupled to those types within sTypesDisabledForExperiment. I wonder if we should just get rid of sTypesDisabledForExperiment altogether. https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:70: public static String toString(Integer modelType) { Given the other comment about ModelTypeToString being confusing, I think this should probably be renamed as well, possibly toNotificationtype? https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:75: return "PROXY_TABS"; Didn't PROXY_TABS just previously have the string "NULL"? https://codereview.chromium.org/1247853007/diff/140001/sync/android/model_typ... File sync/android/model_type_helper.cc (right): https://codereview.chromium.org/1247853007/diff/140001/sync/android/model_typ... sync/android/model_type_helper.cc:14: static jstring ModelTypeToString(JNIEnv* env, ModelTypeToString already exists in model_type.h, and does something different. I'd rather call this something else. Perhaps JavaModelTypeToNotificationType?
Despite having internet, ssh does not seem to work with JetBlue's wifi, so I'll make the actual changes tomorrow morning. https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:49: return !FieldTrialList.findFullName("AndroidSessionNotifications").equals("Disabled"); On 2015/07/30 22:20:13, Nicolas Zea wrote: > Seems like this is a bit of a layering invalidation. Either the notion of > Sessions/Favicons being part of an experiment should be tightly coupled to the > field trial here, or the field trial should be coupled to those types within > sTypesDisabledForExperiment. I wonder if we should just get rid of > sTypesDisabledForExperiment altogether. I wanted to avoid calling toString on the relevant types and constructing a set for them every time isInvalidationType was called. If experiment values can't change except on a Chrome restart, I could do the experiment check in init. Is that the case? https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:70: public static String toString(Integer modelType) { On 2015/07/30 22:20:13, Nicolas Zea wrote: > Given the other comment about ModelTypeToString being confusing, I think this > should probably be renamed as well, possibly toNotificationtype? NotificationType is a more confusing name to me since it's actually a string key, but I can change it. https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:75: return "PROXY_TABS"; On 2015/07/30 22:20:13, Nicolas Zea wrote: > Didn't PROXY_TABS just previously have the string "NULL"? That's what I thought at first, but no... Previously all the ModelTypes had two string representations. Their .name(), which is just the text of the actual enum text, and the string that was passed into their constructor (stored as mModelType). The name seems to be what was stored in invalidation preferences, and the mModelType value was only used when they were converted to ObjectIds. PROXY_TABS should be filtered out before being converted to an ObjectId, and my understanding is that the pref value has to match. I could add a check in toObjectId that returns "NULL" if !isInvalidationType just in case someone doesn't use modelTypeStringsToObjectIds properly; would that be better?
https://codereview.chromium.org/1247853007/diff/140001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1247853007/diff/140001/chrome/chrome_browser.... chrome/chrome_browser.gypi:3302: '../sync/sync.gyp:sync', On 2015/07/30 22:20:13, Nicolas Zea wrote: > Did you mean to remove this (see line 3216 above)? Yes, I just missed it in my first pass splitting up the CL and didn't want a patch with just that change :P Done now. https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:30: private static Set<String> sNonInvalidationTypes = null; On 2015/07/30 22:20:13, Nicolas Zea wrote: > Would be good to comment what these fields are for (e.g. why is PROXY_TABS > treated differently). Done. https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:31: private static Set<String> sTypesDisabledForExperiment = null; On 2015/07/30 22:20:13, Nicolas Zea wrote: > nit: they're not being disabled per se, just not being registered for > invalidations. Perhaps name this sNonInvalidationTypesExperiment? Done. https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:33: private static void initNonInvalidationTypes() { On 2015/07/30 22:20:13, Nicolas Zea wrote: > In general I prefer being generous with commenting methods for anything that's > not ultra trivial. As such, I think it would be good too comment > initNonInvalidationTypes and isInvalidationType. Done. https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:70: public static String toString(Integer modelType) { On 2015/07/31 00:49:40, maxbogue wrote: > On 2015/07/30 22:20:13, Nicolas Zea wrote: > > Given the other comment about ModelTypeToString being confusing, I think this > > should probably be renamed as well, possibly toNotificationtype? > > NotificationType is a more confusing name to me since it's actually a string > key, but I can change it. I would actually prefer to not have the concept of notification type in the Java code, especially since with the PROXY_TABS thing that's not even really what this is. https://codereview.chromium.org/1247853007/diff/140001/sync/android/model_typ... File sync/android/model_type_helper.cc (right): https://codereview.chromium.org/1247853007/diff/140001/sync/android/model_typ... sync/android/model_type_helper.cc:14: static jstring ModelTypeToString(JNIEnv* env, On 2015/07/30 22:20:13, Nicolas Zea wrote: > ModelTypeToString already exists in model_type.h, and does something different. > I'd rather call this something else. Perhaps JavaModelTypeToNotificationType? Changed it to ModelTypeToNotificationType. Let me know if you felt strongly about the Java.
nyquist@chromium.org changed reviewers: + pkotwicz@chromium.org
+pkotwicz FYI
https://codereview.chromium.org/1247853007/diff/160001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/160001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:39: private static void initNonInvalidationTypes() { This doesn't seem thread safe. Could you at least add an assert that you are on the UI thread or something along those lines? https://codereview.chromium.org/1247853007/diff/160001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:54: if (sNonInvalidationTypes == null) { Same about threading here. https://codereview.chromium.org/1247853007/diff/160001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:85: public static String toString(Integer modelType) { Nit: Could this be 'int'? https://codereview.chromium.org/1247853007/diff/160001/sync/android/model_typ... File sync/android/model_type_helper.cc (right): https://codereview.chromium.org/1247853007/diff/160001/sync/android/model_typ... sync/android/model_type_helper.cc:15: jclass clazz, Nit: Is the indent off here, or is it just rietveld?
Related change: https://codereview.chromium.org/1263773007/
https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:49: return !FieldTrialList.findFullName("AndroidSessionNotifications").equals("Disabled"); On 2015/07/31 00:49:39, maxbogue wrote: > On 2015/07/30 22:20:13, Nicolas Zea wrote: > > Seems like this is a bit of a layering invalidation. Either the notion of > > Sessions/Favicons being part of an experiment should be tightly coupled to the > > field trial here, or the field trial should be coupled to those types within > > sTypesDisabledForExperiment. I wonder if we should just get rid of > > sTypesDisabledForExperiment altogether. > > I wanted to avoid calling toString on the relevant types and constructing a set > for them every time isInvalidationType was called. If experiment values can't > change except on a Chrome restart, I could do the experiment check in init. Is > that the case? Experiments get polled every couple of hours and can therefore change on the fly as I understand it (you can look up the finch experiments documentation to find out the details). I don't feel too strongly about this though, I guess it's okay like this. https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:70: public static String toString(Integer modelType) { On 2015/07/31 15:49:56, maxbogue wrote: > On 2015/07/31 00:49:40, maxbogue wrote: > > On 2015/07/30 22:20:13, Nicolas Zea wrote: > > > Given the other comment about ModelTypeToString being confusing, I think > this > > > should probably be renamed as well, possibly toNotificationtype? > > > > NotificationType is a more confusing name to me since it's actually a string > > key, but I can change it. > > I would actually prefer to not have the concept of notification type in the Java > code, especially since with the PROXY_TABS thing that's not even really what > this is. What would you suggest in place of toString then, given ModelTypeToString is already defined to mean something else in the native code? I'm not sure what you mean by notification type not being what this is. Eventually this is just returning the native RealModelTypeToNotificationType, right? What do you consider this value to be? Notification type as far as I recall is just a string that can be used to create an object id. https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:75: return "PROXY_TABS"; On 2015/07/31 00:49:40, maxbogue wrote: > On 2015/07/30 22:20:13, Nicolas Zea wrote: > > Didn't PROXY_TABS just previously have the string "NULL"? > > That's what I thought at first, but no... Previously all the ModelTypes had two > string representations. Their .name(), which is just the text of the actual enum > text, and the string that was passed into their constructor (stored as > mModelType). The name seems to be what was stored in invalidation preferences, > and the mModelType value was only used when they were converted to ObjectIds. > PROXY_TABS should be filtered out before being converted to an ObjectId, and my > understanding is that the pref value has to match. I could add a check in > toObjectId that returns "NULL" if !isInvalidationType just in case someone > doesn't use modelTypeStringsToObjectIds properly; would that be better? I see. What information did the preference store related to PROXY_TABS? What would happen if this wasn't handled? https://codereview.chromium.org/1247853007/diff/160001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/160001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:58: return !FieldTrialList.findFullName("AndroidSessionNotifications").equals("Disabled"); To be clear, the experiment being "on" in this case corresponds to the value "Disabled" right? Everyone else defaults to "Enabled"?
https://codereview.chromium.org/1247853007/diff/160001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/160001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:39: private static void initNonInvalidationTypes() { On 2015/08/03 20:07:10, nyquist wrote: > This doesn't seem thread safe. Could you at least add an assert that you are on > the UI thread or something along those lines? Decided to make it actually thread safe; PTAL. https://codereview.chromium.org/1247853007/diff/160001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:54: if (sNonInvalidationTypes == null) { On 2015/08/03 20:07:10, nyquist wrote: > Same about threading here. Done. https://codereview.chromium.org/1247853007/diff/160001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:85: public static String toString(Integer modelType) { On 2015/08/03 20:07:10, nyquist wrote: > Nit: Could this be 'int'? Yes, thanks! https://codereview.chromium.org/1247853007/diff/160001/sync/android/model_typ... File sync/android/model_type_helper.cc (right): https://codereview.chromium.org/1247853007/diff/160001/sync/android/model_typ... sync/android/model_type_helper.cc:15: jclass clazz, On 2015/08/03 20:07:10, nyquist wrote: > Nit: Is the indent off here, or is it just rietveld? Looks fine in vim, though I see it misaligned in rietveld too :/
https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:49: return !FieldTrialList.findFullName("AndroidSessionNotifications").equals("Disabled"); On 2015/08/04 17:04:53, Nicolas Zea wrote: > On 2015/07/31 00:49:39, maxbogue wrote: > > On 2015/07/30 22:20:13, Nicolas Zea wrote: > > > Seems like this is a bit of a layering invalidation. Either the notion of > > > Sessions/Favicons being part of an experiment should be tightly coupled to > the > > > field trial here, or the field trial should be coupled to those types within > > > sTypesDisabledForExperiment. I wonder if we should just get rid of > > > sTypesDisabledForExperiment altogether. > > > > I wanted to avoid calling toString on the relevant types and constructing a > set > > for them every time isInvalidationType was called. If experiment values can't > > change except on a Chrome restart, I could do the experiment check in init. Is > > that the case? > > Experiments get polled every couple of hours and can therefore change on the fly > as I understand it (you can look up the finch experiments documentation to find > out the details). > > I don't feel too strongly about this though, I guess it's okay like this. Yeah, in that case I don't see a much better way to do this. https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:70: public static String toString(Integer modelType) { On 2015/08/04 17:04:53, Nicolas Zea wrote: > On 2015/07/31 15:49:56, maxbogue wrote: > > On 2015/07/31 00:49:40, maxbogue wrote: > > > On 2015/07/30 22:20:13, Nicolas Zea wrote: > > > > Given the other comment about ModelTypeToString being confusing, I think > > this > > > > should probably be renamed as well, possibly toNotificationtype? > > > > > > NotificationType is a more confusing name to me since it's actually a string > > > key, but I can change it. > > > > I would actually prefer to not have the concept of notification type in the > Java > > code, especially since with the PROXY_TABS thing that's not even really what > > this is. > > What would you suggest in place of toString then, given ModelTypeToString is > already defined to mean something else in the native code? > > I'm not sure what you mean by notification type not being what this is. > Eventually this is just returning the native RealModelTypeToNotificationType, > right? What do you consider this value to be? > > Notification type as far as I recall is just a string that can be used to create > an object id. Ah, I wasn't thinking the native ModelTypeToString would conflict with this. Re: notification type, I was simply referring to the PROXY_TABS exception, but I guess that doesn't matter so much. I'll change this to toNotificationType. https://codereview.chromium.org/1247853007/diff/140001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:75: return "PROXY_TABS"; On 2015/08/04 17:04:53, Nicolas Zea wrote: > On 2015/07/31 00:49:40, maxbogue wrote: > > On 2015/07/30 22:20:13, Nicolas Zea wrote: > > > Didn't PROXY_TABS just previously have the string "NULL"? > > > > That's what I thought at first, but no... Previously all the ModelTypes had > two > > string representations. Their .name(), which is just the text of the actual > enum > > text, and the string that was passed into their constructor (stored as > > mModelType). The name seems to be what was stored in invalidation preferences, > > and the mModelType value was only used when they were converted to ObjectIds. > > PROXY_TABS should be filtered out before being converted to an ObjectId, and > my > > understanding is that the pref value has to match. I could add a check in > > toObjectId that returns "NULL" if !isInvalidationType just in case someone > > doesn't use modelTypeStringsToObjectIds properly; would that be better? > > I see. What information did the preference store related to PROXY_TABS? What > would happen if this wasn't handled? My understanding is that it would store "PROXY_TABS", but then when it was going to register the prefs values, PROXY_TABS would get stripped out in the conversion to object IDs. This is how it still works in this CL; PROXY_TABS should be removed in modelTypeStringsToObjectIds and never be registered. However, previously if somehow that filter was bypassed, PROXY_TABS would be registered using the string "NULL". In the current state of this CL, it would be registered with "PROXY_TABS". If we'd like to fix this, I'd propose just adding a isInvalidationType() check to toObjectId() that uses the string "NULL" if it fails. Alternatively, we could just assert isInvalidationType. https://codereview.chromium.org/1247853007/diff/160001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/160001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:58: return !FieldTrialList.findFullName("AndroidSessionNotifications").equals("Disabled"); On 2015/08/04 17:04:53, Nicolas Zea wrote: > To be clear, the experiment being "on" in this case corresponds to the value > "Disabled" right? Everyone else defaults to "Enabled"? Yep.
Btw, https://codereview.chromium.org/1271003002/ has just landed. Can you rebase against that CL? I think the CL makes things slightly easier for you
https://codereview.chromium.org/1247853007/diff/200001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/200001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:64: * Converts a model type string into an ObjectId. s/modelType/notificationType/ in this method? (and comments) https://codereview.chromium.org/1247853007/diff/200001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:75: return toObjectId(toString(modelType)); shouldn't this be toNotificationType? https://codereview.chromium.org/1247853007/diff/200001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:102: public static Set<ObjectId> modelTypeStringsToObjectIds(Collection<String> modelTypes) { What is this called by (didn't find a reference in this patch)
On 2015/08/04 20:21:07, pkotwicz wrote: > Btw, https://codereview.chromium.org/1271003002/ has just landed. Can you rebase > against that CL? I think the CL makes things slightly easier for you Done; I've removed all the experiment stuff from ModelTypeHelper since it was moved in that CL. https://codereview.chromium.org/1247853007/diff/200001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/200001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:64: * Converts a model type string into an ObjectId. On 2015/08/04 22:30:36, Nicolas Zea wrote: > s/modelType/notificationType/ in this method? (and comments) Done, should be consistent across the file now. Let me know if I missed anything. https://codereview.chromium.org/1247853007/diff/200001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:75: return toObjectId(toString(modelType)); On 2015/08/04 22:30:35, Nicolas Zea wrote: > shouldn't this be toNotificationType? Yeah, apparently when I thought I built this patch, it didn't actually build my changes. Fixed now. https://codereview.chromium.org/1247853007/diff/200001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:102: public static Set<ObjectId> modelTypeStringsToObjectIds(Collection<String> modelTypes) { On 2015/08/04 22:30:35, Nicolas Zea wrote: > What is this called by (didn't find a reference in this patch) https://codereview.chromium.org/1262413006/diff/20001/components/invalidation...
LGTM!
great CL https://codereview.chromium.org/1247853007/diff/260001/sync/android/model_typ... File sync/android/model_type_helper.cc (right): https://codereview.chromium.org/1247853007/diff/260001/sync/android/model_typ... sync/android/model_type_helper.cc:15: jclass clazz, align these with the first param?
https://codereview.chromium.org/1247853007/diff/260001/sync/android/model_typ... File sync/android/model_type_helper.cc (right): https://codereview.chromium.org/1247853007/diff/260001/sync/android/model_typ... sync/android/model_type_helper.cc:15: jclass clazz, On 2015/08/05 17:23:49, pvalenzuela wrote: > align these with the first param? Done. I feel like I'm going mad; Tommy made this comment in a previous patch and I looked at the file in vim and things were aligned properly. This time I looked and they were off... I might have been in the wrong branch before, the one where the two patches were together.
lgtm, but it would be great if pkotwicz did a quick review before you landed. pkotwicz: Could you verify that the ModelTypeHelper incorporates what it needs to?
One comment. Looks good otherwise https://codereview.chromium.org/1247853007/diff/280001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/280001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:101: public static Set<ObjectId> notificationTypesToObjectIds(Collection<String> notificationTypes) { It looks like there is a change in behavior w.r.t to ModelType#syncTypesToObjectIds() In ModelType#syncTypesToObjectIds() if I pass it a Collection with a the single string "Ha!" I get an empty Set back. ModelTypeHelper#notificationTypesToObjectIds() returns a Set with one element.
https://codereview.chromium.org/1247853007/diff/280001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/280001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:32: ModelType.PROXY_TABS Is there a reason why this is not a String array?
https://codereview.chromium.org/1247853007/diff/280001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/280001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:32: ModelType.PROXY_TABS On 2015/08/06 19:27:36, pkotwicz wrote: > Is there a reason why this is not a String array? Because I don't have access to the strings when this is made. (Requires native to be loaded.) https://codereview.chromium.org/1247853007/diff/280001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:101: public static Set<ObjectId> notificationTypesToObjectIds(Collection<String> notificationTypes) { On 2015/08/06 19:20:00, pkotwicz wrote: > It looks like there is a change in behavior w.r.t to > ModelType#syncTypesToObjectIds() > > In ModelType#syncTypesToObjectIds() if I pass it a Collection with a the single > string "Ha!" I get an empty Set back. > ModelTypeHelper#notificationTypesToObjectIds() returns a Set with one element. I have currently have no way of validating that there aren't arbitrary strings passed into this function. Previously they were converted to ModelTypes and then to ObjectIds, but there is no longer a notification type -> ModelType (int) mapping, at least in java. I'm relying on the fact that only strings generated by toNotificationType() should ever be passed in here; would it be sufficient to add that to the comment? Or would you like me to add a JNI binding for the String -> ModelType conversion?
LGTM https://codereview.chromium.org/1247853007/diff/280001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/280001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:101: public static Set<ObjectId> notificationTypesToObjectIds(Collection<String> notificationTypes) { Adding to the comment would be sufficient.
https://codereview.chromium.org/1247853007/diff/280001/sync/android/java/src/... File sync/android/java/src/org/chromium/sync/ModelTypeHelper.java (right): https://codereview.chromium.org/1247853007/diff/280001/sync/android/java/src/... sync/android/java/src/org/chromium/sync/ModelTypeHelper.java:101: public static Set<ObjectId> notificationTypesToObjectIds(Collection<String> notificationTypes) { On 2015/08/06 21:00:01, pkotwicz wrote: > Adding to the comment would be sufficient. Done.
The CQ bit was checked by maxbogue@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org, nyquist@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/1247853007/#ps300001 (title: "Update comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1247853007/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1247853007/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/a909f56705b64f3cd067fdba76225b330f8bca48 Cr-Commit-Position: refs/heads/master@{#342260} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
