Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2838833003:
Introduces snackbar enum, & other snackbar classes on android for translate (Closed)
|
Created:
3 years, 8 months ago by ramyasharma Modified:
3 years, 7 months ago Reviewers:
gone CC:
chromium-reviews, agrieve+watch_chromium.org, dfalcantara+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduces snackbar enum, & other snackbar classes on android for translate user actions.
CL#1 This snackbar will be used to convey to the user the result
of any action performed on the Translate infobar.
BUG=713514
TBR=groby@chromium.org
Review-Url: https://codereview.chromium.org/2838833003
Cr-Commit-Position: refs/heads/master@{#468302}
Committed: https://chromium.googlesource.com/chromium/src/+/7111808ad97f5eea6a9701132a24a98406a26806
Patch Set 1 #
Total comments: 16
Patch Set 2 #
Total comments: 4
Patch Set 3 #
Total comments: 2
Patch Set 4 #
Total comments: 3
Patch Set 5 #
Messages
Total messages: 60 (45 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) 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 #1 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Patchset #1 (id:80001) has been deleted
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 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 #1 (id:100001) has been deleted
ramyasharma@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi Dan, This CL only introduces the snackbar classses, I have not used them in TranslateCompactInfobar. The next CL will get the snackbar manager in TranslateCompactInfobar.java and show this snackbar. PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Description was changed from ========== Introduces snackbar classes on native and java side. This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 ========== to ========== Introduces snackbar classes on native and java side. CL#1This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 ==========
Description was changed from ========== Introduces snackbar classes on native and java side. CL#1This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 ========== to ========== Introduces snackbar classes on native and java side. CL#1 This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 ==========
https://codereview.chromium.org/2838833003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java (right): https://codereview.chromium.org/2838833003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:147: protected boolean closeInfoBar() { I'm really wary of exposing this method. It allows anyone to close the infobar incorrectly. Can you: 1) Add a protected function called closeInfoBarInternal() that you override. 2) Make closeInfoBar() call it. 3) Mark closeInfoBar() as final https://codereview.chromium.org/2838833003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateSnackbarController.java (right): https://codereview.chromium.org/2838833003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateSnackbarController.java:9: class TranslateSnackbarController implements SnackbarController { class javadoc https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/infobar_android.h (right): https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/infobar_android.h:58: virtual void CloseJavaInfoBar(); Yeah, I'm really wary of you overriding this one. https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:85: JNIEnv* env = base::android::AttachCurrentThread(); I'd make the Java side closeInfoBarInternal call into native when it gets called to create your TranslateSnackbar. Extra JNI hop, but it avoids divergence. https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:124: TranslateCompactInfoBar::CloseJavaInfoBar(); You shouldn't need to scope TranslateCompactInfoBar. You're already in it. https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.h (right): https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.h:56: int snackbar_type; C++ member fields always end with _. https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/translate_snackbar.cc (right): https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/translate_snackbar.cc:17: // TODO(ramyasharma): Implement. You don't actually need to create this object, do you? This could just be a static function that gets passed the snackbar_type from the java side when it needs to do whatever it needs to do. https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/translate_snackbar.h (right): https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/translate_snackbar.h:22: int type; member fields end with _. https://google.github.io/styleguide/cppguide.html#Variable_Names
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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Patchset #2 (id:140001) 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...
Thanks Dan. PTAL? https://codereview.chromium.org/2838833003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java (right): https://codereview.chromium.org/2838833003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:147: protected boolean closeInfoBar() { On 2017/04/26 17:19:51, slow (dfalcantara) wrote: > I'm really wary of exposing this method. It allows anyone to close the infobar > incorrectly. Can you: > > 1) Add a protected function called closeInfoBarInternal() that you override. > 2) Make closeInfoBar() call it. > 3) Mark closeInfoBar() as final Thank Dan. But I dont need this anymore, I dont have to call close infobar explicitly. https://codereview.chromium.org/2838833003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateSnackbarController.java (right): https://codereview.chromium.org/2838833003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateSnackbarController.java:9: class TranslateSnackbarController implements SnackbarController { On 2017/04/26 17:19:51, slow (dfalcantara) wrote: > class javadoc Done. https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/infobar_android.h (right): https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/infobar_android.h:58: virtual void CloseJavaInfoBar(); On 2017/04/26 17:19:51, slow (dfalcantara) wrote: > Yeah, I'm really wary of you overriding this one. Thanks for tips on a different approach. But this is not used anymore. https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:85: JNIEnv* env = base::android::AttachCurrentThread(); On 2017/04/26 17:19:51, slow (dfalcantara) wrote: > I'd make the Java side closeInfoBarInternal call into native when it gets called > to create your TranslateSnackbar. Extra JNI hop, but it avoids divergence. Thanks. Not using this approach anymore. https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:124: TranslateCompactInfoBar::CloseJavaInfoBar(); On 2017/04/26 17:19:51, slow (dfalcantara) wrote: > You shouldn't need to scope TranslateCompactInfoBar. You're already in it. Done. https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.h (right): https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.h:56: int snackbar_type; On 2017/04/26 17:19:51, slow (dfalcantara) wrote: > C++ member fields always end with _. Done. https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/translate_snackbar.cc (right): https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/translate_snackbar.cc:17: // TODO(ramyasharma): Implement. On 2017/04/26 17:19:51, slow (dfalcantara) wrote: > You don't actually need to create this object, do you? This could just be a > static function that gets passed the snackbar_type from the java side when it > needs to do whatever it needs to do. That is correct as of now. However, hopefully in the next couple of CLs I will be adding the translate delegate as a member function, which will be set in the constructor. Hence following this approach. https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/translate_snackbar.h (right): https://codereview.chromium.org/2838833003/diff/120001/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/translate_snackbar.h:22: int type; On 2017/04/26 17:19:51, slow (dfalcantara) wrote: > member fields end with _. > https://google.github.io/styleguide/cppguide.html#Variable_Names Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
https://codereview.chromium.org/2838833003/diff/160001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2838833003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:85: if (GetJavaInfoBar() != nullptr) { early return like line 62. if (!GetJavaInfoBar()) return; https://codereview.chromium.org/2838833003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:87: TranslateSnackbar* snackbar = new TranslateSnackbar(snackbar_type_); This is a memory leak; you never delete it.
Thanks Dan. PTAL? https://codereview.chromium.org/2838833003/diff/160001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2838833003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:85: if (GetJavaInfoBar() != nullptr) { On 2017/04/27 18:42:45, slow (dfalcantara) wrote: > early return like line 62. > > if (!GetJavaInfoBar()) > return; Done. https://codereview.chromium.org/2838833003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:87: TranslateSnackbar* snackbar = new TranslateSnackbar(snackbar_type_); On 2017/04/27 18:42:45, slow (dfalcantara) wrote: > This is a memory leak; you never delete it. Oh, thanks . Done.
https://codereview.chromium.org/2838833003/diff/180001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2838833003/diff/180001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:91: reinterpret_cast<intptr_t>(&snackbar)); I'm not sure how this works. It's a pointer to an object on the stack, which gets nuked when the function returns. That means the Java side is pointing at something that's already gone when it eventually uses it. I don't think it'll get culled if it's a member variable of the TranslateCompactInfoBar, but you'll need to test that.
Patchset #4 (id:200001) has been deleted
https://codereview.chromium.org/2838833003/diff/180001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2838833003/diff/180001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:91: reinterpret_cast<intptr_t>(&snackbar)); On 2017/04/28 00:48:11, slow (dfalcantara) wrote: > I'm not sure how this works. It's a pointer to an object on the stack, which > gets nuked when the function returns. That means the Java side is pointing at > something that's already gone when it eventually uses it. > > I don't think it'll get culled if it's a member variable of the > TranslateCompactInfoBar, but you'll need to test that. Thanks Dan. Good point. It does go out of scope, but I don't think keeping it as a member variable will solve my problem, as I need this to be alive beyond the scope of the infobar. So now, I have taken your previous suggestion to convert this to a static function, and tackle the translate delegate issue in the next CL.
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2838833003/diff/180002/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/translate_snackbar.h (right): https://codereview.chromium.org/2838833003/diff/180002/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/translate_snackbar.h:14: class TranslateSnackbar { You don't actually need this class for this CL, do you? Plop it in when you actually need it, since its functions aren't even defined in your CC file.
Description was changed from ========== Introduces snackbar classes on native and java side. CL#1 This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 ========== to ========== Introduces snackbar enum, and hooks in android side to show snackbar. CL#1 This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 ==========
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...
Description was changed from ========== Introduces snackbar enum, and hooks in android side to show snackbar. CL#1 This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 ========== to ========== Introduces snackbar enum, and other snackbar classes on android side to show snackbar. CL#1 This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 ==========
Thanks Dan. PTAL? https://codereview.chromium.org/2838833003/diff/180002/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/translate_snackbar.h (right): https://codereview.chromium.org/2838833003/diff/180002/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/translate_snackbar.h:14: class TranslateSnackbar { On 2017/04/30 02:09:44, slow (dfalcantara) wrote: > You don't actually need this class for this CL, do you? Plop it in when you > actually need it, since its functions aren't even defined in your CC file. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2838833003/diff/180002/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/translate_snackbar.h (right): https://codereview.chromium.org/2838833003/diff/180002/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/translate_snackbar.h:14: class TranslateSnackbar { On 2017/05/01 01:42:39, ramyasharma wrote: > On 2017/04/30 02:09:44, slow (dfalcantara) wrote: > > You don't actually need this class for this CL, do you? Plop it in when you > > actually need it, since its functions aren't even defined in your CC file. > > Done. I meant that you didn't actually construct any instances of this class; you just defined it but didn't implement it anywhere. I just wanted to see if the whole CL would still compile if you deleted just this "class TranslateSnackbar" block.
Thanks Dan. Yes, I wasn't constructing an instance of this class. I have moved these changes to the next CL, and kept java side changes in this CL.
lgtm
Er, lgtm % updating the CL description.
Description was changed from ========== Introduces snackbar enum, and other snackbar classes on android side to show snackbar. CL#1 This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 ========== to ========== Introduces snackbar enum, and other snackbar classes on android side to show a snackbar on translate user actions. CL#1 This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 ==========
Description was changed from ========== Introduces snackbar enum, and other snackbar classes on android side to show a snackbar on translate user actions. CL#1 This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 ========== to ========== Introduces snackbar enum, & other snackbar classes on android for translate user actions. CL#1 This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 ==========
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...
Description was changed from ========== Introduces snackbar enum, & other snackbar classes on android for translate user actions. CL#1 This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 ========== to ========== Introduces snackbar enum, & other snackbar classes on android for translate user actions. CL#1 This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 TBR=groby@chromium.org ==========
CQ is committing da patch.
Bot data: {"patchset_id": 230001, "attempt_start_ts": 1493622003633000,
"parent_rev": "71f3bd1682b75efac412a7431adae78afb8a903b", "commit_rev":
"7111808ad97f5eea6a9701132a24a98406a26806"}
Message was sent while issue was closed.
Description was changed from ========== Introduces snackbar enum, & other snackbar classes on android for translate user actions. CL#1 This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 TBR=groby@chromium.org ========== to ========== Introduces snackbar enum, & other snackbar classes on android for translate user actions. CL#1 This snackbar will be used to convey to the user the result of any action performed on the Translate infobar. BUG=713514 TBR=groby@chromium.org Review-Url: https://codereview.chromium.org/2838833003 Cr-Commit-Position: refs/heads/master@{#468302} Committed: https://chromium.googlesource.com/chromium/src/+/7111808ad97f5eea6a9701132a24... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:230001) as https://chromium.googlesource.com/chromium/src/+/7111808ad97f5eea6a9701132a24... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
