Bug 174594 – Cleanup: Use OptionSet to represent marker types

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

Class DocumentMarker::MarkerTypes represents bitmask of marker types and duplicates most of the functionality of OptionSet. Unlike OptionSet, DocumentMarker::MarkerTypes does not support iteration and hence code, such as AlternativeTextController::applyAlternativeTextToRange(), uses Vector<DocumentMarker::MarkerType> to represent a set of marker types. We can use OptionSet to support both uses cases in a space- and time- efficient manner.

[Daniel Bates]

Created attachment 315713 [details]
Patch

[Darin Adler]

Comment on attachment 315713 [details]
Patch

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

> Source/WebCore/dom/DocumentMarker.h:164
> +        AcceptedCandidate
> +        , Autocorrected
> +        , CorrectionIndicator
> +        , DeletedAutocorrection
> +        , DictationAlternatives
> +        , Grammar
> +        , RejectedCorrection
> +        , Replacement
> +        , SpellCheckingExemption
> +        , Spelling
> +        , TextMatch
> +#if ENABLE(TELEPHONE_NUMBER_DETECTION)
> +        , TelephoneNumber
> +#endif
> +#if PLATFORM(IOS)
> +        , DictationPhraseWithAlternatives
> +        , DictationResult
> +#endif
> +        , DraggedContent

I would like to see this sorted alphabetically (including "DraggedContent", but not including the conditional items. And we should use trailing commas instead of leading commas, with the extra one at the end that C++ allows in initializer lists. That way we don’t have to have one line different from the others.

> Source/WebCore/editing/Editor.cpp:2752
> +        DocumentMarker::Spelling, DocumentMarker::CorrectionIndicator, DocumentMarker::SpellCheckingExemption,
> +        DocumentMarker::DictationAlternatives
> +#if !PLATFORM(IOS)
> +        , DocumentMarker::Grammar
>  #endif

Trailing commas after each item is a cleaner way to do this since you can have an extra comma.

[Darin Adler]

You will have to rebase and merge with r219595 since it overlaps some functions you changed.

[Daniel Bates]

(In reply to Darin Adler from comment #2)
> > Source/WebCore/dom/DocumentMarker.h:164
> > +        AcceptedCandidate
> > +        , Autocorrected
> > +        , CorrectionIndicator
> > +        , DeletedAutocorrection
> > +        , DictationAlternatives
> > +        , Grammar
> > +        , RejectedCorrection
> > +        , Replacement
> > +        , SpellCheckingExemption
> > +        , Spelling
> > +        , TextMatch
> > +#if ENABLE(TELEPHONE_NUMBER_DETECTION)
> > +        , TelephoneNumber
> > +#endif
> > +#if PLATFORM(IOS)
> > +        , DictationPhraseWithAlternatives
> > +        , DictationResult
> > +#endif
> > +        , DraggedContent
> 
> I would like to see this sorted alphabetically (including "DraggedContent",
> but not including the conditional items. And we should use trailing commas
> instead of leading commas, with the extra one at the end that C++ allows in
> initializer lists. That way we don’t have to have one line different from
> the others.
> 

Will fix.

> > Source/WebCore/editing/Editor.cpp:2752
> > +        DocumentMarker::Spelling, DocumentMarker::CorrectionIndicator, DocumentMarker::SpellCheckingExemption,
> > +        DocumentMarker::DictationAlternatives
> > +#if !PLATFORM(IOS)
> > +        , DocumentMarker::Grammar
> >  #endif
> 
> Trailing commas after each item is a cleaner way to do this since you can
> have an extra comma.


Will fix.

[Daniel Bates]

Committed r219597: <http://trac.webkit.org/changeset/219597>

[Konstantin Tokarev]

This change seems to break WinCairo bot

WebFrame.cpp.obj : error LNK2019: unresolved external symbol "public: void __cdecl WebCore::DocumentMarkerController::removeMarkers(class WebCore::DocumentMarker::MarkerTypes)" (?removeMarkers@DocumentMarkerController@WebCore@@QEAAXVMarkerTypes@DocumentMarker@2@@Z) referenced in function "public: void __cdecl WebFrame::unmarkAllMisspellings(void)" (?unmarkAllMisspellings@WebFrame@@QEAAXXZ)

https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/4428/steps/compile-webkit/logs/stdio

[Darin Adler]

That looks like a build dependency error that we would get if we tried to link WebKitLegacy against WebCore and had not recompiled WebKitLegacy, rather than a mistake in this patch.

Could force the WinCairo bot to do a clean build, or try to figure out how to fix the build system used by WinCairo to correctly understand this dependency.