Introduce chrome.braveTheme api

[simonhong]

With this new api, shields, rewards and many other webui can get brave
theme information.

Related Issue: brave/brave-browser#793
Close brave/brave-browser#2348

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).
• Verified that these changes build without errors on
  • Windows
  • macOS
  • Linux
• Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
  • Windows
  • macOS
  • Linux
• Ran git rebase master (if needed).
• 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/Yes or QA/No) to include the closed issue in milestone

Test Plan:

yarn test brave_browser_tests --filter=BraveThemeAPIBrowserTest*

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

[simonhong]

Will remove WIP after adding onBraveThemeTypeChanged event.

[simonhong]

event is added . Will remove WIP after adding test code for event.

[simonhong]

Kindly ping..

[bbondy]

works well, tests pass, code looks good. One minor comment on the json api description. I think to be useful we also need to allow the extensions and webui to differentiate to know what Default means.

[petemill]

Apologies for the delay - I was out sick. On the topic of @bbondy 's comment, I'd go further and suggest that this JS API should not expose GetUserPreferredBraveThemeType (which could be default, dark, light) but should instead expose GetActiveBraveThemeType which can only be dark, light.
Shouldn't we should keep all the logic about how default is interpreted in one place? It will also not matter in almost all circumstances whether the theme is chosen directly from the user or is the default.

In fact, we can use GetActiveBraveThemeType and still only keep 1 API as even the settings page doesn't really need to know about default or provide the 'default' option to the user - it should just provide dark and light and it will already be defaulted because GetActiveBraveThemeType will translate default to the appropriate value.

[petemill]

Great job, I love the unified JS api approach. As per the comment, I'm suggesting we simplify and use GetActiveBraveThemeType instead of GetUserPreferredBraveThemeType. This should be backwards compatible with everything that's there, but only extra step we would need is to remove the 'default' select option on the settings page.

[petemill]

(another reason not to introduce parsing Default in another layer is that the result of that logic is subject to change, e.g. if / when our default changes to follow OS-set light / dark value)

[simonhong]

I agree with @petemill. Default isn't another different theme option.
It's just translated to existing another option. @bbondy WDYT?

[petemill]

Received 👍 from design team on removing Default option for theme on Settings page so hopefully this is a straightforward call swap

[simonhong]

GetUserPreferredBraveThemeType() is replaced with GetActiveBraveThemeType() in theme api.
Also default option is removed from brave theme option list.

[bbondy]

I'd go further and suggest that this JS API should not expose GetUserPreferredBraveThemeType (which could be default, dark, light) but should instead expose GetActiveBraveThemeType which can only be dark, light.

I considered that too but wondered if it'll ever be useful for us to know if the user has changed from what the default is or not. I'm ok with always doing light or dark though.

[simonhong]

@petemill I think this PR is ready to merge :)

[petemill]

@simonhong I'm getting the following build error:

ERROR at //chrome/chrome_repack_locales.gni:5:1: Can't load input file.
import("//brave/brave_repack_locales.gni")
^----------------------------------------
Unable to load:
  /Users/xyz/Development/Brave/brave-browser/src/brave/brave_repack_locales.gni
I also checked in the secondary tree for:
  /Users/xyz/Development/Brave/brave-browser/src/build/secondary/brave/brave_repack_locales.gni
See //chrome/chrome_paks.gni:9:1: whence it was imported.
import("chrome_repack_locales.gni")
^---------------------------------
See //chrome/BUILD.gn:16:1: whence it was imported.
import("//chrome/chrome_paks.gni")
^--------------------------------
See //BUILD.gn:101:7: which caused the file to be included.
      "//chrome",

[bbondy]

You likely have a patch applied unrelated to this commit that shouldn't exist for this branch. You can rebase to master or re-init.

[simonhong]

@petemill Rebased this PR on latest master now.

[petemill]

This is working great! I'll add a wiki doc on the usage

[simonhong]

@petemill Thanks! 👍

[petemill]

Documented at https://github.com/brave/brave-browser/wiki/UI-Color-Themes

[bbondy]

Can we close brave/brave-browser#793 @petemill ? If so pls also update the issue milestone too, thanks!.

[simonhong]

@bbondy For this PR, brave/brave-browser#2348 is created and closed.
I think brave/brave-browser#793 could be closed after our webui pages are aware about dark/light theme.