[webkit-reviews] review denied: [Bug 110252] Implement img element's srcset attribute : [Attachment 190345] updated_patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Vineet Chaudhary (vineetc)
<rgf748 at motorola.com>'s request for review:
Bug 110252: Implement img element's srcset attribute
https://bugs.webkit.org/show_bug.cgi?id=110252

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=190345&action=review


Where are we selecting the correct image?

> Source/WebCore/ChangeLog:9
> +	   Spec :
http://www.whatwg.org/specs/web-apps/current-work/multipage/embedded-content-1.
html#the-img-element

Could you refer to the specific version of the spec you used?

> Source/WebCore/dom/Element.cpp:1132
> +const QualifiedName& Element::imageSourceSetAttributeName() const
> +{
> +    return srcsetAttr;
> +}

Why do we need this function? Since there is no alternative name to srcsetAttr
we should just hardcode it.

> Source/WebCore/html/HTMLImageElement.cpp:308
> +    return document()->completeURL(getAttribute(srcsetAttr));

How does this work? completeURL doesn't support srcset syntax. r- because of
this.

>> Source/WebCore/loader/ImageLoader.cpp:205
>> +	}
> 
> This parsing code is messy and way too complicated for a simple format like
the srcset descriptor list. For starters, why did you write all those 5-line
while loops instead of using String::find?

I completely agree with Maciej here. r- because of this as well.
In addition, this code should probably live in HTMLImageElement instead.
ImageLoader should only be concerned about loading images, not a particular
syntax used in an element that loads an image.

> Source/WebCore/loader/ImageLoader.cpp:214
> +void ImageLoader::processImageSourceAttributes(String& urlString, String&
descString)

I would also put this on HTMLImageElement.

> Source/WebCore/loader/ImageLoader.h:47
> +    int width;
> +    int height;

Can we ever have negative width & height?