Bug 226395 - Clients of optional should use has_value instead of relying on hasValue macro
Summary: Clients of optional should use has_value instead of relying on hasValue macro
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-28 14:34 PDT by Darin Adler
Modified: 2021-05-29 15:10 PDT (History)
49 users (show)

See Also:


Attachments
Patch (128.26 KB, patch)
2021-05-28 14:51 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (193.06 KB, patch)
2021-05-28 17:28 PDT, Darin Adler
cdumez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>