Summary: | Reduce surface area of the ExtendedColor class to a bare minimum | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | cdumez, changseok, darin, esprehn+autocc, ews-watchlist, fred.wang, glenn, gyuyoung.kim, Hironori.Fujii, kangil.han, kondapallykalyan, macpherson, menard, mifenton, pdr, simon.fraser, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=214229 https://bugs.webkit.org/show_bug.cgi?id=214260 |
||||||||||||||||||||||
Attachments: |
|
Description
Sam Weinig
2020-07-10 15:11:58 PDT
Created attachment 404014 [details]
Patch
Created attachment 404021 [details]
Patch
Created attachment 404023 [details]
Patch
Created attachment 404033 [details]
Patch
Created attachment 404035 [details]
Patch
Comment on attachment 404033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404033&action=review > Source/WebCore/platform/graphics/Color.h:56 > +// - Special "invalid color" state maybe saying "treated as transparent black but distinguishable" > Source/WebCore/platform/graphics/Color.h:102 > bool isSemantic() const { return isInline() && (m_colorData.inlineColorAndFlags & isSemanticInlineColorBit); } > > + > bool isOpaque() const { return isExtended() ? asExtended().alpha() == 1.0 : asInline().alpha == 255; } Not a fan of the various double spaces; I don’t think they really help this much. > Source/WebCore/platform/graphics/Color.h:117 > + WEBCORE_EXPORT std::pair<ColorSpace, ColorComponents<float>> colorSpaceAndComponents() const; Better as a tuple than as a struct (food for future thought)? > Source/WebCore/platform/graphics/Color.h:172 > + static bool isBlack(const Color&); > + static bool isWhite(const Color&); These functions have unclear semantics. Why are they better than "== Color::black"? > Source/WebCore/platform/graphics/Color.h:287 > +} ASSERT_NOT_REACHED()? Or is invalid color a good outcome? > Source/WebCore/platform/graphics/Color.h:336 > + auto [c1, c2, c3, alpha] = asExtended().components(); > + return computeHash(c1, c2, c3, alpha, asExtended().colorSpace()); I think computeHash already knows how to hash tuples, so this can be: return computeHash(asExtended().components(), asExtended().colorSpace()); > Source/WebCore/platform/graphics/Color.h:410 > + auto converted = convertToComponentBytes(color); > + if (color == convertToComponentFloats(converted)) { > + setColor(converted); > + return; > + } Big new behavior! This has some consequences after this point, for the many Color functions that do subtly different operations on simple and extended colors. > Source/WebCore/platform/graphics/ColorSerialization.cpp:106 > if (WTF::areEssentiallyEqual(alpha, 1.0f)) Why does this use areEssentiallyEqual instead of ==? Does not seem motivated. > Source/WebCore/platform/graphics/ExtendedColor.h:37 > class ExtendedColor : public RefCounted<ExtendedColor> { Seems like should become a private member class of Color if you finish the job of trimming it down like this. > Tools/ChangeLog:8 > + FIXME: DO NOT LAND UNTIL ADDING NEW TESTS FOR NEW BEHAVIOR OF STORING REPRESENTABLE SRGBA<float> INLINE. Ah, the "big new behavior" comment apparently is something not lost on you. Why not leave out that one bit of code and land it separately from this refactoring? It’s not intertwined with the rest. (In reply to Darin Adler from comment #6) > Comment on attachment 404033 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=404033&action=review > > > Source/WebCore/platform/graphics/Color.h:56 > > +// - Special "invalid color" state > > maybe saying "treated as transparent black but distinguishable" > > > Source/WebCore/platform/graphics/Color.h:102 > > bool isSemantic() const { return isInline() && (m_colorData.inlineColorAndFlags & isSemanticInlineColorBit); } > > > > + > > bool isOpaque() const { return isExtended() ? asExtended().alpha() == 1.0 : asInline().alpha == 255; } > > Not a fan of the various double spaces; I don’t think they really help this > much. Yeah, an attempt was made, but it didn't work. Will remove them. > > > Source/WebCore/platform/graphics/Color.h:117 > > + WEBCORE_EXPORT std::pair<ColorSpace, ColorComponents<float>> colorSpaceAndComponents() const; > > Better as a tuple than as a struct (food for future thought)? Yes. > > > Source/WebCore/platform/graphics/Color.h:172 > > + static bool isBlack(const Color&); > > + static bool isWhite(const Color&); > > These functions have unclear semantics. Why are they better than "== > Color::black"? The difference from == Color::black, is that it it will return true for non-sRGB colors. The implementation is really asking, are all the components at there maximum (or minimum for Color::isWhite). That is not necessarily the same color as Color::black for, say, DisplayP3 colors. All that said, the real question is what do the callers want. There are only a couple callers of these functions, so let's try to figure out what they are really after: 1) Document::compositeOperatorForBackgroundColor() calls Color::isWhite(). It seems like what this code is trying to do is determine if it needs to comply with the punchOutWhiteBackgroundsInDarkMode() setting. In this case, I think color == Color::white is probably ok. If a user sets the background color of message to color(display-p3 0 0 0), it's probably ok not to punch out, just like if they set their background to color: rgb(0, 0, 1). My interpretation of the use of isWhite() here is really trying to determine if the background color is the default background color, which is Color::white. Perhaps there is actually a better way to determine that? 2) normalizedColor in HTMLConverter.cpp. Once again about dark mode. I think a similar analysis as for #1 can be made here, and that using == Color::white or == Color::black would probably be sufficient. 3) RenderLayerBacking::shouldDumpPropertyForLayer calls Color::isWhite. This one definitely can use == Color::white. This is just to not dump background colors when they are white. If it's a non-sRGB white, that's probably interesting enough to dump. 4) Editor::fontAttributesAtSelectionStart calls Color::isBlack. This one seems to be about whether we set foregroundColor font attribute. I think Color::black would be correct here as well, as a non-sRGB black might be something we want to preserve in the attributes. So, in conclusion, yeah, we should probably get rid of Color::isBlack/Color::isWhite. Thoughts? > > > Source/WebCore/platform/graphics/Color.h:287 > > +} > > ASSERT_NOT_REACHED()? Or is invalid color a good outcome? I didn't add the ASSERT_NOT_REACHED() because if other color spaces are added, it will be a compile failure (due to no default case in the switch). > > > Source/WebCore/platform/graphics/Color.h:336 > > + auto [c1, c2, c3, alpha] = asExtended().components(); > > + return computeHash(c1, c2, c3, alpha, asExtended().colorSpace()); > > I think computeHash already knows how to hash tuples, so this can be: > > return computeHash(asExtended().components(), asExtended().colorSpace()); Very cool. > > > Source/WebCore/platform/graphics/Color.h:410 > > + auto converted = convertToComponentBytes(color); > > + if (color == convertToComponentFloats(converted)) { > > + setColor(converted); > > + return; > > + } > > Big new behavior! This has some consequences after this point, for the many > Color functions that do subtly different operations on simple and extended > colors. Yeah, going to rip this out of this change. Asked Simon and he says we have to preserve the current serializations. So: color: color(srgb 1 1 1); has to serialize to itself, and not: color: rgb(255, 255, 255); which is what this change does. So, if we do bring this back, we will have to keep a bit in Color indicating it should use the color() serialization (unless we can start serializing all colors as color(srgb 1 1 1), Simon has not gotten back to me on that). c > > > Source/WebCore/platform/graphics/ColorSerialization.cpp:106 > > if (WTF::areEssentiallyEqual(alpha, 1.0f)) > > Why does this use areEssentiallyEqual instead of ==? Does not seem motivated. Not sure, but also not new, so I am going to leave it alone for this change. > > > Source/WebCore/platform/graphics/ExtendedColor.h:37 > > class ExtendedColor : public RefCounted<ExtendedColor> { > > Seems like should become a private member class of Color if you finish the > job of trimming it down like this. Yep. > > > Tools/ChangeLog:8 > > + FIXME: DO NOT LAND UNTIL ADDING NEW TESTS FOR NEW BEHAVIOR OF STORING REPRESENTABLE SRGBA<float> INLINE. > > Ah, the "big new behavior" comment apparently is something not lost on you. > > Why not leave out that one bit of code and land it separately from this > refactoring? It’s not intertwined with the rest. Yeah, it's going. (I didn't mean for this to be up for review yet, I just forgot to add --no-review to my webkit-patch post, but non-the-less, thank you for the review). Created attachment 404038 [details]
Patch
Created attachment 404039 [details]
Patch
(In reply to Sam Weinig from comment #7) > (In reply to Darin Adler from comment #6) > > Comment on attachment 404033 [details] > > Patch > > > > > Source/WebCore/platform/graphics/Color.h:336 > > > + auto [c1, c2, c3, alpha] = asExtended().components(); > > > + return computeHash(c1, c2, c3, alpha, asExtended().colorSpace()); > > > > I think computeHash already knows how to hash tuples, so this can be: > > > > return computeHash(asExtended().components(), asExtended().colorSpace()); > > Very cool. Looks like it's a bit too strict about how tuple-ish the tuple needs to be. Going to leave it out of this, but will try to fix that up to see if I can make it work with any destructurable object. Comment on attachment 404039 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404039&action=review > Source/WebCore/ChangeLog:35 > + - I'll remove this before landed. (In reply to Sam Weinig from comment #10) > (In reply to Sam Weinig from comment #7) > > (In reply to Darin Adler from comment #6) > > > Comment on attachment 404033 [details] > > > Patch > > > > > > > Source/WebCore/platform/graphics/Color.h:336 > > > > + auto [c1, c2, c3, alpha] = asExtended().components(); > > > > + return computeHash(c1, c2, c3, alpha, asExtended().colorSpace()); > > > > > > I think computeHash already knows how to hash tuples, so this can be: > > > > > > return computeHash(asExtended().components(), asExtended().colorSpace()); > > > > Very cool. > > Looks like it's a bit too strict about how tuple-ish the tuple needs to be. > Going to leave it out of this, but will try to fix that up to see if I can > make it work with any destructurable object. Added support to computeHash in https://bugs.webkit.org/show_bug.cgi?id=214224. Created attachment 404072 [details]
Patch
Comment on attachment 404072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404072&action=review > Source/WebCore/platform/graphics/Color.h:340 > + return functor(asInline()); Wonder what determines if we should call the function like this or use std::invoke/std::forward. I am guessing std::invoke works on a slightly larger variety of functors? > Source/WebCore/platform/graphics/ColorSerialization.cpp:167 > + return color.callOnUnderlyingType([] (auto c) { Would have preferred a word rather than the letter "c". > Source/WebCore/platform/graphics/ColorSerialization.cpp:174 > + return color.callOnUnderlyingType([] (auto c) { Ditto. > Source/WebCore/platform/graphics/ColorSerialization.cpp:181 > + return color.callOnUnderlyingType([] (auto c) { Ditto. > Source/WebCore/platform/graphics/ColorTypes.h:33 > +template<typename T> struct ComponentTraits; No need for the "T" here. > Source/WebCore/platform/graphics/ColorTypes.h:53 > + static constexpr ColorSpace colorSpace = ColorSpace::SRGB; I would do "constexpr auto" instead of "constexpr ColorSpace". (In reply to Darin Adler from comment #14) > Comment on attachment 404072 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=404072&action=review > > > Source/WebCore/platform/graphics/Color.h:340 > > + return functor(asInline()); > > Wonder what determines if we should call the function like this or use > std::invoke/std::forward. I am guessing std::invoke works on a slightly > larger variety of functors? Yeah, we should probably use invoke here (and probably in more generic contexts). Will fix. > > > Source/WebCore/platform/graphics/ColorSerialization.cpp:167 > > + return color.callOnUnderlyingType([] (auto c) { > > Would have preferred a word rather than the letter "c". Fixed. > > > Source/WebCore/platform/graphics/ColorSerialization.cpp:174 > > + return color.callOnUnderlyingType([] (auto c) { > > Ditto. Fixed. > > > Source/WebCore/platform/graphics/ColorSerialization.cpp:181 > > + return color.callOnUnderlyingType([] (auto c) { > > Ditto. Fixed. > > > Source/WebCore/platform/graphics/ColorTypes.h:33 > > +template<typename T> struct ComponentTraits; > > No need for the "T" here. Fixed. > > > Source/WebCore/platform/graphics/ColorTypes.h:53 > > + static constexpr ColorSpace colorSpace = ColorSpace::SRGB; > > I would do "constexpr auto" instead of "constexpr ColorSpace". Fixed. Created attachment 404075 [details]
Patch
Committed r264272: <https://trac.webkit.org/changeset/264272> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404075 [details]. Committed r264289: <https://trac.webkit.org/changeset/264289> I’m not sure moving the include to the header was the best fix. We might have been able to just forward declare in the header and include only in the files that were using that template. (In reply to Darin Adler from comment #20) > I’m not sure moving the include to the header was the best fix. We might > have been able to just forward declare in the header and include only in the > files that were using that template. I'm surprised by your comment. WebCore has a lot of template functions and template classes, but less forward declarations for them. Is it really common idiom in WebCore? In this case ComponentTraits is used only in ColorUtilities.h. It can be moved to ColorUtilities.h. (In reply to Fujii Hironori from comment #21) > (In reply to Darin Adler from comment #20) > > I’m not sure moving the include to the header was the best fix. We might > > have been able to just forward declare in the header and include only in the > > files that were using that template. > > I'm surprised by your comment. > WebCore has a lot of template functions and template classes, but less > forward declarations for them. > Is it really common idiom in WebCore? Maybe not. But forward declaring whenever we can *is* common in WebCore, we may not be doing it as much as we need with templates. Note that template functions don't always require all their prerequisites just because a header is included. Instantiating the function then requires the prerequisites. That’s how I understand it at least. Faster building requires including the minimum and reducing includes. (In reply to Darin Adler from comment #22) > Maybe not. But forward declaring whenever we can *is* common in WebCore, we > may not be doing it as much as we need with templates. Note that template > functions don't always require all their prerequisites just because a header > is included. Instantiating the function then requires the prerequisites. > That’s how I understand it at least. OK. Forward template declarations can be useful for the case of template funcitons and the case of using the pointer or reference of template classes. In this case, ComponentTraits is a template class, and ColorUtilities.h is using ComponentTraits members. Thus, ColorUtilities.h needs ComponentTraits definition. (In reply to Fujii Hironori from comment #23) > (In reply to Darin Adler from comment #22) > In this case, ComponentTraits is a template class, and ColorUtilities.h is > using ComponentTraits members. > Thus, ColorUtilities.h needs ComponentTraits definition. Is that right? ComponentTraits members are used inside *function templates* in ColorUtilities.h. Does that really mean the class template needs to be *defined* when ColorUtilities.h is included but those function templates are not instantiated? (In reply to Darin Adler from comment #24) > (In reply to Fujii Hironori from comment #23) > > (In reply to Darin Adler from comment #22) > > In this case, ComponentTraits is a template class, and ColorUtilities.h is > > using ComponentTraits members. > > Thus, ColorUtilities.h needs ComponentTraits definition. > > Is that right? > > ComponentTraits members are used inside *function templates* in > ColorUtilities.h. Does that really mean the class template needs to be > *defined* when ColorUtilities.h is included but those function templates are > not instantiated? You are right. Will fix it. Committed r264588: <https://trac.webkit.org/changeset/264588> Committed r264589: <https://trac.webkit.org/changeset/264589> (In reply to Fujii Hironori from comment #25) > You are right. Will fix it. Filed: Bug 214540 – Use forward template declarations for ColorComponents instead of including ColorComponents.h |