Linux Aura: Use system title bar is now set by preference, not flag.

Linux Aura: Use system title bar is now set by preference, not flag.

The "Use system title bar and borders" preference can now be set on
Linux Aura, through the settings page or tabstrip menu, and is used to
determine whether the system title bar or custom Chrome title bar style
is used. Changing the preference dynamically updates the browser style.

Removed the --use-system-title-bar flag.

BUG=317859
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250995

[Matt Giuca, 2014-02-03 04:13:14]

Huge review, sorry. I can split up certain features (like adding to the menu)
and refactoring if somebody objects to the size of this CL, but I thought it was
best to get it all done in one go.

Please note that this CL is dependent on http://crrev.com/136093007/ which has
not yet been reviewed.

cpu: chrome/app/chrome_command_ids.h
erg: ui/views/widget/desktop_aura/desktop_root_window_host_x11.*
msw: chrome/browser/ui/views/*
pam: chrome/browser/prefs/*, chrome/browser/resources/*
sky: ui/views/* (except desktop_root_window_host_x11)
   ^
   |
Achievement unlocked: 5 reviewers all with 3-character login names on the same
CL!

[cpu_(ooo_6.6-7.5), 2014-02-03 22:50:02]

lgtm on file: chrome_command_ids.h

[Elliot Glaysher, 2014-02-03 23:48:06]

On 2014/02/03 22:50:02, cpu wrote:
> lgtm on file: chrome_command_ids.h

lgtm.

(I tried patching this in locally to try it, but the patch didn't apply. Is this
dependent on another patch?)

[Matt Giuca, 2014-02-03 23:57:30]

Yes, it is dependent on http://crrev.com/136093007/

[sky, 2014-02-04 01:03:17]

https://codereview.chromium.org/146363002/diff/340001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.h (right):

https://codereview.chromium.org/146363002/diff/340001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.h:59: virtual void
FrameTypeChanged(Widget::FrameType new_type) OVERRIDE;
Why can't whoever needs this use Widget::frame_type() to query for it?

[Pam (message me for reviews), 2014-02-05 13:11:30]

chrome/browser/prefs* and chrome/browser/resources/* lgtm

- pam

[Matt Giuca, 2014-02-06 00:28:50]

https://codereview.chromium.org/146363002/diff/340001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.h (right):

https://codereview.chromium.org/146363002/diff/340001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.h:59: virtual void
FrameTypeChanged(Widget::FrameType new_type) OVERRIDE;
On 2014/02/04 01:03:18, sky wrote:
> Why can't whoever needs this use Widget::frame_type() to query for it?

Done.

Umm... feeling kind of silly that I didn't spot this :(

[msw, 2014-02-06 00:50:26]

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1959: IDS_FLAGS_USE_SYSTEM_TITLE_BAR,
You should remove these two string resources from
chrome/app/generated_resources.grd as well.

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:85: <if expr="is_posix and
not pp_ifdef('chromeos') and not is_macosx">
Will this break GTK, which is shipping for M-24?

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller.cc:477: #if defined(OS_LINUX) &&
!defined(OS_CHROMEOS)
nit: && !defined(TOOLKIT_GTK)?

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller.cc:907: #if defined(OS_LINUX) &&
!defined(OS_CHROMEOS)
nit: && !defined(TOOLKIT_GTK)?

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:39:
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
nit: why is the function called RegisterBrowserView*Syncable*Prefs if this pref
is an UNSYNCABLE_PREF? Can you clarify the syncability?

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame.cc:295:
set_frame_type(UseCustomFrame() ? Widget::FRAME_TYPE_DEFAULT
When is use_custom_frame_pref_ updated? I only see the init.
(will use_custom_frame_pref_ be updated before this call?)

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame.h (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame.h:138: // Whether the custom chrome
frame pref is set.  Normally you want to use
nit: capitalize "Chrome"; remove "Normally you want to"; consider: "Check
UseCustomFrame()"

[Elliot Glaysher, 2014-02-06 00:52:30]

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:85: <if expr="is_posix and
not pp_ifdef('chromeos') and not is_macosx">
On 2014/02/06 00:50:27, msw wrote:
> Will this break GTK, which is shipping for M-24?

Theoretically, this shouldn't.

[Pam (message me for reviews), 2014-02-06 10:31:26]

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:39:
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
On 2014/02/06 00:50:27, msw wrote:
> nit: why is the function called RegisterBrowserView*Syncable*Prefs if this
pref
> is an UNSYNCABLE_PREF? Can you clarify the syncability?

It's because the method takes a PrefRegistrySyncable, because of how and when
it's called. We could in principle reorganize browser_prefs.cc a bit to allow
classes that only need UNSYNCABLE_PREFs to make that self-evident by using a
PrefRegistrySimple instead, but right now this is the consistent way to register
profile prefs, even if they don't want to be synced.

Naming this RegisterBrowserViewProfilePrefs, and renaming
RegisterBrowserViewPrefs to RegisterBrowserViewLocalPrefs, would make it all
clearer.

[Matt Giuca, 2014-02-11 04:57:38]

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1959: IDS_FLAGS_USE_SYSTEM_TITLE_BAR,
On 2014/02/06 00:50:27, msw wrote:
> You should remove these two string resources from
> chrome/app/generated_resources.grd as well.

Done.

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:85: <if expr="is_posix and
not pp_ifdef('chromeos') and not is_macosx">
Tested on GTK; it still shows the checkbox. (Not sure what the concern was.)

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller.cc:477: #if defined(OS_LINUX) &&
!defined(OS_CHROMEOS)
On 2014/02/06 00:50:27, msw wrote:
> nit: && !defined(TOOLKIT_GTK)?

Done.

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller.cc:907: #if defined(OS_LINUX) &&
!defined(OS_CHROMEOS)
On 2014/02/06 00:50:27, msw wrote:
> nit: && !defined(TOOLKIT_GTK)?

Done.

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:39:
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
Are you saying I should do that renaming now?

It's a little unclear to me. What is a "local pref"? Is it the same thing as a
SimplePref? Why not call it RegisterBrowserViewSimplePrefs (instead of
LocalPrefs).

A SyncablePref is not necessarily a pref that is synced, it's a pref whose
syncability is customizable. Once you understand what a SyncablePref is, it
doesn't seem like RegisterBrowserViewSyncablePrefs is an inappropriate name.

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame.cc:295:
set_frame_type(UseCustomFrame() ? Widget::FRAME_TYPE_DEFAULT
On 2014/02/06 00:50:27, msw wrote:
> When is use_custom_frame_pref_ updated? I only see the init.
> (will use_custom_frame_pref_ be updated before this call?)

It gets updated automatically when PrefService::SetBoolean is called (see
browser_command_controller.cc).

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame.h (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame.h:138: // Whether the custom chrome
frame pref is set.  Normally you want to use
On 2014/02/06 00:50:27, msw wrote:
> nit: capitalize "Chrome"; remove "Normally you want to"; consider: "Check
> UseCustomFrame()"

Done. Just removed the second sentence -- there is no point having every field
mention its associated getter. (This was just copy+pasted from
browser_window_gtk.h).

[msw, 2014-02-11 19:32:58]

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:85: <if expr="is_posix and
not pp_ifdef('chromeos') and not is_macosx">
On 2014/02/11 04:57:43, Matt Giuca wrote:
> Tested on GTK; it still shows the checkbox. (Not sure what the concern was.)

I wasn't sure if we want this on GTK or not. You addressed my
browser_command_controller.cc nits to exclude GTK, but then you noted that the
checkbox is still available on GTK here. I'm still a bit confused.

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:39:
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
On 2014/02/11 04:57:43, Matt Giuca wrote:
> Are you saying I should do that renaming now?
> 
> It's a little unclear to me. What is a "local pref"? Is it the same thing as a
> SimplePref? Why not call it RegisterBrowserViewSimplePrefs (instead of
> LocalPrefs).
> 
> A SyncablePref is not necessarily a pref that is synced, it's a pref whose
> syncability is customizable. Once you understand what a SyncablePref is, it
> doesn't seem like RegisterBrowserViewSyncablePrefs is an inappropriate name.

Can't you just use the existing RegisterBrowserViewPrefs above to register this
un-syncable pref with the PrefRegistrySimple interface?

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame.cc:90: base::Unretained(this)));
Using base::Unretained is generally unsafe. Is the callback discarded when this
BooleanPrefMember instance is destroyed (ie. when the BrowserFrame is being
destroyed)? Otherwise, this could crash if it were called after the BrowserFrame
instance were destroyed; which is unlikely, but possible. I think it'd be
preferable to use a WeakPtrFactory or some observer pattern, unless the
BooleanPrefMember destruction invalidates the callback.

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame.cc:295:
set_frame_type(UseCustomFrame() ? Widget::FRAME_TYPE_DEFAULT
If the default is force native, won't this toggle do nothing? Maybe default is
not native, since DesktopWindowTreeHostX11::ShouldUseNativeFrame() and
NativeWidgetAura::ShouldUseNativeFrame() return false, but perhaps this toggle
should force between native and custom anyway.

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame.h (right):

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame.h:138: // Whether the custom Chrome
frame pref is set.
nit: s/pref/preference/

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:10: #include
"base/command_line.h"
nit: remove this include.

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:27: #include
"chrome/common/chrome_switches.h"
nit: remove this include.

[Matt Giuca, 2014-02-12 01:09:18]

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:85: <if expr="is_posix and
not pp_ifdef('chromeos') and not is_macosx">
This is already a GTK feature. We aren't trying to remove it on GTK, just add it
on Aura. (Maybe that explains the confusion?)

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/146363002/diff/620001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:39:
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
I assumed that the simple pref registry would cause it to be synced by default.
I was just copying the code for the existing GTK implementation, which
explicitly uses PrefRegistrySyncable to register this as an unsyncable pref.

Maybe pam can elaborate; I'm not too familiar with the pref registry system.

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame.cc:90: base::Unretained(this)));
Good question.

It looks like your last sentence is true: the destructor for PrefMember calls
RemovePrefObserver to remove itself automatically, which should ensure the
callback is not called after the destruction of `this`.

Again, I am copying this code from the existing GTK implementation which has
been working fine for years, and followed the same model (see BrowserWindowGtk).

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame.cc:295:
set_frame_type(UseCustomFrame() ? Widget::FRAME_TYPE_DEFAULT
On 2014/02/11 19:32:59, msw wrote:
> If the default is force native, won't this toggle do nothing?

The default is force custom, with a weird condition that if the native flag is
on when the window is created, it is default with a native frame.

> Maybe default is
> not native, since DesktopWindowTreeHostX11::ShouldUseNativeFrame() and
> NativeWidgetAura::ShouldUseNativeFrame() return false, but perhaps this toggle
> should force between native and custom anyway.

Yeah, that's probably better. I have changed it so that on Linux, it is never
the default -- it is always set to FORCE_CUSTOM or FORCE_NATIVE depending on the
startup setting, and this toggle switches between the two. That avoids the weird
edge case involving DEFAULT mentioned above.

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame.h (right):

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame.h:138: // Whether the custom Chrome
frame pref is set.
On 2014/02/11 19:32:59, msw wrote:
> nit: s/pref/preference/

Done.

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:10: #include
"base/command_line.h"
On 2014/02/11 19:32:59, msw wrote:
> nit: remove this include.

Done.

https://codereview.chromium.org/146363002/diff/940001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:27: #include
"chrome/common/chrome_switches.h"
On 2014/02/11 19:32:59, msw wrote:
> nit: remove this include.

Done.

[msw, 2014-02-12 18:03:42]

LGTM, though I'd like to see use of PrefRegistrySimple if that is non-syncable.
(sadly, the class comment doesn't clarify if it syncs or not; +CC joi for
advice)

[Jói, 2014-02-12 19:50:17]

If the preferences you added are per-profile, then you should use
PrefRegistrySyncable, and you should rename your function
RegisterBrowserViewProfilePrefs.

If they are local state (i.e. prefs that are not per profile) then
they never get synced and you should use the existing registration
function, which takes a PrefRegistrySimple.

Cheers,
Jói


On Wed, Feb 12, 2014 at 6:03 PM,  <msw@chromium.org> wrote:
> LGTM, though I'd like to see use of PrefRegistrySimple if that is
> non-syncable.
> (sadly, the class comment doesn't clarify if it syncs or not; +CC joi for
> advice)
>
> https://codereview.chromium.org/146363002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Matt Giuca, 2014-02-12 23:12:30]

On 2014/02/12 19:50:17, Jói wrote:
> If the preferences you added are per-profile, then you should use
> PrefRegistrySyncable, and you should rename your function
> RegisterBrowserViewProfilePrefs.

Ah, thanks. That makes sense. So these are per-profile prefs, but not synced.

So I am renaming RegisterBrowserViewPrefs to RegisterBrowserViewLocalPrefs, and
RegisterBrowserViewSyncablePrefs to RegisterBrowserViewProfilePrefs. See patch
set 16. Is that good?

[msw, 2014-02-12 23:35:25]

LGTM, thanks!

[Matt Giuca, 2014-02-13 00:11:15]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-02-13 00:12:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/146363002/1450001

[commit-bot: I haz the power, 2014-02-13 02:21:48]

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

[commit-bot: I haz the power, 2014-02-13 02:21:49]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, google_apis_unittests,
gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Matt Giuca, 2014-02-13 05:03:09]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-02-13 05:03:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/146363002/1450001

[commit-bot: I haz the power, 2014-02-13 14:43:38]

Change committed as 250995