New issue
Advanced search Search tips

Issue 852117 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Remove redundant icons from top toolbar when bottom toolbar is activated

Project Member Reported by amaralp@chromium.org, Jun 12 2018

Issue description

The tab switcher and overflow menu are redundant when the bottom toolbar
is active. They should be hidden.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e83d1f12d67bfacbc9a6bde53593302326176817

commit e83d1f12d67bfacbc9a6bde53593302326176817
Author: Pedro Amaral <amaralp@chromium.org>
Date: Tue Jun 26 23:19:27 2018

Don't show top toolbar items when bottom toolbar is enabled

Remove the top toolbar buttons (home, tab switcher, and menu buttons)
from their parent view and set them to null. Also add a bunch of
null checks since now things can be null.

Bug:  852117 
Change-Id: I620f35f81388d1f4ac8791a9eb45576566474604
Reviewed-on: https://chromium-review.googlesource.com/1105569
Commit-Queue: Pedro Amaral <amaralp@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570578}
[modify] https://crrev.com/e83d1f12d67bfacbc9a6bde53593302326176817/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java
[modify] https://crrev.com/e83d1f12d67bfacbc9a6bde53593302326176817/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
[modify] https://crrev.com/e83d1f12d67bfacbc9a6bde53593302326176817/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java

Status: Fixed (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8438c4f922b5fbe03fc625c59fb00cec72bf91a6

commit 8438c4f922b5fbe03fc625c59fb00cec72bf91a6
Author: Peter E Conn <peconn@chromium.org>
Date: Tue Jul 03 14:04:16 2018

Revert "Don't show top toolbar items when bottom toolbar is enabled"

This reverts commit e83d1f12d67bfacbc9a6bde53593302326176817.

Reason for revert: https://crbug.com/857437

Essentially, as well as adding if statements to all usages of mMenuButton within the class, you need to add if statements to all other classes calling 'getMenuButton()' (and ToolbarManager.getMenuButton() transitively).

Unfortunately this isn't so trivial for all the cases (eg, I'm guessing we still want to show the data saver when this feature is enabled, or the download text bubble?), so I'm reverting instead of attempting and TBRing a fix myself. Sorry!

Original change's description:
> Don't show top toolbar items when bottom toolbar is enabled
> 
> Remove the top toolbar buttons (home, tab switcher, and menu buttons)
> from their parent view and set them to null. Also add a bunch of
> null checks since now things can be null.
> 
> Bug:  852117 
> Change-Id: I620f35f81388d1f4ac8791a9eb45576566474604
> Reviewed-on: https://chromium-review.googlesource.com/1105569
> Commit-Queue: Pedro Amaral <amaralp@chromium.org>
> Reviewed-by: Ted Choc <tedchoc@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#570578}

TBR=tedchoc@chromium.org,amaralp@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  852117 
Change-Id: I2c16533f8dec11c5d11b920693157735e7438e89
Reviewed-on: https://chromium-review.googlesource.com/1124579
Commit-Queue: Peter Conn <peconn@chromium.org>
Reviewed-by: Peter Conn <peconn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572193}
[modify] https://crrev.com/8438c4f922b5fbe03fc625c59fb00cec72bf91a6/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java
[modify] https://crrev.com/8438c4f922b5fbe03fc625c59fb00cec72bf91a6/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
[modify] https://crrev.com/8438c4f922b5fbe03fc625c59fb00cec72bf91a6/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf930470154ff70cf27b2455114343b964341597

commit bf930470154ff70cf27b2455114343b964341597
Author: Pedro Amaral <amaralp@chromium.org>
Date: Mon Jul 16 19:03:30 2018

Don't show top toolbar items when bottom toolbar is enabled

The original CL for this (https://chromium-review.googlesource.com/1105569)
was reverted because of a null pointer bug. This CL does the same thing
as the reverted CL and fixes the null pointer exception by having
ToolbarManager#getMenuButton() return the bottom toolbar menu button when
the bottom toolbar is enabled.

Bug:  852117 

Change-Id: Ic2668a3cac6a552bee4476d1cfb58f8e6c290bfd
Reviewed-on: https://chromium-review.googlesource.com/1135776
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Pedro Amaral <amaralp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575372}
[modify] https://crrev.com/bf930470154ff70cf27b2455114343b964341597/chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarCoordinator.java
[modify] https://crrev.com/bf930470154ff70cf27b2455114343b964341597/chrome/android/java/src/org/chromium/chrome/browser/toolbar/MenuButton.java
[modify] https://crrev.com/bf930470154ff70cf27b2455114343b964341597/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java
[modify] https://crrev.com/bf930470154ff70cf27b2455114343b964341597/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
[modify] https://crrev.com/bf930470154ff70cf27b2455114343b964341597/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java

 Issue 863463  has been merged into this issue.

Sign in to add a comment