Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2953523002:
Add 'Other' category on the Data Saver site-breakdown page (Closed)
DescriptionAdd 'Other' category on the Data Saver site-breakdown page
This category includes chrome-services and service-worker traffic. This
should be fixed at the bottom of the per-site list.
BUG=735257
Review-Url: https://codereview.chromium.org/2953523002
Cr-Commit-Position: refs/heads/master@{#483172}
Committed: https://chromium.googlesource.com/chromium/src/+/6dc9ace1c175a5747f6656ca543e23325d14492f
Patch Set 1 #
Total comments: 7
Patch Set 2 : Addressed comments #
Total comments: 4
Patch Set 3 : Moved UI string to cpp #
Total comments: 2
Patch Set 4 : remove UI string jni interface #
Total comments: 4
Patch Set 5 : fixed comments #Patch Set 6 : fixed message description #
Messages
Total messages: 26 (9 generated)
rajendrant@google.com changed reviewers: + megjablon@chromium.org, rajendrant@google.com
ptal https://codereview.chromium.org/2953523002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2953523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:36: private static final String OTHER_HOST_NAME = "Other"; Not sure if a new string should be added in android_chrome_strings.grd. Since this is a URL, it needs no translation. wdyt
https://codereview.chromium.org/2953523002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2953523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:36: private static final String OTHER_HOST_NAME = "Other"; On 2017/06/20 23:24:25, rajendrant wrote: > Not sure if a new string should be added in android_chrome_strings.grd. > Since this is a URL, it needs no translation. > wdyt This should definitely be translated. It will probably need to go in generated_strings.grd so that we can get it in the observer, and then we'll have a jni call to native to get it here. https://codereview.chromium.org/2953523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:137: public int compare(DataReductionDataUseItem lhs, DataReductionDataUseItem rhs) { Please add a comment as to what this is doing. At least to me, it isn't instantly intuitive that is this if forcing the "Other" category to the bottom. https://codereview.chromium.org/2953523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:158: public int compare(DataReductionDataUseItem lhs, DataReductionDataUseItem rhs) { Also a comment here.
https://codereview.chromium.org/2953523002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2953523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:36: private static final String OTHER_HOST_NAME = "Other"; On 2017/06/20 23:45:46, megjablon wrote: > On 2017/06/20 23:24:25, rajendrant wrote: > > Not sure if a new string should be added in android_chrome_strings.grd. > > Since this is a URL, it needs no translation. > > wdyt > > This should definitely be translated. It will probably need to go in > generated_strings.grd so that we can get it in the observer, and then we'll have > a jni call to native to get it here. OK. I am adding an entry in android_chrome_strings.grd and using that while showing the UI. The DB will still have it keyed as 'Other'. Translated string will be used only in the UI. wdyt. Is this fine. Or do you find it hacky. https://codereview.chromium.org/2953523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:137: public int compare(DataReductionDataUseItem lhs, DataReductionDataUseItem rhs) { On 2017/06/20 23:45:46, megjablon wrote: > Please add a comment as to what this is doing. At least to me, it isn't > instantly intuitive that is this if forcing the "Other" category to the bottom. Done. https://codereview.chromium.org/2953523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:158: public int compare(DataReductionDataUseItem lhs, DataReductionDataUseItem rhs) { On 2017/06/20 23:45:46, megjablon wrote: > Also a comment here. Done.
https://codereview.chromium.org/2953523002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2953523002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:37: private static final String OTHER_HOST_NAME = "Other"; Leaving this as "Other" key sounds good to me, but can we still add a native call to getOtherHostNameKey (or something of that sort)? I have a bug filed against me for "all strings and all core data reduction proxy protocol logic should be in C++ code," and while this isn't the protocol logic, I'd like to keep that sentiment of cleaning up and not adding new duplicate strings in Android and native. We've had bugs in the past with strings getting out of sync. https://codereview.chromium.org/2953523002/diff/20001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2953523002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1059: <message name="IDS_DATA_REDUCTION_BREAKDOWN_OTHER_HOST_NAME" desc="Host name of the other category on the Data Reduction statistics page. This category groups the chrome services traffic."> The translators don't get context of the other strings or what the UI looks like so I try to be as verbose as possible. I don't think they'll necessarily know what 'chrome services traffic' means. What do you think of: "Host name of the other category on the Data Reduction statistics page. This category groups data that cannot be linked to a site. The breakdown lists the top ten sites with the greatest amount of data usage or mobile data that was saved and then groups the remaining sites together." And can you add 'meaning="All words should start capitalized."'
On 2017/06/21 18:21:15, megjablon wrote: > https://codereview.chromium.org/2953523002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java > (right): > > https://codereview.chromium.org/2953523002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:37: > private static final String OTHER_HOST_NAME = "Other"; > Leaving this as "Other" key sounds good to me, but can we still add a native > call to getOtherHostNameKey (or something of that sort)? I have a bug filed > against me for "all strings and all core data reduction proxy protocol logic > should be in C++ code," and while this isn't the protocol logic, I'd like to > keep that sentiment of cleaning up and not adding new duplicate strings in > Android and native. We've had bugs in the past with strings getting out of sync. > > https://codereview.chromium.org/2953523002/diff/20001/chrome/android/java/str... > File chrome/android/java/strings/android_chrome_strings.grd (right): > > https://codereview.chromium.org/2953523002/diff/20001/chrome/android/java/str... > chrome/android/java/strings/android_chrome_strings.grd:1059: <message > name="IDS_DATA_REDUCTION_BREAKDOWN_OTHER_HOST_NAME" desc="Host name of the other > category on the Data Reduction statistics page. This category groups the chrome > services traffic."> > The translators don't get context of the other strings or what the UI looks like > so I try to be as verbose as possible. I don't think they'll necessarily know > what 'chrome services traffic' means. What do you think of: > > "Host name of the other category on the Data Reduction statistics page. This > category groups data that cannot be linked to a site. The breakdown lists the > top ten sites with the greatest amount of data usage or mobile data that was > saved and then groups the remaining sites together." > > And can you add 'meaning="All words should start capitalized."' Hey Raj, can we get this landed soon?
rajendrant@chromium.org changed reviewers: + bengr@chromium.org - rajendrant@google.com
bengr: ptal DataReductionProxyAndroidUIStrings.java https://codereview.chromium.org/2953523002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2953523002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:37: private static final String OTHER_HOST_NAME = "Other"; On 2017/06/21 at 18:21:14, megjablon wrote: > Leaving this as "Other" key sounds good to me, but can we still add a native call to getOtherHostNameKey (or something of that sort)? I have a bug filed against me for "all strings and all core data reduction proxy protocol logic should be in C++ code," and while this isn't the protocol logic, I'd like to keep that sentiment of cleaning up and not adding new duplicate strings in Android and native. We've had bugs in the past with strings getting out of sync. Done https://codereview.chromium.org/2953523002/diff/20001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2953523002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1059: <message name="IDS_DATA_REDUCTION_BREAKDOWN_OTHER_HOST_NAME" desc="Host name of the other category on the Data Reduction statistics page. This category groups the chrome services traffic."> On 2017/06/21 at 18:21:14, megjablon wrote: > The translators don't get context of the other strings or what the UI looks like so I try to be as verbose as possible. I don't think they'll necessarily know what 'chrome services traffic' means. What do you think of: > > "Host name of the other category on the Data Reduction statistics page. This category groups data that cannot be linked to a site. The breakdown lists the top ten sites with the greatest amount of data usage or mobile data that was saved and then groups the remaining sites together." > > And can you add 'meaning="All words should start capitalized."' Done
rajendrant@chromium.org changed reviewers: + dfalcantara@chromium.org
megjablon: ptal all files dfalcantara: ptal DataReductionSiteBreakdownView.java chrome_jni_registrar.cc bengr: ptal DataReductionProxyAndroidUIStrings.java
https://codereview.chromium.org/2953523002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2953523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:202: hostName = DataReductionProxyAndroidUIStrings.getOtherHostName(); Since the translated string is being added in Android and "Other" is just being used as a key we don't need this jni interface and the string can be put in android_chrome_strings.grd
rajendrant@chromium.org changed reviewers: - bengr@chromium.org
Reverted to strings in java code. dfalcantara: ptal DataReductionSiteBreakdownView.java https://codereview.chromium.org/2953523002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2953523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:202: hostName = DataReductionProxyAndroidUIStrings.getOtherHostName(); On 2017/06/27 at 18:03:52, megjablon wrote: > Since the translated string is being added in Android and "Other" is just being used as a key we don't need this jni interface and the string can be put in android_chrome_strings.grd Done
lgtm
dfalcantara@chromium.org changed reviewers: + srahim@chromium.org
https://codereview.chromium.org/2953523002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2953523002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:34: /* Hostname used for the other bucket which consists of chrome-services traffic. Fix javadoc syntax. See line 27. https://codereview.chromium.org/2953523002/diff/60001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2953523002/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1058: <message name="IDS_DATA_REDUCTION_BREAKDOWN_OTHER_HOST_NAME" desc="Host name of the other category on the Data Reduction statistics page. This category groups data that cannot be linked to a site. The breakdown lists the top ten sites with the greatest amount of data usage or mobile data that was saved and then groups the remaining sites together."> Has srahim@ reviewed this string?
https://codereview.chromium.org/2953523002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2953523002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:34: /* Hostname used for the other bucket which consists of chrome-services traffic. On 2017/06/27 at 21:47:25, dfalcantara wrote: > Fix javadoc syntax. See line 27. Done Looks like google java style allows this, while not allowed in chrome/android java style. https://google.github.io/styleguide/javaguide.html#s4.8.6-comments https://codereview.chromium.org/2953523002/diff/60001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2953523002/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1058: <message name="IDS_DATA_REDUCTION_BREAKDOWN_OTHER_HOST_NAME" desc="Host name of the other category on the Data Reduction statistics page. This category groups data that cannot be linked to a site. The breakdown lists the top ten sites with the greatest amount of data usage or mobile data that was saved and then groups the remaining sites together."> On 2017/06/27 at 21:47:26, dfalcantara wrote: > Has srahim@ reviewed this string? cced srahim@ to the crbug as well.
On 2017/06/27 22:09:03, Raj wrote: > https://codereview.chromium.org/2953523002/diff/60001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java > (right): > > https://codereview.chromium.org/2953523002/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:34: > /* Hostname used for the other bucket which consists of chrome-services traffic. > On 2017/06/27 at 21:47:25, dfalcantara wrote: > > Fix javadoc syntax. See line 27. > > Done > Looks like google java style allows this, while not allowed in chrome/android > java style. > https://google.github.io/styleguide/javaguide.html#s4.8.6-comments > > https://codereview.chromium.org/2953523002/diff/60001/chrome/android/java/str... > File chrome/android/java/strings/android_chrome_strings.grd (right): > > https://codereview.chromium.org/2953523002/diff/60001/chrome/android/java/str... > chrome/android/java/strings/android_chrome_strings.grd:1058: <message > name="IDS_DATA_REDUCTION_BREAKDOWN_OTHER_HOST_NAME" desc="Host name of the other > category on the Data Reduction statistics page. This category groups data that > cannot be linked to a site. The breakdown lists the top ten sites with the > greatest amount of data usage or mobile data that was saved and then groups the > remaining sites together."> > On 2017/06/27 at 21:47:26, dfalcantara wrote: > > Has srahim@ reviewed this string? > > cced srahim@ to the crbug as well. "Other" makes sense, but please update the message description for the translators: Appears in a table that has a Site column and a Used column. URLs of sites the user has visited appear in the Site column, and the amount of data the site has used appears under Used. The “Other” entry appears at the bottom of the Site column, and is a grouping for sites and services where the URL is unavailable.
dfalcantara: ptal DataReductionSiteBreakdownView.java
lgtm
The CQ bit was checked by rajendrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from megjablon@chromium.org Link to the patchset: https://codereview.chromium.org/2953523002/#ps100001 (title: "fixed message description")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498684232961090,
"parent_rev": "48ea1273a02f225c876093ff16574b8cf2139eee", "commit_rev":
"6dc9ace1c175a5747f6656ca543e23325d14492f"}
Message was sent while issue was closed.
Description was changed from ========== Add 'Other' category on the Data Saver site-breakdown page This category includes chrome-services and service-worker traffic. This should be fixed at the bottom of the per-site list. BUG=735257 ========== to ========== Add 'Other' category on the Data Saver site-breakdown page This category includes chrome-services and service-worker traffic. This should be fixed at the bottom of the per-site list. BUG=735257 Review-Url: https://codereview.chromium.org/2953523002 Cr-Commit-Position: refs/heads/master@{#483172} Committed: https://chromium.googlesource.com/chromium/src/+/6dc9ace1c175a5747f6656ca543e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/6dc9ace1c175a5747f6656ca543e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
