[webkit-reviews] review granted: [Bug 133504] Align srcset parser with recent spec changes : [Attachment 232468] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jun 4 15:18:11 PDT 2014
Darin Adler <darin at apple.com> has granted Yoav Weiss <yoav at yoav.ws>'s request
for review:
Bug 133504: Align srcset parser with recent spec changes
https://bugs.webkit.org/show_bug.cgi?id=133504
Attachment 232468: Patch
https://bugs.webkit.org/attachment.cgi?id=232468&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=232468&action=review
Patch is OK but I still see a lot of things that are either mysterious or need
a bit of work. I’m especially concerned about the code here that does "length -
1"
> Source/WTF/ChangeLog:9
> + I've exposed charactersToInt so that it can be used by
> + HTMLSrcsetParser.cpp.
The best way for us to do this is to make versions that take StringView
arguments in WTF.
> Source/WebCore/html/HTMLImageElement.cpp:144
> + fastGetAttribute(srcAttr),
> + fastGetAttribute(srcsetAttr));
These would look better on one line rather than successive lines.
> Source/WebCore/html/parser/HTMLParserIdioms.h:93
> +template<typename CharType>
> +inline bool isComma(CharType character)
> +{
> + return character == ',';
> +}
Do we really need a function for this? I think that using == ',' at the call
site would be fine and it’s not enhanced with a function.
> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:110
> + ImageCandidate imageCandidate =
bestFitSourceForImageAttributes(m_deviceScaleFactor,
> + m_urlToLoad,
> + m_srcSetAttribute);
I think this would read better all on one line.
> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:112
> + String srcMatchingScale = imageCandidate.url();
> setUrlToLoad(srcMatchingScale, true);
I don’t think the local variable is helpful here. I suggest combining these two
into a single line.
But also, it’s really strange to call the url() function returning an
AtomicString but then just put it into a String.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:40
> +static bool compareByDensity(const ImageCandidate& first, const
ImageCandidate& second)
Should mark this inline.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:56
> + descriptorStart = 0;
Please use nullptr.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:72
> +template<typename CharType>
> +static bool isEOF(const CharType* position, const CharType* end)
> +{
> + return position >= end;
> +}
Does this helper function aid in readability?
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:77
> +template<typename CharType>
> +static void tokenizeDescriptors(const CharType*& position,
> + const CharType* attributeEnd,
> + Vector<StringView>& descriptors)
Please put this all on one line rather than splitting it up vertically.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:82
> + while (true) {
I think this would read better as a for loop:
for (; ; ++position) {
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:134
> +static float stringViewToFloat(const StringView& string, bool& isValid)
> +{
> + if (string.is8Bit())
> + return charactersToFloat(string.characters8(), string.length() - 1,
&isValid);
> + return charactersToFloat(string.characters16(), string.length() - 1,
&isValid);
> }
This function belongs in WTF. And also it need not have string view in its
name. Also, why length - 1?
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:141
> +static int stringViewToInt(const StringView& string, bool& isValid)
> {
> - ASSERT(imageCandidates.isEmpty());
> + if (string.is8Bit())
> + return charactersToInt(string.characters8(), string.length() - 1,
&isValid);
> + return charactersToInt(string.characters16(), string.length() - 1,
&isValid);
> +}
This function belongs in WTF. And also it need not have string view in its
name. Also, why length - 1?
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:145
> + for (Vector<StringView>::iterator descriptor = descriptors.begin();
descriptor != descriptors.end(); ++descriptor) {
An iterator should not be named "descriptor". Also, we should almost never use
iterators explicitly to iterate through vectors; iterating with an index is
almost always better, except when using something generic that always uses
iterators. But, we can use a modern for loop to solve both those problems:
for (auto& descriptor : descriptors) {
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:147
> + if (!descriptor->length())
> + continue;
Normally we’d use isEmpty() for this.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:148
> + UChar c = (*descriptor)[descriptor->length() - 1];
Please consider a word rather than a letter for the name of this variable.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:179
> +static void parseImageCandidatesFromSrcsetAttribute(const String& attribute,
const CharType* attributeStart, unsigned length, Vector<ImageCandidate>&
imageCandidates)
This function does not need the attribute argument. You can make a StringView
from a pointer and length, you don’t need to go back to the original String to
make it.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:184
> + while (position < attributeEnd) {
I suggest a for loop instead of a while loop here.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:229
> +static void parseImageCandidatesFromSrcsetAttribute(const String& attribute,
Vector<ImageCandidate>& imageCandidates)
This function should take a StringView argument rather than a const String&.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:232
> + if (attribute.isNull())
> + return;
No need for this check. The code would already do the right thing without it
and it’s not something we need to optimize for.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:237
> + if (attribute.is8Bit())
> + parseImageCandidatesFromSrcsetAttribute<LChar>(attribute,
attribute.characters8(), attribute.length(), imageCandidates);
> + else
> + parseImageCandidatesFromSrcsetAttribute<UChar>(attribute,
attribute.characters16(), attribute.length(), imageCandidates);
I’m not sure we need separately optimized versions for 8-bit and 16-bit
characters. We should considering using StringView more for the argument types
and indexing rather than chasing pointers during the parsing process.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:241
> +static ImageCandidate pickBestImageCandidate(float deviceScaleFactor,
> + Vector<ImageCandidate>& imageCandidates)
Please put this on one line.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:243
> + const float defaultDensityValue = 1.0;
I don’t think this constant adds much to the readability of the code, but I
guess maybe it’s OK. It’s strange to define it here, though, high up in the
function. Also, I see places that are using a default of 1.0 elsewhere without
a named constant.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:244
> + bool ignoreSrc = false;
What’s the point of this? It’s always false?
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:249
> + for (Vector<ImageCandidate>::iterator it = imageCandidates.begin(); it
!= imageCandidates.end(); ++it) {
Please use a modern for loop here:
for (auto& candidate : imageCandidates) {
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:250
> + if (it->density() < 0)
It’s really strange to have this "< 0" here. Is there some cleaner way for us
to do this? I noticed in other places you have both a named constant and a
hasDensity function, but here we are following a different pattern.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:279
> +ImageCandidate bestFitSourceForImageAttributes(float deviceScaleFactor,
> + const String& srcAttribute,
> + const String& srcsetAttribute)
Please put this all on one line.
The argument types should probably be const AtomicString& if they are attribute
values since that’s what attribute values are.
> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:295
> + return pickBestImageCandidate(deviceScaleFactor,
> + imageCandidates);
This should be a single line.
> Source/WebCore/html/parser/HTMLSrcsetParser.h:36
> #include <wtf/text/WTFString.h>
Can remove this include since StringView.h includes WTFString.h.
> Source/WebCore/html/parser/HTMLSrcsetParser.h:69
> +class ImageCandidate {
I think it would be better to use a struct rather than a class for this. The
class isn’t adding much encapsulation.
> Source/WebCore/html/parser/HTMLSrcsetParser.h:81
> + ImageCandidate(const String& source, unsigned start, unsigned length,
const DescriptorParsingResult& result, OriginAttribute originAttribute)
This constructor should just take a StringView rather than source, start, and
length.
> Source/WebCore/html/parser/HTMLSrcsetParser.h:84
> + ,
m_density(result.hasDensity()?result.density():UninitializedDescriptor)
> + ,
m_resourceWidth(result.hasWidth()?result.resourceWidth():UninitializedDescripto
r)
Please put spaces around the ? and the : in the ?: operator.
> Source/WebCore/html/parser/HTMLSrcsetParser.h:92
> + String toString() const
> {
> - return m_scaleFactor;
> + return String(m_string.toString());
> + }
The getter should just return the StringView. We don’t need to build a toString
function into this class.
It’s also wrong to write String(x.toString()). Should just be x.toString().
> Source/WebCore/html/parser/HTMLSrcsetParser.h:97
> + AtomicString url() const
> + {
> + return AtomicString(m_string.toString());
> + }
We should not have this function. Also, we would use toAtomicString for this,
not toString, if we needed it.
> Source/WebCore/html/parser/HTMLSrcsetParser.h:122
> + inline bool isEmpty() const
> + {
> + return m_string.isEmpty();
> }
We don’t need a separate function for this. We should just use the function
that returns a StringView and call isEmpty on that.
Also, no need to mark this inline. All functions defined in a class like this
are inline.
> Source/WebCore/html/parser/HTMLSrcsetParser.h:133
> +ImageCandidate bestFitSourceForImageAttributes(float deviceScaleFactor,
> + const String& srcAttribute,
> + const String& srcsetAttribute);
Should be all on one line. There’s this new vertical coding style you are using
a lot. Maybe it’s a Google/Blink thing?
> Source/WebCore/html/parser/ParsingUtilities.h:42
> +template<typename CharType>
> +bool skipExactly(const CharType*& position, const CharType* end, CharType
delimiter)
> +{
> + if (position < end && *position == delimiter) {
> + ++position;
> + return true;
> + }
> + return false;
> +}
We should consider writing these utilities for a StringView instead of a
character pointer so we can do this kind of parsing without having two whole
copies of every function.
More information about the webkit-reviews
mailing list