New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 828278 link

Starred by 1 user
Status: Started
Owner:
alancutter@chromium.org
Cc:
calamity@chromium.org
tapted@chromium.org
dominickn@chromium.org
█ bgalbs@google.com
pcovell@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 788051
issue 822146



Sign in to add a comment

Unite ToolbarView and HostedAppButtonContainer under a common toolbar interface

Project Member Reported by alancutter@chromium.org, Apr 3 2018 
In regular tabbed Chrome UI the ToolbarView is where buttons like location settings and the app menu live.
In hosted app windows on chromeOS those instead live inside the HostedAppButtonContainer in the non client frame.

Many parts of the code explicitly reference buttons inside LocationBarView which lives in ToolbarView. This behaviour would be broken for hosted app windows where ToolbarView is not visible and the buttons inaccessible to the user. They should instead reference the buttons that live in the HostedAppButtonContainer instead.

We should add a layer of indirection via a generic toolbar button interface that is handled by either the HostedAppButtonContainer or the ToolbarView depending on which one is visible at the time.

This refactor is blocking the addition of more buttons to HostedAppButtonContainer (issue 788051) and upgrading HostedAppButtonContainer to be a focus pane target accessible via F6 ( issue 822146 ).

Comment 1 by alancutter@chromium.org, Apr 4 2018 

Categorisation of the existing ToolbarView/LocationBarView buttons here:
https://docs.google.com/spreadsheets/d/1TdsGZHSte18gSF9o69dcYtkJZdITxeSvoTXs-I1hvB8/edit#gid=44569935
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 11 2018 

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

commit 5a63bcb640a503288e906852b1eba64945da6d55
Author: Alan Cutter <alancutter@chromium.org>
Date: Wed Apr 11 05:51:52 2018

Rename BrowserViewButtonProvider as ToolbarButtonProvider

This CL is a pure rename and has no behavioural changes.
This renames BrowserViewButtonProvider to ToolbarButtonProvider.
This is intended to be an interface that HostedAppButtonContainer
and ToolbarView implement to expose access to their common buttons.

Bug: 828278
Change-Id: I1ad846e260abd3bd9de4de48cce26a0845f93bca
Reviewed-on: https://chromium-review.googlesource.com/991534
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: calamity <calamity@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549787}
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/accessibility/invert_bubble_view.cc
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/bubble_anchor_util_views.cc
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/frame/browser_view.h
[delete] https://crrev.com/8fc901b5c10c55bd9999960b68b55e850a19251c/chrome/browser/ui/views/frame/browser_view_button_provider.h
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/frame/hosted_app_button_container.cc
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/frame/hosted_app_button_container.h
[add] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/frame/toolbar_button_provider.h
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/global_error_bubble_view.cc
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/ime/ime_warning_bubble_view.cc
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/session_crashed_bubble_view.cc
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/toolbar/browser_actions_container.cc
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/toolbar/toolbar_view.cc
[modify] https://crrev.com/5a63bcb640a503288e906852b1eba64945da6d55/chrome/browser/ui/views/toolbar/toolbar_view.h
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 16 2018 

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

commit fc6824fa378f3ce6cffc496f8e6a274894830e78
Author: Alan Cutter <alancutter@chromium.org>
Date: Mon Apr 16 08:27:52 2018

Remove ShowPageInfoDialog() from LocationBar interface

This CL cleans up the LocationBar interface.
LocationBar::ShowPageInfoDialog() was only ever used with the
LocationBarView implementation of LocationBar and can be removed
from the higher level interface.

There are no behavioural changes in this CL.

Bug: 828278
Change-Id: Ied8af6849f16bda72bb89c7f5fcffdfd599adcbe
Reviewed-on: https://chromium-review.googlesource.com/1011884
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550948}
[modify] https://crrev.com/fc6824fa378f3ce6cffc496f8e6a274894830e78/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h
[modify] https://crrev.com/fc6824fa378f3ce6cffc496f8e6a274894830e78/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
[modify] https://crrev.com/fc6824fa378f3ce6cffc496f8e6a274894830e78/chrome/browser/ui/location_bar/location_bar.h
[modify] https://crrev.com/fc6824fa378f3ce6cffc496f8e6a274894830e78/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/fc6824fa378f3ce6cffc496f8e6a274894830e78/chrome/browser/ui/views/location_bar/location_bar_view.h
[modify] https://crrev.com/fc6824fa378f3ce6cffc496f8e6a274894830e78/chrome/test/base/test_browser_window.cc
[modify] https://crrev.com/fc6824fa378f3ce6cffc496f8e6a274894830e78/chrome/test/base/test_browser_window.h
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc6824fa378f3ce6cffc496f8e6a274894830e78

commit fc6824fa378f3ce6cffc496f8e6a274894830e78
Author: Alan Cutter <alancutter@chromium.org>
Date: Mon Apr 16 08:27:52 2018

Remove ShowPageInfoDialog() from LocationBar interface

This CL cleans up the LocationBar interface.
LocationBar::ShowPageInfoDialog() was only ever used with the
LocationBarView implementation of LocationBar and can be removed
from the higher level interface.

There are no behavioural changes in this CL.

Bug: 828278
Change-Id: Ied8af6849f16bda72bb89c7f5fcffdfd599adcbe
Reviewed-on: https://chromium-review.googlesource.com/1011884
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550948}
[modify] https://crrev.com/fc6824fa378f3ce6cffc496f8e6a274894830e78/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h
[modify] https://crrev.com/fc6824fa378f3ce6cffc496f8e6a274894830e78/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
[modify] https://crrev.com/fc6824fa378f3ce6cffc496f8e6a274894830e78/chrome/browser/ui/location_bar/location_bar.h
[modify] https://crrev.com/fc6824fa378f3ce6cffc496f8e6a274894830e78/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/fc6824fa378f3ce6cffc496f8e6a274894830e78/chrome/browser/ui/views/location_bar/location_bar_view.h
[modify] https://crrev.com/fc6824fa378f3ce6cffc496f8e6a274894830e78/chrome/test/base/test_browser_window.cc
[modify] https://crrev.com/fc6824fa378f3ce6cffc496f8e6a274894830e78/chrome/test/base/test_browser_window.h
►

Sign in to add a comment