New issue
Advanced search Search tips

Issue 687349 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 671820
issue 687340
issue 691897



Sign in to add a comment

Refactor how ViewsDelegate/LayoutDelegate expose metrics

Project Member Reported by pkasting@chromium.org, Jan 31 2017

Issue description

From internal discussion with sky et al.:

* Eliminate one-off GetXYZMetric()-type functions from ViewsDelegate, replace with the sorts of enum-based functions currently on LayoutDelegate
* Only expose enums values in ui/views/ that ui/views/ needs
* In the ChromeViewsDelegate, implement like:

class ChromeViewsDelegate : public ViewsDelegate {
  enum ChromeMetric {
    XXX = ViewsMetric::LAST_VALUE,
    YYY,
  }

  int GetMetric(ChromeMetric metric) const;

 private:
  int GetMetric(ViewsMetric metric) const override {
    return GetMetric(static_cast<ChromeMetric>(metric));
  }
};

* Probably at this point we can eliminate LayoutDelegate entirely and move anything left on it to ChromeViewsDelegate

+CC some sanity-checkers
 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 1 2017

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

commit 1f4e396f609abc324f544bd0378c56fc1749e22b
Author: pkasting <pkasting@chromium.org>
Date: Wed Feb 01 20:04:51 2017

Implement ChromeViewsDelegate methods in terms of LayoutDelegate where possible.

Regardless of how we ultimately choose to organize this functionality, this
reduces the number of distinct implementations, and thus makes that reorg easier
and safer.

This also adds const qualifiers to these functions while I'm touching them.

BUG= 687349 
TEST=none

Review-Url: https://codereview.chromium.org/2664073006
Cr-Commit-Position: refs/heads/master@{#447586}

[modify] https://crrev.com/1f4e396f609abc324f544bd0378c56fc1749e22b/chrome/browser/ui/views/chrome_views_delegate.cc
[modify] https://crrev.com/1f4e396f609abc324f544bd0378c56fc1749e22b/chrome/browser/ui/views/chrome_views_delegate.h
[modify] https://crrev.com/1f4e396f609abc324f544bd0378c56fc1749e22b/ui/views/views_delegate.cc
[modify] https://crrev.com/1f4e396f609abc324f544bd0378c56fc1749e22b/ui/views/views_delegate.h

Blocking: 687340
Scott suggests not exposing a lot of stuff directly on ChromeViewsDelegate, so if there is very much on LayoutDelegate besides just one or two metric functions, we should probably instead hoist LayoutDelegate as a concept up to the views layer, and pull all this stuff out of [Chrome]ViewsDelegate.
Blocking: 691897
Cc: pkasting@chromium.org
Owner: kylixrd@chromium.org
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 10 2017

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

commit c34b8926fd843300658dcdaec4c69c6967cf86c6
Author: kylixrd <kylixrd@chromium.org>
Date: Fri Mar 10 16:38:49 2017

Refactor ViewsDelegate and MD-ify the icon-to-text spacing for checkbox and radiobutton.

ViewsDelegate should have the layout related functions coalesced into a handful of functions which take an enum element for each metric.

BUG= 652024 , 687349 

Review-Url: https://codereview.chromium.org/2696263002
Cr-Commit-Position: refs/heads/master@{#456078}

[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/first_run/try_chrome_dialog_view.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/chrome_views_delegate.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/chrome_views_delegate.h
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/chrome_views_delegate_win.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/extensions/media_galleries_dialog_views_unittest.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/global_error_bubble_view_unittest.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/harmony/layout_delegate.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/toolbar/reload_button_unittest.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/bubble/bubble_dialog_delegate.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/bubble/bubble_frame_view.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/controls/button/label_button.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/controls/button/md_text_button.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/examples/dialog_example.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/layout/layout_constants.h
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/views_delegate.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/views_delegate.h
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/window/dialog_client_view.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/window/dialog_delegate.cc

Comment 7 Deleted

I think this is done now?
Nope, this is mostly not done.
Blocking: 671820
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 13 2017

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

commit 4e8cac4101f13be9f86b455f06ec7b46fc707e93
Author: kylixrd <kylixrd@chromium.org>
Date: Thu Apr 13 17:15:56 2017

Broke out layout metric information from ViewsDelegate to ViewsLayoutDelegate

BUG= 687349 
TBR=msw@chromium.org

Review-Url: https://codereview.chromium.org/2758323002
Cr-Commit-Position: refs/heads/master@{#464448}

[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ash/mus/app_launch_unittest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ash/test/ash_test_helper.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ash/test/ash_test_helper.h
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/chromeos/accessibility/select_to_speak_event_handler_unittest.cc
[delete] https://crrev.com/1262351d14e829049153dfe0b8f14b21c4b3b909/chrome/browser/chromeos/options/DEPS
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/chromeos/options/vpn_config_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/chromeos/options/wifi_config_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/chromeos/options/wimax_config_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/first_run/try_chrome_dialog_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_ash_unittest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/arc_app_dialog_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/chrome_browser_main_extra_parts_views.h
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/chrome_views_delegate.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/chrome_views_delegate.h
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/collected_cookies_views.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/content_setting_bubble_contents.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/cookie_info_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/device_chooser_content_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/extensions/chooser_dialog_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/extensions/chooser_dialog_view_unittest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/extensions/media_galleries_dialog_views_unittest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/extensions/media_gallery_checkbox_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/global_error_bubble_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/global_error_bubble_view_unittest.cc
[add] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/harmony/chrome_layout_provider.cc
[add] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/harmony/chrome_layout_provider.h
[delete] https://crrev.com/1262351d14e829049153dfe0b8f14b21c4b3b909/chrome/browser/ui/views/harmony/harmony_layout_delegate.cc
[delete] https://crrev.com/1262351d14e829049153dfe0b8f14b21c4b3b909/chrome/browser/ui/views/harmony/harmony_layout_delegate.h
[add] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/harmony/harmony_layout_provider.cc
[add] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/harmony/harmony_layout_provider.h
[delete] https://crrev.com/1262351d14e829049153dfe0b8f14b21c4b3b909/chrome/browser/ui/views/harmony/layout_delegate.cc
[delete] https://crrev.com/1262351d14e829049153dfe0b8f14b21c4b3b909/chrome/browser/ui/views/harmony/layout_delegate.h
[rename] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/harmony/layout_provider_unittest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/hung_renderer_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/importer/import_lock_dialog_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/login_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/page_info/page_info_bubble_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/page_info/page_info_bubble_view_unittest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/sad_tab_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/task_manager_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/webshare/webshare_target_picker_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/browser/ui/views/webshare/webshare_target_picker_view_unittest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/test/BUILD.gn
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/test/base/browser_with_test_window_test.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/chrome/test/base/browser_with_test_window_test.h
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/components/constrained_window/constrained_window_views_unittest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/BUILD.gn
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/bubble/bubble_dialog_delegate.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/bubble/bubble_frame_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/controls/button/image_button_factory.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/controls/button/label_button.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/controls/button/md_text_button.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/examples/dialog_example.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/layout/grid_layout.cc
[add] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/layout/layout_provider.cc
[add] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/layout/layout_provider.h
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/mus/aura_init.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/style/typography.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/test/scoped_views_test_helper.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/test/scoped_views_test_helper.h
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/test/test_views_delegate.h
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/test/test_views_delegate_aura.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/test/test_views_delegate_mac.mm
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/test/views_test_base.h
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/views_delegate.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/views_delegate.h
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11_unittest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/widget/root_view_unittest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/widget/widget_interactive_uitest.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/window/dialog_client_view.cc
[modify] https://crrev.com/4e8cac4101f13be9f86b455f06ec7b46fc707e93/ui/views/window/dialog_delegate.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Apr 14 2017

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

commit d61d7204eab9c8d63173a8fbadeaac9a2e816fae
Author: kylixrd <kylixrd@chromium.org>
Date: Fri Apr 14 19:36:31 2017

Move LayoutProvider initialization to later in the startup process.

CreateLayoutProvider() relies on ui::MaterialDesignController::Intialize() having already been called. That function cannot be called until the command-line arguments are available, which happens when PreCreateThreads() is called.

BUG= 687349 

Review-Url: https://codereview.chromium.org/2818123002
Cr-Commit-Position: refs/heads/master@{#464777}

[modify] https://crrev.com/d61d7204eab9c8d63173a8fbadeaac9a2e816fae/chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc

Status: Fixed (was: Assigned)

Sign in to add a comment