Bug 200553

Summary: shouldRespectImageOrientation should be a value in ImageOrientation
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: ImagesAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 89052    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2019-08-08 16:32:44 PDT
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.
Comment 1 Said Abou-Hallawa 2019-08-08 16:59:01 PDT
Created attachment 375863 [details]
Patch
Comment 2 Said Abou-Hallawa 2019-08-08 17:14:25 PDT
Created attachment 375865 [details]
Patch
Comment 3 Said Abou-Hallawa 2019-08-08 17:23:29 PDT
Created attachment 375868 [details]
Patch
Comment 4 Said Abou-Hallawa 2019-08-08 17:54:30 PDT
Created attachment 375871 [details]
Patch
Comment 5 Said Abou-Hallawa 2019-08-12 13:10:41 PDT
Created attachment 376089 [details]
Patch
Comment 6 Simon Fraser (smfr) 2019-08-12 14:00:30 PDT
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.
Comment 7 Said Abou-Hallawa 2019-08-13 09:44:56 PDT
Created attachment 376177 [details]
Patch
Comment 8 Said Abou-Hallawa 2019-08-13 10:04:36 PDT
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.
Comment 9 Said Abou-Hallawa 2019-08-13 10:11:16 PDT
Created attachment 376178 [details]
Patch
Comment 10 Said Abou-Hallawa 2019-08-13 10:28:33 PDT
Created attachment 376181 [details]
Patch
Comment 11 WebKit Commit Bot 2019-08-13 14:11:55 PDT
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
Comment 12 Said Abou-Hallawa 2019-08-13 16:43:47 PDT
Created attachment 376224 [details]
Patch
Comment 13 WebKit Commit Bot 2019-08-13 18:59:01 PDT
Comment on attachment 376224 [details]
Patch

Clearing flags on attachment: 376224

Committed r248657: <https://trac.webkit.org/changeset/248657>
Comment 14 WebKit Commit Bot 2019-08-13 18:59:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2019-08-13 19:00:18 PDT
<rdar://problem/54284527>