Bug 226395

Summary: Clients of optional should use has_value instead of relying on hasValue macro
Product: WebKit Reporter: Darin Adler <darin>
Component: WebKit Misc.Assignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, benjamin, berto, calvaris, cdumez, cgarcia, changseok, cmarcelo, dino, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, fred.wang, glenn, gustavo, gyuyoung.kim, hi, jamesr, japhet, jer.noble, jfernandez, jiewen_tan, joepeck, jsbell, kangil.han, keith_miller, kondapallykalyan, luiz, macpherson, mark.lam, menard, mifenton, msaboff, pdr, philipj, pnormand, rego, saam, sabouhallawa, sam, schenney, sergio, svillar, tonikitoo, tzagallo, vjaquez, webkit-bug-importer
Priority: P3 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch cdumez: review+

Description Darin Adler 2021-05-28 14:34:30 PDT
Clients of optional should use has_value instead of relying on hasValue macro
Comment 1 Darin Adler 2021-05-28 14:51:14 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2021-05-28 14:52:09 PDT
Couldn’t do this with a global replace because hasValue is a common name used in classes other than WTF::Optional.
Comment 3 Darin Adler 2021-05-28 14:52:30 PDT
First version will probably fail EWS if I missed things on non-Mac platforms.
Comment 4 Darin Adler 2021-05-28 17:28:46 PDT
Created attachment 430077 [details]
Patch
Comment 5 EWS Watchlist 2021-05-28 17:30:05 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 6 Chris Dumez 2021-05-28 19:01:40 PDT
Comment on attachment 430077 [details]
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::FloatSize());

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().
Comment 7 Darin Adler 2021-05-29 15:09:07 PDT
Committed r278244 (238281@main): <https://commits.webkit.org/238281@main>
Comment 8 Radar WebKit Bug Importer 2021-05-29 15:10:18 PDT
<rdar://problem/78653580>