[webkit-reviews] review denied: [Bug 95299] [GTK][WPE] Missing Exif Orientation support : [Attachment 395680] WIP patch updating Cairo::drawSurface

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has denied  review:
Bug 95299: [GTK][WPE] Missing Exif Orientation support
https://bugs.webkit.org/show_bug.cgi?id=95299

Attachment 395680: WIP patch updating Cairo::drawSurface

https://bugs.webkit.org/attachment.cgi?id=395680&action=review




--- Comment #7 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 395680
  --> https://bugs.webkit.org/attachment.cgi?id=395680
WIP patch updating Cairo::drawSurface

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

> Source/WebCore/platform/graphics/cairo/CairoOperations.cpp:890
> +void drawSurface(PlatformContextCairo& platformContext, cairo_surface_t*
surface, const FloatRect& destRect, const FloatRect& originalSrcRect,
InterpolationQuality imageInterpolationQuality, float globalAlpha, const
ShadowState& shadowState, bool didUseWidthAsHeight)

In general we tend to frown upon boolean flags; instead, it's better to use
an “enum”, which is more descriptive:

  // Somewhere in the header.
  enum class Sizing { Normal, WidthAsHeight };

  void drawSurface(/* ... */, Sizing sizing = Sizing::Normal);

(Also you may want to have a default value for the parameter, so you don't need
to add the usual one by hand on some callsites. Also feel free to suggest a
better
name for the “enum”, I just wrote down the first thing that came to mind.)

> Source/WebCore/platform/graphics/cairo/CairoOperations.cpp:952
> +	   std::fabs(srcRect.height() / destRect.width());

The result of the calculations here are not being stored anywhere,
which means the computed values are not used at all. Same below in
the “else” branch.

> Source/WebKit/Shared/cairo/ShareableBitmapCairo.cpp:79
> +    Cairo::drawSurface(*context.platformContext(), surface.get(), destRect,
srcRect, state.imageInterpolationQuality, state.alpha,
Cairo::ShadowState(state), false);

If you add a default value to the function prototype as suggested, you
don't need to modify this callsite.

> Source/WebKit/WebProcess/WebCoreSupport/gtk/WebDragClientGtk.cpp:58
> +    Cairo::drawSurface(*graphicsContext->platformContext(), surface,
IntRect(IntPoint(), imageSize), IntRect(IntPoint(), imageSize),
state.imageInterpolationQuality, state.alpha, Cairo::ShadowState(state),
false);

Ditto.