Skip to content

skia token name enum can extend brave chromium enum #343

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.

Sign up for GitHub

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

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

skia token name enum can extend brave chromium enum #343

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/tokens/transformation/skia/templates/colors.h.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

#pragma once

#include "brave/browser/ui/color/brave_color_id.h"
#include "chrome/browser/ui/color/chrome_color_id.h"
#include "third_party/skia/include/core/SkColor.h"

namespace leo::light {
Expand All @@ -26,8 +28,10 @@ enum class Theme {
kDark
};

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

@sangwoo108 sangwoo108 Jul 27, 2023

Copy link
Copy Markdown
Collaborator

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
};

@sangwoo108 sangwoo108 Jul 27, 2023

Copy link
Copy Markdown
Collaborator

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 😄

@fallaciousreasoning fallaciousreasoning Jul 27, 2023

Copy link
Copy Markdown
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!

@petemill petemill Aug 16, 2023

Copy link
Copy Markdown
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


constexpr SkColor GetColor(Color color, Theme theme) {
Expand Down