Bug 218960 – Standardize enums that are used by Settings in preparation for autogeneration

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Standardize enums that are used by Settings in preperation for autogeneration

[Sam Weinig]

Created attachment 414170 [details]
Patch

[Sam Weinig]

Created attachment 414172 [details]
Patch

[Sam Weinig]

Created attachment 414174 [details]
Patch

[Sam Weinig]

Created attachment 414176 [details]
Patch

[Sam Weinig]

Created attachment 414178 [details]
Patch

[Sam Weinig]

Created attachment 414181 [details]
Patch

[Sam Weinig]

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?

[Darin Adler]

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.

[Sam Weinig]

(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.

[Sam Weinig]

Created attachment 414183 [details]
Can I remove the file completely?

[Sam Weinig]

(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.

[Sam Weinig]

Using https://bugs.webkit.org/show_bug.cgi?id=218966 to fixup the test case.

[Sam Weinig]

Created attachment 414243 [details]
Patch

[Sam Weinig]

(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.

[EWS]

Committed r269888: <https://trac.webkit.org/changeset/269888>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414243 [details].

[Radar WebKit Bug Importer]

<rdar://problem/71466896>

[Darin Adler]

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?

[Sam Weinig]

(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.