Standardize enums that are used by Settings in preperation for autogeneration
Created attachment 414170 [details] Patch
Created attachment 414172 [details] Patch
Created attachment 414174 [details] Patch
Created attachment 414176 [details] Patch
Created attachment 414178 [details] Patch
Created attachment 414181 [details] Patch
It looks like I can't change the file LayoutTests/fast/forms/selection-direction.html, as when I do, the bots all fail to apply the patch with the error: (Stripping trailing CRs from patch.) patching file LayoutTests/fast/forms/selection-direction.html Hunk #1 FAILED at 91. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/fast/forms/selection-direction.html.rej Darin, I remember you working on stripping CRs recently, any idea what to do about this?
I did not successfully strip CRs yet; I want to try but not sure how I will succeed. I came up with a script, then made a first patch and it does not apply. I suggest that in *this* patch you skip the old test (with a comment explaining why and the intention of eventually deleting it), and adding a new version of the test that does not have CRs. Too bad the test already has a good name, since this is also an opportunity to improve the tests name.
(In reply to Darin Adler from comment #8) > I did not successfully strip CRs yet; I want to try but not sure how I will > succeed. I came up with a script, then made a first patch and it does not > apply. > > I suggest that in *this* patch you skip the old test (with a comment > explaining why and the intention of eventually deleting it), and adding a > new version of the test that does not have CRs. Too bad the test already has > a good name, since this is also an opportunity to improve the tests name. Ok. Will do. Thanks. Going to do a little bit of experimenting as well.
Created attachment 414183 [details] Can I remove the file completely?
(In reply to Sam Weinig from comment #10) > Created attachment 414183 [details] > Can I remove the file completely? Looks like I can remove it entirely. So I can do two changes, one to remove it, one to add it back without the CRs.
Using https://bugs.webkit.org/show_bug.cgi?id=218966 to fixup the test case.
Created attachment 414243 [details] Patch
(In reply to Sam Weinig from comment #13) > Created attachment 414243 [details] > Patch Ok, looks like we have that all sorted and this can be reviewed now.
Committed r269888: <https://trac.webkit.org/changeset/269888> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414243 [details].
<rdar://problem/71466896>
Comment on attachment 414243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414243&action=review > Source/WebCore/accessibility/ForcedAccessibilityValue.h:34 > +enum class ForcedAccessibilityValue : uint8_t { > + System, > + On, > + Off > +}; When they are super short like this I like to do these on a single line. Not sure if you agree. I also typically use the optional trailing comma on the last line when we are doing it vertically. > Source/WebCore/editing/EditingBehaviorType.h:42 > + Ios Would be nice if we could fix the spelling; not critical, though. > Source/WebCore/editing/cocoa/DataDetectorTypes.h:34 > +enum class DataDetectorTypes : uint8_t { Seems like this should be a singular noun phrase, not a plural, if it’s a value to be used in an OptionSet. > Source/WebCore/editing/cocoa/DataDetectorTypes.h:35 > + None = 0, Should not have a "None" when this is an enum for use in an OptionSet. > Source/WebCore/html/HTMLAnchorElement.cpp:223 > + // Don't set the link to be active if the current selection is in the same editable block as > + // this link Should add the period since we are reformatting here. Maybe merge it into a single line. > Source/WebCore/html/parser/HTMLParserScriptingFlagPolicy.h:30 > +enum class HTMLParserScriptingFlagPolicy : uint8_t { Maybe bool instead of uint8_t? > Source/WebCore/html/parser/HTMLParserScriptingFlagPolicy.h:31 > + OnlyIfScriptIsEnabled, This seems like an unwieldy name. > Source/WebCore/loader/FrameLoader.cpp:2583 > + auto types = OptionSet<DataDetectorTypes> { m_frame.settings().dataDetectorTypes() }; This looks like an anti-pattern. If the setting is a set of types, then it should be an OptionSet. > Source/WebCore/page/DebugPageOverlays.cpp:417 > + auto activeOverlayRegions = OptionSet<DebugOverlayRegions>::fromRaw(page.settings().visibleDebugOverlayRegions()); Same frustrating anti-pattern. > Source/WebCore/page/FrameView.h:-83 > -enum class FrameFlattening; Why can’t we use a forward declaration any more? > Source/WebCore/page/PDFImageCachingPolicy.h:39 > +#if PLATFORM(IOS_FAMILY) > + Default = BelowMemoryLimit > +#else > + Default = Enabled > +#endif Could this be a separate constant rather than an enum class member?
(In reply to Darin Adler from comment #17) > Comment on attachment 414243 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=414243&action=review > > > Source/WebCore/accessibility/ForcedAccessibilityValue.h:34 > > +enum class ForcedAccessibilityValue : uint8_t { > > + System, > > + On, > > + Off > > +}; > > When they are super short like this I like to do these on a single line. Not > sure if you agree. > > I also typically use the optional trailing comma on the last line when we > are doing it vertically. I agree with both of these. Not sure why I was inconsistent here. Will update to make these inline. > > > Source/WebCore/editing/EditingBehaviorType.h:42 > > + Ios > > Would be nice if we could fix the spelling; not critical, though. Yeah, this was to make the bindings generator happy. This is fixable though. > > > Source/WebCore/editing/cocoa/DataDetectorTypes.h:34 > > +enum class DataDetectorTypes : uint8_t { > > Seems like this should be a singular noun phrase, not a plural, if it’s a > value to be used in an OptionSet. Ok. > > > Source/WebCore/editing/cocoa/DataDetectorTypes.h:35 > > + None = 0, > > Should not have a "None" when this is an enum for use in an OptionSet. Does having "None" hurt things? It was useful for the default value in the .yaml files. Once this is always a OptionSet it hopefully won't be necessary, so its not a big deal to remove in a bit. > > > Source/WebCore/html/HTMLAnchorElement.cpp:223 > > + // Don't set the link to be active if the current selection is in the same editable block as > > + // this link > > Should add the period since we are reformatting here. Maybe merge it into a > single line. Will fix. > > > Source/WebCore/html/parser/HTMLParserScriptingFlagPolicy.h:30 > > +enum class HTMLParserScriptingFlagPolicy : uint8_t { > > Maybe bool instead of uint8_t? Heh, I did this and undid it. Not clear to me if there is a benefit to using bool vs. uint8_t here. What do you think it adds? > > > Source/WebCore/html/parser/HTMLParserScriptingFlagPolicy.h:31 > > + OnlyIfScriptIsEnabled, > > This seems like an unwieldy name. Agreed, though not sure what to replace it with. > > > Source/WebCore/loader/FrameLoader.cpp:2583 > > + auto types = OptionSet<DataDetectorTypes> { m_frame.settings().dataDetectorTypes() }; > > This looks like an anti-pattern. If the setting is a set of types, then it > should be an OptionSet. This will be fixed in a follow on changes. Wanted to keep this relatively small so didn't want to add the infrastructure in the generator to return OptionSets yet. (same for all below). > > > Source/WebCore/page/FrameView.h:-83 > > -enum class FrameFlattening; > > Why can’t we use a forward declaration any more? We can. Will fix. > > > Source/WebCore/page/PDFImageCachingPolicy.h:39 > > +#if PLATFORM(IOS_FAMILY) > > + Default = BelowMemoryLimit > > +#else > > + Default = Enabled > > +#endif > > Could this be a separate constant rather than an enum class member? Yeah, good idea.