It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Thanks!

I had to git commit --amend --reset-author (I'm not sure I've used that command in half a decade) but it appears happy now.

@flar Since you wrote the original issue I'd love to get your review as well. Thanks!

You'll need to update flutter licenses, the instructions on how to do so are in the output of the Linux linux_license shard: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8774624389745971521/+/u/test:_licenses_check/stdout

For the Fuchsia failures, those compilation errors seem bogus. I would see if they go away when you push another commit, or use the rebase button in the github UI.

This is one half of it. The other part of that Issue was that the Impeller code was handing Skia color constants to APIs that were expecting DlColor objects - unit tests mainly.

As the issue says, DlColor and SkColor are auto-converted between each other so the compiler and linter don't call that practice out. Perhaps grep the sources:

% cd <engine>/impeller
% git grep -i skcolor
% git grep -i sk_color

DlColor has the DlColor::k<ColorName>() constants which should generally be used instead - or if the Impeller code is talking to its own Paint object, it should be using the impeller constants.

Thanks @flar. Just to be clear, do you expect all of these updated:

$ rg -i 'sk(_)?color'
typographer/backends/skia/text_render_context_skia.cc
171:  auto glyph_color = has_color ? SK_ColorWHITE : SK_ColorBLACK;

image/backends/skia/compressed_image_skia.cc
57:                                kRGBA_8888_SkColorType, kPremul_SkAlphaType);

display_list/skia_conversions.h
14:#include "third_party/skia/include/core/SkColor.h"
15:#include "third_party/skia/include/core/SkColorType.h"
47:std::optional<impeller::PixelFormat> ToPixelFormat(SkColorType type);

display_list/dl_unittests.cc
33:#include "third_party/skia/include/core/SkColor.h"
124:  paint.setColor(SK_ColorRED);
212:    paint.setColor(SK_ColorGREEN);
215:    paint.setColor(SK_ColorRED);
229:    paint.setColor(SK_ColorRED);
401:  paint.setColor(SK_ColorRED);
430:    paint.setColor(SK_ColorYELLOW);
483:    auto filter = flutter::DlBlendColorFilter(SK_ColorYELLOW,
493:        flutter::DlBlendColorFilter(SK_ColorRED, flutter::DlBlendMode::kScreen);
863:                        SK_ColorTRANSPARENT, 15, false, 1);

display_list/skia_conversions.cc
172:std::optional<impeller::PixelFormat> ToPixelFormat(SkColorType type) {
174:    case kRGBA_8888_SkColorType:
176:    case kBGRA_8888_SkColorType:
178:    case kRGBA_F16_SkColorType:
180:    case kBGR_101010x_XR_SkColorType:

Looks like, minus tree status, this is ready to merge 🥳 .

@flar at your convenience, want to double-check I made the changes you expected? Thanks!

@flar at your convenience, want to double-check I made the changes you expected? Thanks!

I didn't do an exhaustive search. If there are others then they will likely be eliminated as we start omitting more and more Skia includes from code that isn't directly responsible for adapting the 2 modules. For example, you were able to get rid of the include of SkColor.h from the Impeller unit tests which is both a good sign and a step in the right direction. The main thing was to get rid of the obvious ones for now.

Thanks @flar. Just to be clear, do you expect all of these updated:

Not the "color type" ones, but the others, yes.

Looks like, minus tree status, this is ready to merge 🥳 .

The standard practice is to add the autosubmit label and then a bot will come back and merge it when the tree goes green (and it has the required reviews and passes all of the tests). This should be the way we merge all of our PRs, but it is especially convenient when the tree is red and you'd rather have the bot do the waiting for you.