Bug 200553 – shouldRespectImageOrientation should be a value in ImageOrientation

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Currently we have 

1. enum ImageOrientationEnum {

   };

which list the EXIF image orientation values.

2. enum RespectImageOrientationEnum {

   };

which tells whether the image EXIF image orientation is respected or not. The function RenderElement::shouldRespectImageOrientation() returns RespectImageOrientation for the ImageDocument or if the settings().shouldRespectImageOrientation() is true. Otherwise it returns DoNotRespectImageOrientation.

3. struct ImageOrientationDescription {
   
   };

which combines ImageOrientationEnum and RespectImageOrientationEnum in one structure and can be used with ImagePaintingOptions.

4. class ImageOrientation {

   }

which is a wrapper for ImageOrientationEnum.

5. ImageOrientationEnum RenderStyle::imageOrientation()
   {

   }

which returns the css image-orientation property but it still is under a feature control flag CSS_IMAGE_ORIENTATION. Otherwise it returns DefaultImageOrientation which is OriginTopLeft, i.e. no image orientation.

Instead of having ImageOrientationEnum, ImageOrientationDescription, ImageOrientation and RespectImageOrientationEnum we are going to have a single structure ImageOrientation which is a wrapper for ImageOrientationEnum. The RespectImageOrientation will be represented as a new enum value 'FromImage'. The struct will have a constructor and casting operator such that assigning an enum value and comparing with an enum value will be done implicitly.

[Said Abou-Hallawa]

Created attachment 375863 [details]
Patch

[Said Abou-Hallawa]

Created attachment 375865 [details]
Patch

[Said Abou-Hallawa]

Created attachment 375868 [details]
Patch

[Said Abou-Hallawa]

Created attachment 375871 [details]
Patch

[Said Abou-Hallawa]

Created attachment 376089 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 376089 [details]
Patch

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

We're going to force call sites to be explicit about whether they want from-image.

> Source/WebCore/platform/graphics/ImageOrientation.h:56
> +        Default           = None,
> +
> +        EXIFFirst         = OriginTopLeft,
> +        EXIFLast          = OriginLeftBottom,
> +
> +        First             = FromImage,
> +        Last              = EXIFLast

Maybe moved these out and just define them where they get used.

> Source/WebCore/platform/graphics/ImageOrientation.h:71
> +    operator int() const { return static_cast<int>(m_orientation); }

Remove this.

> Source/WebCore/platform/graphics/ImageOrientation.h:-79
> -    ImageOrientationEnum imageOrientation() { return m_orientation; }

Keep this, call it .value()? Callers should switch on this.

[Said Abou-Hallawa]

Created attachment 376177 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 376089 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageOrientation.h:56
>> +        Last              = EXIFLast
> 
> Maybe moved these out and just define them where they get used.

The first and last values are made private static constants inside the struct.

>> Source/WebCore/platform/graphics/ImageOrientation.h:71
>> +    operator int() const { return static_cast<int>(m_orientation); }
> 
> Remove this.

I changed it to be:
    operator Orientation() const { return m_orientation; }

This can make this code possible:
    if (orientation == ImageOrientation::None)

And this also:
    switch (orientation) {
    case ImageOrientation::None:
    // Compiler will complain if an enum value is missed or out of range value is used.
    ...
    }

>> Source/WebCore/platform/graphics/ImageOrientation.h:-79
>> -    ImageOrientationEnum imageOrientation() { return m_orientation; }
> 
> Keep this, call it .value()? Callers should switch on this.

I think the enum casting operator makes sense for this class since it is just a wrapper around the enum value.

So I think
    if (orientation == ImageOrientation::None)

Looks natural and it is better than 

    if (orientation.value() == ImageOrientation::None)

Or

    if (orientation == ImageOrientation(ImageOrientation::None))

The casting operator and the ImageOrientation(Orientation) constructor the hide fact that ImageOrientation is struct and make it look like an enum which is the common case for the ImageOrientation usage.

[Said Abou-Hallawa]

Created attachment 376178 [details]
Patch

[Said Abou-Hallawa]

Created attachment 376181 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 376181 [details]
Patch

Rejecting attachment 376181 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 376181, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
e.xcodeproj/project.pbxproj.rej
patching file Source/WebCore/css/CSSPrimitiveValueMappings.h
patching file Source/WebCore/dom/DataTransfer.cpp
patching file Source/WebCore/html/HTMLCanvasElement.cpp
patching file Source/WebCore/loader/cache/CachedImage.cpp
patching file Source/WebCore/page/DragController.cpp
patching file Source/WebCore/platform/DragImage.cpp
patching file Source/WebCore/platform/DragImage.h
patching file Source/WebCore/platform/graphics/BitmapImage.cpp
patching file Source/WebCore/platform/graphics/BitmapImage.h
patching file Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp
patching file Source/WebCore/platform/graphics/CrossfadeGeneratedImage.h
patching file Source/WebCore/platform/graphics/CustomPaintImage.cpp
patching file Source/WebCore/platform/graphics/CustomPaintImage.h
patching file Source/WebCore/platform/graphics/GeneratedImage.h
patching file Source/WebCore/platform/graphics/GradientImage.cpp
patching file Source/WebCore/platform/graphics/GradientImage.h
patching file Source/WebCore/platform/graphics/GraphicsContext.cpp
patching file Source/WebCore/platform/graphics/GraphicsContext.h
patching file Source/WebCore/platform/graphics/GraphicsContextImpl.cpp
patching file Source/WebCore/platform/graphics/Image.cpp
patching file Source/WebCore/platform/graphics/Image.h
patching file Source/WebCore/platform/graphics/ImageFrame.h
patching file Source/WebCore/platform/graphics/ImageOrientation.cpp
rm 'Source/WebCore/platform/graphics/ImageOrientation.cpp'
patching file Source/WebCore/platform/graphics/ImageOrientation.h
patching file Source/WebCore/platform/graphics/ImageSource.cpp
patching file Source/WebCore/platform/graphics/NamedImageGeneratedImage.cpp
patching file Source/WebCore/platform/graphics/NamedImageGeneratedImage.h
patching file Source/WebCore/platform/graphics/NativeImage.h
patching file Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm
patching file Source/WebCore/platform/graphics/cairo/CairoOperations.cpp
patching file Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp
patching file Source/WebCore/platform/graphics/cairo/NativeImageCairo.cpp
patching file Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp
patching file Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp
patching file Source/WebCore/platform/graphics/cg/NativeImageCG.cpp
patching file Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp
patching file Source/WebCore/platform/graphics/cg/PDFDocumentImage.h
patching file Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
patching file Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp
patching file Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h
patching file Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp
patching file Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.h
patching file Source/WebCore/platform/graphics/win/Direct2DOperations.cpp
patching file Source/WebCore/platform/graphics/win/ImageCGWin.cpp
patching file Source/WebCore/platform/graphics/win/ImageCairoWin.cpp
patching file Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp
patching file Source/WebCore/platform/graphics/win/ImageDirect2D.cpp
patching file Source/WebCore/platform/graphics/win/NativeImageDirect2D.cpp
patching file Source/WebCore/platform/gtk/DragImageGtk.cpp
patching file Source/WebCore/platform/image-decoders/ScalableImageDecoderFrame.h
patching file Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
patching file Source/WebCore/platform/ios/DragImageIOS.mm
patching file Source/WebCore/platform/mac/DragImageMac.mm
patching file Source/WebCore/platform/win/DragImageCGWin.cpp
patching file Source/WebCore/platform/win/DragImageCairoWin.cpp
patching file Source/WebCore/platform/win/DragImageDirect2D.cpp
patching file Source/WebCore/rendering/RenderBoxModelObject.cpp
patching file Source/WebCore/rendering/RenderElement.cpp
patching file Source/WebCore/rendering/RenderElement.h
patching file Source/WebCore/rendering/RenderEmbeddedObject.cpp
patching file Source/WebCore/rendering/RenderImage.cpp
patching file Source/WebCore/rendering/RenderLayerBacking.cpp
patching file Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp
patching file Source/WebCore/rendering/style/RenderStyle.h
patching file Source/WebCore/rendering/style/StyleRareInheritedData.h
patching file Source/WebCore/svg/graphics/SVGImage.cpp
patching file Source/WebCore/svg/graphics/SVGImage.h
patching file Source/WebCore/svg/graphics/SVGImageForContainer.cpp
patching file Source/WebCore/svg/graphics/SVGImageForContainer.h
patching file Source/WebKit/ChangeLog
Hunk #1 succeeded at 1 with fuzz 1.
patching file Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Simon Fraser']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/12906694

[Said Abou-Hallawa]

Created attachment 376224 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 376224 [details]
Patch

Clearing flags on attachment: 376224

Committed r248657: <https://trac.webkit.org/changeset/248657>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54284527>