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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=91566

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

------- 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) =
0;

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.