Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
978623002:
New UMA settings fragment for Chrome on Android. (Closed)
|
Created:
5 years, 9 months ago by gayane -on leave until 09-2017 Modified:
5 years, 9 months ago CC:
asvitkine+watch_chromium.org, cbentzel+watch_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNew UMA settings fragment for Chrome on Android.
This CL is part of new UMA upload logic. The change is for getting more usage data for Chrome users who exclusively use wireless networks. For that, a new binary on/off-switch is added instead of previous three-option model. However, the upload logic is changed on Android to reduce the data usage.
The new binary switch is also connected to a different pref then previous switch. For the beginning the new settings UI would be enabled for users assigned to experimental group.
BUG=455847
Committed: https://crrev.com/c68d47f75c20971faee1b04d896bf25482aa2b03
Cr-Commit-Position: refs/heads/master@{#321575}
Patch Set 1 #Patch Set 2 : Test fixed #
Total comments: 26
Patch Set 3 : Renames and formatting fixes #
Total comments: 2
Patch Set 4 : Visible for testing added + formatting #
Total comments: 14
Patch Set 5 : change PrefServiceBridge contructore for tests nits #Patch Set 6 : merge #Patch Set 7 : fixes for findbug errors fix #
Total comments: 20
Patch Set 8 : Fixed clean up after PrivacyPrefencesManagerTest nits #
Total comments: 4
Patch Set 9 : Null check removed. #Messages
Total messages: 44 (24 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
gayane@chromium.org changed reviewers: + asvitkine@chromium.org
Patchset #2 (id:60001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:220001) has been deleted
Patchset #2 (id:240001) has been deleted
Patchset #2 (id:260001) has been deleted
Patchset #2 (id:280001) has been deleted
asvitkine@ would you have a look before owners review.
asvitkine@chromium.org changed reviewers: + dfalcantara@chromium.org
+dfalcantara who can do a better job reviewing the Android-specific bits. Since this is a public CL, I suggest expanding the CL description with a brief summary of what's at the go link (don't need to go to too much detail, but just a few lines about what's being done and how). Also, you have an extra space in the CL description and "modal" should be "model". https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/res... File chrome/android/java/res/xml/usage_and_crash_reports_preferences.xml (right): https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/res... chrome/android/java/res/xml/usage_and_crash_reports_preferences.xml:10: Nit: Remove extra empty line. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java (right): https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java:93: getPreferenceScreen().removePreference(uploadCrashDumpPref); Nit: Cache getPreferenceScreen() result in a var above the if, since it's used multiple times. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:227: } else { If you have an early return, no need for else. Same below.
https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:109: protected PrefServiceBridge(boolean forTestingOnly) { nit: protected & public methods require javadocs https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:772: * @return whether Metrics reporting is enabled Add @VisibleForTesting to every method that you use only for tests. You'll break the downstream compile, otherwise, because ProGuard will strip out the functions. nit: javadoc indentation is off everywhere in this file https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:786: * Sets whether the metrics reporting should be enabled. nit: fix this comment. not sure if the name "is set metrics reporting" makes much sense, though. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:59: */ Add @VisibleForTesting, ForTesting https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:229: return mSharedPreferences.getBoolean(PREF_CRASH_DUMP_UPLOAD_NO_CELLULAR, false); why did you flip these from down in isNeverUploadCrashDump()? making the positive the top case means you don't have to ! the condition https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:245: **/ nit: none of these javadocs should have a **/ on the last line. Should be */ also indentation is off in the comments, everywhere. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:277: public boolean isCellularEnabledByExperiment() { nit: add javadoc https://codereview.chromium.org/978623002/diff/300001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/978623002/diff/300001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:290: return local_state->HasPrefPath(prefs::kMetricsReportingEnabled); the function name is more confusing when I look at the body of the function :/
Addressing asvitkine@'s and dfalcantara@'s comments. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/res... File chrome/android/java/res/xml/usage_and_crash_reports_preferences.xml (right): https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/res... chrome/android/java/res/xml/usage_and_crash_reports_preferences.xml:10: On 2015/03/09 18:26:27, Alexei Svitkine wrote: > Nit: Remove extra empty line. Done. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:109: protected PrefServiceBridge(boolean forTestingOnly) { On 2015/03/09 19:53:28, dfalcantara wrote: > nit: protected & public methods require javadocs Done. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:772: * @return whether Metrics reporting is enabled I have Mock PrefServiceBridge which overrides these functions. Do I still need to declare these functions @VisibleForTesting? indentation: It took me a while to see whats wrong with comments. i hope I fixed them. On 2015/03/09 19:53:28, dfalcantara wrote: > Add @VisibleForTesting to every method that you use only for tests. You'll > break the downstream compile, otherwise, because ProGuard will strip out the > functions. > > nit: javadoc indentation is off everywhere in this file https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:786: * Sets whether the metrics reporting should be enabled. On 2015/03/09 19:53:28, dfalcantara wrote: > nit: fix this comment. not sure if the name "is set metrics reporting" makes > much sense, though. Comment fixed. I renamed the function to hasSetMetricsReporting. Please feel free to suggest more reasonable name. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java (right): https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java:93: getPreferenceScreen().removePreference(uploadCrashDumpPref); On 2015/03/09 18:26:27, Alexei Svitkine wrote: > Nit: Cache getPreferenceScreen() result in a var above the if, since it's used > multiple times. Done. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:59: */ On 2015/03/09 19:53:28, dfalcantara wrote: > Add @VisibleForTesting, ForTesting Done. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:227: } else { On 2015/03/09 18:26:27, Alexei Svitkine wrote: > If you have an early return, no need for else. Same below. Done. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:229: return mSharedPreferences.getBoolean(PREF_CRASH_DUMP_UPLOAD_NO_CELLULAR, false); On 2015/03/09 19:53:29, dfalcantara wrote: > why did you flip these from down in isNeverUploadCrashDump()? making the > positive the top case means you don't have to ! the condition I am not sure I understand you correctly. isNeverUploadCrashDump was not used here and I have added only the first "if" in this function. Anyway, I flipped the check with isMobileNetworkCapable(). If this is correct then i will do the same change in other functions as well. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:245: **/ On 2015/03/09 19:53:29, dfalcantara wrote: > nit: none of these javadocs should have a **/ on the last line. Should be */ > also indentation is off in the comments, everywhere. Done. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:277: public boolean isCellularEnabledByExperiment() { On 2015/03/09 19:53:29, dfalcantara wrote: > nit: add javadoc Done. https://codereview.chromium.org/978623002/diff/300001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/978623002/diff/300001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:290: return local_state->HasPrefPath(prefs::kMetricsReportingEnabled); On 2015/03/09 19:53:29, dfalcantara wrote: > the function name is more confusing when I look at the body of the function :/ changed to hasSetMetricsReporting.
https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:772: * @return whether Metrics reporting is enabled On 2015/03/09 23:07:25, gayane wrote: > I have Mock PrefServiceBridge which overrides these functions. Do I still need > to declare these functions @VisibleForTesting? > > indentation: It took me a while to see whats wrong with comments. i hope I fixed > them. > > On 2015/03/09 19:53:28, dfalcantara wrote: > > Add @VisibleForTesting to every method that you use only for tests. You'll > > break the downstream compile, otherwise, because ProGuard will strip out the > > functions. > > > > nit: javadoc indentation is off everywhere in this file > > if your MockPrefServiceBridge is only used in Tests, then yes, you need to mark them as @VisibleForTesting. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:229: return mSharedPreferences.getBoolean(PREF_CRASH_DUMP_UPLOAD_NO_CELLULAR, false); On 2015/03/09 23:07:25, gayane wrote: > On 2015/03/09 19:53:29, dfalcantara wrote: > > why did you flip these from down in isNeverUploadCrashDump()? making the > > positive the top case means you don't have to ! the condition > > > I am not sure I understand you correctly. isNeverUploadCrashDump was not used > here and I have added only the first "if" in this function. > > Anyway, I flipped the check with isMobileNetworkCapable(). If this is correct > then i will do the same change in other functions as well. It wasn't used here, but it looks like the function body was copied over. Yeah, I was just asking you to flip the conditional. https://codereview.chromium.org/978623002/diff/320001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/978623002/diff/320001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:61: public void setCellularExperimentFortesting(boolean isCellularEnabledByExperiment) { Fortesting -> ForTesting
VisbleForTesting added + formatting https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:772: * @return whether Metrics reporting is enabled On 2015/03/09 23:11:00, dfalcantara wrote: > On 2015/03/09 23:07:25, gayane wrote: > > I have Mock PrefServiceBridge which overrides these functions. Do I still need > > to declare these functions @VisibleForTesting? > > > > indentation: It took me a while to see whats wrong with comments. i hope I > fixed > > them. > > > > On 2015/03/09 19:53:28, dfalcantara wrote: > > > Add @VisibleForTesting to every method that you use only for tests. You'll > > > break the downstream compile, otherwise, because ProGuard will strip out the > > > functions. > > > > > > nit: javadoc indentation is off everywhere in this file > > > > > > if your MockPrefServiceBridge is only used in Tests, then yes, you need to mark > them as @VisibleForTesting. Done. https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/978623002/diff/300001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:229: return mSharedPreferences.getBoolean(PREF_CRASH_DUMP_UPLOAD_NO_CELLULAR, false); On 2015/03/09 23:11:00, dfalcantara wrote: > On 2015/03/09 23:07:25, gayane wrote: > > On 2015/03/09 19:53:29, dfalcantara wrote: > > > why did you flip these from down in isNeverUploadCrashDump()? making the > > > positive the top case means you don't have to ! the condition > > > > > > I am not sure I understand you correctly. isNeverUploadCrashDump was not used > > here and I have added only the first "if" in this function. > > > > Anyway, I flipped the check with isMobileNetworkCapable(). If this is correct > > then i will do the same change in other functions as well. > > It wasn't used here, but it looks like the function body was copied over. Yeah, > I was just asking you to flip the conditional. Done. https://codereview.chromium.org/978623002/diff/320001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/978623002/diff/320001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:61: public void setCellularExperimentFortesting(boolean isCellularEnabledByExperiment) { On 2015/03/09 23:11:00, dfalcantara wrote: > Fortesting -> ForTesting Done.
lgtm
gayane@chromium.org changed reviewers: + nyquist@chromium.org
nyquist@chromium.org: Please review changes in chrome/android/java/src/* chrome/android/java/strings/android_chrome_strings.grd chrome/browser/android/preferences/pref_service_bridge.cc
https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:114: if (!forTestingOnly) getInstance(); Wait - how is this supposed to work? If the boolean parameter is false, then this calls getInstance() which will create a new PrefsServiceBridge - but it actually will be a different object than this one. So now there's two different objects. This seems very confusing. If that's really the intention here, it should have a comment explaining why we need such a strange construct. https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java (right): https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java:93: if (PrivacyPreferencesManager.getInstance(getActivity()).isCellularEnabledByExperiment()) { Nit: PrivacyPreferencesManager.getInstance(getActivity()) is used on line 74 above and also on line 58. Let's cache it in a var. https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:45: private Boolean mIsCellularEnabledByExperiment; Why a Boolean and not a boolean? Add a comment. https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:62: mIsCellularEnabledByExperiment = new Boolean(isCellularEnabledByExperiment); Nit: Boolean.valueOf() https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:281: public boolean isCellularEnabledByExperiment() { How about isCellularUploadingEnabled(). "isCellular" doesn't convey enough info and "by experiment" is an implementation detail. https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:284: mIsCellularEnabledByExperiment = new Boolean(group_name.equals("Enabled")); Boolean.valueOf https://codereview.chromium.org/978623002/diff/340001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerTest.java (right): https://codereview.chromium.org/978623002/diff/340001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerTest.java:159: PrefServiceBridge.setInstanceForTesting(prefServiceBridge); Shouldn't we restore the old instance after the test? Otherwise, tests run after this one can be flaky - as left-over state from this test may affect their behavior - and they may behave differently depending if this test was run first or not.
Patchset #6 (id:380001) has been deleted
Patchset #7 (id:420001) has been deleted
asvitkine@ comments mainly address in patch set #5. https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:114: if (!forTestingOnly) getInstance(); You are right this is not correct. I just wanted to have a constructor with some parameter which I can call from subclass constructor otherwise by default super() will be called which loads TemplateUrlService, etc. I changed it, I hope this is more reasonable. On 2015/03/11 19:45:59, Alexei Svitkine (slow) wrote: > Wait - how is this supposed to work? > > If the boolean parameter is false, then this calls getInstance() which will > create a new PrefsServiceBridge - but it actually will be a different object > than this one. So now there's two different objects. This seems very confusing. > If that's really the intention here, it should have a comment explaining why we > need such a strange construct. https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java (right): https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java:93: if (PrivacyPreferencesManager.getInstance(getActivity()).isCellularEnabledByExperiment()) { On 2015/03/11 19:45:59, Alexei Svitkine (slow) wrote: > Nit: PrivacyPreferencesManager.getInstance(getActivity()) is used on line 74 > above and also on line 58. Let's cache it in a var. Done. https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:45: private Boolean mIsCellularEnabledByExperiment; On 2015/03/11 19:45:59, Alexei Svitkine (slow) wrote: > Why a Boolean and not a boolean? > > Add a comment. Done. https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:62: mIsCellularEnabledByExperiment = new Boolean(isCellularEnabledByExperiment); On 2015/03/11 19:45:59, Alexei Svitkine (slow) wrote: > Nit: Boolean.valueOf() Done. https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:281: public boolean isCellularEnabledByExperiment() { On 2015/03/11 19:45:59, Alexei Svitkine (slow) wrote: > How about isCellularUploadingEnabled(). > > "isCellular" doesn't convey enough info and "by experiment" is an implementation > detail. I have changed it, But tts just there are two kind of enabled here by user and by experiments https://codereview.chromium.org/978623002/diff/340001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:284: mIsCellularEnabledByExperiment = new Boolean(group_name.equals("Enabled")); On 2015/03/11 19:45:59, Alexei Svitkine (slow) wrote: > Boolean.valueOf Done. https://codereview.chromium.org/978623002/diff/340001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerTest.java (right): https://codereview.chromium.org/978623002/diff/340001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerTest.java:159: PrefServiceBridge.setInstanceForTesting(prefServiceBridge); On 2015/03/11 19:45:59, Alexei Svitkine (slow) wrote: > Shouldn't we restore the old instance after the test? > > Otherwise, tests run after this one can be flaky - as left-over state from this > test may affect their behavior - and they may behave differently depending if > this test was run first or not. Done.
https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:114: protected PrefServiceBridge(boolean forTestingOnly) { This is an odd construct to me. The |forTestingOnly| variable is never used, which is a little bit odd. I wonder, could we do something like renaming the variable to |loadTemplateUrlService| and then make this constructor do someething like: if (loadTemplateUrlService) TemplateUrlService.getInstance().load(); Then you could make the private constructor call this(true), and then update the MockPrefServiceBridge to pass inn false to super. Or were you planning on the name |forTestingOnly| being sufficient for new readers to understand that they should not use this constructor? If that's the case, feel free to leave the name |forTestingOnly| and use that as the inverse conditional for he if-statement. What do you think? https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:223: * @return boolean to whether to allow uploading crash dump. Nit: Could you change this to just be "@return whether to allow..." https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:228: if (isCellularUploadingEnabled() && prefServiceBridge.hasSetMetricsReporting()) Nit: Missing curly brackets. It is only OK to skip then in Java-code if the conditional and the consequent fits on a single line, and there's no else. https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:246: * @return boolean to whether uploads are allowed at all or not. Nit: @return whether up... https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:252: if (isCellularEnabledByExperiment && prefServiceBridge.hasSetMetricsReporting()) Nit: Missing curly brackets. https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:255: if (isCellularEnabledByExperiment && !prefServiceBridge.hasSetMetricsReporting()) Nit: Missing curly brackets. https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:264: * @return boolean to whether uploads are enabled by old pref. Nit: @return whether up... https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:281: * @return boolean to whether user is assigned to experimental group. Nit: @return whether user is... https://codereview.chromium.org/978623002/diff/440001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerTest.java (right): https://codereview.chromium.org/978623002/diff/440001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerTest.java:177: PrefServiceBridge.setInstanceForTesting(prevInstance); Should this be part of a try/finally test, so there won't be issues with following tests?
asvitkine@chromium.org changed reviewers: + holte@chromium.org
lgtm https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/UsageAndCrashReportsPreferenceFragment.java (right): https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/UsageAndCrashReportsPreferenceFragment.java:42: boolean isEnabled = (boolean) newValue; Can newValue ever be null? If so, this will throw an exception.
Patchset #8 (id:460001) has been deleted
Patchset #8 (id:480001) has been deleted
Addressing comments by asvitkine@ and nyquist@ https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:114: protected PrefServiceBridge(boolean forTestingOnly) { Well I though to keep that constructor for testing only, but your suggestion makes sense. Changed. On 2015/03/17 21:21:46, nyquist wrote: > This is an odd construct to me. The |forTestingOnly| variable is never used, > which is a little bit odd. I wonder, could we do something like renaming the > variable to |loadTemplateUrlService| and then make this constructor do > someething like: > if (loadTemplateUrlService) TemplateUrlService.getInstance().load(); > Then you could make the private constructor call this(true), and then update the > MockPrefServiceBridge to pass inn false to super. > > Or were you planning on the name |forTestingOnly| being sufficient for new > readers to understand that they should not use this constructor? > > If that's the case, feel free to leave the name |forTestingOnly| and use that as > the inverse conditional for he if-statement. > > What do you think? https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:223: * @return boolean to whether to allow uploading crash dump. On 2015/03/17 21:21:46, nyquist wrote: > Nit: Could you change this to just be "@return whether to allow..." Done. https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:228: if (isCellularUploadingEnabled() && prefServiceBridge.hasSetMetricsReporting()) On 2015/03/17 21:21:46, nyquist wrote: > Nit: Missing curly brackets. > It is only OK to skip then in Java-code if the conditional and the consequent > fits on a single line, and there's no else. Done. https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:246: * @return boolean to whether uploads are allowed at all or not. On 2015/03/17 21:21:46, nyquist wrote: > Nit: @return whether up... Done. https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:252: if (isCellularEnabledByExperiment && prefServiceBridge.hasSetMetricsReporting()) On 2015/03/17 21:21:46, nyquist wrote: > Nit: Missing curly brackets. Done. https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:255: if (isCellularEnabledByExperiment && !prefServiceBridge.hasSetMetricsReporting()) On 2015/03/17 21:21:46, nyquist wrote: > Nit: Missing curly brackets. Done. https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:264: * @return boolean to whether uploads are enabled by old pref. On 2015/03/17 21:21:46, nyquist wrote: > Nit: @return whether up... Done. https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:281: * @return boolean to whether user is assigned to experimental group. On 2015/03/17 21:21:46, nyquist wrote: > Nit: @return whether user is... Done. https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/UsageAndCrashReportsPreferenceFragment.java (right): https://codereview.chromium.org/978623002/diff/440001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/UsageAndCrashReportsPreferenceFragment.java:42: boolean isEnabled = (boolean) newValue; On 2015/03/17 22:40:48, Alexei Svitkine (away) wrote: > Can newValue ever be null? > > If so, this will throw an exception. As this function triggers when user changes a preference and newvalue is the value chosen by user its hard to imagine when it would be null but just in case I have added a check. https://codereview.chromium.org/978623002/diff/440001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerTest.java (right): https://codereview.chromium.org/978623002/diff/440001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerTest.java:177: PrefServiceBridge.setInstanceForTesting(prevInstance); On 2015/03/17 21:21:46, nyquist wrote: > Should this be part of a try/finally test, so there won't be issues with > following tests? Done.
https://codereview.chromium.org/978623002/diff/500001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/978623002/diff/500001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:115: if (loadTemplateUrlService) This must be on a single line, or add curly brackets. https://codereview.chromium.org/978623002/diff/500001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/UsageAndCrashReportsPreferenceFragment.java (right): https://codereview.chromium.org/978623002/diff/500001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/UsageAndCrashReportsPreferenceFragment.java:42: if (newValue == null) Please remove this null-check. It is unnecessary and only causes confusion.
Null value check removed. https://codereview.chromium.org/978623002/diff/500001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/978623002/diff/500001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:115: if (loadTemplateUrlService) On 2015/03/18 22:19:14, nyquist wrote: > This must be on a single line, or add curly brackets. Done. https://codereview.chromium.org/978623002/diff/500001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/UsageAndCrashReportsPreferenceFragment.java (right): https://codereview.chromium.org/978623002/diff/500001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/UsageAndCrashReportsPreferenceFragment.java:42: if (newValue == null) On 2015/03/18 22:19:14, nyquist wrote: > Please remove this null-check. It is unnecessary and only causes confusion. Done.
lgtm
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/978623002/#ps520001 (title: "Null check removed.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/978623002/520001
Message was sent while issue was closed.
Committed patchset #9 (id:520001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/c68d47f75c20971faee1b04d896bf25482aa2b03 Cr-Commit-Position: refs/heads/master@{#321575}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:520001) has been created in https://codereview.chromium.org/1048803003/ by gayane@chromium.org. The reason for reverting is: reverting back to fix crashes during Release tests crbug.com/470016 crbug.com/470062. |
