[webkit-reviews] review denied: [Bug 135448] URLs in srcset attributes are not made absolute upon copy and paste : [Attachment 235796] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 135448: URLs in srcset attributes are not made absolute upon copy and paste
https://bugs.webkit.org/show_bug.cgi?id=135448

Attachment 235796: Patch
https://bugs.webkit.org/attachment.cgi?id=235796&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=235796&action=review


review- because MarkupAccumulator::appendAttribute won’t work

> Source/WebCore/dom/Element.h:396
> +    virtual String completeUrlAttributeValue(const URL& base, const
Attribute&) const;

The URL acronym should not be spelled “Url”.

> Source/WebCore/html/HTMLImageElement.cpp:351
> +	   || attribute.name() == srcsetAttr

Changing this to return true will have effects in others places, not just in
the completeURLs function that is updated in this patch.

We will also have to figure out what to do inside
MarkupAccumulator::appendAttribute, which also calls isURLAttribute, and make
sure we have a test case that covers that path.

Also, the function isJavaScriptURLAttribute calls isURLAttribute, and then
assumes it can process the attribute as a URL, which would be wrong for srcset.
Maybe we’ll get lucky and it will return false, but I think it’s sloppy to let
it try to look for a protocol in a srcset. Especially since the reason this
function exists is for security purposes.

It’s also an error to call getURLAttribute or getNonEmptyURLAttribute on
srcset, but the assertions in those functions won’t fire any more because
isURLAttribute will return true. OK, but not great.

> Source/WebCore/html/HTMLImageElement.cpp:362
> +	  
parseImageCandidatesFromSrcsetAttribute(StringView(attribute.value()),
imageCandidates);

Why do we have to specify StringView() explicitly?

> Source/WebCore/html/HTMLImageElement.cpp:366
> +		   result.append(", ");

Should be appendLiteral.

>> Source/WebCore/html/HTMLImageElement.cpp:369
>> +		    result.append(String::format(" %fx", candidate.density));
> 
> It looks more efficient to call append(' '), appendNumber(candidate.density),
and then append('x').

We should also double check on number formatting. I’m pretty sure that %f puts
in lots of extra zeros after the decimal point. I think appendNumber does what
you’d want.

> Source/WebCore/html/parser/HTMLSrcsetParser.h:107
> +void parseImageCandidatesFromSrcsetAttribute(StringView attribute,
Vector<ImageCandidate>& result);

Should probably change this to return the vector as a return value. No reason
we need to use an out argument for this.