Improve some naming

Improve some naming

"Native" theme is sometimes synonymous with "system" theme, and sometimes not. Change some uses of "native" to "system" to avoid confusion with the class called NativeTheme.

BUG=none

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

[Elliot Glaysher, 2014-04-21 23:31:47]

+msw

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
File chrome/browser/themes/theme_service_aurax11.h (right):

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service_aurax11.h:14: public
views::SystemThemeChangeObserver {
I think you're overshooting here, though this might be a collision with msw@'s
patch; NativeThemes can change for reasons other than being the system theme.

[Evan Stade, 2014-04-22 00:28:21]

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
File chrome/browser/themes/theme_service_aurax11.h (right):

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service_aurax11.h:14: public
views::SystemThemeChangeObserver {
On 2014/04/21 23:31:47, Elliot Glaysher wrote:
> I think you're overshooting here, though this might be a collision with msw@'s
> patch; NativeThemes can change for reasons other than being the system theme.

Note that there's another method with the same name, View::OnNativeThemeChanged,
which is NOT an implementation of this interface method. (This CL hopes to make
that less confusing by giving the methods different names.)

[Elliot Glaysher, 2014-04-22 17:47:21]

oh. oops. lgtm.

[msw, 2014-04-22 18:02:12]

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service.cc:439: else if
(ShouldInitWithNativeTheme())
nit: shouldn't this match SetNativeTheme / UseSystemTheme?

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service.h:83: virtual bool ShouldUseNativeFrame()
const OVERRIDE;
nit: this should match UseSystemTheme / SetNativeTheme.

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service.h:147: virtual bool
ShouldInitWithNativeTheme() const;
nit: this should match UseSystemTheme / SetNativeTheme.

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
File chrome/browser/themes/theme_service_aurax11.cc (right):

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service_aurax11.cc:98: if (UsingSystemTheme())
NotifyThemeChanged should be called regardless of this condition, as my
<https://codereview.chromium.org/239093007/> notes, "(Linux Aura didn't update
on changes with "Use Classic Theme", now it does)." You can test this by opting
to use the Classic Theme in about:settings and then changing the system theme to
High Contrast Inverse; the omnibox etc. won't update immediately.

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
File chrome/browser/themes/theme_service_aurax11.h (right):

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service_aurax11.h:14: public
views::SystemThemeChangeObserver {
On 2014/04/22 00:28:22, Evan Stade wrote:
> On 2014/04/21 23:31:47, Elliot Glaysher wrote:
> > I think you're overshooting here, though this might be a collision with
msw@'s
> > patch; NativeThemes can change for reasons other than being the system
theme.
> 
> Note that there's another method with the same name,
View::OnNativeThemeChanged,
> which is NOT an implementation of this interface method. (This CL hopes to
make
> that less confusing by giving the methods different names.)

Hopefully ThemeServiceAuraX11 won't need to observe NativeTheme objects at all
after <https://codereview.chromium.org/239093007>. Let's coordinate.

https://codereview.chromium.org/244893004/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/244893004/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:1236:
web_ui()->CallJavascriptFunction("BrowserOptions.setNativeThemeButtonEnabled",
Q: will you followup and rename this to be consistent?

https://codereview.chromium.org/244893004/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:1254: void
BrowserOptionsHandler::ThemesSetNative(const base::ListValue* args) {
Q: will you followup and rename this to be consistent?

https://codereview.chromium.org/244893004/diff/1/ui/views/linux_ui/system_the...
File ui/views/linux_ui/system_theme_change_observer.h (right):

https://codereview.chromium.org/244893004/diff/1/ui/views/linux_ui/system_the...
ui/views/linux_ui/system_theme_change_observer.h:13: class VIEWS_EXPORT
SystemThemeChangeObserver {
nit: I think this should become NativeThemeObserver (it's an interface that
objects may implement to be notified of changes to ui::NativeTheme instances) as
in my <https://codereview.chromium.org/239093007/>.

[Evan Stade, 2014-04-22 18:16:05]

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service.cc:439: else if
(ShouldInitWithNativeTheme())
On 2014/04/22 18:02:13, msw wrote:
> nit: shouldn't this match SetNativeTheme / UseSystemTheme?

good catch, I changed this partially but didn't get this instance. I guess it
compiled because I wasn't using clang.

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service.h:83: virtual bool ShouldUseNativeFrame()
const OVERRIDE;
On 2014/04/22 18:02:13, msw wrote:
> nit: this should match UseSystemTheme / SetNativeTheme.

you mean ShouldUseSystemFrame? Maybe, but "NativeFrame"/"SystemFrame" doesn't
lead to any ambiguity and that would be another largish orthogonal change.

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
File chrome/browser/themes/theme_service_aurax11.cc (right):

https://codereview.chromium.org/244893004/diff/1/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service_aurax11.cc:98: if (UsingSystemTheme())
On 2014/04/22 18:02:13, msw wrote:
> NotifyThemeChanged should be called regardless of this condition, as my
> <https://codereview.chromium.org/239093007/> notes, "(Linux Aura didn't update
> on changes with "Use Classic Theme", now it does)." You can test this by
opting
> to use the Classic Theme in about:settings and then changing the system theme
to
> High Contrast Inverse; the omnibox etc. won't update immediately.

Yea, I noticed that, but there's also a difference between a theme changing
(i.e. a different Chrome extension is in use) and a native theme changing.
Dealing with proper updating is a TODO. This CL is just a name change with
minimal behavioral differences.

https://codereview.chromium.org/244893004/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/244893004/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:1236:
web_ui()->CallJavascriptFunction("BrowserOptions.setNativeThemeButtonEnabled",
On 2014/04/22 18:02:13, msw wrote:
> Q: will you followup and rename this to be consistent?

possibly

https://codereview.chromium.org/244893004/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:1254: void
BrowserOptionsHandler::ThemesSetNative(const base::ListValue* args) {
On 2014/04/22 18:02:13, msw wrote:
> Q: will you followup and rename this to be consistent?

possibly

[Evan Stade, 2014-04-25 21:57:37]

updated, PTAL

[msw, 2014-04-25 23:28:36]

lgtm with nits and a question about the ntp behavior change.

https://codereview.chromium.org/244893004/diff/10001/chrome/browser/themes/th...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/244893004/diff/10001/chrome/browser/themes/th...
chrome/browser/themes/theme_service.h:146: // Returns true if the ThemeService
should use the native theme on startup.
nit: update comment.

https://codereview.chromium.org/244893004/diff/50001/chrome/browser/search_en...
File chrome/browser/search_engines/search_terms_data.cc (left):

https://codereview.chromium.org/244893004/diff/50001/chrome/browser/search_en...
chrome/browser/search_engines/search_terms_data.cc:183: // NTP is considered
themed if the theme is not default and not native (GTK+).
q: how does this change affect the NTP appearance for the system theme?

https://codereview.chromium.org/244893004/diff/50001/chrome/browser/themes/th...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/244893004/diff/50001/chrome/browser/themes/th...
chrome/browser/themes/theme_service.h:146: // Returns true if the ThemeService
should use the native theme on startup.
nit: s/native/system/

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

https://codereview.chromium.org/244893004/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:898: // We want to
use theme images provided by the platform theme when enabled,
nit: s/platform/system/

[sky, 2014-04-28 13:39:28]

LGTM

[Evan Stade, 2014-05-07 00:50:37]

https://codereview.chromium.org/244893004/diff/50001/chrome/browser/search_en...
File chrome/browser/search_engines/search_terms_data.cc (left):

https://codereview.chromium.org/244893004/diff/50001/chrome/browser/search_en...
chrome/browser/search_engines/search_terms_data.cc:183: // NTP is considered
themed if the theme is not default and not native (GTK+).
On 2014/04/25 23:28:37, msw wrote:
> q: how does this change affect the NTP appearance for the system theme?

I have reverted this change. I did the boolean logic in my head and thought it
didn't make sense, but writing it out, I think the clause I deleted does make
sense.

https://codereview.chromium.org/244893004/diff/50001/chrome/browser/themes/th...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/244893004/diff/50001/chrome/browser/themes/th...
chrome/browser/themes/theme_service.h:146: // Returns true if the ThemeService
should use the native theme on startup.
On 2014/04/25 23:28:37, msw wrote:
> nit: s/native/system/

Done.

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

https://codereview.chromium.org/244893004/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:898: // We want to
use theme images provided by the platform theme when enabled,
On 2014/04/25 23:28:37, msw wrote:
> nit: s/platform/system/

Done.

[Evan Stade, 2014-05-07 00:50:43]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-05-07 00:53:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/244893004/70001

[commit-bot: I haz the power, 2014-05-07 14:35:32]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_clang_dbg on tryserver.chromium
  linux_chromium_rel on tryserver.chromium
  mac_chromium_compile_dbg on tryserver.chromium
  mac_chromium_rel on tryserver.chromium
  win_chromium_compile_dbg on tryserver.chromium
  win_chromium_rel on tryserver.chromium
  win_chromium_x64_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 19:52:13]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_clang_dbg on tryserver.chromium
  linux_chromium_rel on tryserver.chromium
  mac_chromium_compile_dbg on tryserver.chromium
  mac_chromium_rel on tryserver.chromium
  win_chromium_rel on tryserver.chromium
  win_chromium_x64_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 19:59:27]

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

[commit-bot: I haz the power, 2014-05-07 19:59:28]

Try jobs failed on following builders:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_clang_dbg on tryserver.chromium
  linux_chromium_rel on tryserver.chromium

[Evan Stade, 2014-05-07 21:07:13]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-05-07 21:10:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/244893004/90001

[Evan Stade, 2014-05-07 23:11:36]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-05-07 23:14:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/244893004/110001

[commit-bot: I haz the power, 2014-05-08 14:01:06]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-08 15:51:22]

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

[commit-bot: I haz the power, 2014-05-08 15:51:23]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[Evan Stade, 2014-05-12 22:40:30]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-05-12 22:40:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/244893004/120001

[commit-bot: I haz the power, 2014-05-13 03:22:59]

Change committed as 269985