Summary: | [IPC] Add WTF::EnumTraits<> for every enum type used in IPC | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||
Component: | WebKit2 | Assignee: | 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
David Kilzer (:ddkilzer)
2020-06-11 13:38:43 PDT
Created attachment 401682 [details]
Patch v1
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 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 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". Created attachment 401700 [details]
Patch for landing
Created attachment 401704 [details]
Patch for landing v2
(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.) Committed r262933: <https://trac.webkit.org/changeset/262933> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401704 [details]. (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. (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 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 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. |