[webkit-reviews] review granted: [Bug 179981] Fix dictionary leak in lookup, convert FindOptions to OptionSet, tweak code style nearby : [Attachment 327555] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Darin Adler <darin at apple.com>'s request
for review:
Bug 179981: Fix dictionary leak in lookup, convert FindOptions to OptionSet,
tweak code style nearby
https://bugs.webkit.org/show_bug.cgi?id=179981

Attachment 327555: Patch

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




--- Comment #9 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 327555
  --> https://bugs.webkit.org/attachment.cgi?id=327555
Patch

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

> Source/WTF/wtf/OptionSet.h:151
> +    constexpr friend OptionSet operator|(OptionSet lhs, OptionSet rhs)
> +    {
> +	   return fromRaw(lhs.m_storage | rhs.m_storage);
> +    }
> +
> +    constexpr friend OptionSet operator|(OptionSet lhs, T rhs)
> +    {
> +	   return lhs | OptionSet { rhs };
> +    }
> +
>      constexpr friend OptionSet operator-(OptionSet lhs, OptionSet rhs)
>      {
> -	   return OptionSet::fromRaw(lhs.m_storage & ~rhs.m_storage);
> +	   return fromRaw(lhs.m_storage & ~rhs.m_storage);
> +    }
> +
> +    constexpr friend OptionSet operator-(OptionSet lhs, T rhs)
> +    {
> +	   return lhs - OptionSet { rhs };
>      }

It would be good to add test cases for these to the existing OptionSet tests in
TestWebKitAPI.

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:111
> +    std::tuple<String, RetainPtr<PDFSelection>, RetainPtr<NSDictionary>>
lookupTextAtLocation(const WebCore::FloatPoint&, WebHitTestResultData&) const;

I am surprised you don't have to #include <tuple>