Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2762393003:
Creates translate_compact_infobar class, which holds the logic for the new translate UI (Closed)
|
Created:
3 years, 9 months ago by ramyasharma Modified:
3 years, 9 months ago CC:
chromium-reviews, Marti Wong, Leo Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreates translate_compact_infobar class, which holds the logic for the new translate UI.
This translate_compact_infobar is created in the native code, and calls the
new Translate infobar in Android UI.
BUG=705310
TBR=groby@chromium.org
Review-Url: https://codereview.chromium.org/2762393003
Cr-Commit-Position: refs/heads/master@{#460002}
Committed: https://chromium.googlesource.com/chromium/src/+/5f3bcbb0eae7cb2aaf21bd7cc025c86a82d595d4
Patch Set 1 : a #
Total comments: 19
Patch Set 2 : a #Patch Set 3 : a #Messages
Total messages: 42 (33 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (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 #1 (id:60001) has been deleted
Description was changed from ========== a a BUG= ========== to ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Description was changed from ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI. ========== to ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI in the native code, and calls the new Translate infobar in Android UI. ==========
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...
ramyasharma@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi Dan, This CL refactors translate_infobar.class into 2 classes, one for the new translate infobar, and one for the legacy infobar. This is just to keep the code clean and be able to experiment in the new infobar without breaking anything in the existing one.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Code review change descriptions should include a BUG= line to indicate what bug this corresponds to. Bots scrape that to know when to email people on the bug, and also makes it easier to discuss things that don't fit in code review settings. Is there a bug for this? https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.h (right): https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.h:32: void PassJavaInfoBar(InfoBarAndroid* source) override; You shouldn't have to implement this for your new pathway. This is only a thing needed by the original TranslateInfoBar because it replaced itself in-place. https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.h:37: translate::TranslateStep new_type); Ditto here. https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.h:38: bool ShouldDisplayNeverTranslateInfoBarOnCancel(); Also here. AFAICT from the mocks you don't ever display a never bar. https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_infobar.cc (right): https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_infobar.cc:54: TranslateUtils::GetLanguageCode(env, delegate->original_language_code()); Why does this need to be pulled out? It's a single line call that's pretty standard for converting strings. https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_utils.cc (right): https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_utils.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_utils.cc:5: #include "chrome/browser/ui/android/infobars/translate_utils.h" This should be taken out of the ui directory. Maybe try chrome/browser/translate/android. https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_utils.h (right): https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_utils.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2017.
Description was changed from ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI in the native code, and calls the new Translate infobar in Android UI. ========== to ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI in the native code, and calls the new Translate infobar in Android UI. 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...
Thanks Dan. Added a bug to the CL, and addressed your comments. What's the policy with tests? I don't see any existing tests in any of these folders. https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2017/03/24 20:34:30, dfalcantara (load balance plz) wrote: > 2017 Done. https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.h (right): https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2017/03/24 20:34:30, dfalcantara (load balance plz) wrote: > 2017 Done. https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.h:32: void PassJavaInfoBar(InfoBarAndroid* source) override; On 2017/03/24 20:34:30, dfalcantara (load balance plz) wrote: > You shouldn't have to implement this for your new pathway. This is only a thing > needed by the original TranslateInfoBar because it replaced itself in-place. Done. https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.h:37: translate::TranslateStep new_type); On 2017/03/24 20:34:30, dfalcantara (load balance plz) wrote: > Ditto here. Done. https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.h:38: bool ShouldDisplayNeverTranslateInfoBarOnCancel(); On 2017/03/24 20:34:30, dfalcantara (load balance plz) wrote: > Also here. AFAICT from the mocks you don't ever display a never bar. Yes, we have a toast for never/always cases. Wondering if that is the same as the existing one, and should be hooked here? I have tagged you in the mocks for this. https://folio.googleplex.com/chrome-ux/mocks/421-easier-translate/2017%2002%2... https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_infobar.cc (right): https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_infobar.cc:54: TranslateUtils::GetLanguageCode(env, delegate->original_language_code()); On 2017/03/24 20:34:30, dfalcantara (load balance plz) wrote: > Why does this need to be pulled out? It's a single line call that's pretty > standard for converting strings. Done. https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_utils.cc (right): https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_utils.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2017/03/24 20:34:30, dfalcantara (load balance plz) wrote: > 2017 Done. https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_utils.cc:5: #include "chrome/browser/ui/android/infobars/translate_utils.h" On 2017/03/24 20:34:30, dfalcantara (load balance plz) wrote: > This should be taken out of the ui directory. Maybe try > chrome/browser/translate/android. Thanks. Have moved this to chrome/browser/translate/android. https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_utils.h (right): https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_utils.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2017/03/24 20:34:31, dfalcantara (load balance plz) wrote: > 2017. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Policy for tests is that you should add them when they don't exist. I'm not super-familiar with native-side C++ tests, but Java tests for infobars go into a completely different directory: chrome/android/javatests/src/org/chromium/chrome/browser/infobar/ If you want to add one, I guess you could make a test that makes sure that one of these compact infobars pops up when they're triggered. I can give you some pointers there if you want to go that route. https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/translate_compact_infobar.h (right): https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/translate_compact_infobar.h:38: bool ShouldDisplayNeverTranslateInfoBarOnCancel(); On 2017/03/27 03:36:59, ramyasharma wrote: > On 2017/03/24 20:34:30, dfalcantara (load balance plz) wrote: > > Also here. AFAICT from the mocks you don't ever display a never bar. > > Yes, we have a toast for never/always cases. Wondering if that is the same as > the existing one, and should be hooked here? I have tagged you in the mocks for > this. > https://folio.googleplex.com/chrome-ux/mocks/421-easier-translate/2017%2002%2... The Snackbar is a completely different controls, so you shouldn't have to worry about them from the infobar itself. The delegate should be throwing them up when necessary.
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...
On 2017/03/27 21:27:42, dfalcantara (load balance plz) wrote: > Policy for tests is that you should add them when they don't exist. I'm not > super-familiar with native-side C++ tests, but Java tests for infobars go into a > completely different directory: > > chrome/android/javatests/src/org/chromium/chrome/browser/infobar/ > > If you want to add one, I guess you could make a test that makes sure that one > of these compact infobars pops up when they're triggered. I can give you some > pointers there if you want to go that route. > > https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... > File chrome/browser/ui/android/infobars/translate_compact_infobar.h (right): > > https://codereview.chromium.org/2762393003/diff/100001/chrome/browser/ui/andr... > chrome/browser/ui/android/infobars/translate_compact_infobar.h:38: bool > ShouldDisplayNeverTranslateInfoBarOnCancel(); > On 2017/03/27 03:36:59, ramyasharma wrote: > > On 2017/03/24 20:34:30, dfalcantara (load balance plz) wrote: > > > Also here. AFAICT from the mocks you don't ever display a never bar. > > > > Yes, we have a toast for never/always cases. Wondering if that is the same as > > the existing one, and should be hooked here? I have tagged you in the mocks > for > > this. > > > https://folio.googleplex.com/chrome-ux/mocks/421-easier-translate/2017%2002%2... > > The Snackbar is a completely different controls, so you shouldn't have to worry > about them from the infobar itself. The delegate should be throwing them up > when necessary. Thanks Dan. Since this CL is more on the C++ side, I don't think i will be adding Java tests in this Cl. Maybe once we start adding more functionality we will add the tests. PTAL? Done, removed ShouldDisplayNeverTranslateInfoBarOnCancel.
lgtm You should split format your commit message like this to prevent the review site from looking weird: ============================================== TITLE: at most 80 chars The rest of your CL description details broken up however you want. BUG=123456 ==============================================
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI in the native code, and calls the new Translate infobar in Android UI. BUG=705310 ========== to ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI. translate_compact_infobar is created in the native code, and calls the new Translate infobar in Android UI. BUG=705310 ==========
Description was changed from ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI. translate_compact_infobar is created in the native code, and calls the new Translate infobar in Android UI. BUG=705310 ========== to ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI. translate_compact_infobar is created in the native code, and calls the new Translate infobar in Android UI. BUG=705310 ==========
On 2017/03/28 00:24:13, dfalcantara (load balance plz) wrote: > lgtm > > You should split format your commit message like this to prevent the review site > from looking weird: > > ============================================== > TITLE: at most 80 chars > > The rest of your CL description details broken > up however you want. > > BUG=123456 > ============================================== Thanks Dan. Done.
Description was changed from ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI. translate_compact_infobar is created in the native code, and calls the new Translate infobar in Android UI. BUG=705310 ========== to ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI. This translate_compact_infobar is created in the native code, and calls the new Translate infobar in Android UI. BUG=705310 ==========
Description was changed from ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI. This translate_compact_infobar is created in the native code, and calls the new Translate infobar in Android UI. BUG=705310 ========== to ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI. This translate_compact_infobar is created in the native code, and calls the new Translate infobar in Android UI. BUG=705310 TBR=groby@chromium.org ==========
ramyasharma@chromium.org changed reviewers: + groby@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": 140001, "attempt_start_ts": 1490669452055570,
"parent_rev": "69726f4ba3947fd4ddbfb07fd9f1a596289cdae9", "commit_rev":
"5f3bcbb0eae7cb2aaf21bd7cc025c86a82d595d4"}
Message was sent while issue was closed.
Description was changed from ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI. This translate_compact_infobar is created in the native code, and calls the new Translate infobar in Android UI. BUG=705310 TBR=groby@chromium.org ========== to ========== Creates translate_compact_infobar class, which holds the logic for the new translate UI. This translate_compact_infobar is created in the native code, and calls the new Translate infobar in Android UI. BUG=705310 TBR=groby@chromium.org Review-Url: https://codereview.chromium.org/2762393003 Cr-Commit-Position: refs/heads/master@{#460002} Committed: https://chromium.googlesource.com/chromium/src/+/5f3bcbb0eae7cb2aaf21bd7cc025... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:140001) as https://chromium.googlesource.com/chromium/src/+/5f3bcbb0eae7cb2aaf21bd7cc025... |
