Brave theme #351

simonhong · 2018-08-22T00:37:28Z

Make our theme color chosen at first
Introduce kBraveThemeType preference to switch light and dark theme

Close brave/brave-browser#808
Issue brave/brave-browser#789

Captured image on Win10.

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.
Add appropriate QA labels (QA/Needed or QA/No-QA-Needed) to include the closed issue in milestone

Test Plan:

yarn test brave_browser_tests --filter=BraveThemeServiceTest.BraveThemeChangeTest

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

bridiver · 2018-08-22T04:47:38Z

patches/chrome-browser-themes-theme_service.cc.patch

+
+ SkColor ThemeService::BrowserThemeProvider::GetColor(int id) const {
+#if defined(BRAVE_CHROMIUM_BUILD)
+  MAYBE_OVERRIDE_DEFAULT_COLOR_FOR_BRAVE(id, incognito_, theme_service_.profile())


why aren't we just using a theme service with the color we want?

Clients can get color value from ui::ThemeProvider interface and BrowserThemeProvider is the implementation.

I don't understand. The ThemeService comes from ThemeServiceFactory and we can change that by creating our own and doing a preprocessor replace in ThemeServiceFactory

#define ThemeService BraveThemeService, #define ThemeServiceWin BraveThemeServiceWin, etc... no patch or additional prefs, etc... required

prefs is needed to store current theme type(dark or light).

I think CustomThemeSupplier is not an option for us if we will allow user theme extension install. Only one theme supplier is used at once.
When user installs theme, we don't have any chance to overriding any color values.

If we want to override default colors,
calling MAYBE_OVERRIDE_DEFAULT_COLOR_FOR_BRAVE(id, incognito, profile())
before GetDefaultColor(id, incognito) like below is not bad.

SkColor ThemeService::GetColor(int id, bool incognito, bool* has_custom_color) const { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); if (has_custom_color) *has_custom_color = false; // The incognito NTP always uses the default background color, unless there is // a custom NTP background image. See also https://crbug.com/21798#c114. if (id == ThemeProperties::COLOR_NTP_BACKGROUND && incognito && !HasCustomImage(IDR_THEME_NTP_BACKGROUND)) { return ThemeProperties::GetDefaultColor(id, incognito); } SkColor color; const int theme_supplier_id = incognito ? GetIncognitoId(id) : id; if (theme_supplier_ && theme_supplier_->GetColor(theme_supplier_id, &color)) { if (has_custom_color) *has_custom_color = true; return color; } #if defined(BRAVE_CHROMIUM_BUILD) MAYBE_OVERRIDE_DEFAULT_COLOR_FOR_BRAVE(id, incognito, profile()) #endif return GetDefaultColor(id, incognito); }

With this, user installed theme is applied(if exist) and we can override NotOverwritableByUserThemeProperty properties.

Also, we can do this by overriding GetDefaultColor().
This method will introduce new three classes BraveThemeServiceWin,
BraveThemeServiceAuraX11 and BraveThemeService.
We can use overriding technique for BraveThemeServiceWin and BraveThemeServiceAuraX11 w/o patching. However, BraveThemeService needs patching because there are many ThemeService in theme_service_factory.cc.

So, overriding GetDefaultColor() causes three new classes and patch file.

BraveThemeService does not need patching

and we don't even necessarily need our own CustomThemeSupplier if we're overriding ThemeService

this just isn't a scalable method of making changes, we can't keep patching in hacks like this now that we have better ways to handle it

bridiver · 2018-08-22T14:55:44Z

browser/themes/BUILD.gn

@@ -2,8 +2,11 @@ source_set("themes") {
  sources = [
    "theme_properties.cc",
    "theme_properties.h",
+    "theme_util.cc",


these should be in a test only target

those apis will be used by settings handler to change theme preference.

bridiver · 2018-08-23T03:00:19Z

chromium_src/chrome/browser/themes/theme_service_factory.cc

+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "build/build_config.h"  // For OS_LINUX
+


we need to include theme_service.h before we do anything else so we don't accidentally modify it

bridiver · 2018-08-23T03:11:45Z

browser/themes/theme_properties.h


-#define MAYBE_OVERRIDE_DEFAULT_COLOR_FOR_BRAVE(id, incognito) \
-  const base::Optional<SkColor> braveColor = MaybeGetDefaultColorForBraveUi(id, incognito); \
+#define MAYBE_OVERRIDE_DEFAULT_COLOR_FOR_BRAVE(id, incognito, profile) \


I don't think we need this anymore, we can just call BraveThemeService::GetColor

Sorry, I can't catch what you mean.
ThemeService::GetColor() is private method.
Also, BraveThemeService::GetDefaultColor() gets color value using MAYBE_OVERRIDE....

I'm saying that BraveThemeService::GetColor shouldn't use MAYBE_OVERRIDE_DEFAULT_COLOR_FOR_BRAVE and everything else should call BraveThemeService::GetColor

there's no reason to use a define for this anymore. It was a hack to allow us to return in the calling method

at the very least you could call MaybeGetDefaultColorForBraveUi directly, but we can move the logic from MaybeGetDefaultColorForBraveUi into BraveThemeService and get rid of both

the problem with the macro is that it hides the fact that it will sometimes return early in the method. That should be explicit inside the method.

This macro is deleted.

Also, ThemeServiceWin and ThemeServiceAuraX11 became a subclass of BraveThemeService.
I think it's more clear class hierarchy.

bridiver · 2018-08-23T05:44:47Z

browser/brave_profile_prefs.cc

@@ -24,6 +25,8 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {

  RegisterAlternatePrivateSearchEngineProfilePrefs(registry);

+  RegisterProfilePrefsForBraveThemeType(registry);


RegisterProfilePrefsForBraveThemeType -> BraveThemeService::RegisterProfilePrefs

Done, all apis in theme_util is moved to static method of BraveThemeService.

simonhong · 2018-08-23T11:47:50Z

browser/themes/theme_properties_unittest.cc

-#include "chrome/browser/themes/theme_properties.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-TEST(BraveThemeTest, ObtainsBraveOverrideColors) {


@bbondy This test isn't valid anymore because overriding isn't done in ThemeProperties::GetDefaultColor.
Instead, newly added BraveThemeServiceTest covers it.

bridiver · 2018-08-23T15:01:21Z

browser/themes/BUILD.gn

  deps = [
+    "//base",
+    "//components/version_info",


looks like we also need //brave/common, //components/prefs, //components/pref_registry and possibly //chrome/browser unless that causes a circular dependency issue

also //chrome/common from theme_properties

Done except //chrome/browser(circular)

bridiver · 2018-08-23T15:05:38Z

browser/themes/BUILD.gn

  deps = [
+    "//base",
+    "//components/version_info",
+    "//net",


Deleted. gn complained about net headers but I think it is fixed by above addition.

bridiver · 2018-08-23T15:10:09Z

chromium_src/chrome/browser/themes/theme_service_win.h

+#define DISALLOW_COPY_AND_ASSIGN(TypeName) \
+  DISALLOW_COPY(TypeName);                 \
+  DISALLOW_ASSIGN(TypeName);               \
+  friend class BraveThemeServiceWin


I think we need to check TypeName here to prevent accidentally inserting the friend into other headers

bridiver · 2018-08-23T15:11:57Z

chromium_src/chrome/browser/themes/theme_service_win.h

+  DISALLOW_COPY(TypeName);                 \
+  DISALLOW_ASSIGN(TypeName)
+
+#undef ThemeService


I think we need to restore the original ThemeService here

I added #define ThemeService ThemeService.
However, there is no problem whether it added or not.

yes, but it's safer this way in case something else includes this

bridiver · 2018-08-23T22:13:36Z

browser/themes/brave_theme_service.cc

+}
+
+// static
+int BraveThemeService::GetBraveThemeType(Profile* profile) {


I think GetBraveThemeType and SetBraveThemeType should go into brave_theme_service_browsertest.cc or into another file target that is testonly. We don't want people to accidentally use these outside of tests

We will have a setting option to switch dark or light. These apis can be used from settings hander.

I think we would handle that with a pref observer. In either case I don't think we should call NotifyThemeChanged like that outside of tests. It isn't correct usage because the theme hasn't actually changed at that point

I don't mind GetBraveThemeType, but it should return a BraveThemeType

In fact I think we need a pref observer now for this to work correctly. We're basically counting on something else updating the UI and triggering a refresh of the theme values

I guess technically the theme has changed when you change the pref value, but the pref observer should call NotifyThemeChanged because that isn't even happening outside of tests right now

also the prefs UI isn't set up to deal with something like SetBraveThemeType. It just changes the pref value and pref observers are supposed to take care of the rest

How about making SetBraveThemeType just change prefs and BraveThemeService act as our prefs observer?

I guess that's fine, I just don't see the point of having SetBraveThemeType if it's just setting the pref. We'll never use it in the browser because the UI will set the pref directly.

Setter is moved to test file and BraveThemeService observers prefs and will trigger theme change notification.
Getter is used by MaybeGetDefaultColorForBraveUi().

bridiver · 2018-08-24T02:04:36Z

chromium_src/chrome/browser/themes/theme_service_win.h

+#define DISALLOW_COPY_AND_ASSIGN(TypeName)                                                    \
+  DISALLOW_COPY(TypeName);                                                                    \
+  DISALLOW_ASSIGN(TypeName);                                                                  \
+  static_assert(strings_equal(#TypeName, "ThemeServiceWin"), "Use only for ThemeServiceWin"); \


just leaving a comment here from our DM discussion to make sure we don't forget to change this to selectively apply friend instead of asserting

Will be reverted to patching friend class. I can't find how conditionally add in macro.

I concur. Not worth the time right now

With kBraveThemeType, we can switch dark/light theme.

With this, theme extension have a chance to override theme color.

BraveThemeService became subclass of ThemeServiceAuraX11 and ThemeServiceWin. Also apis in theme_util.h are moved to static methods of BraveThemeService.

This unittest can't be used because override is done in ThemeService. BraveThemeServiceTest can cover this test. Also static check is added to prevent other class should not use newly defined DISALLOW_COPY_AND_ASSIGN.

I tried to conditionally add friend statement in DISALLOW_COPY_AND_ASSIGN. However, I can't find how to do it in macro.

bridiver · 2018-08-24T16:06:27Z

patches/chrome-browser-themes-theme_service_win.h.patch

+   ~ThemeServiceWin() override;
+
+  private:
+#if defined(BRAVE_CHROMIUM_BUILD)


just fyi - we don't need #if defined(BRAVE_CHROMIUM_BUILD) for these because they don't change behavior for the original class

done. Thanks!

Add support for translating brave_generated_resources.grd

simonhong self-assigned this Aug 22, 2018

simonhong requested a review from bbondy August 22, 2018 00:37

simonhong changed the title ~~Make our theme color chosen at first~~ Brave theme Aug 22, 2018

simonhong requested a review from petemill August 22, 2018 04:28

simonhong force-pushed the brave_theme branch from 0390d83 to 1c451c5 Compare August 22, 2018 04:30

bridiver reviewed Aug 22, 2018

View reviewed changes

simonhong force-pushed the brave_theme branch 2 times, most recently from 71c89a9 to daf9f68 Compare August 23, 2018 02:47

bridiver reviewed Aug 23, 2018

View reviewed changes

simonhong force-pushed the brave_theme branch from daf9f68 to 4a78998 Compare August 23, 2018 04:02

bridiver reviewed Aug 23, 2018

View reviewed changes

simonhong force-pushed the brave_theme branch 5 times, most recently from 995074b to b6bc7f1 Compare August 23, 2018 11:31

simonhong commented Aug 23, 2018

View reviewed changes

bridiver reviewed Aug 23, 2018

View reviewed changes

simonhong force-pushed the brave_theme branch 2 times, most recently from 48ee6f3 to d98787e Compare August 24, 2018 01:29

bridiver reviewed Aug 24, 2018

View reviewed changes

simonhong added 3 commits August 24, 2018 13:02

Make our theme color chosen at first

902a241

Introduce brave theme type prefs

1abed3d

With kBraveThemeType, we can switch dark/light theme.

Call brave method for overring default theme color in GetColor

cffca8d

With this, theme extension have a chance to override theme color.

simonhong added 3 commits August 24, 2018 13:02

Subclassing ThemeService to override GetDefaultColor()

f98349c

BraveThemeService became subclass of ThemeServiceAuraX11 and ThemeServiceWin. Also apis in theme_util.h are moved to static methods of BraveThemeService.

Remove BraveThemeTest.ObtainsBraveOverrideColors

48f5558

This unittest can't be used because override is done in ThemeService. BraveThemeServiceTest can cover this test. Also static check is added to prevent other class should not use newly defined DISALLOW_COPY_AND_ASSIGN.

Reverted to patching friend class

450eacf

I tried to conditionally add friend statement in DISALLOW_COPY_AND_ASSIGN. However, I can't find how to do it in macro.

petemill mentioned this pull request Aug 24, 2018

Ability to switch between Dark / Light mode brave/brave-browser#789

Closed

2 tasks

simonhong force-pushed the brave_theme branch 2 times, most recently from d7fffa3 to 3a72c6c Compare August 24, 2018 11:03

bridiver reviewed Aug 24, 2018

View reviewed changes

bridiver previously approved these changes Aug 24, 2018

View reviewed changes

Make BraveThemeService observe kBraveThemeType prefs

e53cff5

simonhong dismissed bridiver’s stale review via e53cff5 August 24, 2018 21:50

simonhong force-pushed the brave_theme branch from 3a72c6c to e53cff5 Compare August 24, 2018 21:50

bridiver approved these changes Aug 24, 2018

View reviewed changes

simonhong merged commit 6fb6a64 into master Aug 24, 2018

simonhong deleted the brave_theme branch August 24, 2018 22:02

simonhong mentioned this pull request Aug 24, 2018

Upgrade to Chromium 70.0.3528.4 #358

Merged

11 tasks

petemill mentioned this pull request Aug 28, 2018

Light/Dark should be selectable in development brave/brave-browser#863

Closed

simonhong mentioned this pull request Aug 29, 2018

Some unittest failed with Check failed: prefs_->FindPreference(pref_name_). brave.theme.type not registered. brave/brave-browser#881

Closed

fmarier pushed a commit that referenced this pull request Oct 29, 2019

Merge pull request #351 from brave/brave_generated_resources

c1dd13e

Add support for translating brave_generated_resources.grd

		* You can obtain one at http://mozilla.org/MPL/2.0/. */

		#include "build/build_config.h" // For OS_LINUX

		@@ -24,6 +25,8 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {

		RegisterAlternatePrivateSearchEngineProfilePrefs(registry);

		RegisterProfilePrefsForBraveThemeType(registry);

Brave theme #351

Brave theme #351

Conversation

simonhong commented Aug 22, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

Choose a reason for hiding this comment

simonhong Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

simonhong Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

simonhong Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

simonhong Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

bridiver Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Aug 22, 2018 •

edited

Loading

simonhong Aug 22, 2018 •

edited

Loading

bridiver Aug 22, 2018 •

edited

Loading

simonhong Aug 22, 2018 •

edited

Loading

simonhong Aug 22, 2018 •

edited

Loading

simonhong Aug 22, 2018 •

edited

Loading

bridiver Aug 23, 2018 •

edited

Loading

bridiver Aug 24, 2018 •

edited

Loading

bridiver Aug 24, 2018 •

edited

Loading

bridiver Aug 24, 2018 •

edited

Loading