[Webkit-unassigned] [Bug 133504] Align srcset parser with recent spec changes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=133504





--- Comment #4 from Yoav Weiss <yoav at yoav.ws>  2014-06-05 02:24:47 PST ---
(From update of attachment 232468)
View in context: https://bugs.webkit.org/attachment.cgi?id=232468&action=review

>> Source/WebCore/html/parser/HTMLParserIdioms.h:93
>> +}
> 
> 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.

The function is needed in order to pass it to reverseSkipWhile in HTMLSrcsetParser.

>> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:72
>> +}
> 
> Does this helper function aid in readability?

I think it does since it aligns well with the spec's language.

>> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:134
>>  }
> 
> This function belongs in WTF. And also it need not have string view in its name. Also, why length - 1?

OK. The length-1 is there since we need to ignore the latest char, which is the descriptor char (e.g. 'x').
I've changed it to make it clearer.

>> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:184
>> +    while (position < attributeEnd) {
> 
> I suggest a for loop instead of a while loop here.

Since this loop doesn't advance position on every step (according to spec), I thought a while is more appropriate.

>> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:237
>> +        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.

OK. I've added a FIXME for that. I'll address it in a separate patch.

>> Source/WebCore/html/parser/ParsingUtilities.h:42
>> +}
> 
> 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.

I think that would be required in order to move the parsing to working with StringView and position instead of direct pointers.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.