Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider brave theme type to devtools' theme #891

Merged
merged 1 commit into from
Nov 20, 2018
Merged

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Nov 12, 2018

When devtool ui is loaded, kDevToolsPreferences pref values are fetched.
In that dictionary prefs, uiTheme represents theme type(light/dark) of devtool.
To apply brave theme type to devtools, uiTheme value is replaced with brave
native theme type. If user sets devtools' theme type explicitely, that value
is used instead of brave theme type. Otherwise, brave theme type is applied.

Fix brave/brave-browser#784

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=BraveDevToolsUIBindingsBrowserTest.ThemeTest

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

When devtool ui is loaded, kDevToolsPreferences pref values are fetched.
In that dictionary prefs, uiTheme represents theme type(light/dark) of devtool.
To apply brave theme type to devtools, uiTheme value is replaced with brave
native theme type. If user sets devtools' theme type explicitely, that value
is used instead of brave theme type. Otherwise, brave theme type is applied.
@simonhong simonhong self-assigned this Nov 12, 2018
@simonhong simonhong requested review from petemill and bbondy November 12, 2018 14:49
@simonhong
Copy link
Member Author

Kindly ping.. I think this PR would not be affected by chromium's in-progress platform theme type checking because this only depends on our active theme type.

Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works great. The one issue I have is that if the user changes the devtools Theme setting, there is no option to clear it and start 'tracking' the UI theme. However, the primary goal of this is to make it so that initially when the user changes the UI theme the devtools theme is changed. It's usually not a common user task to switch between themes, and if the user has changed devtools theme specifically, then they know where that setting is, so can change it there too. Just something to keep in mind when we consider the OS-level vs Browser-level (and now devtools-level) choice for themes.

It's probably just simplest for the user and for the code to remove the setting in devtools entirely, and have it track the single browser-level setting. When we clean up the settings for the OS-level choice, perhaps we can consider that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switching between built-in Dark and Light theme should change DevTools theme setting
3 participants