Bug 174604

Created attachment 315752 [details] Patch

Comment on attachment 315752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315752&action=review > Source/WebCore/editing/AlternativeTextController.h:115 > + Variant<NoDetails, AutocorrectionReplacement, AlternativeDictationContext> details; What would you think about std::optional<Variant<AutocorrectionReplacement, AlternativeDictationContext>> ?

Comment on attachment 315752 [details] 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_alternativeTextInfo.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_alternativeTextInfo.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.

(In reply to Chris Dumez from comment #2) > Comment on attachment 315752 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315752&action=review > > > Source/WebCore/editing/AlternativeTextController.h:115 > > + Variant<NoDetails, AutocorrectionReplacement, AlternativeDictationContext> details; > > What would you think about std::optional<Variant<AutocorrectionReplacement, > AlternativeDictationContext>> ? It is sufficient to use Variant<AutocorrectionReplacement, AlternativeDictationContext> and let AutocorrectionReplacement be default value initialized.

(In reply to Darin Adler from comment #3) > > Source/WebCore/editing/AlternativeTextController.cpp:291 > > + if (!m_alternativeTextInfo.rangeWithAlternative || !WTF::holds_alternative<AlternativeTextInfo::AutocorrectionReplacement>(m_alternativeTextInfo.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. Will fix. > 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. > Making use of the null string is sufficient. > > Source/WebCore/editing/AlternativeTextController.cpp:328 > > + if (!m_alternativeTextInfo.rangeWithAlternative || !WTF::holds_alternative<AlternativeTextInfo::AlternativeDictationContext>(m_alternativeTextInfo.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. Will fix. > 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. > Making use of the null string is sufficient. > > 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. > Will change to separate members and remove struct AlternativeTextInfo. > > 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. > Will do. > >> Source/WebCore/editing/AlternativeTextController.h:115 > >> + Variant<NoDetails, AutocorrectionReplacement, AlternativeDictationContext> details; > [...] > 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. Will do.

Committed r219613: <http://trac.webkit.org/changeset/219613>

Commit additional changes I inadvertently didn't save in <http://trac.webkit.org/changeset/219614>.