Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1310183009:
Add ability for Widgets to override the DefaultThemeProvider (Closed)
|
Created:
5 years, 3 months ago by jonross Modified:
5 years, 3 months ago CC:
chromium-reviews, tfarina, tdanderson+views_chromium.org, dzhioev+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ability for Widgets to override the DefaultThemeProvider
Certain views access layout information from ThemeProvider::GetDisplayProperty()
However some Widgets created for dialogs and popups do not provide a ThemeProvider. This impacts the re-use of these views.
Update Widget to allow for a theme provider to be set as an override.
Update CaptivePortalWindowTest to set the appropriate ThemeProvider so that LocationBarView can properly layout.
Update OmniboxPopupContentsView to set the appropriate ThemeProvider so that it will be able to stop using LocationBarView as a way to get the appropriate theme.
TEST=CaptivePortalWindowTest
BUG=524204
Patch Set 1 #Patch Set 2 : #
Total comments: 3
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 3
Patch Set 5 : Rebase #
Total comments: 6
Patch Set 6 : Only Set ThemeProvider on Top Level Widget #
Total comments: 5
Depends on Patchset: Messages
Total messages: 28 (5 generated)
jonross@chromium.org changed reviewers: + tdanderson@chromium.org
Hey Terry, Can you take a first look at this change? I plan to also update the Omnibox widget in this change, just want a review of the pattern first. Thanks, Jon
Seems reasonable to me so far.
jonross@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@chromium.org: Please review changes in Widget
https://codereview.chromium.org/1310183009/diff/20001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1310183009/diff/20001/ui/views/widget/widget.... ui/views/widget/widget.cc:764: theme_provider_ = theme_provider; If a Widget has a theme-provider, then there's no reason to create the DefaultThemeProvider for the Widget in the first-place, right? So, would it make sense to include an optional theme-provider in InitParams when initializaing the Widget? https://codereview.chromium.org/1310183009/diff/20001/ui/views/widget/widget.... ui/views/widget/widget.cc:766: non_client_view_->Layout(); Should this just call ThemeChanged() instead? (the Views that want a re-layout on theme-change should trigger that from OnThemeChanged() override)
https://codereview.chromium.org/1310183009/diff/20001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1310183009/diff/20001/ui/views/widget/widget.... ui/views/widget/widget.cc:764: theme_provider_ = theme_provider; On 2015/08/31 17:51:07, sadrul wrote: > If a Widget has a theme-provider, then there's no reason to create the > DefaultThemeProvider for the Widget in the first-place, right? So, would it make > sense to include an optional theme-provider in InitParams when initializaing the > Widget? We would not need the DefaultThemeProvider if they are using an alternate theme. I could move this to the InitParams instead, as long as it is fine having Init params not owned by the Widget
lgtm The line-wrapping in the CL description is a little weird right now. https://codereview.chromium.org/1310183009/diff/40001/ui/views/widget/widget.h File ui/views/widget/widget.h (right): https://codereview.chromium.org/1310183009/diff/40001/ui/views/widget/widget.... ui/views/widget/widget.h:892: // is provided. This is not owned by Widget. is *used?
https://codereview.chromium.org/1310183009/diff/40001/ui/views/widget/widget.h File ui/views/widget/widget.h (right): https://codereview.chromium.org/1310183009/diff/40001/ui/views/widget/widget.... ui/views/widget/widget.h:892: // is provided. This is not owned by Widget. On 2015/09/02 17:21:58, sadrul wrote: > is *used? Done.
jonross@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@chromium.org: Please review changes in chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc Thanks, Jon
jonross@chromium.org changed reviewers: + oshima@chromium.org
oshima@chromium.org: Please review changes in chrome/browser/chromeos/login/ui/captive_portal_window_proxy.cc Thanks, Jon
https://codereview.chromium.org/1310183009/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/ui/captive_portal_window_proxy.cc (right): https://codereview.chromium.org/1310183009/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/ui/captive_portal_window_proxy.cc:34: ProfileManager::GetActiveUserProfile()); You should get the profile associated with the parent, instead of active one. Please see MultiUserWindowManager::GetUserPresentingWindow, and ask skuhne@ if you have a question about MultiUserWindowManager. https://codereview.chromium.org/1310183009/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/1310183009/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:237: ProfileManager::GetActiveUserProfile()); ditto
What oshima said, plus another comment. https://codereview.chromium.org/1310183009/diff/80001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1310183009/diff/80001/ui/views/widget/widget.... ui/views/widget/widget.cc:753: return theme_provider_; It seems strange that we prefer root widget's default theme provider over our own default theme provider, but we prefer our own non-default theme provider over root widget's. I think this is a symptom of fixing the problem the wrong way. Instead of setting an overriding theme provider on a child widget, shouldn't we be fixing this problem by setting the correct (non-default) theme provider on the root widgets for these various cases? Then we wouldn't need to touch this function.
https://codereview.chromium.org/1310183009/diff/80001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1310183009/diff/80001/ui/views/widget/widget.... ui/views/widget/widget.cc:753: return theme_provider_; On 2015/09/02 21:44:14, Peter Kasting wrote: > It seems strange that we prefer root widget's default theme provider over our > own default theme provider, but we prefer our own non-default theme provider > over root widget's. > > I think this is a symptom of fixing the problem the wrong way. Instead of > setting an overriding theme provider on a child widget, shouldn't we be fixing > this problem by setting the correct (non-default) theme provider on the root > widgets for these various cases? Then we wouldn't need to touch this function. I had debated that approach, and am still open to it. I wasn't sure if setting the ThemeProvider on the root widget would impact the expectations of sibling widgets. I also didn't want to litter the code with overrides of GetThemeProvider like there are in BrowserFrame.
On 2015/09/02 22:02:27, jonross wrote: > https://codereview.chromium.org/1310183009/diff/80001/ui/views/widget/widget.cc > File ui/views/widget/widget.cc (right): > > https://codereview.chromium.org/1310183009/diff/80001/ui/views/widget/widget.... > ui/views/widget/widget.cc:753: return theme_provider_; > On 2015/09/02 21:44:14, Peter Kasting wrote: > > It seems strange that we prefer root widget's default theme provider over our > > own default theme provider, but we prefer our own non-default theme provider > > over root widget's. > > > > I think this is a symptom of fixing the problem the wrong way. Instead of > > setting an overriding theme provider on a child widget, shouldn't we be fixing > > this problem by setting the correct (non-default) theme provider on the root > > widgets for these various cases? Then we wouldn't need to touch this > function. > > I had debated that approach, and am still open to it. > > I wasn't sure if setting the ThemeProvider on the root widget would impact the > expectations of sibling widgets. If it impacts them, it seems like those impacts would be correct. I can't think why we'd have a root widget that would want to contain widgets using different theme providers from each other. If you're worried, ask sky@ about this.
https://codereview.chromium.org/1310183009/diff/80001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1310183009/diff/80001/ui/views/widget/widget.... ui/views/widget/widget.cc:766: return default_theme_provider_.get(); Maybe this function should look like: if (root_widget = GetTopLevelWidget()) return root_widget->GetThemeProvider(); return theme_provider_ ? theme_provider_ : default_theme_provider_.get(); ? Also, maybe a check in ::Init() that InitParams::theme_provider is not set for child widgets?
https://codereview.chromium.org/1310183009/diff/80001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1310183009/diff/80001/ui/views/widget/widget.... ui/views/widget/widget.cc:766: return default_theme_provider_.get(); On 2015/09/02 22:17:50, sadrul wrote: > Maybe this function should look like: > > if (root_widget = GetTopLevelWidget()) > return root_widget->GetThemeProvider(); > return theme_provider_ ? theme_provider_ : default_theme_provider_.get(); > > ? > > Also, maybe a check in ::Init() that InitParams::theme_provider is not set for > child widgets? I had considered and rejected doing just the change you propose for this function (because setting overrides on child widgets wouldn't have the desired effect), but if we couple this with a guarantee that you can't set a theme provider on a child widget as you suggest, then I think this would be a win.
Patchset #6 (id:100001) has been deleted
I've updated the change to only set the ThemeProvider on the top level widget. Widget has been updated to reflect only applying ThemeProvider setting on the top level widget. PTAL. https://codereview.chromium.org/1310183009/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/ui/captive_portal_window_proxy.cc (right): https://codereview.chromium.org/1310183009/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/ui/captive_portal_window_proxy.cc:34: ProfileManager::GetActiveUserProfile()); On 2015/09/02 18:44:12, oshima wrote: > You should get the profile associated with the parent, instead of active one. > > Please see MultiUserWindowManager::GetUserPresentingWindow, and ask skuhne@ if > you have a question about MultiUserWindowManager. I have switched to using MultiUserWindowManager, however it is not always able to provide a Profile. So I have fallbacks to ProfileManager for when that occurs. https://codereview.chromium.org/1310183009/diff/80001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1310183009/diff/80001/ui/views/widget/widget.... ui/views/widget/widget.cc:753: return theme_provider_; On 2015/09/02 22:02:26, jonross wrote: > On 2015/09/02 21:44:14, Peter Kasting wrote: > > It seems strange that we prefer root widget's default theme provider over our > > own default theme provider, but we prefer our own non-default theme provider > > over root widget's. > > > > I think this is a symptom of fixing the problem the wrong way. Instead of > > setting an overriding theme provider on a child widget, shouldn't we be fixing > > this problem by setting the correct (non-default) theme provider on the root > > widgets for these various cases? Then we wouldn't need to touch this > function. > > I had debated that approach, and am still open to it. > > I wasn't sure if setting the ThemeProvider on the root widget would impact the > expectations of sibling widgets. > > I also didn't want to litter the code with overrides of GetThemeProvider like > there are in BrowserFrame. I've updated the change to only apply the desired ThemeProvider on the top level widget. https://codereview.chromium.org/1310183009/diff/80001/ui/views/widget/widget.... ui/views/widget/widget.cc:766: return default_theme_provider_.get(); On 2015/09/02 22:25:18, Peter Kasting wrote: > On 2015/09/02 22:17:50, sadrul wrote: > > Maybe this function should look like: > > > > if (root_widget = GetTopLevelWidget()) > > return root_widget->GetThemeProvider(); > > return theme_provider_ ? theme_provider_ : default_theme_provider_.get(); > > > > ? > > > > Also, maybe a check in ::Init() that InitParams::theme_provider is not set for > > child widgets? > > I had considered and rejected doing just the change you propose for this > function (because setting overrides on child widgets wouldn't have the desired > effect), but if we couple this with a guarantee that you can't set a theme > provider on a child widget as you suggest, then I think this would be a win. I've updated this to always request the root widget's theme provider. Init now guarantees that the ThemeProvider is only set on the top level widget.
A couple of micro-nits, but this lgtm. https://codereview.chromium.org/1310183009/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/ui/lock_window_aura.cc (right): https://codereview.chromium.org/1310183009/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/ui/lock_window_aura.cc:69: // profile. Which indicates that multi-user is not active and an anonymous nit: change . to , between first two sentences (here and elsewhere in the CL) https://codereview.chromium.org/1310183009/diff/120001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1310183009/diff/120001/ui/views/widget/widget... ui/views/widget/widget.cc:361: default_theme_provider_.reset(new ui::DefaultThemeProvider); nit: newline after line 361
https://codereview.chromium.org/1310183009/diff/120001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1310183009/diff/120001/ui/views/widget/widget... ui/views/widget/widget.cc:361: default_theme_provider_.reset(new ui::DefaultThemeProvider); I was thinking this could be more like: theme_provider_ = in_params.theme_provider; if (theme_provider_) { DCHECK(is_toplevel_); } else { default_theme_provider_.reset(....); } Because silently ignoring a custom theme_provider would make it difficult to figure out what's going on when a developer adds one for a non-toplevel widget.
It seems like this CL may be on hold pending moving around some ThemeProvider methods, but if we do decide to move forward here, I suggest finding some common place for the currently-duplicated code to calculate the right theme provider to supply in the window params. The obvious location would be the callee that receives these window params. Why can't it just always calculate the right theme provider itself? Seems like all it needs is the |parent| field of |params|.
captive_portal_window_proxy.cc is no longer in this CL. Is it intentional? https://codereview.chromium.org/1310183009/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right): https://codereview.chromium.org/1310183009/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1023: ash::kShellWindowId_LockScreenContainer); this parent will never have associated profile. The screen is always locked by the 1st user, so i guess you have to use that? https://codereview.chromium.org/1310183009/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/1310183009/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:258: // On login screen and logged in as an anonymous user you might not get a Is it impossible to have omnibox in login screen, isn't it? Also, login screen is chromeos specific, so you should move that inside if defined cros.
On 2015/09/10 00:20:38, oshima wrote: > captive_portal_window_proxy.cc is no longer in this CL. Is it intentional? > > https://codereview.chromium.org/1310183009/diff/120001/chrome/browser/chromeo... > File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right): > > https://codereview.chromium.org/1310183009/diff/120001/chrome/browser/chromeo... > chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1023: > ash::kShellWindowId_LockScreenContainer); > this parent will never have associated profile. The screen is always locked by > the 1st user, so i guess you have to use that? > > https://codereview.chromium.org/1310183009/diff/120001/chrome/browser/ui/view... > File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): > > https://codereview.chromium.org/1310183009/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:258: // On login > screen and logged in as an anonymous user you might not get a > Is it impossible to have omnibox in login screen, isn't it? > > Also, login screen is chromeos specific, so you should move that inside if > defined cros. This CL is on hold pending: https://codereview.chromium.org/1338103002/ It is likely that this specific CL will not be needed. However the changes in Widget may become necessary pending work on theme coloration. If so I will upload a new review. I will close this review if the updates in 1338103002 allow for https://codereview.chromium.org/1306173002/ to land without needing this theming change. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
