Linux Aura: Use Aura theme on all windows when 'Use Classic theme' is used

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.

Continued from https://codereview.chromium.org/2667703002

BUG=132847
R=sky@chromium.org
CC=erg@chromium.org

Review-Url: https://codereview.chromium.org/2697513008
Cr-Commit-Position: refs/heads/master@{#451479}
Committed: https://chromium.googlesource.com/chromium/src/+/8919e3b79d0662a6fed9be43caf123861cd83408

[Tom (Use chromium acct), 2017-02-16 00:39:58]

sky ptal

The differences from the other cl:
* Use kThemeProfileKey instead of kProfileKey
* Use GetOriginalProfile for kThemeProfileKey

I've verified that the task manager has the same position/size when relaunching,
and there is no longer a browser crash when closing an incognito browser when
the task manager is open.

The following case also works:
1. Open browser windows for "person 1" and "person 2"
2. Open a task manager from "person 2"'s window
3. Close "person 2"'s window and delete "person 2"'s profile
4. Verify that the task manager still works

https://codereview.chromium.org/2697513008/diff/1/chrome/browser/ui/views/chr...
File chrome/browser/ui/views/chrome_views_delegate.cc (left):

https://codereview.chromium.org/2697513008/diff/1/chrome/browser/ui/views/chr...
chrome/browser/ui/views/chrome_views_delegate.cc:459: Profile* parent_profile =
reinterpret_cast<Profile*>(
note that we no longer set kProfileKey from the parent, as I interpreted from
your email comment

[sky, 2017-02-16 01:05:30]

You are assuming we never destroy the original profile. I'm not sure that is the
case. Did you verify that? Say you end up with multiple profiles and then close
the last window in that profile, or remove the profile.

[sky, 2017-02-16 01:06:36]

Also, kThemeProfileKey is only used in c/b/ui/views. Move it some where there.

[Tom (Use chromium acct), 2017-02-16 01:09:17]

On 2017/02/16 01:05:30, sky wrote:
> You are assuming we never destroy the original profile. I'm not sure that is
the
> case. Did you verify that? Say you end up with multiple profiles and then
close
> the last window in that profile, or remove the profile.

Is this verification sufficient: 

On 2017/02/16 00:39:58, Tom Anderson wrote:
> The following case also works:
> 1. Open browser windows for "person 1" and "person 2"
> 2. Open a task manager from "person 2"'s window
> 3. Close "person 2"'s window and delete "person 2"'s profile
> 4. Verify that the task manager still works

[sky, 2017-02-16 03:45:17]

I would prefer you ask an OWNER of c/b/profiles to see if we ever
destroy them once created.

  -Scott

On Wed, Feb 15, 2017 at 5:09 PM,  <thomasanderson@google.com> wrote:
> On 2017/02/16 01:05:30, sky wrote:
>> You are assuming we never destroy the original profile. I'm not sure that
>> is
> the
>> case. Did you verify that? Say you end up with multiple profiles and then
> close
>> the last window in that profile, or remove the profile.
>
> Is this verification sufficient:
>
> On 2017/02/16 00:39:58, Tom Anderson wrote:
>> The following case also works:
>> 1. Open browser windows for "person 1" and "person 2"
>> 2. Open a task manager from "person 2"'s window
>> 3. Close "person 2"'s window and delete "person 2"'s profile
>> 4. Verify that the task manager still works
>
>
> https://codereview.chromium.org/2697513008/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tom (Use chromium acct), 2017-02-16 19:26:18]

Patchset #2 (id:20001) has been deleted

[Tom (Use chromium acct), 2017-02-16 19:27:44]

thomasanderson@google.com changed reviewers:
+ erg@chromium.org

[Tom (Use chromium acct), 2017-02-16 19:27:45]

On 2017/02/16 03:45:17, sky wrote:
> I would prefer you ask an OWNER of c/b/profiles to see if we ever
> destroy them once created.
> 
>   -Scott
> 

+erg as a reviewer since he's an owner

On 2017/02/16 01:06:36, sky wrote:
> Also, kThemeProfileKey is only used in c/b/ui/views. Move it some where there.

moved it into c/b/ui/views/frame/browser_view.h

[Elliot Glaysher, 2017-02-16 20:20:10]

it's been a very long time since i've worked on profiles, but i believe profiles
will live until shutdown and aren't destroyed when windows are destroyed.

a quick grep through profile_manager.cc hints that this is correct;
browser_counts_ doesn't clear out a profile when it receives a
NOTIFICATION_BROWSER_CLOSED.

[sky, 2017-02-16 22:10:32]

https://codereview.chromium.org/2697513008/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
(right):

https://codereview.chromium.org/2697513008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:32:
ui::NativeTheme* GetNativeThemeForWindow(aura::Window* window) {
How is the window that is passed here obtained?

https://codereview.chromium.org/2697513008/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2697513008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.cc:450: #elif defined(OS_LINUX) &&
!defined(OS_CHROMEOS)
Won't this break windows? By that I mean windows no longer hits this branch,
right?

https://codereview.chromium.org/2697513008/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/2697513008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.h:107: static const char
kThemeProfileKey[];
As this is used in places other than browser_view, move to a separate place.
Maybe c/b/ui/views/theme_profile_key. I think it would be good if you move the
getter and the setter there and put the key in the .cc. I'm thinking of:

void SetThemeProfileForWindow(aura::Window* window, Profile* profile);
Profile* GetThemeProfileForWindow(aura::Window*);

I would also use UI_CLASS_PROPERTY rather than
SetNativeWindowProperty/GetNativeWindowProperty. Doing that means you won't need
the reinterpret_cast.

[Tom (Use chromium acct), 2017-02-17 00:35:32]

https://codereview.chromium.org/2697513008/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
(right):

https://codereview.chromium.org/2697513008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:32:
ui::NativeTheme* GetNativeThemeForWindow(aura::Window* window) {
On 2017/02/16 22:10:32, sky wrote:
> How is the window that is passed here obtained?

it's obtained from Widget
https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?rc...

https://codereview.chromium.org/2697513008/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2697513008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.cc:450: #elif defined(OS_LINUX) &&
!defined(OS_CHROMEOS)
On 2017/02/16 22:10:32, sky wrote:
> Won't this break windows? By that I mean windows no longer hits this branch,
> right?

windows no longer hits this branch

based on your email, I thought you meant to remove it:
"In looking at the patch again I think inheriting the profile from the
parent/context is problematic. We don't always want it (the task
manager is a good example of that, no doubt there are others). I think
we should be explicit and not attempt to infer the profile."

But I think I misinterpreted what you said, so I've added inheriting the
profile back on Windows.

https://codereview.chromium.org/2697513008/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/2697513008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.h:107: static const char
kThemeProfileKey[];
On 2017/02/16 22:10:32, sky wrote:
> As this is used in places other than browser_view, move to a separate place.
> Maybe c/b/ui/views/theme_profile_key. I think it would be good if you move the
> getter and the setter there and put the key in the .cc. I'm thinking of:
> 
> void SetThemeProfileForWindow(aura::Window* window, Profile* profile);
> Profile* GetThemeProfileForWindow(aura::Window*);
> 
> I would also use UI_CLASS_PROPERTY rather than
> SetNativeWindowProperty/GetNativeWindowProperty. Doing that means you won't
need
> the reinterpret_cast.

Done.

[sky, 2017-02-17 17:02:37]

LGTM with no namespace

https://codereview.chromium.org/2697513008/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
(right):

https://codereview.chromium.org/2697513008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:32:
ui::NativeTheme* GetNativeThemeForWindow(aura::Window* window) {
On 2017/02/17 00:35:31, Tom Anderson wrote:
> On 2017/02/16 22:10:32, sky wrote:
> > How is the window that is passed here obtained?
> 
> it's obtained from Widget
>
https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?rc...

Perfect!

https://codereview.chromium.org/2697513008/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/theme_profile_key.h (right):

https://codereview.chromium.org/2697513008/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/theme_profile_key.h:14: namespace ThemeProfileKey {
We generally don't use namespaces for files, and if we did it would typically be
lower_case. See long discussions on chromium-dev if curious. In other words, no
namespace here.

[Tom (Use chromium acct), 2017-02-17 21:30:54]

https://codereview.chromium.org/2697513008/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/theme_profile_key.h (right):

https://codereview.chromium.org/2697513008/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/theme_profile_key.h:14: namespace ThemeProfileKey {
On 2017/02/17 17:02:37, sky wrote:
> We generally don't use namespaces for files, and if we did it would typically
be
> lower_case. See long discussions on chromium-dev if curious. In other words,
no
> namespace here.

Done.

[Tom (Use chromium acct), 2017-02-17 21:30:59]

The CQ bit was checked by thomasanderson@google.com

[Tom (Use chromium acct), 2017-02-17 21:31:00]

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/2697513008/#ps100001
(title: "No namespace")

[commit-bot: I haz the power, 2017-02-17 21:31:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 23:30:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 23:30:51]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Tom (Use chromium acct), 2017-02-18 18:17:53]

The CQ bit was checked by thomasanderson@google.com

[commit-bot: I haz the power, 2017-02-18 18:18:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-18 18:22:12]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487441873270730,
"parent_rev": "3d591708efa7d967c4158fc03369a2bf11aeacc2", "commit_rev":
"8919e3b79d0662a6fed9be43caf123861cd83408"}

[commit-bot: I haz the power, 2017-02-18 18:22:53]

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.

Continued from https://codereview.chromium.org/2667703002

BUG=132847
R=sky@chromium.org
CC=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.

Continued from https://codereview.chromium.org/2667703002

BUG=132847
R=sky@chromium.org
CC=erg@chromium.org

Review-Url: https://codereview.chromium.org/2697513008
Cr-Commit-Position: refs/heads/master@{#451479}
Committed:
https://chromium.googlesource.com/chromium/src/+/8919e3b79d0662a6fed9be43caf1...
==========

[commit-bot: I haz the power, 2017-02-18 18:22:54]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/8919e3b79d0662a6fed9be43caf1...