Make Event object slightly smaller.
Created attachment 347419 [details] Cleanup
Attachment 347419 [details] did not pass style-queue: ERROR: Source/WebCore/dom/Event.cpp:39: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 347419 [details] Cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=347419&action=review > Source/WebCore/dom/Event.cpp:37 > +ALWAYS_INLINE Event::Event(MonotonicTime createTime, const AtomicString& type, IsTrusted isTrusted, CanBubble canBubble, IsCancelable cancelable, IsComposed composed) This feels like a step backwards from using default initializers. I am assuming it is a memory footprint win. Once we move to C++20 we can use default initalizers for bitfields. > Source/WebCore/dom/Event.h:171 > + unsigned m_eventPhase : 2; // 13-bits I wish we had a good solution for this. Very easy for a person to add a new every phase and forget to update the width (if needed). This problem is not unique to this code. One tactic I have seen is to define a constexpr for the width above the enum in the hope that a person sees it. Another tactic is to add placeholder enum after last enumerator then compute width.
Comment on attachment 347419 [details] Cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=347419&action=review >> Source/WebCore/dom/Event.h:171 >> + unsigned m_eventPhase : 2; // 13-bits > > I wish we had a good solution for this. Very easy for a person to add a new every phase and forget to update the width (if needed). This problem is not unique to this code. One tactic I have seen is to define a constexpr for the width above the enum in the hope that a person sees it. Another tactic is to add placeholder enum after last enumerator then compute width. It took me a few seconds to realize that the "13 bits" didn't mean that m_eventPhase needs 13 bits. I think this comment is confusing, and dump-class-layout can show bit counts so there's no need for the comment now.
(In reply to Daniel Bates from comment #3) > Comment on attachment 347419 [details] > Cleanup > > View in context: > https://bugs.webkit.org/attachment.cgi?id=347419&action=review > > > Source/WebCore/dom/Event.cpp:37 > > +ALWAYS_INLINE Event::Event(MonotonicTime createTime, const AtomicString& type, IsTrusted isTrusted, CanBubble canBubble, IsCancelable cancelable, IsComposed composed) > > This feels like a step backwards from using default initializers. I am > assuming it is a memory footprint win. Once we move to C++20 we can use > default initalizers for bitfields. Yeah, it would be nice to be able to use default initializers with bitfields. (In reply to Simon Fraser (smfr) from comment #4) > Comment on attachment 347419 [details] > Cleanup > > View in context: > https://bugs.webkit.org/attachment.cgi?id=347419&action=review > > >> Source/WebCore/dom/Event.h:171 > >> + unsigned m_eventPhase : 2; // 13-bits > > > > I wish we had a good solution for this. Very easy for a person to add a new every phase and forget to update the width (if needed). This problem is not unique to this code. One tactic I have seen is to define a constexpr for the width above the enum in the hope that a person sees it. Another tactic is to add placeholder enum after last enumerator then compute width. > > It took me a few seconds to realize that the "13 bits" didn't mean that > m_eventPhase needs 13 bits. I think this comment is confusing, and > dump-class-layout can show bit counts so there's no need for the comment now. Okay, will remove the comment.
Committed r235009: <https://trac.webkit.org/changeset/235009>
<rdar://problem/43454338>
Nice!
Mass move bugs into the DOM component.