|
|
Created:
3 years, 8 months ago by ramyasharma Modified:
3 years, 7 months ago CC:
chromium-reviews, agrieve+watch_chromium.org, dfalcantara+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionShows snackbar in translate infobar, after certain user actions.
CL#2 Plumbs through snackbar manager from Infobar container to
TranslateCompactInfobar, and shows a snackbar on actions like
always / never translate.
BUG=713514
TBR=tedchoc@chromium.org
Review-Url: https://codereview.chromium.org/2840933003
Cr-Commit-Position: refs/heads/master@{#468917}
Committed: https://chromium.googlesource.com/chromium/src/+/48808f1acfda5746d21186fc83fbc60ffe0646d9
Patch Set 1 #
Total comments: 7
Patch Set 2 #
Total comments: 4
Patch Set 3 #Patch Set 4 #Messages
Total messages: 96 (82 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
Patchset #1 (id:40001) 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
Description was changed from ========== Shows snackbar in translate infobar, after certain user actions. Plumbs through snackbar manager from Infobar container to TranslateCompactInfobar, and shows a snackbar on actions like always / never translate. BUG=713514 ========== to ========== Shows snackbar in translate infobar, after certain user actions. CL#2 Plumbs through snackbar manager from Infobar container to TranslateCompactInfobar, and shows a snackbar on actions like always / never translate. BUG=713514 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
https://codereview.chromium.org/2840933003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java (right): https://codereview.chromium.org/2840933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:168: } This won't work if the tab is sent to another Activity (via "reparenting"). You'll probably have to (at least) listen to onReparentingFinished() in mTabObserver. https://codereview.chromium.org/2840933003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2840933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:206: private Snackbar createSnackbar(String title, int type, int nativePtr) { You don't actually use the nativePtr. https://codereview.chromium.org/2840933003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/Snackbar.java (right): https://codereview.chromium.org/2840933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/snackbar/Snackbar.java:36: * UMA Identifiers of features using snackbar. See SnackbarIdentifier enum in histograms. The comment here says you need to edit histograms.xml.
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:80001) has been deleted
Patchset #2 (id:100001) 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/2840933003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java (right): https://codereview.chromium.org/2840933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:168: } On 2017/04/26 17:27:02, slow (dfalcantara) wrote: > This won't work if the tab is sent to another Activity (via "reparenting"). > You'll probably have to (at least) listen to onReparentingFinished() in > mTabObserver. Thanks done. How can I test re-parenting from the UI? I am not sure what exactly re-parenting means? https://codereview.chromium.org/2840933003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2840933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:206: private Snackbar createSnackbar(String title, int type, int nativePtr) { On 2017/04/26 17:27:02, slow (dfalcantara) wrote: > You don't actually use the nativePtr. ah yes. Not yet, but in the next cl I will. Removed this for now. https://codereview.chromium.org/2840933003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/Snackbar.java (right): https://codereview.chromium.org/2840933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/snackbar/Snackbar.java:36: * UMA Identifiers of features using snackbar. See SnackbarIdentifier enum in histograms. On 2017/04/26 17:27:02, slow (dfalcantara) wrote: > The comment here says you need to edit histograms.xml. Oops. Thanks for pointing that. Done.
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_...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2840933003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java (right): https://codereview.chromium.org/2840933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:168: } On 2017/04/27 08:03:11, ramyasharma wrote: > On 2017/04/26 17:27:02, slow (dfalcantara) wrote: > > This won't work if the tab is sent to another Activity (via "reparenting"). > > You'll probably have to (at least) listen to onReparentingFinished() in > > mTabObserver. > > Thanks done. How can I test re-parenting from the UI? I am not sure what exactly > re-parenting means? Re-parenting currently moves a tab from a CustomTabActivity and makes it owned by a ChromeTabbedActivity (with an entirely different set of UI). You can test this by doing something like: 1) Open a link from Gmail 2) It should pop up a picker that lets you choose your locally built Chromium 3) It should appear as a Custom Tab (https://developer.chrome.com/multidevice/android/customtabs) 4) From the menu, select "open in browser" or something; if your Chromium build is set as your default browser then it should reparent the tab over. https://codereview.chromium.org/2840933003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java (right): https://codereview.chromium.org/2840933003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:115: if (activity != null && activity instanceof ChromeActivity) { instanceof already includes an implicit null check https://codereview.chromium.org/2840933003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:172: if (activity != null && activity instanceof ChromeActivity) { don't duplicate this logic; make a new private function
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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Patchset #3 (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...
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...)
Thanks Dan. PTAL? https://codereview.chromium.org/2840933003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java (right): https://codereview.chromium.org/2840933003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:115: if (activity != null && activity instanceof ChromeActivity) { On 2017/04/27 18:48:58, slow (dfalcantara) wrote: > instanceof already includes an implicit null check Thanks Done. https://codereview.chromium.org/2840933003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:172: if (activity != null && activity instanceof ChromeActivity) { On 2017/04/27 18:48:58, slow (dfalcantara) wrote: > don't duplicate this logic; make a new private function Done. Thanks.
lgtm
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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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/2840933003/#ps180001 (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...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:180001) 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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Patchset #3 (id:200001) 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #3 (id:220001) 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...
ramyasharma@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@ for histograms.xml
Description was changed from ========== Shows snackbar in translate infobar, after certain user actions. CL#2 Plumbs through snackbar manager from Infobar container to TranslateCompactInfobar, and shows a snackbar on actions like always / never translate. BUG=713514 ========== to ========== Shows snackbar in translate infobar, after certain user actions. CL#2 Plumbs through snackbar manager from Infobar container to TranslateCompactInfobar, and shows a snackbar on actions like always / never translate. BUG=713514 TBR=tedchoc@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
lgtm
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/2840933003/#ps240001 (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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Patchset #3 (id:240001) 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #3 (id:260001) has been deleted
Patchset #3 (id:280001) 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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:300001) 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 #3 (id:320001) has been deleted
Patchset #4 (id:360001) has been deleted
Patchset #4 (id:380001) 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 checked by ramyasharma@chromium.org to run a CQ dry run
Patchset #3 (id:340001) 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ramyasharma@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2840933003/#ps420001 (title: "")
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": 420001, "attempt_start_ts": 1493799346490000, "parent_rev": "9432ee01aaa7821c3ea5cbff746a969a0725a34b", "commit_rev": "48808f1acfda5746d21186fc83fbc60ffe0646d9"}
Message was sent while issue was closed.
Description was changed from ========== Shows snackbar in translate infobar, after certain user actions. CL#2 Plumbs through snackbar manager from Infobar container to TranslateCompactInfobar, and shows a snackbar on actions like always / never translate. BUG=713514 TBR=tedchoc@chromium.org ========== to ========== Shows snackbar in translate infobar, after certain user actions. CL#2 Plumbs through snackbar manager from Infobar container to TranslateCompactInfobar, and shows a snackbar on actions like always / never translate. BUG=713514 TBR=tedchoc@chromium.org Review-Url: https://codereview.chromium.org/2840933003 Cr-Commit-Position: refs/heads/master@{#468917} Committed: https://chromium.googlesource.com/chromium/src/+/48808f1acfda5746d21186fc83fb... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:420001) as https://chromium.googlesource.com/chromium/src/+/48808f1acfda5746d21186fc83fb... |