Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2854243004:
Moves translate snackbar functionality to Android side. (Closed)
|
Created:
3 years, 7 months ago by ramyasharma Modified:
3 years, 7 months ago CC:
chromium-reviews, agrieve+watch_chromium.org, dfalcantara+watch_chromium.org, Leo Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMoves translate snackbar functionality to Android side.
To keep the implementation simple, we are not
going back and forth between C++ and java. Instead
of applying a translate option, and then undoing it
when CANCEL is pressed in the snackbar, we do the
following:
1. When snackbar is dismissed we apply options
and the rest of the flow continues.
2. Otherwise if CANCEL (on snackbar) is pressed,
we don't apply options, and the infobar stays.
Demo: https://drive.google.com/open?id=0B-aCbwkKD2XZcXpteDhTM25WZ2c
BUG=713514
TBR=tedchoc@chromium.org
Review-Url: https://codereview.chromium.org/2854243004
Cr-Commit-Position: refs/heads/master@{#469932}
Committed: https://chromium.googlesource.com/chromium/src/+/3ca1f3e7c2ace2dba216a9f9bd099bcb3fea982c
Patch Set 1 #
Total comments: 8
Patch Set 2 #
Total comments: 4
Patch Set 3 #
Messages
Total messages: 49 (39 generated)
Description was changed from ========== Moves all snackbar functionality to Android side. To keep the implementation simple. BUG=713514 ========== to ========== Moves all snackbar functionality to Android side. To keep the implementation simple, we are not going back and forth between C++ and java. Instead of applying a translate option, and then undoing it when CANCEL is pressed in the snackbar, we do the following: 1. When snackbar is dismissed we apply options and the rest of the flow continues. 2. Otherwise if CANCEL (on snackbar) is pressed, we don't apply options, and the infobar stays. BUG=713514 ==========
Description was changed from ========== Moves all snackbar functionality to Android side. To keep the implementation simple, we are not going back and forth between C++ and java. Instead of applying a translate option, and then undoing it when CANCEL is pressed in the snackbar, we do the following: 1. When snackbar is dismissed we apply options and the rest of the flow continues. 2. Otherwise if CANCEL (on snackbar) is pressed, we don't apply options, and the infobar stays. BUG=713514 ========== to ========== Moves all snackbar functionality to Android side. To keep the implementation simple, we are not going back and forth between C++ and java. Instead of applying a translate option, and then undoing it when CANCEL is pressed in the snackbar, we do the following: 1. When snackbar is dismissed we apply options and the rest of the flow continues. 2. Otherwise if CANCEL (on snackbar) is pressed, we don't apply options, and the infobar stays. Demo: https://drive.google.com/open?id=0B-aCbwkKD2XZcXpteDhTM25WZ2c BUG=713514 ==========
Description was changed from ========== Moves all snackbar functionality to Android side. To keep the implementation simple, we are not going back and forth between C++ and java. Instead of applying a translate option, and then undoing it when CANCEL is pressed in the snackbar, we do the following: 1. When snackbar is dismissed we apply options and the rest of the flow continues. 2. Otherwise if CANCEL (on snackbar) is pressed, we don't apply options, and the infobar stays. Demo: https://drive.google.com/open?id=0B-aCbwkKD2XZcXpteDhTM25WZ2c BUG=713514 ========== to ========== Moves translate snackbar functionality to Android side. To keep the implementation simple, we are not going back and forth between C++ and java. Instead of applying a translate option, and then undoing it when CANCEL is pressed in the snackbar, we do the following: 1. When snackbar is dismissed we apply options and the rest of the flow continues. 2. Otherwise if CANCEL (on snackbar) is pressed, we don't apply options, and the infobar stays. Demo: https://drive.google.com/open?id=0B-aCbwkKD2XZcXpteDhTM25WZ2c BUG=713514 ==========
Patchset #1 (id:1) has been deleted
ramyasharma@chromium.org changed reviewers: + dfalcantara@chromium.org
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2854243004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2854243004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:47: ; 1) git cl format? This ; on its own line is weird. 2) Don't use enums; they're bloated in Java. Define private static final ints like here: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2854243004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:49: class TranslateSnackbarController implements SnackbarController { javadoc https://codereview.chromium.org/2854243004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:50: private TranslateSnackbarType mSnackbarType; final https://codereview.chromium.org/2854243004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:53: this.snackbarType = snackbarType; Does this compile?
Patchset #2 (id:40001) has been deleted
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...
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Thanks Dan. PTAL? https://codereview.chromium.org/2854243004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2854243004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:47: ; On 2017/05/04 17:20:52, slow (dfalcantara) wrote: > 1) git cl format? This ; on its own line is weird. > > 2) Don't use enums; they're bloated in Java. Define private static final ints > like here: > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Yes, git cl format put that ";" on the next line :-/. Getting rid of the enum. thanks. https://codereview.chromium.org/2854243004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:49: class TranslateSnackbarController implements SnackbarController { On 2017/05/04 17:20:52, slow (dfalcantara) wrote: > javadoc done. https://codereview.chromium.org/2854243004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:50: private TranslateSnackbarType mSnackbarType; On 2017/05/04 17:20:52, slow (dfalcantara) wrote: > final Done. https://codereview.chromium.org/2854243004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:53: this.snackbarType = snackbarType; On 2017/05/04 17:20:52, slow (dfalcantara) wrote: > Does this compile? argh.. sorry. No it doesn't. I missed updating other places. Have made some more changes. Not defining my constants for snackbar type, instead using the existing menu item.
martiw@chromium.org changed reviewers: + martiw@chromium.org
https://codereview.chromium.org/2854243004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2854243004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:61: // Do nothing. Should we add a TODO here for logging the "Cancel" event in snackbars?
lgtm https://codereview.chromium.org/2854243004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2854243004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:51: this.mMenuItemId = menuItemId; don't need the "this." bit; you generally only need it if you're defining some public member field that's using the same name as the argument you're passing into the constructor
https://codereview.chromium.org/2854243004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2854243004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:51: this.mMenuItemId = menuItemId; On 2017/05/05 17:23:52, slow (dfalcantara) wrote: > don't need the "this." bit; you generally only need it if you're defining some > public member field that's using the same name as the argument you're passing > into the constructor Done. https://codereview.chromium.org/2854243004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:61: // Do nothing. On 2017/05/05 06:57:46, Marti Wong wrote: > Should we add a TODO here for logging the "Cancel" event in snackbars? Thanks Marti. Yes, we should probably add a logging metric here to track cancel events. Done.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
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_...)
Patchset #3 (id:180001) has been deleted
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ramyasharma@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2854243004/#ps200001 (title: "")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Moves translate snackbar functionality to Android side. To keep the implementation simple, we are not going back and forth between C++ and java. Instead of applying a translate option, and then undoing it when CANCEL is pressed in the snackbar, we do the following: 1. When snackbar is dismissed we apply options and the rest of the flow continues. 2. Otherwise if CANCEL (on snackbar) is pressed, we don't apply options, and the infobar stays. Demo: https://drive.google.com/open?id=0B-aCbwkKD2XZcXpteDhTM25WZ2c BUG=713514 ========== to ========== Moves translate snackbar functionality to Android side. To keep the implementation simple, we are not going back and forth between C++ and java. Instead of applying a translate option, and then undoing it when CANCEL is pressed in the snackbar, we do the following: 1. When snackbar is dismissed we apply options and the rest of the flow continues. 2. Otherwise if CANCEL (on snackbar) is pressed, we don't apply options, and the infobar stays. Demo: https://drive.google.com/open?id=0B-aCbwkKD2XZcXpteDhTM25WZ2c BUG=713514 TBR=groby@chromium.org TBR=tedchoc@chromium.org ==========
Description was changed from ========== Moves translate snackbar functionality to Android side. To keep the implementation simple, we are not going back and forth between C++ and java. Instead of applying a translate option, and then undoing it when CANCEL is pressed in the snackbar, we do the following: 1. When snackbar is dismissed we apply options and the rest of the flow continues. 2. Otherwise if CANCEL (on snackbar) is pressed, we don't apply options, and the infobar stays. Demo: https://drive.google.com/open?id=0B-aCbwkKD2XZcXpteDhTM25WZ2c BUG=713514 TBR=groby@chromium.org TBR=tedchoc@chromium.org ========== to ========== Moves translate snackbar functionality to Android side. To keep the implementation simple, we are not going back and forth between C++ and java. Instead of applying a translate option, and then undoing it when CANCEL is pressed in the snackbar, we do the following: 1. When snackbar is dismissed we apply options and the rest of the flow continues. 2. Otherwise if CANCEL (on snackbar) is pressed, we don't apply options, and the infobar stays. Demo: https://drive.google.com/open?id=0B-aCbwkKD2XZcXpteDhTM25WZ2c BUG=713514 TBR=tedchoc@chromium.org ==========
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": 200001, "attempt_start_ts": 1494223486090180,
"parent_rev": "9499e291d08f34855a3c849f5b3e48ab62a9910b", "commit_rev":
"3ca1f3e7c2ace2dba216a9f9bd099bcb3fea982c"}
Message was sent while issue was closed.
Description was changed from ========== Moves translate snackbar functionality to Android side. To keep the implementation simple, we are not going back and forth between C++ and java. Instead of applying a translate option, and then undoing it when CANCEL is pressed in the snackbar, we do the following: 1. When snackbar is dismissed we apply options and the rest of the flow continues. 2. Otherwise if CANCEL (on snackbar) is pressed, we don't apply options, and the infobar stays. Demo: https://drive.google.com/open?id=0B-aCbwkKD2XZcXpteDhTM25WZ2c BUG=713514 TBR=tedchoc@chromium.org ========== to ========== Moves translate snackbar functionality to Android side. To keep the implementation simple, we are not going back and forth between C++ and java. Instead of applying a translate option, and then undoing it when CANCEL is pressed in the snackbar, we do the following: 1. When snackbar is dismissed we apply options and the rest of the flow continues. 2. Otherwise if CANCEL (on snackbar) is pressed, we don't apply options, and the infobar stays. Demo: https://drive.google.com/open?id=0B-aCbwkKD2XZcXpteDhTM25WZ2c BUG=713514 TBR=tedchoc@chromium.org Review-Url: https://codereview.chromium.org/2854243004 Cr-Commit-Position: refs/heads/master@{#469932} Committed: https://chromium.googlesource.com/chromium/src/+/3ca1f3e7c2ace2dba216a9f9bd09... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:200001) as https://chromium.googlesource.com/chromium/src/+/3ca1f3e7c2ace2dba216a9f9bd09... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
