Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2667703002:
Linux Aura: Use Aura theme on all windows when 'Use Classic theme' is used (Closed)
|
Created:
3 years, 10 months ago by Tom (Use chromium acct) Modified:
3 years, 10 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinux Aura: Use Aura theme on all windows when 'Use Classic theme' is used
Previously, windows like tooltips, context menus, and the task manager
always used the Gtk theme even when "Use Classic theme" was selected
(whereas browser windows would be affected). This CL makes it so the
Aura theme is used on all windows if the classic theme is used.
BUG=132847
R=sky@chromium.org,erg@chromium.org
Review-Url: https://codereview.chromium.org/2667703002
Cr-Commit-Position: refs/heads/master@{#447312}
Committed: https://chromium.googlesource.com/chromium/src/+/66478cfd170f564932f42a2ca7cbb0b5bf779480
Patch Set 1 #
Total comments: 6
Patch Set 2 : address sky@'s comments #
Total comments: 1
Patch Set 3 : Don't use NativeWidgetPrivate #
Total comments: 4
Patch Set 4 : nullptr -> profile, move SetNativeWindowProperty #
Total comments: 2
Patch Set 5 : widget -> native_widget #Patch Set 6 : && -> || #
Total comments: 1
Messages
Total messages: 26 (11 generated)
sky@ please review erg@ FYI erg: This fixes the bug we discussed on Friday where menus and the task manager would still use the Gtk theme even when the classic theme was used I've tested with multiple profiles, and everything seems to work https://codereview.chromium.org/2667703002/diff/1/chrome/browser/ui/views/chr... File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc (left): https://codereview.chromium.org/2667703002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:37: // Window types not listed here (such as tooltips) will never use Chrome I'm not sure why this was added initially, but it makes more sense to me that the setting should be applied globally https://codereview.chromium.org/2667703002/diff/1/chrome/browser/ui/views/chr... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2667703002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_views_delegate.cc:459: auto* context = params->parent ? params->parent : params->context; Not entirely sure if params->context is OK to use here, but params->parent is sometimes null (on tooltips and the task manager)
https://codereview.chromium.org/2667703002/diff/1/chrome/browser/ui/views/chr... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2667703002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_views_delegate.cc:459: auto* context = params->parent ? params->parent : params->context; On 2017/01/30 20:40:32, Tom Anderson wrote: > Not entirely sure if params->context is OK to use here, but params->parent is > sometimes null (on tooltips and the task manager) context is ok as long as we only use GetNativeWindow()/GetNativeView() to set it, which we generally do. Please doesn't use auto here as increased readability is better than the slight savings. Similarly don't name this context as that's too confusing given there is params->context and what you are setting here isn't the same. https://codereview.chromium.org/2667703002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_views_delegate.cc:462: reinterpret_cast<views::internal::NativeWidgetPrivate*>( static_cast?
https://codereview.chromium.org/2667703002/diff/1/chrome/browser/ui/views/chr... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2667703002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_views_delegate.cc:462: reinterpret_cast<views::internal::NativeWidgetPrivate*>( On 2017/01/31 00:44:12, sky wrote: > static_cast? Done.
https://codereview.chromium.org/2667703002/diff/1/chrome/browser/ui/views/chr... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2667703002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_views_delegate.cc:459: auto* context = params->parent ? params->parent : params->context; On 2017/01/31 00:44:12, sky wrote: > On 2017/01/30 20:40:32, Tom Anderson wrote: > > Not entirely sure if params->context is OK to use here, but params->parent is > > sometimes null (on tooltips and the task manager) > > context is ok as long as we only use GetNativeWindow()/GetNativeView() to set > it, which we generally do. > > Please doesn't use auto here as increased readability is better than the slight > savings. Similarly don't name this context as that's too confusing given there > is params->context and what you are setting here isn't the same. Done.
https://codereview.chromium.org/2667703002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2667703002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:461: auto* native_widget = static_cast<views::internal::NativeWidgetPrivate*>( Sorry, one more. While params->native_widget is definitely a NativeWidgetPrivate, NativeWidgetPrivate is meant only to be used by Widget. So, please put put the SetNativeWindowProperty() calls in each branch above. If you combine the branches there isn't as much code duplication, e.g.: gfx::NativeWindow parent_or_context = (what you call parent); if (!params->parent && !params->context && !params->child && !use_non_toplevel_window) { create DesktopNativeWidgetAura; } else { Create NativeWidgetAura; }
On 2017/01/31 05:14:31, sky wrote: > https://codereview.chromium.org/2667703002/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/chrome_views_delegate.cc (right): > > https://codereview.chromium.org/2667703002/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/chrome_views_delegate.cc:461: auto* native_widget = > static_cast<views::internal::NativeWidgetPrivate*>( > Sorry, one more. While params->native_widget is definitely a > NativeWidgetPrivate, NativeWidgetPrivate is meant only to be used by Widget. So, > please put put the SetNativeWindowProperty() calls in each branch above. If you > combine the branches there isn't as much code duplication, e.g.: > > gfx::NativeWindow parent_or_context = (what you call parent); > if (!params->parent && !params->context && !params->child && > !use_non_toplevel_window) { > create DesktopNativeWidgetAura; > } else { > Create NativeWidgetAura; > } Done, but I had to make SetNativeWindowProperty public in ui/views/widget/desktop_aura/desktop_native_widget_aura.h
https://codereview.chromium.org/2667703002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2667703002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:465: widget->SetNativeWindowProperty(Profile::kProfileKey, nullptr); Don't you want profile here? Also, you should be able to call SetNativeWindowProperty with null, so no conditional. https://codereview.chromium.org/2667703002/diff/40001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.h (right): https://codereview.chromium.org/2667703002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.h:101: void SetNativeWindowProperty(const char* name, void* value) override; Move implementation to match position.
https://codereview.chromium.org/2667703002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2667703002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:465: widget->SetNativeWindowProperty(Profile::kProfileKey, nullptr); On 2017/01/31 17:37:18, sky wrote: > Don't you want profile here? Also, you should be able to call > SetNativeWindowProperty with null, so no conditional. Done. Yes it was broken with nullptr https://codereview.chromium.org/2667703002/diff/40001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.h (right): https://codereview.chromium.org/2667703002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.h:101: void SetNativeWindowProperty(const char* name, void* value) override; On 2017/01/31 17:37:18, sky wrote: > Move implementation to match position. Done.
LGTM with names changed. https://codereview.chromium.org/2667703002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2667703002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:461: views::DesktopNativeWidgetAura* widget = As DesktopNativeWidgetAura (and NativeWidgetAura below) are not widgets, using a variable with the name widget is confusing. widget->native_widget here and below.
https://codereview.chromium.org/2667703002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2667703002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:461: views::DesktopNativeWidgetAura* widget = On 2017/01/31 18:14:23, sky wrote: > As DesktopNativeWidgetAura (and NativeWidgetAura below) are not widgets, using a > variable with the name widget is confusing. widget->native_widget here and > below. Done.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2667703002/#ps80001 (title: "widget -> native_widget")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by thomasanderson@google.com
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2667703002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2667703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate.cc:459: if ((!params->parent && !params->context && !params->child) || For the record, I also had to fix the logic that got changed when the branches of this conditional got merged. It should have been || instead of &&. This caused me not to be able to launch the task manager. The trybots were still green though
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2667703002/#ps100001 (title: "&& -> ||")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485893380791950,
"parent_rev": "5062447d399be019735314f765d8d21b9433d995", "commit_rev":
"66478cfd170f564932f42a2ca7cbb0b5bf779480"}
Message was sent while issue was closed.
Description was changed from ========== Linux Aura: Use Aura theme on all windows when 'Use Classic theme' is used Previously, windows like tooltips, context menus, and the task manager always used the Gtk theme even when "Use Classic theme" was selected (whereas browser windows would be affected). This CL makes it so the Aura theme is used on all windows if the classic theme is used. BUG=132847 R=sky@chromium.org,erg@chromium.org ========== to ========== Linux Aura: Use Aura theme on all windows when 'Use Classic theme' is used Previously, windows like tooltips, context menus, and the task manager always used the Gtk theme even when "Use Classic theme" was selected (whereas browser windows would be affected). This CL makes it so the Aura theme is used on all windows if the classic theme is used. BUG=132847 R=sky@chromium.org,erg@chromium.org Review-Url: https://codereview.chromium.org/2667703002 Cr-Commit-Position: refs/heads/master@{#447312} Committed: https://chromium.googlesource.com/chromium/src/+/66478cfd170f564932f42a2ca7cb... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/66478cfd170f564932f42a2ca7cb...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2693783005/ by thomasanderson@google.com. The reason for reverting is: Browser crash repro: 1. Open browser window 2. Open incognito window 3. Open task manager from incognito window 4. Close incognito window thomasanderson@ and sky@ are currently discussing a solution offline. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
