[webkit-reviews] review granted: [Bug 174604] Cleanup: Use Variant to represent alternative text info details : [Attachment 315752] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 174604: Cleanup: Use Variant to represent alternative text info details
https://bugs.webkit.org/show_bug.cgi?id=174604

Attachment 315752: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 315752
  --> https://bugs.webkit.org/attachment.cgi?id=315752
Patch

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

I think Variant is a little wordy here, and all we are doing is saving a tiny
bit of storage by sharing storage for the replacement string and the dictation
context, at the cost of considerably more obfuscated code. I think maybe we
should just use two separate data members for these and not bother with
Variant.

> Source/WebCore/editing/AlternativeTextController.cpp:291
> +	   if (!m_alternativeTextInfo.rangeWithAlternative ||
!WTF::holds_alternative<AlternativeTextInfo::AutocorrectionReplacement>(m_alter
nativeTextInfo.details))
> +	       break;

The old code used an unconditional static_cast, and the corresponding idiom is
to just call WTF::get with no previous check. So there is no need to write out
the wordy holds_alternative call. Except that I don’t understand why there is a
chance that details could have been null. In the new code, I believe that would
be a null string and so require no special case. But we should audit the code
paths to make sure.

> Source/WebCore/editing/AlternativeTextController.cpp:328
> +	   if (!m_alternativeTextInfo.rangeWithAlternative ||
!WTF::holds_alternative<AlternativeTextInfo::AlternativeDictationContext>(m_alt
ernativeTextInfo.details))
> +	       return;

The old code used an unconditional static_cast, and the corresponding idiom is
to just call WTF::get with no previous check. So there is no need to write out
the wordy holds_alternative call. Except that I don’t understand why there is a
chance that details could have been null. In the new code, I believe that would
be a null string and so it would require the call to holds_alternative. But we
should audit the code paths to make sure this is needed.

> Source/WebCore/editing/AlternativeTextController.cpp:331
> +	   if (!dictationContext)
>	       return;

This is so strange. Why would we bother storing a dictation context of 0 in the
first place? I do see that the old code checks for this value, though, so safer
not to change it.

> Source/WebCore/editing/AlternativeTextController.h:106
> +    struct AlternativeTextInfo {

This use of a struct is quite peculiar. These should just all be separate data
members. There is nothing that binds them together as far as I can tell. The
code uses them each separately. Should be m_rangeWithAlternative, m_isActive,
m_type, m_originalText, m_details.

> Source/WebCore/editing/AlternativeTextController.h:109
> +	   using AutocorrectionReplacement = String;
> +	   using AlternativeDictationContext = uint64_t;
> +	   struct NoDetails { };

Since this is already inside the AlternativeTextController class, I would
suggest putting these outside the struct so we can use them without saying
AlternativeTextInfo all the time. Would make the code easier to read. And if
you take out the struct as I suggest, then you will have to do that anyway.

>> Source/WebCore/editing/AlternativeTextController.h:115
>> +	    Variant<NoDetails, AutocorrectionReplacement,
AlternativeDictationContext> details;
> 
> What would you think about std::optional<Variant<AutocorrectionReplacement,
AlternativeDictationContext>> ?

This is a significant improvement.

Even more elegant would be to combine the type with the data in a single
variant by using a separate struct for each type, but that is likely result in
uglier code since there are so many types that have no data.

Either way, we should consider removing NoDetails. Without it, I believe this
would default to a null string since AutoCorrectionReplacement is the first
type in the variant. To me that seems completely safe. If we really felt we
needed this to be optional, std::optional might be better. Not really sure.