[webkit-reviews] review granted: [Bug 123832] Use srcset's pixel density to determine intrinsic size : [Attachment 222659] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Yoav Weiss <yoav at yoav.ws>'s request
for review:
Bug 123832: Use srcset's pixel density to determine intrinsic size
https://bugs.webkit.org/show_bug.cgi?id=123832

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=222659&action=review


Some small things to fix up.

> Source/WebCore/ChangeLog:39543
>  2013-11-14  Gyuyoung Kim  <gyuyoung.kim at samsung.com>
> -
>	   Introduce FILTER_TYPE_CASTS for child filter class

What?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1209
> +    CachedImage* cachedImage = image->cachedImage();
> +    if (cachedImage) {

I think this should go back to being if (CachedImage* cachedImage = ....)

> LayoutTests/ChangeLog:14
> +	   Layout test changes include modifications of existing tests to
accomodate the new image dimensions, as well as new tests for this
> +	   specific functionality

Nit, missing .

> LayoutTests/ChangeLog:27
> +	   * fast/hidpi/image-srcset-invalid-inputs-expected.txt: Added.

I'm confused here - I don't see the actual test, just the expected results. And
it's a DRT dump, so probably should be in platform.

> LayoutTests/TestExpectations:62
> +webkit.org/b/124342 fast/hidpi/image-srcset-svg-canvas.html [ Skip ]
> +webkit.org/b/124342 fast/hidpi/image-srcset-svg-canvas-2x.html [ Skip ]
> +webkit.org/b/124349 fast/hidpi/image-srcset-relative-svg-canvas-2x.html [
Skip ]
> +webkit.org/b/124349 fast/hidpi/image-srcset-relative-svg-canvas.html [ Skip
]
> +

Why are you skipping these? Your changelog doesn't mention it. Why skip tests
that you're adding in this patch? It might be better to check in failing
results.

> LayoutTests/fast/hidpi/image-srcset-invalid-inputs-expected.txt:11
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600
> +  RenderBlock {HTML} at (0,0) size 800x600
> +    RenderBody {BODY} at (8,8) size 784x584
> +	 RenderBlock {DIV} at (0,0) size 784x17
> +	   RenderText {#text} at (0,0) size 779x17
> +	     text run at (0,0) width 779: "This test passes if this img tag
below is empty and displays nothing. It ensures that the srcset attribute
supports invalid inputs"
> +	 RenderBlock (anonymous) at (0,17) size 784x100
> +	   RenderImage {IMG} at (0,0) size 100x100
> +	   RenderText {#text} at (0,0) size 0x0

See above for comment.