Replace SkColor with blink::Color in FontPalette [chromium/src : main]
Munira Tursunova (Gerrit)
Munira Tursunova uploaded patch set #2 to this change.
Replace SkColor with blink::Color in FontPalette
Using blink::Color instead of SkColor for blink's font-palette
representation. It is needed for the future modifications in the
palette-mix funciton, to be able to interpolate in different color
spaces, compare section "Mixing Palettes: the ‘palette-mix()’ Function"
of the Explainer:
https://docs.google.com/document/d/1XMTrKH003KBOes6hxzI-3E7LTwp5YwFC-rnzoFpFrfw/edit?usp=sharing
Bug: 1400620
Change-Id: I08e33f6dbe4d1cad1506b1e0dea299c20f6ea380
---
M third_party/blink/renderer/core/css/style_rule_font_palette_values.cc
M third_party/blink/renderer/platform/fonts/font_custom_platform_data.cc
M third_party/blink/renderer/platform/fonts/font_palette.cc
M third_party/blink/renderer/platform/fonts/font_palette.h
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup.cc
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup.h
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup_test.cc
M third_party/blink/renderer/platform/fonts/palette_interpolation.cc
M third_party/blink/renderer/platform/fonts/palette_interpolation.h
M third_party/blink/renderer/platform/fonts/palette_interpolation_test.cc
10 files changed, 95 insertions(+), 103 deletions(-)
To view, visit change 4597888. To unsubscribe, or for help writing mail filters, visit settings.
Munira Tursunova (Gerrit)
Attention is currently required from: Dominik Röttsches.
Munira Tursunova would like Dominik Röttsches to review this change.
Replace SkColor with blink::Color in FontPalette
Using blink::Color instead of SkColor for blink's font-palette
representation. It is needed for the future modifications in the
palette-mix funciton, to be able to interpolate in different color
spaces, compare section "Mixing Palettes: the ‘palette-mix()’ Function"
of the Explainer:
https://docs.google.com/document/d/1XMTrKH003KBOes6hxzI-3E7LTwp5YwFC-rnzoFpFrfw/edit?usp=sharing
Bug: 1400620
Change-Id: I08e33f6dbe4d1cad1506b1e0dea299c20f6ea380
---
M third_party/blink/renderer/core/css/style_rule_font_palette_values.cc
M third_party/blink/renderer/platform/fonts/font_custom_platform_data.cc
M third_party/blink/renderer/platform/fonts/font_palette.cc
M third_party/blink/renderer/platform/fonts/font_palette.h
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup.cc
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup.h
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup_test.cc
M third_party/blink/renderer/platform/fonts/palette_interpolation.cc
M third_party/blink/renderer/platform/fonts/palette_interpolation.h
M third_party/blink/renderer/platform/fonts/palette_interpolation_test.cc
10 files changed, 95 insertions(+), 103 deletions(-)
To view, visit change 4597888. To unsubscribe, or for help writing mail filters, visit settings.
Munira Tursunova (Gerrit)
Attention is currently required from: Dominik Röttsches, Munira Tursunova.
Set Ready For Review
Munira Tursunova (Gerrit)
Attention is currently required from: Dominik Röttsches, Munira Tursunova.
Munira Tursunova uploaded patch set #5 to this change.
Replace SkColor with blink::Color in FontPalette
Using blink::Color instead of SkColor for blink's font-palette
representation. It is needed for the future modifications in the
palette-mix function, to be able to interpolate in different color
spaces, compare section "Mixing Palettes: the ‘palette-mix()’ Function"
of Explainer:
https://docs.google.com/document/d/1XMTrKH003KBOes6hxzI-3E7LTwp5YwFC-rnzoFpFrfw/edit?usp=sharing
Bug: 1400620
Change-Id: I08e33f6dbe4d1cad1506b1e0dea299c20f6ea380
---
M third_party/blink/renderer/core/css/style_rule_font_palette_values.cc
M third_party/blink/renderer/platform/fonts/font_custom_platform_data.cc
M third_party/blink/renderer/platform/fonts/font_palette.cc
M third_party/blink/renderer/platform/fonts/font_palette.h
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup.cc
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup.h
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup_test.cc
M third_party/blink/renderer/platform/fonts/palette_interpolation.cc
M third_party/blink/renderer/platform/fonts/palette_interpolation.h
M third_party/blink/renderer/platform/fonts/palette_interpolation_test.cc
10 files changed, 145 insertions(+), 128 deletions(-)
To view, visit change 4597888. To unsubscribe, or for help writing mail filters, visit settings.
Dominik Röttsches (Gerrit)
Attention is currently required from: Munira Tursunova.
7 comments:
File third_party/blink/renderer/platform/fonts/font_custom_platform_data.cc:
Patch Set #6, Line 34:
#include <cstdlib>What do you need that for?
SkFontArguments::Palette::Override* sk_overrides =
new SkFontArguments::Palette::Override[color_overrides.size()];I think this will leak, which is why luckily the `makeClone()` call works below.
Allocate a new array into a `unique_ptr` that is declared at the same scope as the call to `makeClone()`. If you'd allocate as `unique_ptr` inside the `if` statement, this would go out of scope.
For example, in lline 245:
`std::unique_ptr<SkFontArguments::Palette::Override[]> skia_overrides;`then in the `if` block:
`skia_overrides.reset(new SkFontArguments::Palette::Override[color_overrides.size()]);`Patch Set #6, Line 252:
for (wtf_size_t i = 0; i < color_overrides.size(); i++) {Consider moving the conversion to a helper function in an anonymous namespace.
Patch Set #6, Line 254:
*(sk_overrides + i)how about `sk_overrides[i]`?
Patch Set #6, Line 256:
sk_palette.overrides = std::move(sk_overrides);SkFontArguments::Palette::overrides is not an owning pointer, so the `move` here has the desired effect of passing ownership, see above.
`SkFontArguments` is intended as a transient struct only for passing parameters to `makeClone()`. It doesn't own anything and references allocated arrays only outside itself.
File third_party/blink/renderer/platform/fonts/palette_interpolation_test.cc:
Patch Set #6, Line 58:
CheckColorsEqualityInSRGBPerhaps `ExpectColorsEqualInSRGB()`.
Patch Set #6, Line 66:
color1.ConvertToColorSpace(blink::Color::ColorSpace::kSRGB);Do you need the color space conversions? Does the `Color` class, have some comparison methods independent of color space?
To view, visit change 4597888. To unsubscribe, or for help writing mail filters, visit settings.
Munira Tursunova (Gerrit)
Attention is currently required from: Dominik Röttsches.
7 comments:
File third_party/blink/renderer/platform/fonts/font_custom_platform_data.cc:
Patch Set #6, Line 34:
#include <cstdlib>What do you need that for?
Sorry, didn't noticed it and yes, we don't need it.
SkFontArguments::Palette::Override* sk_overrides =
new SkFontArguments::Palette::Override[color_overrides.size()];
I think this will leak, which is why luckily the `makeClone()` call works below. […]
Added `unique_ptr`, thanks for explaining.
Patch Set #6, Line 252:
for (wtf_size_t i = 0; i < color_overrides.size(); i++) {Consider moving the conversion to a helper function in an anonymous namespace.
Moved to ConvertPaletteOverridesToSkiaOverrides()
Patch Set #6, Line 254:
*(sk_overrides + i)how about `sk_overrides[i]`?
Right,thanks!
Patch Set #6, Line 256:
sk_palette.overrides = std::move(sk_overrides);
SkFontArguments::Palette::overrides is not an owning pointer, so the `move` here has the desired eff […]
Thanks, fixed
File third_party/blink/renderer/platform/fonts/palette_interpolation_test.cc:
Patch Set #6, Line 58:
CheckColorsEqualityInSRGBPerhaps `ExpectColorsEqualInSRGB()`.
Right, thanks!
Patch Set #6, Line 66:
color1.ConvertToColorSpace(blink::Color::ColorSpace::kSRGB);
Do you need the color space conversions? Does the `Color` class, have some comparison methods indepe […]
I have not found such methods.
I suppose we need it, I wanted to store all the expected color records in SRGB since I thought it would be easier to understand. Also since now we should be able to interpolate in different color spaces, actual color records may have different color spaces and we need to add tests for such cases (tests are in the next cl in chain, compare: https://chromium-review.googlesource.com/c/chromium/src/+/4604943/1/third_party/blink/renderer/platform/fonts/palette_interpolation_test.cc)
To view, visit change 4597888. To unsubscribe, or for help writing mail filters, visit settings.
Dominik Röttsches (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Munira Tursunova.
Dominik Röttsches would like Anders Hartvoll Ruud to review this change authored by Munira Tursunova.
Replace SkColor with blink::Color in FontPalette
Using blink::Color instead of SkColor for blink's font-palette
representation. It is needed for the future modifications in the
palette-mix function, to be able to interpolate in different color
spaces, compare section "Mixing Palettes: the ‘palette-mix()’ Function"
of Explainer:
https://docs.google.com/document/d/1XMTrKH003KBOes6hxzI-3E7LTwp5YwFC-rnzoFpFrfw/edit?usp=sharing
Bug: 1400620
Change-Id: I08e33f6dbe4d1cad1506b1e0dea299c20f6ea380
---
M third_party/blink/renderer/core/css/style_rule_font_palette_values.cc
M third_party/blink/renderer/platform/fonts/font_custom_platform_data.cc
M third_party/blink/renderer/platform/fonts/font_palette.cc
M third_party/blink/renderer/platform/fonts/font_palette.h
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup.cc
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup.h
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup_test.cc
M third_party/blink/renderer/platform/fonts/palette_interpolation.cc
M third_party/blink/renderer/platform/fonts/palette_interpolation.h
M third_party/blink/renderer/platform/fonts/palette_interpolation_test.cc
10 files changed, 152 insertions(+), 128 deletions(-)
Dominik Röttsches (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Munira Tursunova.
4 comments:
Patchset:
platform/fonts looks almost ready from my point of view with a suggestion to edit the comparison based, see below. Adding Anders for the CSS side.
File third_party/blink/renderer/platform/fonts/palette_interpolation_test.cc:
Patch Set #8, Line 15:
#include "third_party/blink/renderer/platform/testing/unit_test_helpers.h"IWYU: Add #include "third_party/blink/renderer/platform/graphics/color.h"
Patch Set #8, Line 25:
kEpsilon2Once you use `DifferenceSquared(const Color&, const Color&)` I would not call that epsilon anymore but give it a new name.
Patch Set #8, Line 68:
EXPECT_EQ(color1.GetColorSpace(), color2.GetColorSpace());See `DifferenceSquared(const Color&, const Color&)`, can you work with that?
To view, visit change 4597888. To unsubscribe, or for help writing mail filters, visit settings.
Munira Tursunova (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Dominik Röttsches.
3 comments:
File third_party/blink/renderer/platform/fonts/palette_interpolation_test.cc:
Patch Set #8, Line 15:
#include "third_party/blink/renderer/platform/testing/unit_test_helpers.h"
IWYU: Add #include "third_party/blink/renderer/platform/graphics/color. […]
Done
Patch Set #8, Line 25:
kEpsilon2
Once you use `DifferenceSquared(const Color&, const Color&)` I would not call that epsilon anymore b […]
Replied in the comment below https://chromium-review.googlesource.com/c/chromium/src/+/4597888/comment/4379d0e4_48126df5/
Patch Set #8, Line 68:
EXPECT_EQ(color1.GetColorSpace(), color2.GetColorSpace());See `DifferenceSquared(const Color&, const Color&)`, can you work with that?
It doesn't include the difference between alpha parameter, I think it's better to check alpha as well, look at the test "MixScaledAndAddedPalettes" there we modify the alpha parameter.
I guess I can use this method, but we don't actually need epsilon comparison here, we need it only for alpha parameter, since Red(), Green() and Blue() are ints.
To view, visit change 4597888. To unsubscribe, or for help writing mail filters, visit settings.
Dominik Röttsches (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Munira Tursunova.
1 comment:
File third_party/blink/renderer/platform/fonts/palette_interpolation_test.cc:
Patch Set #8, Line 68:
EXPECT_EQ(color1.GetColorSpace(), color2.GetColorSpace());It doesn't include the difference between alpha parameter, I think it's better to check alpha as wel […]
Then how about extracting the alpha parameters from the two colors, comparing those, then overriding them and using the `operator==()` comparison of color for the rest?
To view, visit change 4597888. To unsubscribe, or for help writing mail filters, visit settings.
Munira Tursunova (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Dominik Röttsches.
1 comment:
File third_party/blink/renderer/platform/fonts/palette_interpolation_test.cc:
Patch Set #8, Line 68:
EXPECT_EQ(color1.GetColorSpace(), color2.GetColorSpace());
Then how about extracting the alpha parameters from the two colors, comparing those, then overriding […]
`operator==` compares `Param1()`, `Param2`, etc, but they are float values, after conversion from one space to another we might loose in precision, so we need to compare `Param1()`, `Param0()`, etc. with epsilon comparison, but `operator==` uses just regular comparison without epsilon.
To view, visit change 4597888. To unsubscribe, or for help writing mail filters, visit settings.
Dominik Röttsches (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Munira Tursunova.
Patch set 9:Code-Review +1
1 comment:
File third_party/blink/renderer/platform/fonts/palette_interpolation_test.cc:
Patch Set #8, Line 68:
EXPECT_EQ(color1.GetColorSpace(), color2.GetColorSpace());
`operator==` compares `Param1()`, `Param2`, etc, but they are float values, after conversion from on […]
Ah, so you're combining the two now `DifferenceSquared` and a separate alpha comparison, that looks good then. Thanks for the explanations.
To view, visit change 4597888. To unsubscribe, or for help writing mail filters, visit settings.
Anders Hartvoll Ruud (Gerrit)
Attention is currently required from: Munira Tursunova.
To view, visit change 4597888. To unsubscribe, or for help writing mail filters, visit settings.
Munira Tursunova (Gerrit)
Patch set 9:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Replace SkColor with blink::Color in FontPalette
Using blink::Color instead of SkColor for blink's font-palette
representation. It is needed for the future modifications in the
palette-mix function, to be able to interpolate in different color
spaces, compare section "Mixing Palettes: the ‘palette-mix()’ Function"
of Explainer:
https://docs.google.com/document/d/1XMTrKH003KBOes6hxzI-3E7LTwp5YwFC-rnzoFpFrfw/edit?usp=sharing
Bug: 1400620
Change-Id: I08e33f6dbe4d1cad1506b1e0dea299c20f6ea380
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4597888
Reviewed-by: Anders Hartvoll Ruud <and...@chromium.org>
Reviewed-by: Dominik Röttsches <dr...@chromium.org>
Commit-Queue: Munira Tursunova <moo...@google.com>
Cr-Commit-Position: refs/heads/main@{#1157468}
---
M third_party/blink/renderer/core/css/style_rule_font_palette_values.cc
M third_party/blink/renderer/platform/fonts/font_custom_platform_data.cc
M third_party/blink/renderer/platform/fonts/font_palette.cc
M third_party/blink/renderer/platform/fonts/font_palette.h
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup.cc
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup.h
M third_party/blink/renderer/platform/fonts/opentype/open_type_cpal_lookup_test.cc
M third_party/blink/renderer/platform/fonts/palette_interpolation.cc
M third_party/blink/renderer/platform/fonts/palette_interpolation.h
M third_party/blink/renderer/platform/fonts/palette_interpolation_test.cc
10 files changed, 149 insertions(+), 128 deletions(-)