Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1111133002:
Changes necessary for limiting crash report uploads for cellular networks (Closed)
|
Created:
5 years, 7 months ago by gayane -on leave until 09-2017 Modified:
5 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChanges necessary for limiting crash report uploads for cellular networks.
BUG=455847
Committed: https://crrev.com/1b84b594d835468de3d9e9d3d19a15594023d261
Cr-Commit-Position: refs/heads/master@{#329630}
Patch Set 1 #
Total comments: 6
Patch Set 2 : fix comments, remove extra check #
Total comments: 6
Patch Set 3 : fix comments #
Total comments: 2
Patch Set 4 : fix comment typos #
Messages
Total messages: 17 (5 generated)
gayane@chromium.org changed reviewers: + asvitkine@chromium.org
Please have a look on part of the changes. Other part of the changes will be sent over through clank review tool.
https://codereview.chromium.org/1111133002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java (right): https://codereview.chromium.org/1111133002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java:20: * Check whether to uploading crash dump should be constrained mode based on user experiments. Nit: "whether to" -> "whether" Same thing below on the @return line Also, please explain what constrained means. https://codereview.chromium.org/1111133002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/1111133002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:267: if (!mSharedPreferences.contains(PREF_CELLULAR_EXPERIMENT)) return false; Add a comment to clarify why this line is needed. https://codereview.chromium.org/1111133002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:353: * Check whether to uploading crash dump should be constrained mode based on user experiments. Same as the other comments.
Thanks for having a look. Fixed the comments. Sending to the owner. https://codereview.chromium.org/1111133002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java (right): https://codereview.chromium.org/1111133002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java:20: * Check whether to uploading crash dump should be constrained mode based on user experiments. On 2015/04/29 15:09:34, Alexei Svitkine wrote: > Nit: "whether to" -> "whether" > > Same thing below on the @return line > > Also, please explain what constrained means. Done. https://codereview.chromium.org/1111133002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/1111133002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:267: if (!mSharedPreferences.contains(PREF_CELLULAR_EXPERIMENT)) return false; On 2015/04/29 15:09:34, Alexei Svitkine wrote: > Add a comment to clarify why this line is needed. Removed the line as getBoolean will return passed default value false when the pref does not exists. https://codereview.chromium.org/1111133002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:353: * Check whether to uploading crash dump should be constrained mode based on user experiments. On 2015/04/29 15:09:34, Alexei Svitkine wrote: > Same as the other comments. Done.
lgtm
gayane@chromium.org changed reviewers: + nyquist@chromium.org
nyquist@chromium.org: Please have a look.
The commit description is very short. Some tips for expansion: What are the necessary parts? What does the new limitations map to? How do they correspond to the old isIploadPermitted? https://codereview.chromium.org/1111133002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java (right): https://codereview.chromium.org/1111133002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java:20: * Check whether uploading crash dump should be constrained mode based on user experiments. Nit: This sentence seems a bit off as it is written now. Maybe "should be in constrained mode"? https://codereview.chromium.org/1111133002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java:22: * @return boolean to whether uploading logic should be constrained. Nit: Just "@return whether uploading..." You don't need to re-iterate the return type in the comment. https://codereview.chromium.org/1111133002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java:24: public boolean isUploadLimited(); It's a little bit unclear to me how this method interacts with isUploadPermitted(). Could you expand a bit in the comment about how they work together and/or alone? Which method should I call? What happens if one returns true and the other returns false?
fixed the comments. Please have one more look. https://codereview.chromium.org/1111133002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java (right): https://codereview.chromium.org/1111133002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java:20: * Check whether uploading crash dump should be constrained mode based on user experiments. On 2015/04/29 16:36:22, nyquist wrote: > Nit: This sentence seems a bit off as it is written now. Maybe "should be in > constrained mode"? Done. https://codereview.chromium.org/1111133002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java:22: * @return boolean to whether uploading logic should be constrained. On 2015/04/29 16:36:22, nyquist wrote: > Nit: Just "@return whether uploading..." > You don't need to re-iterate the return type in the comment. Done. https://codereview.chromium.org/1111133002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java:24: public boolean isUploadLimited(); On 2015/04/29 16:36:22, nyquist wrote: > It's a little bit unclear to me how this method interacts with > isUploadPermitted(). Could you expand a bit in the comment about how they work > together and/or alone? Which method should I call? What happens if one returns > true and the other returns false? Done.
lgtm with comments https://codereview.chromium.org/1111133002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java (right): https://codereview.chromium.org/1111133002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java:22: * determine whether crash uploads are currently possible or not. Use |isUplaodPermitted| Nit: Typo in isUplaodPermitted, but also, use {@link #isUploadPermitted()}. Same in the other file.
Patchset #4 (id:60001) has been deleted
Fixed typos. Going to submit now. https://codereview.chromium.org/1111133002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java (right): https://codereview.chromium.org/1111133002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java:22: * determine whether crash uploads are currently possible or not. Use |isUplaodPermitted| On 2015/05/13 06:34:41, nyquist wrote: > Nit: Typo in isUplaodPermitted, but also, use {@link #isUploadPermitted()}. > Same in the other file. Done.
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/1111133002/#ps80001 (title: "fix comment typos")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1111133002/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1b84b594d835468de3d9e9d3d19a15594023d261 Cr-Commit-Position: refs/heads/master@{#329630} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
