Bug 213093 – [IPC] Add WTF::EnumTraits<> for every enum type used in IPC

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[David Kilzer (:ddkilzer)]

Add WTF::EnumTraits<> for every enum type used in IPC.

This is to add WTF::EnumTraits<> for all the enum types that don't have it but need it for IPC.

[David Kilzer (:ddkilzer)]

Created attachment 401682 [details]
Patch v1

[Radar WebKit Bug Importer]

<rdar://problem/64269767>

[David Kilzer (:ddkilzer)]

Alex Christensen suggested I define all of these before actually using them (Bug 211988) so that if enabling the enum checking caused a regression, then it would easy to roll out the patch for Bug 211988 instead of rolling out this much larger patch.

[Darin Adler]

Comment on attachment 401682 [details]
Patch v1

Super irritating how every enum has to be listed out twice. We have got to find a better idiom for this. Seems like the failure mode when you forget to include one of the values is also non-obvious, especially if it’s a value that’s only occasionally sent cross-process.

[David Kilzer (:ddkilzer)]

Comment on attachment 401682 [details]
Patch v1

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

> Source/WebCore/platform/ScrollTypes.cpp:63
> +TextStream& operator<<(TextStream& ts, ScrollBehaviorForFixedElements behavior)
> +{
> +    switch (behavior) {
> +    case ScrollBehaviorForFixedElements::StickToDocumentBounds:
> +        ts << "stickToDocumentBounds";
> +        break;
> +    case ScrollBehaviorForFixedElements::StickToViewportBounds:
> +        ts << "stickToViewportBounds";
> +        break;
> +    }
> +    return ts;
> +}

This caused layout tests to fail since the previous operator<<() method output "0" and "1" instead of "stickToDocumentBounds" and "stickToViewportBounds".

Will fix in a "Patch for landing".

[David Kilzer (:ddkilzer)]

Created attachment 401700 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

Created attachment 401704 [details]
Patch for landing v2

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #7)
> Created attachment 401704 [details]
> Patch for landing v2

Restored the order of two `bool` enum classes in Source/WebCore/loader/ResourceLoaderOptions.h to try to fix Windows layout tests.

If Windows layout tests are still broken, then it's probably this change in Source/WebCore/loader/FrameLoaderTypes.h:

 enum class AllowsContentJavaScript : bool {
-    Yes,
     No,
+    Yes,
 };

(Which would be so wrong.)

[EWS]

Committed r262933: <https://trac.webkit.org/changeset/262933>

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

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #8)
> Restored the order of two `bool` enum classes in
> Source/WebCore/loader/ResourceLoaderOptions.h to try to fix Windows layout
> tests.
> 
> If Windows layout tests are still broken, then it's probably this change in
> Source/WebCore/loader/FrameLoaderTypes.h:
> 
>  enum class AllowsContentJavaScript : bool {
> -    Yes,
>      No,
> +    Yes,
>  };
> 
> (Which would be so wrong.)

I’m thinking that a nice, but unimportant, cleanup step would be to return to this and figure out how Windows depends on it and fix it.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #10)
> (In reply to David Kilzer (:ddkilzer) from comment #8)
> > Restored the order of two `bool` enum classes in
> > Source/WebCore/loader/ResourceLoaderOptions.h to try to fix Windows layout
> > tests.
> 
> I’m thinking that a nice, but unimportant, cleanup step would be to return
> to this and figure out how Windows depends on it and fix it.

Yes, I wanted to try to find out where this was happening.  The two enums I had to revert to fix Windows layout tests were in Source/WebCore/loader/ResourceLoaderOptions.h.

Filed Bug 213138: Windows layout tests fail when changing ContentSniffingPolicy/ContentEncodingSniffingPolicy enum values.

[David Kilzer (:ddkilzer)]

Comment on attachment 401682 [details]
Patch v1

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

> Source/WebCore/platform/ContextMenuItem.h:226
> +#if PLATFORM(GTK)
> +        WebCore::ContextMenuAction::ContextMenuItemTagDelete,

This is missing WebCore::ContextMenuAction::ContextMenuItemTagPasteAsPlainText that was added in r261800.

This will be fixed in Bug 211988 which starts using WTF::EnumTraits<WebCore::ContextMenuAction>.

[Chris Dumez]

Comment on attachment 401682 [details]
Patch v1

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

> Source/WebCore/platform/DragData.h:149
> +template<> struct EnumTraits<WebCore::DragApplicationFlags> {

I believe this introduced a top crasher (rdar://problem/59344091). This is not a regular enum but rather a set of flags. If more than one flag is set, I believe we will now fail to decode it after IPC.