New issue
Advanced search Search tips

Issue 687340 link

Starred by 2 users
Status: Fixed
Owner:
ellyjo...@chromium.org
Closed: Apr 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocked on:
issue 687349


Sign in to add a comment

Unify GridLayout::CreatePanel() and layout_utils::CreatePanelLayout()

Project Member Reported by pkasting@chromium.org, Jan 31 2017 
These two methods are almost the same, except that the former does the wrong thing for Harmony, and the latter sets the layout manager on the host while the former doesn't.

I suspect all code calling the former really wants to call the latter.  I also suspect the least code churn, and most clarity, would be achieved by eliminating the layout_utils file, moving the implementation of this function into GridLayout::CreatePanel(), and then removing the SetLayoutManager() calls from current callers.

This would have helped prevent a bug today where the newly-converted WifiConfigView tried to switch from CreatePanel() to a broken hand-rolled version of CreatePanelLayout(), and will help ensure other callers are Harmonized correctly automatically.

Elly, looks like you added this.  Thoughts?

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

Labels: -Pri-2 Pri-1

Comment 2 by ellyjo...@chromium.org, Feb 6 2017 

Hm.

We can't just move the semantics of CreatePanelLayout() into Views, since it is dependent on LayoutDelegate, which is right now a /chrome-only concept. If we are planning to move LayoutDelegate into views, we could do this afterwards, though...

Comment 3 by pkasting@chromium.org, Feb 6 2017

Blockedon: 687349

Comment 4 by ellyjo...@chromium.org, Mar 28 2017

Status: Started (was: Assigned)
https://codereview.chromium.org/2779973003/
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 4 2017 

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

commit 08376b4003fdea077ec6df37f88305a65751101c
Author: ellyjones <ellyjones@chromium.org>
Date: Tue Apr 04 16:39:59 2017

views: fold layout_utils::CreatePanelLayout into GridLayout::CreatePanel

This change:
1) Introduces a new type of InsetsMetric in ViewsDelegate: PANEL
2) Changes GridLayout::CreatePanel to use that metric instead of hardcoded insets
3) Changes GridLayout::CreatePanel to always install itself as the layout manager of the provided host View
4) Moves all callers of layout_utils::CreatePanelLayout over to GridLayout::CreatePanel
5) Deletes layout_utils

BUG= 687340 

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

[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/chromeos/enrollment_dialog_view.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/chromeos/options/DEPS
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/chromeos/options/vpn_config_view.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/chromeos/options/wifi_config_view.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/chromeos/options/wimax_config_view.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/chromeos/ui/request_pin_view.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/first_run/try_chrome_dialog_view.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/accessibility/invert_bubble_view.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/certificate_selector.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/chrome_views_delegate.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/collected_cookies_views.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/confirm_bubble_views.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/create_application_shortcut_view.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/crypto_module_password_dialog_view.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/download/download_danger_prompt_views.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/first_run_bubble.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/first_run_dialog.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/hung_renderer_view.cc
[delete] https://crrev.com/803fdf80f842803c7c91dca2a87419afe1b3d60c/chrome/browser/ui/views/layout_utils.cc
[delete] https://crrev.com/803fdf80f842803c7c91dca2a87419afe1b3d60c/chrome/browser/ui/views/layout_utils.h
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/login_view.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/sync/one_click_signin_dialog_view.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/chrome/browser/ui/views/uninstall_view.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/ui/views/layout/grid_layout.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/ui/views/views_delegate.cc
[modify] https://crrev.com/08376b4003fdea077ec6df37f88305a65751101c/ui/views/views_delegate.h

Comment 6 by ellyjo...@chromium.org, Apr 4 2017

Status: Fixed (was: Started)
►

Sign in to add a comment