[webkit-reviews] review granted: [Bug 226395] Clients of optional should use has_value instead of relying on hasValue macro : [Attachment 430077] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 226395: Clients of optional should use has_value instead of relying on
hasValue macro
https://bugs.webkit.org/show_bug.cgi?id=226395

Attachment 430077: Patch

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




--- Comment #6 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 430077
  --> https://bugs.webkit.org/attachment.cgi?id=430077
Patch

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

r=me

> Source/WebCore/css/FontFace.cpp:273
> +    if (!families.has_value())

Don't really need .has_value() here.

> Source/WebCore/css/FontFace.cpp:298
> +    if (!styleWrapped.has_value())

ditto.

> Source/WebCore/css/FontFace.cpp:331
> +    if (!weightWrapped.has_value())

ditto

> Source/WebCore/css/FontFace.cpp:354
> +    if (!stretchWrapped.has_value())

ditto.

> Source/WebCore/css/FontFace.cpp:377
> +    if (!rangesWrapped.has_value())

ditto.

> Source/WebCore/css/FontFace.cpp:392
> +    if (!featureSettingsWrapped.has_value())

ditto.

> Source/WebCore/css/FontFace.cpp:407
> +    if (!loadingBehaviorWrapped.has_value())

ditto.

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:116
> +    if (originalOffsetForDirectionalSnapping.has_value()) {

ditto.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:400
> +	   m_presentationSize =
getVideoResolutionFromCaps(m_demuxerSrcPadCaps.get()).value_or(WebCore::FloatSi
ze());

probably don't need the WebCore:: in WebCore.

> Source/WebCore/svg/SVGSVGElement.cpp:317
>	   transform.setA(matrixInit.a.value());

*matrixInit.a ?

> Source/WebCore/svg/SVGSVGElement.cpp:319
>	   transform.setB(matrixInit.b.value());

ditto.

> Source/WebCore/svg/SVGSVGElement.cpp:321
>	   transform.setC(matrixInit.c.value());

ditto.

> Source/WebCore/svg/SVGSVGElement.cpp:323
>	   transform.setD(matrixInit.d.value());

ditto.

> Source/WebCore/svg/SVGSVGElement.cpp:325
>	   transform.setE(matrixInit.e.value());

ditto.

> Source/WebCore/svg/SVGSVGElement.cpp:327
>	   transform.setF(matrixInit.f.value());

ditto.

> Source/WebCore/svg/SVGTransform.h:81
>	       transform.setA(matrixInit.a.value());

ditto in this function.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:474
> +    if (!orientation.has_value())

I don't think the .has_value() helps readability here.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:479
> +    if (!widthVariant.has_value())

ditto.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:484
> +    if (!textRenderingMode.has_value())

ditto.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:489
> +    if (!size.has_value())

ditto.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:494
> +    if (!syntheticBold.has_value())

ditto.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:499
> +    if (!syntheticOblique.has_value())

ditto.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:509
> +    if (!includesCreationData.has_value())

ditto.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:515
> +	   if (!fontFaceData.has_value())

ditto.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:520
> +	   if (!itemInCollection.has_value())

ditto.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:532
> +	   auto creationData = WebCore::FontPlatformData::CreationData {
fontFaceData.value(), itemInCollection.value() };

I would prefer *fontFaceData instead of fontFaceData.value(). It is more
concise.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:533
> +	   return WebCore::FontPlatformData(ctFont.get(), size.value(),
syntheticBold.value(), syntheticOblique.value(), orientation.value(),
widthVariant.value(), textRenderingMode.value(), &creationData);

ditto.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:538
> +    if (!referenceURL.has_value())

.has_value() is not really useful here.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:543
> +    if (!postScriptName.has_value())

ditto.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:552
> +    return WebCore::FontPlatformData(ctFont.get(), size.value(),
syntheticBold.value(), syntheticOblique.value(), orientation.value(),
widthVariant.value(), textRenderingMode.value());

Could use * instead of .value().