[webkit-reviews] review denied: [Bug 91566] Integrate css3-images image-orientation with existing EXIF support : [Attachment 211530] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 17 21:38:58 PDT 2013

Beth Dakin <bdakin at apple.com> has denied Gyuyoung Kim
<gyuyoung.kim at samsung.com>'s request for review:
Bug 91566: Integrate css3-images image-orientation with existing EXIF support

Attachment 211530: Patch

------- Additional Comments from Beth Dakin <bdakin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=211530&action=review

> Source/WebCore/loader/cache/CachedImage.cpp:261
> +    if (renderer && renderer->style())

It doesn't make any sense to null-check renderer here. If it is ever null, then
you will crash on the previous line since you access it without a null check
there. Figure out if it can ever be null, and if so, null-check it before you
use it at all. If it cannot be null, then don't null check it.

> Source/WebCore/page/DragController.cpp:889
> +    if (element->renderer() && element->renderer()->style())

Same comment about null-checking applies here.

> Source/WebCore/page/Frame.cpp:1040
> +    if (renderer->style()) 

Is it necessary to null-check style?

> Source/WebCore/platform/graphics/Image.h:-206
> -    virtual void draw(GraphicsContext*, const FloatRect& dstRect, const
FloatRect& srcRect, ColorSpace styleColorSpace, CompositeOperator, BlendMode) =

Why is this function being removed?

> Source/WebCore/rendering/RenderImage.cpp:150
> +    if (diff == StyleDifferenceLayout

There's no reason to break this if-condition onto two lines.

> LayoutTests/fast/css/image-orientation/image-orientation.html:8
> +description('Apply image-orientation property and check computed style.');

You should add an explanation to the LayoutTest change log to explain your
changes to this test. Was this description always inaccurate and you are just
correcting it? Please explain.

More information about the webkit-reviews mailing list