[webkit-reviews] review granted: [Bug 81941] -webkit-image-set should support all the image functions WebKit supports, not just url() : [Attachment 388196] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 20 11:32:35 PST 2020
Darin Adler <darin at apple.com> has granted Noam Rosenthal <noam at webkit.org>'s
request for review:
Bug 81941: -webkit-image-set should support all the image functions WebKit
supports, not just url()
https://bugs.webkit.org/show_bug.cgi?id=81941
Attachment 388196: Patch
https://bugs.webkit.org/attachment.cgi?id=388196&action=review
--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 388196
--> https://bugs.webkit.org/attachment.cgi?id=388196
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 at kde.org)
> + * (C) 2000 Antti Koivisto (koivisto at kde.org)
> + * (C) 2000 Dirk Mueller (mueller at 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 at kde.org)
> + * (C) 2000 Antti Koivisto (koivisto at kde.org)
> + * (C) 2000 Dirk Mueller (mueller at 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 at kde.org)
> + * (C) 2000 Antti Koivisto (koivisto at kde.org)
> + * (C) 2000 Dirk Mueller (mueller at kde.org)
> + * Copyright (C) 2003, 2005-2008, 2016 Apple Inc. All rights reserved.
> + * Copyright (C) 2020 Noam Rosenthal (noam at 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 at kde.org)
> +* (C) 2000 Antti Koivisto (koivisto at kde.org)
> +* (C) 2000 Dirk Mueller (mueller at kde.org)
> +* Copyright (C) 2003, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
> +* Copyright (C) 2020 Noam Rosenthal (noam at 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 at kde.org)
> + * (C) 2000 Antti Koivisto (koivisto at kde.org)
> + * (C) 2000 Dirk Mueller (mueller at kde.org)
> + * Copyright (C) 2003, 2005-2008, 2016 Apple Inc. All rights reserved.
> + * Copyright (C) 2020 Noam Rosenthal (noam at 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 at kde.org)
> +* (C) 2000 Antti Koivisto (koivisto at kde.org)
> +* (C) 2000 Dirk Mueller (mueller at kde.org)
> +* Copyright (C) 2003, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
> +* Copyright (C) 2020 Noam Rosenthal (noam at 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.
More information about the webkit-reviews
mailing list