Bug 213093

Summary: [IPC] Add WTF::EnumTraits<> for every enum type used in IPC
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebKit2Assignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alecflett, annulen, beidson, berto, cdumez, cgarcia, changseok, cmarcelo, darin, dino, eric.carlson, esprehn+autocc, ews-watchlist, fred.wang, glenn, gustavo, gyuyoung.kim, hta, jamesr, japhet, jer.noble, jsbell, kangil.han, kondapallykalyan, luiz, mifenton, pdr, philipj, ryuan.choi, sergio, simon.fraser, tommyw, tonikitoo, useafterfree, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=212758
https://bugs.webkit.org/show_bug.cgi?id=212846
https://bugs.webkit.org/show_bug.cgi?id=212870
https://bugs.webkit.org/show_bug.cgi?id=212885
https://bugs.webkit.org/show_bug.cgi?id=212921
https://bugs.webkit.org/show_bug.cgi?id=213138
Bug Depends on: 215341    
Bug Blocks: 211988    
Attachments:
Description Flags
Patch v1
darin: review+
Patch for landing
none
Patch for landing v2 none

Description David Kilzer (:ddkilzer) 2020-06-11 13:38:43 PDT
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.
Comment 1 David Kilzer (:ddkilzer) 2020-06-11 14:24:57 PDT
Created attachment 401682 [details]
Patch v1
Comment 2 Radar WebKit Bug Importer 2020-06-11 14:25:24 PDT
<rdar://problem/64269767>
Comment 3 David Kilzer (:ddkilzer) 2020-06-11 14:26:45 PDT
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.
Comment 4 Darin Adler 2020-06-11 15:42:13 PDT
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.
Comment 5 David Kilzer (:ddkilzer) 2020-06-11 18:03:17 PDT
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".
Comment 6 David Kilzer (:ddkilzer) 2020-06-11 19:36:41 PDT
Created attachment 401700 [details]
Patch for landing
Comment 7 David Kilzer (:ddkilzer) 2020-06-11 20:31:31 PDT
Created attachment 401704 [details]
Patch for landing v2
Comment 8 David Kilzer (:ddkilzer) 2020-06-11 20:33:25 PDT
(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.)
Comment 9 EWS 2020-06-11 23:02:47 PDT
Committed r262933: <https://trac.webkit.org/changeset/262933>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401704 [details].
Comment 10 Darin Adler 2020-06-12 09:37:35 PDT
(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.
Comment 11 David Kilzer (:ddkilzer) 2020-06-12 10:36:47 PDT
(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.
Comment 12 David Kilzer (:ddkilzer) 2020-06-12 17:14:59 PDT
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>.
Comment 13 Chris Dumez 2020-08-10 14:06:52 PDT
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.