Bug 81941

This would require: -

Created attachment 387505 [details] Test case

This would require (IMO): - Fixing the parser helpers to allow generated images inside image-set() - Refactoring StyleImageSet out of StyleCachedImage and allow it to contain both cached and generated images

Created attachment 388193 [details] Patch

Created attachment 388194 [details] Patch

Comment on attachment 388193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388193&action=review > Source/WebCore/rendering/style/StyleMultiImage.cpp:62 > + std::tie(value, scaleFactor) = selectBestFitImage(loader.document()); Given that loader.document() can return null, and that selectBestFitImage needs the Document not to be null, I would suggest either ASSERTing that loader.document() is not null here (if for some reason that can be asserted, I am not sure it can) or null checking the document here and failing the load. In either case, the document should be passed to selectBestFitImage as a reference.

(In reply to Sam Weinig from comment #6) > Comment on attachment 388193 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388193&action=review > > > Source/WebCore/rendering/style/StyleMultiImage.cpp:62 > > + std::tie(value, scaleFactor) = selectBestFitImage(loader.document()); > > Given that loader.document() can return null, and that selectBestFitImage > needs the Document not to be null, I would suggest either ASSERTing that > loader.document() is not null here (if for some reason that can be asserted, > I am not sure it can) or null checking the document here and failing the > load. In either case, the document should be passed to selectBestFitImage as > a reference. It can be asserted there (it's asserted later anyway). Will fix.

Created attachment 388196 [details] Patch

Comment on attachment 388196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388196&action=review Patch looks good with quite a few improvements, not just the new features. I have a lot of comments, almost all about coding style and idiom. I feel like there is significant boilerplate code here and would like to see it reduced and kept to a minimum if possible. Particularly ugly to have lots of "forwarding functions" that just call another function of the same name on a different object, but perhaps this is inevitable. It’s especially irritating that we have so many is<CSSXXX> all over the place. Maybe we should add some more virtual functions to the CSS classes? Would good to go even further with removing includes in headers if possible. I don’t think you should land this as-is. Many of the comments are things that are well worth changing, especially use of OptionSet. > Source/WebCore/css/CSSCursorImageValue.h:63 > + std::pair<CSSValue*, float> selectBestFitImage(const Document&); Four thoughts about the return value here: 1) Not sure how we choose between std::pair and std::tuple when we have a two part return value. 2) When writing similar functions myself I have often chosen a struct with names for the fields instead of a pair/tuple where you have to deduce what the names mean. 3) In modern code, I think we prefer Ref/RefPtr over raw pointer for return values. I think Ref<CSSValue> would be right here. 4) It is kind of sad that the only common base class for CSSImageValue and CSSImageSetValue is CSSValue. Also would be nice to document which types can be returned here. Can it be a CSSImageGeneratorValue? Maybe (3) would lead to a change? > Source/WebCore/css/CSSImageSetValue.cpp:83 > +CachedImage* CSSImageSetValue::cachedImage() const This returns nullptr when m_selectedImageValue is a CSSImageGeneratorValue. Is that correct? > Source/WebCore/css/CSSImageSetValue.cpp:88 > + if (!m_selectedImageValue) > + return nullptr; > + if (is<CSSImageValue>(m_selectedImageValue)) > + return downcast<CSSImageValue>(m_selectedImageValue)->cachedImage(); The "is" function already does null checking and returns false if null. So there is no need fo the "if (!m_selectedImageValue)" if statement. Typically I would use a "*" inside the downcast after doing the type check to sort of "take advantage" of the null check that was already done. Could also use early exit for the nullptr return. > Source/WebCore/css/CSSImageSetValue.cpp:107 > + if (is<CSSImageGeneratorValue>(image.value)) { > + m_selectedImageValue = image.value; > + return { m_selectedImageValue, m_bestFitImageScaleFactor }; > + } This if statement can and should be removed, as long as we also remove the assertion below. The code is exactly the same as what is done below. > Source/WebCore/css/CSSImageSetValue.cpp:131 > + size_t i = 0; > + while (i < length) { Should use a for loop even though we can’t do the "i++" inside the loop. We can use "i += 2". Also might be slightly safer to do the length check in a way that skips the last item if somehow the length gets to be odd, rather than reading off the end of the array, even though that’s impossible, so I wrote it as "i + 1 < length". for (size_t i = 0; i + 1 < length; i += 2) { > Source/WebCore/css/CSSImageSetValue.cpp:138 > + CSSValue* imageValue = item(i); > + Ref<CSSValue> resolvedValue = builderState.resolveImageStyles(*imageValue); > + result->append(WTFMove(resolvedValue)); > + ++i; > + > + result->append(Ref(*item(i))); > + ++i; I think this loop body should be rewritten to be two lines. Makes the code easier to read: result->append(builderState.resolveImageStyles(*itemWithoutBoundsCheck(i))); result->append(*itemWithoutBoundsCheck(i + 1)); Would be even prettier if it called item, but since we’re looping seems we should be calling itemWithoutBoundsCheck. > Source/WebCore/css/CSSImageSetValue.cpp:178 > - if (!m_cachedImage) > + if (!m_selectedImageValue) > return false; > - return handler(*m_cachedImage); > + return m_selectedImageValue->traverseSubresources(handler); I think this kind of thing looks really good and is really easy to read with && instead of an if statement. return m_selectedImageValue && m_selectedImageValue->traverseSubresources(handler); > Source/WebCore/css/CSSImageSetValue.h:49 > - static Ref<CSSImageSetValue> create(LoadedFromOpaqueSource loadedFromOpaqueSource) > + static Ref<CSSImageSetValue> create() > { > - return adoptRef(*new CSSImageSetValue(loadedFromOpaqueSource)); > + return adoptRef(*new CSSImageSetValue()); > } These kinds of create functions really don’t benefit from being inlined in the header; that just ends up putting code at each call site that we’d rather share, and we still end up with a function call at each site, to the constructor. Instead we should put these create functions in the .cpp files and let the constructor be inlined. With modern compilers we get this without even explicitly marking the constructor "inline"; that is only really needed in headers to avoid the "one definition rule". Just moving the function to the .cpp file does everything we’d want. Also, in WebKit coding style we don’t put the empty parentheses after the type name in a case like this. Just new CSSImageSetValue without the (). > Source/WebCore/css/CSSImageSetValue.h:52 > + std::pair<CSSValue*, float> selectBestFitImage(const Document&); We should consider the ImageWithScale type as the return type here instead of using a std::pair. See the comments about std::pair/tuple/struct above and the struct just below. Also, there are two spaces between the return type and the function name (in this new code and the original code too). Might need to move the structure elsewhere where we can share it with other classes more easily? > Source/WebCore/css/CSSImageSetValue.h:58 > + CSSValue* value; We should consider Ref<CSSValue> or RefPtr<CSSValue> for this. In modern code we want to take the performance risk of not using raw pointers to reduce the security risk of a lifetime bug. > Source/WebCore/css/CSSImageSetValue.h:72 > - explicit CSSImageSetValue(LoadedFromOpaqueSource); > + explicit CSSImageSetValue(); Typically we don’t need the explicit keyword any more now that it’s not a single-argument constructor. Although I think it might have some meaning in modern C++. Not sure. > Source/WebCore/css/CSSImageSetValue.h:77 > + CSSValue* m_selectedImageValue { nullptr }; We should consider Ref<CSSValue>. As long as it doesn’t create a reference cycle, i think we want to value security over performance here too. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1722 > + AtomString uri = consumeUrlAsStringView(range).toAtomString(); > + if (!uri.isNull()) > + return CSSImageValue::create(completeURL(context, uri), context.isContentOpaque ? LoadedFromOpaqueSource::Yes : LoadedFromOpaqueSource::No); There are some wasteful things about how this (moved, not new) code uses AtomString: 1) No need to convert something to an AtomString before checking it for null. Could just move the "toAtomString" down to the completeURL function. 2) Wasteful for relative URLs to convert them to AtomString then call completeURL on them. If our goal is to save memory, then we’d like to convert to AtomString in the process of doing completeURL, so that it’s the resulting completed URL that is shared with other instances of the same URL. 3) Unfortunate that calling completeURL requires that we make a String or AtomString at all. We should be able to make a completed URL out of a StringView. However, getting this exactly right is tricky. If we just change the argument to StringView then we introduce problems in the case where the URL string is used untouched where we create a new String even if one already exists. Plus we would need a version that makes an AtomString if we want to avoid wasting time in the case where we’d create and destroying a new String that happens to be the same as an existing AtomString. Could fix some or all of this. But I suppose this can be done later. > Source/WebCore/css/parser/CSSPropertyParserHelpers.h:92 > +enum AllowedImageTypes { This should be enum class, not enum. > Source/WebCore/css/parser/CSSPropertyParserHelpers.h:93 > + Forbid = 0, I don’t think we need this name for "no type at all"; not really compatible with OptionSet. If we want a named constant for it we can write something like this: constexpr OptionSet<AllowedImageType> forbidAllImages = { }; constexpr OptionSet<AllowedImageType> noImageTypesAllowed = { }; But I think it’s unlikely to come up. > Source/WebCore/css/parser/CSSPropertyParserHelpers.h:94 > + UrlFunction = 1, WebKit coding style calls this URLFunction, not UrlFunction. > Source/WebCore/css/parser/CSSPropertyParserHelpers.h:95 > + RawStringAsUrl = 2, WebKit coding style calls this RawStringAsURL, not RawStringAsUrl. > Source/WebCore/css/parser/CSSPropertyParserHelpers.h:100 > +RefPtr<CSSValue> consumeImage(CSSParserTokenRange&, CSSParserContext, unsigned allowedImageTypes = AllowedImageTypes::UrlFunction | AllowedImageTypes::ImageSet | AllowedImageTypes::GeneratedImage); New occurrences of idioms like this one in WebKit code should use OptionSet<AllowedImageTypes>, which is superior to a numeric type with no explicit reference to the enum type. As part of that, we’d name the enum class "AllowedImageType" singular, since it’s the OptionSet, not the enum itself, that can hold multiple. > Source/WebCore/page/animation/CSSPropertyAnimation.cpp:339 > + ASSERT(to); > + ASSERT(from); What guarantees these assertions are true here? Is there any chance a StyleImage might not have a selectedImage? If not, then why is the function returning a pointer rather than a reference? > Source/WebCore/rendering/style/StyleCachedImage.h:32 > class CSSValue; > +class CSSImageValue; Coding style says we sort these alphabetically, so CSSImageValue should come first. > Source/WebCore/rendering/style/StyleCachedImage.h:39 > + static Ref<StyleCachedImage> create(CSSImageValue& cssValue, float scaleFactor = 1) { return adoptRef(*new StyleCachedImage(cssValue, scaleFactor)); } Seems a bit dangerous to default the scaleFactor to 1. What callers need to call this without a scale factor? Also, should not be inlined (see explanation above), should instead be in the .cpp file. > Source/WebCore/rendering/style/StyleCursorImage.cpp:5 > + * Copyright (C) 2000 Lars Knoll (knoll@kde.org) > + * (C) 2000 Antti Koivisto (koivisto@kde.org) > + * (C) 2000 Dirk Mueller (mueller@kde.org) > + * Copyright (C) 2003, 2005-2008, 2016 Apple Inc. All rights reserved. Surprised we need so many copyrights for so little code. Could you check to find out where this code came from to minimize the number of different names? > Source/WebCore/rendering/style/StyleCursorImage.cpp:39 > + if (!is<StyleCursorImage>(other)) > + return false; > + > + return equals(downcast<StyleCursorImage>(other)); Works nicely as an &&. return is<StyleCursorImage>(other) && equals(downcast<StyleCursorImage>(other)); > Source/WebCore/rendering/style/StyleCursorImage.h:5 > + * Copyright (C) 2000 Lars Knoll (knoll@kde.org) > + * (C) 2000 Antti Koivisto (koivisto@kde.org) > + * (C) 2000 Dirk Mueller (mueller@kde.org) > + * Copyright (C) 2003, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved. Surprised we need so many copyrights for so little code. Could you check to find out where this code came from to minimize the number of different names? > Source/WebCore/rendering/style/StyleCursorImage.h:26 > +#include "CachedResourceHandle.h" No need for this include. I see nothing below that takes advantage of it. > Source/WebCore/rendering/style/StyleCursorImage.h:33 > +class CachedImage; > +class Document; No need for these forward declarations. StyleMultiImage already needs these. > Source/WebCore/rendering/style/StyleCursorImage.h:38 > + static Ref<StyleCursorImage> create(CSSCursorImageValue& cssValue) { return adoptRef(*new StyleCursorImage(cssValue)); } This shouldn’t be inlined in the header. (See rationale above.) > Source/WebCore/rendering/style/StyleCursorImage.h:42 > +protected: Makes no sense to have things "protected" rather than "private" in a class that's final. These should all be private. > Source/WebCore/rendering/style/StyleCursorImage.h:48 > + StyleCursorImage(CSSCursorImageValue&); Should mark this "explicit". > Source/WebCore/rendering/style/StyleImageSet.cpp:6 > + * Copyright (C) 2000 Lars Knoll (knoll@kde.org) > + * (C) 2000 Antti Koivisto (koivisto@kde.org) > + * (C) 2000 Dirk Mueller (mueller@kde.org) > + * Copyright (C) 2003, 2005-2008, 2016 Apple Inc. All rights reserved. > + * Copyright (C) 2020 Noam Rosenthal (noam@webkit.org) Surprised we need so many copyrights. Could you check to find out where the code being moved came from to minimize the number of different names? > Source/WebCore/rendering/style/StyleImageSet.cpp:32 > +namespace WebCore { > + > + Double blank line here. Should only be a single. > Source/WebCore/rendering/style/StyleImageSet.cpp:44 > + if (!is<StyleImageSet>(other)) > + return false; > + > + return equals(downcast<StyleImageSet>(other)); Looks good with &&. > Source/WebCore/rendering/style/StyleImageSet.cpp:46 > +} > +StyleImageSet::~StyleImageSet() = default; Missing blank line here between function bodies. > Source/WebCore/rendering/style/StyleImageSet.h:6 > +* Copyright (C) 2000 Lars Knoll (knoll@kde.org) > +* (C) 2000 Antti Koivisto (koivisto@kde.org) > +* (C) 2000 Dirk Mueller (mueller@kde.org) > +* Copyright (C) 2003, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved. > +* Copyright (C) 2020 Noam Rosenthal (noam@webkit.org) Surprised we need so many copyrights. Could you check to find out where the code being moved came from to minimize the number of different names? > Source/WebCore/rendering/style/StyleImageSet.h:31 > +class CSSValue; Not needed. > Source/WebCore/rendering/style/StyleImageSet.h:33 > +class Document; Not needed. > Source/WebCore/rendering/style/StyleImageSet.h:38 > + static Ref<StyleImageSet> create(CSSImageSetValue& cssValue) { return adoptRef(*new StyleImageSet(cssValue)); } Don’t inline. > Source/WebCore/rendering/style/StyleImageSet.h:41 > + Ref<CSSValue> cssValue() const final; Can this be private? > Source/WebCore/rendering/style/StyleImageSet.h:43 > +protected: Should be private, not protected. > Source/WebCore/rendering/style/StyleImageSet.h:47 > + StyleImageSet(CSSImageSetValue&); Need explicit. > Source/WebCore/rendering/style/StyleMultiImage.cpp:6 > + * Copyright (C) 2000 Lars Knoll (knoll@kde.org) > + * (C) 2000 Antti Koivisto (koivisto@kde.org) > + * (C) 2000 Dirk Mueller (mueller@kde.org) > + * Copyright (C) 2003, 2005-2008, 2016 Apple Inc. All rights reserved. > + * Copyright (C) 2020 Noam Rosenthal (noam@webkit.org) Surprised we need so many copyrights. Could you check to find out where the code being moved came from to minimize the number of different names? > Source/WebCore/rendering/style/StyleMultiImage.cpp:42 > +StyleMultiImage::StyleMultiImage() > +{ > +} Use "= default". > Source/WebCore/rendering/style/StyleMultiImage.cpp:48 > + auto& otherCached = downcast<StyleMultiImage>(other); What is this downcast for? Surprised it even compiles. This is already a StyleMultiImage. > Source/WebCore/rendering/style/StyleMultiImage.cpp:50 > + if (&otherCached == this) > + return true; Don’t think we need this special case. > Source/WebCore/rendering/style/StyleMultiImage.cpp:53 > + if (!m_isPending && !otherCached.m_isPending && m_selectedImage.get() == otherCached.m_selectedImage.get()) > + return true; > + return false; No need for if (x) return true; return false; Just return x. > Source/WebCore/rendering/style/StyleMultiImage.cpp:63 > + CSSValue* value = nullptr; > + float scaleFactor = 1; No reason to initialize these. > Source/WebCore/rendering/style/StyleMultiImage.cpp:97 > + if (!m_selectedImage) > + return false; > + return m_selectedImage->canRender(renderer, multiplier); Nice with && > Source/WebCore/rendering/style/StyleMultiImage.cpp:109 > + if (!m_selectedImage) > + return false; > + return m_selectedImage->isLoaded(); Nice with && > Source/WebCore/rendering/style/StyleMultiImage.cpp:116 > + if (!m_selectedImage) > + return false; > + return m_selectedImage->errorOccurred(); Nice with && > Source/WebCore/rendering/style/StyleMultiImage.cpp:130 > + if (!m_selectedImage) > + return false; > + return m_selectedImage->imageHasRelativeWidth(); Nice with && > Source/WebCore/rendering/style/StyleMultiImage.cpp:137 > + if (!m_selectedImage) > + return false; > + return m_selectedImage->imageHasRelativeHeight(); Nice with && > Source/WebCore/rendering/style/StyleMultiImage.cpp:151 > + if (!m_selectedImage) > + return false; > + return m_selectedImage->usesImageContainerSize(); Nice with && > Source/WebCore/rendering/style/StyleMultiImage.h:6 > +* Copyright (C) 2000 Lars Knoll (knoll@kde.org) > +* (C) 2000 Antti Koivisto (koivisto@kde.org) > +* (C) 2000 Dirk Mueller (mueller@kde.org) > +* Copyright (C) 2003, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved. > +* Copyright (C) 2020 Noam Rosenthal (noam@webkit.org) Surprised we need so many copyrights. Could you check to find out where the code being moved came from to minimize the number of different names? > Source/WebCore/rendering/style/StyleMultiImage.h:27 > +#include "CachedResourceHandle.h" Why is this include here? I think it’s not needed. > Source/WebCore/rendering/style/StyleMultiImage.h:35 > +class CSSValue; > +class CSSImageSetValue; > +class CachedImage; > +class Document; These are not needed. All are already declared in StyleImage.h. > Source/WebCore/rendering/style/StyleMultiImage.h:63 > + CachedImage* cachedImage() const final; > + > + WrappedImagePtr data() const final; > + > + bool canRender(const RenderElement*, float multiplier) const final; > + bool isPending() const final; > + void load(CachedResourceLoader&, const ResourceLoaderOptions&) final; > + bool isLoaded() const final; > + bool errorOccurred() const final; > + FloatSize imageSize(const RenderElement*, float multiplier) const final; > + bool imageHasRelativeWidth() const final; > + bool imageHasRelativeHeight() const final; > + void computeIntrinsicDimensions(const RenderElement*, Length& intrinsicWidth, Length& intrinsicHeight, FloatSize& intrinsicRatio) final; > + bool usesImageContainerSize() const final; > + void setContainerContextForRenderer(const RenderElement&, const FloatSize&, float); > + void addClient(RenderElement*) final; > + void removeClient(RenderElement*) final; > + RefPtr<Image> image(RenderElement*, const FloatSize&) const final; > + float imageScaleFactor() const final; > + bool knownToBeOpaque(const RenderElement*) const final; > + const StyleImage* selectedImage() const final { return m_selectedImage.get(); } > + StyleImage* selectedImage() final { return m_selectedImage.get(); } Can these be protected or private instead? We want things to be "as private as possible". > Source/WebCore/rendering/style/StyleMultiImage.h:68 > + virtual std::pair<CSSValue*, float> selectBestFitImage(const Document&) const = 0; Given usage, should be private, not protected. No need to have a function visible to override it, just to call it. Please check the same for others. > Source/WebCore/style/StyleBuilderCustom.h:-1327 > - } else if (is<CSSImageSetValue>(item)) { Seems unnecessary to remove the else; I would have added a little more else myself. But OK either way, I guess.

(In reply to Darin Adler from comment #9) > Comment on attachment 388196 [details] Thanks for the comments! Regarding the copyright, the new files are extracted from StyleCachedImage.cpp/h, so I kept the same copyright headers. I will apply the rest of the comments of course before landing. Those are things that I was contemplating while writing this patch and it gives me a good direction.

> Source/WebCore/rendering/style/StyleCachedImage.h:39 > + static Ref<StyleCachedImage> create(CSSImageValue& cssValue, float scaleFactor = 1) { return adoptRef(*new StyleCachedImage(cssValue, scaleFactor)); } > Seems a bit dangerous to default the scaleFactor to 1. What callers need to call > this without a scale factor? Most StyleCachedImages should be with scale factor 1 - the only time when they don't if they're created by a StyleImageSet. I prefer to leave this as is, but open to suggestions...

> > Source/WebCore/css/CSSImageSetValue.cpp:83 > > +CachedImage* CSSImageSetValue::cachedImage() const > This returns nullptr when m_selectedImageValue is a CSSImageGeneratorValue. Is that correct? Correct.

(In reply to Noam Rosenthal from comment #10) > Regarding the copyright, the new files are extracted from > StyleCachedImage.cpp/h, so I kept the same copyright headers. Maybe not worth doing, but you can do more research to see where the code came from. Often there are too many copyright headers when someone takes a few lines from a huge file that many people contributed to. With a little research you can figure out who actually contributed the code being excerpted. But I suppose that’s too big a project to be worthwhile just to cut them down.

Created attachment 388263 [details] Patch

Comment on attachment 388263 [details] Patch Changed based on Darin's comments.

Created attachment 388264 [details] Patch

Comment on attachment 388264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388264&action=review > Source/WebCore/css/CSSCursorImageValue.h:33 > +struct ImageWithScale; We don’t sort the struct in with the classes. We use a separate paragraph for structs afterward. > Source/WebCore/css/CSSImageSetValue.cpp:91 > + if (!m_selectedImageValue) > + return nullptr; No need for this if statement since is<> handles null. I mentioned that in the first patch, but it was combined with another comment. > Source/WebCore/css/CSSImageSetValue.cpp:127 > + for (size_t i = 0; i < length; i += 2) { I suggest "i + 1 < length" here. > Source/WebCore/css/CSSImageSetValue.cpp:129 > + CSSValue* imageValue = itemWithoutBoundsCheck(i); > + result->append(builderState.resolveImageStyles(*imageValue)); Reads better without the local variable: result->append(builderState.resolveImageStyles(*itemWithoutBoundsCheck(i))); I know this gets rid of the "imageValue" variable name, but I think it’s still better. Maybe you disagree? If we do keep a local variable I suggest a reference rather than a pointer. > Source/WebCore/css/CSSImageSetValue.h:39 > class Document; > +class CSSImageValue; These aren’t sorted properly. CSSImageValue would come before Document. > Source/WebCore/css/CSSImageSetValue.h:50 > + > Double blank line here, we only use one. > Source/WebCore/css/CSSImageSetValue.h:77 > bool m_accessedBestFitImage { false }; I think the code would read better if m_bestFitImage was an Optional<ImageWithScale> instead of having a separate m_accessedBestFitImage boolean. Can do that in a subsequent patch, I suppose. > Source/WebCore/page/animation/CSSPropertyAnimation.cpp:337 > + from = from->selectedImage(); > + to = to->selectedImage(); I’m glad you removed the assertions, but I had intended that we return if they are nullptr, not leave the assumption that they are non-null without assertions. > Source/WebCore/rendering/style/StyleMultiImage.cpp:113 > + if (!m_selectedImage) > + return false; > + return m_selectedImage->imageHasRelativeWidth(); Another place we could use &&. > Source/WebCore/style/StyleBuilderState.cpp:90 > + downcast<CSSFilterImageValue>(value).createFilterOperations(*this); There is no "return" here. This should result in a failing test. I think this shows we don’t have enough test coverage.

> > > Source/WebCore/style/StyleBuilderState.cpp:90 > > + downcast<CSSFilterImageValue>(value).createFilterOperations(*this); > > There is no "return" here. This should result in a failing test. I think > this shows we don’t have enough test coverage. No, this is ok and covered. It returns at line 95. createFilterOperations changes the CSS value without returning a new ref (this is previous behavior, copied from a different place).

Comment on attachment 388264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388264&action=review >> Source/WebCore/style/StyleBuilderState.cpp:90 >> + downcast<CSSFilterImageValue>(value).createFilterOperations(*this); > > There is no "return" here. This should result in a failing test. I think this shows we don’t have enough test coverage. You said this is intentional and was copied and pasted. I think this is really subtle. Probably should put the ImageSet case first, and add a comment saying that doing this side effect and not returning is intentional. But also I think the function name "resolve image styles" does not imply that it’s got responsibilities other than resolving the styles; that there will be a side effect in the case of filter images.

Created attachment 388265 [details] Patch

(In reply to Darin Adler from comment #19) > Comment on attachment 388264 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388264&action=review > > >> Source/WebCore/style/StyleBuilderState.cpp:90 > >> + downcast<CSSFilterImageValue>(value).createFilterOperations(*this); > > > > There is no "return" here. This should result in a failing test. I think this shows we don’t have enough test coverage. > > You said this is intentional and was copied and pasted. > > I think this is really subtle. Probably should put the ImageSet case first, > and add a comment saying that doing this side effect and not returning is > intentional. But also I think the function name "resolve image styles" does > not imply that it’s got responsibilities other than resolving the styles; > that there will be a side effect in the case of filter images. The effect is indeed resolving the image style - for filter images. It's not a side effect... Except in the filter case it's a void function that doesn't create a new reference.

Created attachment 388266 [details] Patch

Created attachment 388267 [details] Patch

Comment on attachment 388266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388266&action=review > Source/WebCore/css/parser/CSSPropertyParserHelpers.h:93 > +enum class AllowedImageTypes : uint8_t { We should rename the enum to be singular, AllowedImageType, since that’s the right way for OptionSet. The type for a single one is AllowedImageType. For the allowed image types, it would be OptionSet<AllowedImageType>. Rename can be done after landing if you don’t have a chance beforehand. > Source/WebCore/rendering/style/StyleCachedImage.h:39 > + static Ref<StyleCachedImage> create(CSSImageValue& cssValue, float scaleFactor = 1) { return adoptRef(*new StyleCachedImage(cssValue, scaleFactor)); } This function should be moved out of the header into the .cpp file. > Source/WebCore/rendering/style/StyleMultiImage.cpp:43 > + auto& otherCached = downcast<StyleMultiImage>(other); I don’t understand why we are casting a StyleMultiImage to another StyleMultiImage. The downcast macros are suppose to make this not compile, since it’s an unnecessary cast.

Created attachment 388283 [details] Patch

Comment on attachment 388283 [details] Patch I think it's ready to be landed... Still need to re-add my committer/reviewer status in contributors.json before I can land it myself (was dormant for a good 6 yeas)

Created attachment 388291 [details] Patch

Comment on attachment 388291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388291&action=review Patch seems fine to land; there are some loose ends which we can refine with a follow-up patch. > Source/WebCore/css/CSSCursorImageValue.h:41 > +namespace Style { > +class BuilderState; > +} A little confused about why we are adding this here since I don’t see any new uses of it in the header below. > Source/WebCore/css/CSSImageSetValue.cpp:97 > + updateDeviceScaleFactor(document); Unclear to me why we do this unconditionally, given that we will ignore the result if m_accessedBestFitImage is true. > Source/WebCore/css/CSSImageSetValue.cpp:110 > { > + > + // FIXME: In the future, we want to take much more than deviceScaleFactor into acount here. Extraneous blank line here. > Source/WebCore/css/CSSImageSetValue.h:38 > +class CSSImageValue; Not sure why we are adding this. I don’t see any new uses of it in this header below. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1706 > +RefPtr<CSSValue> consumeImage(CSSParserTokenRange& range, CSSParserContext context, OptionSet<AllowedImageType> AllowedImageType) WebKit coding style says argument names should be lowercase, so it should be allowedImageType (actually I think it should be types or allowedTypes, it’s a set so it should be plural, and the longer name is not needed since every use will have AllowedImageType right next to it). > Source/WebCore/rendering/style/StyleCachedImage.h:31 > class CSSValue; Seems like we should remove CSSValue since it’s not used here any more. > Source/WebCore/rendering/style/StyleCursorImage.h:33 > +struct ImageWithScale; This seems unneeded, since it’s an overridden function that is using it, so it must be declared in the base class’s header.

The commit-queue encountered the following flaky tests while processing attachment 388291 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 388291 [details] Patch Clearing flags on attachment: 388291 Committed r254861: <https://trac.webkit.org/changeset/254861>

All reviewed patches have been landed. Closing bug.