Bug 123698 – [CSS] Enable css-image-orientation on EFL and GTK ports.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Gyuyoung Kim]

There were some regressions in layout test when css-image-orientation is enabled. The regressions come from wrong early return in ENABLE(CSS_IMAGE_ORIENTATION) blocks. This patch fixes wrong early return, and enable the css-image-orientation on EFL and GTK by default.

[Gyuyoung Kim]

Created attachment 215850 [details]
Patch

[Gyuyoung Kim]

Created attachment 215868 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 215868 [details]
Patch

Attachment 215868 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/20398067

[Gyuyoung Kim]

(In reply to comment #3)
> (From update of attachment 215868 [details])
> Attachment 215868 [details] did not pass gtk-ews (gtk):
> Output: http://webkit-queues.appspot.com/results/20398067

When I build gtk port with this patch, there is no build break. It seems to me gtk ews doesn't generate *CSSPropertyImageOrientation* with this patch. Is this right ?

[Gyuyoung Kim]

Created attachment 215888 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 215888 [details]
Patch

Attachment 215888 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/19648639

[Gyuyoung Kim]

CC'ing Beth and Martin, 

Beth, could you take a look this ?

Martin, in gtk build error cases, there is no build break when I build gtk port using this patch. It looks gtk ews doesn't support to build gtk port from scratch. Could you take a look this ?

[Sergio Correia (qrwteyrutiyoup)]

Comment on attachment 215888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215888&action=review

> Source/WebCore/ChangeLog:9
> +        when enabling css-image-orientaiton. This fixes those wrong implementation as well as enables it on EFL

nit: enabling css-image-orientation.

> Source/WebCore/loader/cache/CachedImage.cpp:266
> +            imageSize = static_cast<BitmapImage*>(m_image.get())->sizeRespectingOrientation(orientationDescription);

There's still a problem here, as we don't set imageSize if we don't get to this inner if, and hence get a zero sized image, since we won't fall in the outer else clause. I suggest you initialize imageSize in its declaration with m_image->size() and remove the outer else clause, to make sure we always have imageSize with a valid value. If needed, it will be changed accordingly inside ENABLE(CSS_IMAGE_ORIENTATION) / ENABLE(SVG).

[Gyuyoung Kim]

Created attachment 216009 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #8)
> (From update of attachment 215888 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215888&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        when enabling css-image-orientaiton. This fixes those wrong implementation as well as enables it on EFL
> 
> nit: enabling css-image-orientation.

Fixed. Thanks.

> > Source/WebCore/loader/cache/CachedImage.cpp:266
> > +            imageSize = static_cast<BitmapImage*>(m_image.get())->sizeRespectingOrientation(orientationDescription);
> 
> There's still a problem here, as we don't set imageSize if we don't get to this inner if, and hence get a zero sized image, since we won't fall in the outer else clause. I suggest you initialize imageSize in its declaration with m_image->size() and remove the outer else clause, to make sure we always have imageSize with a valid value.

Yes, we shouldn't make any situation that *imageSize* is not initialized. Fixed.

[kov's GTK+ EWS bot]

Comment on attachment 216009 [details]
Patch

Attachment 216009 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/19568955

[WebKit Commit Bot]

Comment on attachment 216009 [details]
Patch

Clearing flags on attachment: 216009

Committed r158659: <http://trac.webkit.org/changeset/158659>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Gyuyoung Kim]

*** Bug 123256 has been marked as a duplicate of this bug. ***