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

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Romain Perier
<romain.perier at gmail.com>'s request for review:
Bug 110252: Implement img element's srcset attribute
https://bugs.webkit.org/show_bug.cgi?id=110252

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=206861&action=review


Great work! That is quite something for a first patch :)
Sorry, I did not get the time to do a proper review today. Some quick comments
bellow.

I think we should also add a Feature Flag
(http://trac.webkit.org/wiki/AddingFeatures) if we want to mature this feature.


> Source/WebCore/html/HTMLImageElement.cpp:136
> +    const String& string = static_cast<String>(value);

static_cast<const String&>

> Source/WebCore/html/HTMLImageElement.cpp:138
> +    ImageWithScale image;

I would declare this where is it used instead.

> Source/WebCore/html/HTMLImageElement.cpp:141
> +    string.split(",", srcSet);

"," -> ','

I would move the declaration of srcSet just above this line to reduce the span
declaration<->use.

> Source/WebCore/html/HTMLImageElement.cpp:142
> +    for (size_t i = 0; i < srcSet.size(); i++) {

i++ -> ++i

> Source/WebCore/html/HTMLImageElement.cpp:146
> +	   srcSet[i].stripWhiteSpace().split(" ", data);

" " -> ' '

> Source/WebCore/html/HTMLImageElement.cpp:148
> +	       if (data[1].endsWith("x")) {

"x" -> 'x'

> Source/WebCore/html/HTMLImageElement.cpp:165
> +    std::sort(m_imageSet.begin(), m_imageSet.end(), compareByScaleFactor);

Shouldn't the sort be stable?

What if the srcset gives several input for the same scale factor? The one we
chose should not depends on the sort algorithm.

> Source/WebCore/html/HTMLImageElement.cpp:184
> +	   updateImageSet(value);
> +	   updateBestImageForScaleFactor();

Shouldn't this also be run when srcAttr changes?

> Source/WebCore/html/HTMLImageElement.h:117
> +	   AtomicString imageURL;

Does this really need to be Atomic?

> Source/WebCore/html/HTMLImageElement.h:130
> +    Vector<ImageWithScale> m_imageSet;

The naming is not ideal, because m_imageSet is not a Set :)

> LayoutTests/ChangeLog:16
> +	   * platform/mac/fast/hidpi/image-srcset-simple-expected.png:
> +	   * platform/mac/fast/hidpi/image-srcset-simple-expected.txt:
> +	   * platform/mac/fast/hidpi/image-srcset-src-selection-expected.png:
> +	   * platform/mac/fast/hidpi/image-srcset-src-selection-expected.txt:

I would have more test coverage. We should try to cover as much as possible.
e.g.:
-Various invalid input.
-Valid src, invalid input on the srcset.
-If you have a src (so scaleFactor = 1) and srcset also defines something for
scaleFactor = 1, which image is selected.
-Changing the attributes dynamically from JavaScript.
-Removing the src attribute from JavaScript.

> LayoutTests/fast/hidpi/image-srcset-src-selection.html:34
> +    <div>This test passes if the div below is a blue 100px square when the
deviceScaleFactor is 1. It simply ensures that the src attribute is taken into
account by the selection algorithm when this one is processing the images
candidates</div>

Missing period.