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

skia token name enum can extend brave chromium enum #343

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

petemill
Copy link
Member

@petemill petemill commented Jul 26, 2023

Modifies the skia enum so that it can be used in the brave/chromium color mixer. I didn't worry too much about making it non-chromium (and non-brave) compatible since there are no uses outside that at the moment.

> #include "brave/browser/ui/color/brave_color_id.h"
> #include "chrome/browser/ui/color/chrome_color_id.h"
285c287,288
< enum class Color {
---
> enum class Color : ui::ColorId {
>   kLeoColorsStart = kBraveColorsEnd,
416c419,420
<   kColorLegacyDivider1
---
>   kColorLegacyDivider1,
>   kLeoColorsEnd

Corresponds with brave/brave-core#19440

@petemill petemill self-assigned this Jul 26, 2023
@petemill petemill temporarily deployed to staging July 26, 2023 18:55 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

👋 Thanks for Submitting! This PR is available for preview at the link below.

✅ PR tip preview: https://343.pr.leo.bravesoftware.com/
✅ Commit preview: https://343.pr.leo.bravesoftware.com/commit-0e487240922ca2a7038b8ef17b54c6be489790b1/

Variables Diff
--- ./tokens/css/variables.old.css	2023-07-26 18:56:54.244941229 +0000
+++ ./tokens/css/variables.css	2023-07-26 18:56:23.857379450 +0000
@@ -1,6 +1,6 @@
 /**
  * Do not edit directly
- * Generated on Tue Jul 25 2023 06:44:49 GMT+0000 (Coordinated Universal Time)
+ * Generated on Wed Jul 26 2023 18:56:23 GMT+0000 (Coordinated Universal Time)
  */
 
 :root {

Copy link
Collaborator

@fallaciousreasoning fallaciousreasoning left a comment

Choose a reason for hiding this comment

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

This LGTM but lets rope in a C++ expert.

Comment on lines +31 to 35
enum class Color : ui::ColorId {
kLeoColorsStart = kBraveColorsEnd,
<%= groupedTokens.light.allTokens.map(prop => ` ${prop.name}`).join(',\n') %>,
kLeoColorsEnd
};
Copy link
Contributor

Choose a reason for hiding this comment

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

How about wrapping these E_CPONLY() and define macro so that we can add this to brave_color_ids?

Roughly,

Suggested change
enum class Color : ui::ColorId {
kLeoColorsStart = kBraveColorsEnd,
<%= groupedTokens.light.allTokens.map(prop => ` ${prop.name}`).join(',\n') %>,
kLeoColorsEnd
};
#define LEO_COLOR_IDS \
<%= groupedTokens.light.allTokens.map(prop => ` E_CPONLY(${prop.name}) \
`).join(',\n') %>,
};

and in brave_colod_id

#define BRAVE_COLOR_IDS               \
    BRAVE_COMMON_COLOR_IDS            \
    BRAVE_SEARCH_CONVERSION_COLOR_IDS \
    BRAVE_SIDEBAR_COLOR_IDS           \
    BRAVE_SPEEDREADER_COLOR_IDS       \
    BRAVE_VPN_COLOR_IDS               \
    BRAVE_VERTICAL_TAB_COLOR_IDS      \
-    BRAVE_PLAYLIST_COLOR_IDS
+    BRAVE_PLAYLIST_COLOR_IDS          \
+    LEO_COLOR_IDS
};

Copy link
Contributor

Choose a reason for hiding this comment

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

I wish we have leo_color_mixer generated too 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think @petemill has a solution for that!

Copy link
Member Author

Choose a reason for hiding this comment

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

@sangwoo108 That was the first strategy I tried for this PR, but I got some errors. And ultimately it seemed ok for it to be a separate enum set, similar to how chromium does it with separate enum sets. I'll try again to see what the error is. In the meantime, here's the LeoColorMixer PR https://github.com/brave/brave-core/pull/19440/files

@petemill petemill mentioned this pull request Jul 27, 2023
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.

3 participants