Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2799083004: Implements ApplyTranslateOptions in compact infobar (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 2 weeks ago by ramyasharma
Modified:
2 months, 1 week ago
Reviewers:
dfalcantara
CC:
chromium-reviews, agrieve+watch_chromium.org, dfalcantara+watch_chromium.org, Marti Wong, Leo
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+69 lines, -10 lines) Patch
M chrome/android/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java View 1 2 3 1 chunk +4 lines, -1 line 0 comments Download
M chrome/browser/translate/android/translate_utils.h View 3 chunks +14 lines, -3 lines 0 comments Download
M chrome/browser/ui/android/infobars/translate_compact_infobar.h View 1 2 3 1 chunk +12 lines, -3 lines 0 comments Download
M chrome/browser/ui/android/infobars/translate_compact_infobar.cc View 1 2 3 4 1 chunk +38 lines, -3 lines 0 comments Download
Trybot results:  ios-simulator   win_clang   win_chromium_compile_dbg_ng   win_chromium_x64_rel_ng   win_chromium_rel_ng   mac_chromium_compile_dbg_ng   mac_chromium_rel_ng   ios-simulator   ios-simulator-xcode-clang   ios-device-xcode-clang   linux_chromium_tsan_rel_ng   ios-device   linux_chromium_compile_dbg_ng   linux_chromium_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_chromeos_ozone_rel_ng   chromium_presubmit   linux_chromium_asan_rel_ng   chromeos_daisy_chromium_compile_only_ng   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   cast_shell_android   linux_android_rel_ng   android_n5x_swarming_rel   android_cronet   android_compile_dbg   android_clang_dbg_recipe   android_arm64_dbg_recipe   android_n5x_swarming_rel   win_clang   win_chromium_x64_rel_ng   win_chromium_compile_dbg_ng   win_chromium_rel_ng   mac_chromium_rel_ng   mac_chromium_compile_dbg_ng   ios-simulator-xcode-clang   ios-simulator   ios-device-xcode-clang   ios-device   linux_chromium_rel_ng   linux_chromium_tsan_rel_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_compile_dbg_ng   chromium_presubmit   linux_chromium_asan_rel_ng   chromeos_daisy_chromium_compile_only_ng   chromeos_amd64-generic_chromium_compile_only_ng   linux_android_rel_ng   cast_shell_linux   android_n5x_swarming_rel   cast_shell_android   android_cronet   android_compile_dbg   android_clang_dbg_recipe   android_arm64_dbg_recipe 
Commit queue not available (can’t edit this change).

Messages

Total messages: 39 (23 generated)
ramyasharma
Hi Dan, PTAL?
2 months, 2 weeks ago (2017-04-11 05:22:00 UTC) #10
dfalcantara
https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java#newcode97 chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:97: long nativeTranslateCompactInfoBar, int option, String value); JNI calls are ...
2 months, 2 weeks ago (2017-04-11 17:32:36 UTC) #11
ramyasharma
Thanks Dan. PTAL? https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java#newcode97 chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:97: long nativeTranslateCompactInfoBar, int option, String value); ...
2 months, 2 weeks ago (2017-04-12 00:36:41 UTC) #12
dfalcantara
https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java#newcode97 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, ...
2 months, 2 weeks ago (2017-04-12 00:39:27 UTC) #13
ramyasharma
On 2017/04/12 00:39:27, overloaded wrote: > https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java > File > chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java > (right): > > ...
2 months, 2 weeks ago (2017-04-12 06:00:40 UTC) #14
ramyasharma
On 2017/04/12 00:39:27, overloaded wrote: > https://codereview.chromium.org/2799083004/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java > File > chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java > (right): > > ...
2 months, 2 weeks ago (2017-04-12 06:00:41 UTC) #15
dfalcantara
Don't have any examples of overloading... don't know if I've ever seen that happen honestly. ...
2 months, 1 week ago (2017-04-12 16:16:08 UTC) #17
ramyasharma
Thanks Dan. PTAL? https://codereview.chromium.org/2799083004/diff/200001/chrome/browser/ui/android/infobars/translate_compact_infobar.cc File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2799083004/diff/200001/chrome/browser/ui/android/infobars/translate_compact_infobar.cc#newcode81 chrome/browser/ui/android/infobars/translate_compact_infobar.cc:81: void TranslateCompactInfoBar::ApplyStringTranslateOptions( On 2017/04/12 16:16:07, overloaded ...
2 months, 1 week ago (2017-04-13 04:04:07 UTC) #23
dfalcantara
lgtm % comment https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/android/infobars/translate_compact_infobar.cc File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/android/infobars/translate_compact_infobar.cc#newcode97 chrome/browser/ui/android/infobars/translate_compact_infobar.cc:97: } else need braces here https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/android/infobars/translate_compact_infobar.cc#newcode116 ...
2 months, 1 week ago (2017-04-13 04:06:21 UTC) #24
ramyasharma
https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/android/infobars/translate_compact_infobar.cc File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/android/infobars/translate_compact_infobar.cc#newcode97 chrome/browser/ui/android/infobars/translate_compact_infobar.cc:97: } else On 2017/04/13 04:06:21, overloaded (dfalcantara) wrote: > ...
2 months, 1 week ago (2017-04-13 04:13:55 UTC) #25
ramyasharma
2 months, 1 week ago (2017-04-13 04:13:59 UTC) #26
dfalcantara
https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/android/infobars/translate_compact_infobar.cc File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/android/infobars/translate_compact_infobar.cc#newcode97 chrome/browser/ui/android/infobars/translate_compact_infobar.cc:97: } else On 2017/04/13 04:13:55, ramyasharma wrote: > On ...
2 months, 1 week ago (2017-04-13 04:16:43 UTC) #27
ramyasharma
https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/android/infobars/translate_compact_infobar.cc File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2799083004/diff/220001/chrome/browser/ui/android/infobars/translate_compact_infobar.cc#newcode97 chrome/browser/ui/android/infobars/translate_compact_infobar.cc:97: } else On 2017/04/13 04:16:43, overloaded (dfalcantara) wrote: > ...
2 months, 1 week ago (2017-04-13 04:24:47 UTC) #29
dfalcantara
still good to go lgtm
2 months, 1 week ago (2017-04-13 04:25:32 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2799083004/240001
2 months, 1 week ago (2017-04-17 23:41:53 UTC) #36
commit-bot: I haz the power
2 months, 1 week ago (2017-04-18 02:29:29 UTC) #39
Message was sent while issue was closed.
Committed patchset #5 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/da825a1f6e9d71bf54d5832ccacb...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318