Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2799083004:
Implements ApplyTranslateOptions in compact infobar (Closed)
|
Created:
3 years, 8 months ago by ramyasharma Modified:
3 years, 8 months ago Reviewers:
gone CC:
chromium-reviews, agrieve+watch_chromium.org, dfalcantara+watch_chromium.org, Marti Wong, Leo Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplements ApplyTranslateOptions in compact infobar
Introduces a new enum to capture all possible data
that java frontend will save on the native side.
And uses this enum, as a parameter passed
to ApplyStringTranslateOptions and
ApplyBoolTranslateOptions, along with it's value.
Value is boolean or string based on the method
called.
BUG=705310
TBR=groby@chromium.org
Review-Url: https://codereview.chromium.org/2799083004
Cr-Commit-Position: refs/heads/master@{#465102}
Committed: https://chromium.googlesource.com/chromium/src/+/da825a1f6e9d71bf54d5832ccacb3b718eaa6f81
Patch Set 1 : a #
Total comments: 7
Patch Set 2 : a #Patch Set 3 : a #
Total comments: 8
Patch Set 4 : with review comments #
Total comments: 6
Patch Set 5 : Review comments addressed #
Messages
Total messages: 39 (23 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Description was changed from ========== apply options changes BUG=705310 ========== to ========== Implements ApplyTranslateOptions in compact infobar Introduces a new enum to capture all possible data that java frontend will save on the native side. And uses this enum, as a parameter passed to ApplyTranslateOptions, along with it's value. Value is always of type String, which is then used as Bool/String based on the expected value for the given enum. BUG=705310 ==========
ramyasharma@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi Dan, PTAL?
https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:97: long nativeTranslateCompactInfoBar, int option, String value); JNI calls are expensive -- especially if you're passing strings. 1) Can you just pass five arguments all at once? 2) Why do you need a string? You can just pass booleans and ints (which get converted into jbooleans and jints). https://codereview.chromium.org/2799083004/diff/140001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2799083004/diff/140001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:87: if (option == TranslateUtils::OPTION_SOURCE_CODE) { These should be else ifs. https://codereview.chromium.org/2799083004/diff/140001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:122: const std::string kTrue = "true"; you shouldn't be needing this function after changing the function signature, but constants like these are defined at the top in some anonymous namespace: e.g. https://cs.chromium.org/chromium/src/ui/gfx/icon_util.cc?q=%22const+int%22+pa...
Thanks Dan. PTAL? https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:97: long nativeTranslateCompactInfoBar, int option, String value); On 2017/04/11 17:32:36, overloaded wrote: > JNI calls are expensive -- especially if you're passing strings. > > 1) Can you just pass five arguments all at once? > 2) Why do you need a string? You can just pass booleans and ints (which get > converted into jbooleans and jints). 1. In the current code, 5 parameters are passed in the ApplyTranslateOptions. The reason we are getting rid of that here are: a. We don't need to pass all 5 at once in the new design, user can only change one parameter at a time, and passing the other 4 seems like an unnecessary overhead. b. Having enum, and value format gives us the flexibility to add more parameters in future without changing the API. 2. The reason for picking string, is because it covers all cases of bool, int, string well. I understand there is a trade-off between passing boolean as a string vs passing n number of parameters every time. And I prefer the single parameter way, because it also provides a more stable API. WDYT? https://codereview.chromium.org/2799083004/diff/140001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2799083004/diff/140001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:87: if (option == TranslateUtils::OPTION_SOURCE_CODE) { On 2017/04/11 17:32:36, overloaded wrote: > These should be else ifs. thanks. done. https://codereview.chromium.org/2799083004/diff/140001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:122: const std::string kTrue = "true"; On 2017/04/11 17:32:36, overloaded wrote: > you shouldn't be needing this function after changing the function signature, > but constants like these are defined at the top in some anonymous namespace: > > e.g. > https://cs.chromium.org/chromium/src/ui/gfx/icon_util.cc?q=%22const+int%22+pa... Moved the const the anonymous namespace. Thanks.
https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:97: long nativeTranslateCompactInfoBar, int option, String value); On 2017/04/12 00:36:40, ramyasharma wrote: > On 2017/04/11 17:32:36, overloaded wrote: > > JNI calls are expensive -- especially if you're passing strings. > > > > 1) Can you just pass five arguments all at once? > > 2) Why do you need a string? You can just pass booleans and ints (which get > > converted into jbooleans and jints). > > 1. In the current code, 5 parameters are passed in the ApplyTranslateOptions. > The reason we are getting rid of that here are: > a. We don't need to pass all 5 at once in the new design, user can only change > one parameter at a time, and passing the other 4 seems like an unnecessary > overhead. > b. Having enum, and value format gives us the flexibility to add more parameters > in future without changing the API. > > 2. The reason for picking string, is because it covers all cases of bool, int, > string well. > > I understand there is a trade-off between passing boolean as a string vs passing > n number of parameters every time. And I prefer the single parameter way, > because it also provides a more stable API. WDYT? I'm fine with a single parameter thing, but you should have two different apply functions in that case: one for booleans and one for ints. Passing Strings as a generic parsed container is expensive.
On 2017/04/12 00:39:27, overloaded wrote: > https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java > (right): > > https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:97: > long nativeTranslateCompactInfoBar, int option, String value); > On 2017/04/12 00:36:40, ramyasharma wrote: > > On 2017/04/11 17:32:36, overloaded wrote: > > > JNI calls are expensive -- especially if you're passing strings. > > > > > > 1) Can you just pass five arguments all at once? > > > 2) Why do you need a string? You can just pass booleans and ints (which get > > > converted into jbooleans and jints). > > > > 1. In the current code, 5 parameters are passed in the ApplyTranslateOptions. > > The reason we are getting rid of that here are: > > a. We don't need to pass all 5 at once in the new design, user can only change > > one parameter at a time, and passing the other 4 seems like an unnecessary > > overhead. > > b. Having enum, and value format gives us the flexibility to add more > parameters > > in future without changing the API. > > > > 2. The reason for picking string, is because it covers all cases of bool, int, > > string well. > > > > I understand there is a trade-off between passing boolean as a string vs > passing > > n number of parameters every time. And I prefer the single parameter way, > > because it also provides a more stable API. WDYT? > > I'm fine with a single parameter thing, but you should have two different apply > functions in that case: one for booleans and one for ints. Passing Strings as a > generic parsed container is expensive. Thanks Dan. I have introduced two different methods, one with String param and another with bool. There are no int params currently. However, I was hoping to overload ApplyTranslateOptions, with bool / string params. But I was unable to do this, and got the following error: overloaded function with no contextual type information. Do you have an example of method overloading in JNI?
On 2017/04/12 00:39:27, overloaded wrote: > https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java > (right): > > https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:97: > long nativeTranslateCompactInfoBar, int option, String value); > On 2017/04/12 00:36:40, ramyasharma wrote: > > On 2017/04/11 17:32:36, overloaded wrote: > > > JNI calls are expensive -- especially if you're passing strings. > > > > > > 1) Can you just pass five arguments all at once? > > > 2) Why do you need a string? You can just pass booleans and ints (which get > > > converted into jbooleans and jints). > > > > 1. In the current code, 5 parameters are passed in the ApplyTranslateOptions. > > The reason we are getting rid of that here are: > > a. We don't need to pass all 5 at once in the new design, user can only change > > one parameter at a time, and passing the other 4 seems like an unnecessary > > overhead. > > b. Having enum, and value format gives us the flexibility to add more > parameters > > in future without changing the API. > > > > 2. The reason for picking string, is because it covers all cases of bool, int, > > string well. > > > > I understand there is a trade-off between passing boolean as a string vs > passing > > n number of parameters every time. And I prefer the single parameter way, > > because it also provides a more stable API. WDYT? > > I'm fine with a single parameter thing, but you should have two different apply > functions in that case: one for booleans and one for ints. Passing Strings as a > generic parsed container is expensive. Thanks Dan. I have introduced two different methods, one with String param and another with bool. There are no int params currently. However, I was hoping to overload ApplyTranslateOptions, with bool / string params. But I was unable to do this, and got the following error: overloaded function with no contextual type information. Do you have an example of method overloading in JNI?
Patchset #3 (id:180001) has been deleted
Don't have any examples of overloading... don't know if I've ever seen that happen honestly. Calling it two different things is clearer, at least. 1) Your CL message is out of date. 2) You seem to keep setting your patch set description as "a" when you upload. You generally should describe what the new upload was for, or just enter an empty string. https://codereview.chromium.org/2799083004/diff/200001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2799083004/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:81: void TranslateCompactInfoBar::ApplyStringTranslateOptions( You're only applying one option. Don't pluralize the function name. https://codereview.chromium.org/2799083004/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:97: } DCHECK in the else clause to prevent this function from being used with other options. https://codereview.chromium.org/2799083004/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:104: bool value) { I think this is supposed to be a jboolean, for consistency with other JNI conventions: https://cs.chromium.org/chromium/src/ui/android/event_forwarder.cc?q=jboolean... https://codereview.chromium.org/2799083004/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:115: } Ditto about else clause.
Description was changed from ========== Implements ApplyTranslateOptions in compact infobar Introduces a new enum to capture all possible data that java frontend will save on the native side. And uses this enum, as a parameter passed to ApplyTranslateOptions, along with it's value. Value is always of type String, which is then used as Bool/String based on the expected value for the given enum. BUG=705310 ========== to ========== Implements ApplyTranslateOptions in compact infobar Introduces a new enum to capture all possible data that java frontend will save on the native side. And uses this enum, as a parameter passed to ApplyStringTranslateOptions and ApplyBoolTranslateOptions, along with it's value. Value is boolean or string based on the method called. BUG=705310 ==========
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks Dan. PTAL? https://codereview.chromium.org/2799083004/diff/200001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2799083004/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:81: void TranslateCompactInfoBar::ApplyStringTranslateOptions( On 2017/04/12 16:16:07, overloaded (dfalcantara) wrote: > You're only applying one option. Don't pluralize the function name. Done. https://codereview.chromium.org/2799083004/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:97: } On 2017/04/12 16:16:08, overloaded (dfalcantara) wrote: > DCHECK in the else clause to prevent this function from being used with other > options. Done. https://codereview.chromium.org/2799083004/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:104: bool value) { On 2017/04/12 16:16:08, overloaded (dfalcantara) wrote: > I think this is supposed to be a jboolean, for consistency with other JNI > conventions: > > https://cs.chromium.org/chromium/src/ui/android/event_forwarder.cc?q=jboolean... Done. https://codereview.chromium.org/2799083004/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:115: } On 2017/04/12 16:16:08, overloaded (dfalcantara) wrote: > Ditto about else clause. Thanks good idea. Done.
lgtm % comment https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:97: } else need braces here https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:116: } else ditto
https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:97: } else On 2017/04/13 04:06:21, overloaded (dfalcantara) wrote: > need braces here I totally agree with the braces, but in this file (and others that I have seen), no braces are added for 1 line blocks. And I was asked to be consistent with that, in previous reviews. Should I still add the braces? https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:116: } else On 2017/04/13 04:06:21, overloaded (dfalcantara) wrote: > ditto Acknowledged.
https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:97: } else On 2017/04/13 04:13:55, ramyasharma wrote: > On 2017/04/13 04:06:21, overloaded (dfalcantara) wrote: > > need braces here > > I totally agree with the braces, but in this file (and others that I have seen), > no braces are added for 1 line blocks. And I was asked to be consistent with > that, in previous reviews. > > Should I still add the braces? https://google.github.io/styleguide/cppguide.html#Conditionals If all the clauses didn't need braces then it's fine, but you're mixing here.
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:97: } else On 2017/04/13 04:16:43, overloaded (dfalcantara) wrote: > On 2017/04/13 04:13:55, ramyasharma wrote: > > On 2017/04/13 04:06:21, overloaded (dfalcantara) wrote: > > > need braces here > > > > I totally agree with the braces, but in this file (and others that I have > seen), > > no braces are added for 1 line blocks. And I was asked to be consistent with > > that, in previous reviews. > > > > Should I still add the braces? > > https://google.github.io/styleguide/cppguide.html#Conditionals > > If all the clauses didn't need braces then it's fine, but you're mixing here. ah thanks for clearing that up for me Dan. Done
Description was changed from ========== Implements ApplyTranslateOptions in compact infobar Introduces a new enum to capture all possible data that java frontend will save on the native side. And uses this enum, as a parameter passed to ApplyStringTranslateOptions and ApplyBoolTranslateOptions, along with it's value. Value is boolean or string based on the method called. BUG=705310 ========== to ========== Implements ApplyTranslateOptions in compact infobar Introduces a new enum to capture all possible data that java frontend will save on the native side. And uses this enum, as a parameter passed to ApplyStringTranslateOptions and ApplyBoolTranslateOptions, along with it's value. Value is boolean or string based on the method called. BUG=705310 TBR=groby@chromium.org ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still good to go lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ramyasharma@chromium.org
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": 240001, "attempt_start_ts": 1492472482320040,
"parent_rev": "f1cda031a84bc693c6ceec4b3edb788443c29586", "commit_rev":
"da825a1f6e9d71bf54d5832ccacb3b718eaa6f81"}
Message was sent while issue was closed.
Description was changed from ========== Implements ApplyTranslateOptions in compact infobar Introduces a new enum to capture all possible data that java frontend will save on the native side. And uses this enum, as a parameter passed to ApplyStringTranslateOptions and ApplyBoolTranslateOptions, along with it's value. Value is boolean or string based on the method called. BUG=705310 TBR=groby@chromium.org ========== to ========== Implements ApplyTranslateOptions in compact infobar Introduces a new enum to capture all possible data that java frontend will save on the native side. And uses this enum, as a parameter passed to ApplyStringTranslateOptions and ApplyBoolTranslateOptions, along with it's value. Value is boolean or string based on the method called. BUG=705310 TBR=groby@chromium.org Review-Url: https://codereview.chromium.org/2799083004 Cr-Commit-Position: refs/heads/master@{#465102} Committed: https://chromium.googlesource.com/chromium/src/+/da825a1f6e9d71bf54d5832ccacb... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:240001) as https://chromium.googlesource.com/chromium/src/+/da825a1f6e9d71bf54d5832ccacb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
