188713 2018-08-17 17:31:16 -0700 Pack booleans in Event into a bitfield 2019-02-06 09:19:08 -0800 1 1 1 Unclassified WebKit DOM WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 rniwa rniwa achristensen cdumez dbates esprehn+autocc ews-watchlist kangil.han simon.fraser webkit-bug-importer ysuzuki oldest_to_newest 1451300 0 rniwa 2018-08-17 17:31:16 -0700 Make Event object slightly smaller. 1451304 1 347419 rniwa 2018-08-17 17:41:30 -0700 Created attachment 347419 Cleanup 1451306 2 ews-watchlist 2018-08-17 17:44:47 -0700 Attachment 347419 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. 1451314 3 347419 dbates 2018-08-17 18:20:50 -0700 Comment on attachment 347419 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. 1451328 4 347419 simon.fraser 2018-08-17 19:15:08 -0700 Comment on attachment 347419 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. 1451354 5 rniwa 2018-08-17 23:44:19 -0700 (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. 1451355 6 rniwa 2018-08-17 23:47:24 -0700 Committed r235009: <https://trac.webkit.org/changeset/235009> 1451356 7 webkit-bug-importer 2018-08-17 23:48:23 -0700 <rdar://problem/43454338> 1451442 8 ysuzuki 2018-08-19 01:06:27 -0700 Nice! 1503322 9 lforschler 2019-02-06 09:19:08 -0800 Mass move bugs into the DOM component. 347419 2018-08-17 17:41:30 -0700 2018-08-17 18:20:50 -0700 Cleanup bug-188713-20180817174130.patch text/plain 5548 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIzNTAwMykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDE4LTA4LTE3ICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIFBhY2sgYm9vbGVhbnMgaW4gRXZl bnQgaW50byBhIGJpdGZpZWxkCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xODg3MTMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBVc2UgYml0ZmllbGRzIGZvciBib29sZWFucyBpbiBFdmVudCBjbGFzcy4KKwor ICAgICAgICAqIGRvbS9FdmVudC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpFdmVudDo6RXZlbnQp OgorICAgICAgICAqIGRvbS9FdmVudC5oOgorICAgICAgICAoV2ViQ29yZTo6RXZlbnQ6OklzQ29t cG9zZWQpOiBBZGRlZC4KKyAgICAgICAgKFdlYkNvcmU6OkV2ZW50KTogUGFja2VkIGJvb2xlYW5z IGludG8gYSBiaXRmaWVsZC4KKwogMjAxOC0wOC0xNyAgQWxleCBDaHJpc3RlbnNlbiAgPGFjaHJp c3RlbnNlbkB3ZWJraXQub3JnPgogCiAgICAgICAgIENsZWFuIHVwIENTU1NlbGVjdG9yTGlzdCBh ZnRlciByMjM0ODI1CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9kb20vRXZlbnQuY3BwCj09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2RvbS9FdmVudC5jcHAJKHJldmlzaW9uIDIzNDk5OSkKKysr IFNvdXJjZS9XZWJDb3JlL2RvbS9FdmVudC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTM0LDQxICsz NCw0OCBAQAogCiBuYW1lc3BhY2UgV2ViQ29yZSB7CiAKK0FMV0FZU19JTkxJTkUgRXZlbnQ6OkV2 ZW50KE1vbm90b25pY1RpbWUgY3JlYXRlVGltZSwgY29uc3QgQXRvbWljU3RyaW5nJiB0eXBlLCBJ c1RydXN0ZWQgaXNUcnVzdGVkLCBDYW5CdWJibGUgY2FuQnViYmxlLCBJc0NhbmNlbGFibGUgY2Fu Y2VsYWJsZSwgSXNDb21wb3NlZCBjb21wb3NlZCkKKyAgICA6IG1fdHlwZSB7IHR5cGUgfQorICAg ICwgbV9pc0luaXRpYWxpemVkIHsgIXR5cGUuaXNOdWxsKCkgfQorICAgICwgbV9jYW5CdWJibGUg eyBjYW5CdWJibGUgPT0gQ2FuQnViYmxlOjpZZXMgfQorICAgICwgbV9jYW5jZWxhYmxlIHsgY2Fu Y2VsYWJsZSA9PSBJc0NhbmNlbGFibGU6OlllcyB9CisgICAgLCBtX2NvbXBvc2VkIHsgY29tcG9z ZWQgPT0gSXNDb21wb3NlZDo6WWVzIH0KKyAgICAsIG1fcHJvcGFnYXRpb25TdG9wcGVkIHsgZmFs c2UgfQorICAgICwgbV9pbW1lZGlhdGVQcm9wYWdhdGlvblN0b3BwZWQgeyBmYWxzZSB9CisgICAg LCBtX3dhc0NhbmNlbGVkIHsgZmFsc2UgfQorICAgICwgbV9kZWZhdWx0SGFuZGxlZCB7IGZhbHNl IH0KKyAgICAsIG1faXNEZWZhdWx0RXZlbnRIYW5kbGVySWdub3JlZCB7IGZhbHNlIH0KKyAgICAs IG1faXNUcnVzdGVkIHsgaXNUcnVzdGVkID09IElzVHJ1c3RlZDo6WWVzIH0KKyAgICAsIG1faXNF eGVjdXRpbmdQYXNzaXZlRXZlbnRMaXN0ZW5lciB7IGZhbHNlIH0KKyAgICAsIG1fZXZlbnRQaGFz ZSB7IE5PTkUgfQorICAgICwgbV9jcmVhdGVUaW1lIHsgY3JlYXRlVGltZSB9Cit7Cit9CisKIEV2 ZW50OjpFdmVudChJc1RydXN0ZWQgaXNUcnVzdGVkKQotICAgIDogbV9pc1RydXN0ZWQoaXNUcnVz dGVkID09IElzVHJ1c3RlZDo6WWVzKQotICAgICwgbV9jcmVhdGVUaW1lKE1vbm90b25pY1RpbWU6 Om5vdygpKQorICAgIDogRXZlbnQgeyBNb25vdG9uaWNUaW1lOjpub3coKSwgeyB9LCBpc1RydXN0 ZWQsIENhbkJ1YmJsZTo6Tm8sIElzQ2FuY2VsYWJsZTo6Tm8sIElzQ29tcG9zZWQ6Ok5vIH0KIHsK IH0KIAogRXZlbnQ6OkV2ZW50KGNvbnN0IEF0b21pY1N0cmluZyYgZXZlbnRUeXBlLCBDYW5CdWJi bGUgY2FuQnViYmxlLCBJc0NhbmNlbGFibGUgaXNDYW5jZWxhYmxlKQotICAgIDogbV90eXBlKGV2 ZW50VHlwZSkKLSAgICAsIG1faXNJbml0aWFsaXplZCh0cnVlKQotICAgICwgbV9jYW5CdWJibGUo Y2FuQnViYmxlID09IENhbkJ1YmJsZTo6WWVzKQotICAgICwgbV9jYW5jZWxhYmxlKGlzQ2FuY2Vs YWJsZSA9PSBJc0NhbmNlbGFibGU6OlllcykKLSAgICAsIG1faXNUcnVzdGVkKHRydWUpCi0gICAg LCBtX2NyZWF0ZVRpbWUoTW9ub3RvbmljVGltZTo6bm93KCkpCisgICAgOiBFdmVudCB7IE1vbm90 b25pY1RpbWU6Om5vdygpLCBldmVudFR5cGUsIElzVHJ1c3RlZDo6WWVzLCBjYW5CdWJibGUsIGlz Q2FuY2VsYWJsZSwgSXNDb21wb3NlZDo6Tm8gfQogeworICAgIEFTU0VSVCghZXZlbnRUeXBlLmlz TnVsbCgpKTsKIH0KIAogRXZlbnQ6OkV2ZW50KGNvbnN0IEF0b21pY1N0cmluZyYgZXZlbnRUeXBl LCBDYW5CdWJibGUgY2FuQnViYmxlLCBJc0NhbmNlbGFibGUgaXNDYW5jZWxhYmxlLCBNb25vdG9u aWNUaW1lIHRpbWVzdGFtcCkKLSAgICA6IG1fdHlwZShldmVudFR5cGUpCi0gICAgLCBtX2lzSW5p dGlhbGl6ZWQodHJ1ZSkKLSAgICAsIG1fY2FuQnViYmxlKGNhbkJ1YmJsZSA9PSBDYW5CdWJibGU6 OlllcykKLSAgICAsIG1fY2FuY2VsYWJsZShpc0NhbmNlbGFibGUgPT0gSXNDYW5jZWxhYmxlOjpZ ZXMpCi0gICAgLCBtX2lzVHJ1c3RlZCh0cnVlKQotICAgICwgbV9jcmVhdGVUaW1lKHRpbWVzdGFt cCkKKyAgICA6IEV2ZW50IHsgdGltZXN0YW1wLCBldmVudFR5cGUsIElzVHJ1c3RlZDo6WWVzLCBj YW5CdWJibGUsIGlzQ2FuY2VsYWJsZSwgSXNDb21wb3NlZDo6Tm8gfQogeworICAgIEFTU0VSVCgh ZXZlbnRUeXBlLmlzTnVsbCgpKTsKIH0KIAogRXZlbnQ6OkV2ZW50KGNvbnN0IEF0b21pY1N0cmlu ZyYgZXZlbnRUeXBlLCBjb25zdCBFdmVudEluaXQmIGluaXRpYWxpemVyLCBJc1RydXN0ZWQgaXNU cnVzdGVkKQotICAgIDogbV90eXBlKGV2ZW50VHlwZSkKLSAgICAsIG1faXNJbml0aWFsaXplZCh0 cnVlKQotICAgICwgbV9jYW5CdWJibGUoaW5pdGlhbGl6ZXIuYnViYmxlcykKLSAgICAsIG1fY2Fu Y2VsYWJsZShpbml0aWFsaXplci5jYW5jZWxhYmxlKQotICAgICwgbV9jb21wb3NlZChpbml0aWFs aXplci5jb21wb3NlZCkKLSAgICAsIG1faXNUcnVzdGVkKGlzVHJ1c3RlZCA9PSBJc1RydXN0ZWQ6 OlllcykKLSAgICAsIG1fY3JlYXRlVGltZShNb25vdG9uaWNUaW1lOjpub3coKSkKKyAgICA6IEV2 ZW50IHsgTW9ub3RvbmljVGltZTo6bm93KCksIGV2ZW50VHlwZSwgaXNUcnVzdGVkLAorICAgICAg ICBpbml0aWFsaXplci5idWJibGVzID8gQ2FuQnViYmxlOjpZZXMgOiBDYW5CdWJibGU6Ok5vLAor ICAgICAgICBpbml0aWFsaXplci5jYW5jZWxhYmxlID8gSXNDYW5jZWxhYmxlOjpZZXMgOiBJc0Nh bmNlbGFibGU6Ok5vLAorICAgICAgICBpbml0aWFsaXplci5jb21wb3NlZCA/IElzQ29tcG9zZWQ6 OlllcyA6IElzQ29tcG9zZWQ6Ok5vIH0KIHsKKyAgICBBU1NFUlQoIWV2ZW50VHlwZS5pc051bGwo KSk7CiB9CiAKIEV2ZW50Ojp+RXZlbnQoKSA9IGRlZmF1bHQ7CkluZGV4OiBTb3VyY2UvV2ViQ29y ZS9kb20vRXZlbnQuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9kb20vRXZlbnQuaAko cmV2aXNpb24gMjM0OTk5KQorKysgU291cmNlL1dlYkNvcmUvZG9tL0V2ZW50LmgJKHdvcmtpbmcg Y29weSkKQEAgLTQzLDggKzQzLDkgQEAgcHVibGljOgogICAgIGVudW0gY2xhc3MgSXNUcnVzdGVk IDogdWludDhfdCB7IE5vLCBZZXMgfTsKICAgICBlbnVtIGNsYXNzIENhbkJ1YmJsZSA6IHVpbnQ4 X3QgeyBObywgWWVzIH07CiAgICAgZW51bSBjbGFzcyBJc0NhbmNlbGFibGUgOiB1aW50OF90IHsg Tm8sIFllcyB9OworICAgIGVudW0gY2xhc3MgSXNDb21wb3NlZCA6IHVpbnQ4X3QgeyBObywgWWVz IH07CiAKLSAgICBlbnVtIFBoYXNlVHlwZSB7IAorICAgIGVudW0gUGhhc2VUeXBlIHsKICAgICAg ICAgTk9ORSA9IDAsCiAgICAgICAgIENBUFRVUklOR19QSEFTRSA9IDEsCiAgICAgICAgIEFUX1RB UkdFVCA9IDIsCkBAIC0xNDgsMjQgKzE0OSwyNyBAQCBwcm90ZWN0ZWQ6CiAgICAgdmlydHVhbCB2 b2lkIHJlY2VpdmVkVGFyZ2V0KCkgeyB9CiAKIHByaXZhdGU6Ci0gICAgQXRvbWljU3RyaW5nIG1f dHlwZTsKKyAgICBleHBsaWNpdCBFdmVudChNb25vdG9uaWNUaW1lIGNyZWF0ZVRpbWUsIGNvbnN0 IEF0b21pY1N0cmluZyYgdHlwZSwgSXNUcnVzdGVkLCBDYW5CdWJibGUsIElzQ2FuY2VsYWJsZSwg SXNDb21wb3NlZCk7CiAKICAgICB2b2lkIHNldENhbmNlbGVkRmxhZ0lmUG9zc2libGUoKTsKIAot ICAgIGJvb2wgbV9pc0luaXRpYWxpemVkIHsgZmFsc2UgfTsKLSAgICBib29sIG1fY2FuQnViYmxl IHsgZmFsc2UgfTsKLSAgICBib29sIG1fY2FuY2VsYWJsZSB7IGZhbHNlIH07Ci0gICAgYm9vbCBt X2NvbXBvc2VkIHsgZmFsc2UgfTsKLQotICAgIGJvb2wgbV9wcm9wYWdhdGlvblN0b3BwZWQgeyBm YWxzZSB9OwotICAgIGJvb2wgbV9pbW1lZGlhdGVQcm9wYWdhdGlvblN0b3BwZWQgeyBmYWxzZSB9 OwotICAgIGJvb2wgbV93YXNDYW5jZWxlZCB7IGZhbHNlIH07Ci0gICAgYm9vbCBtX2RlZmF1bHRI YW5kbGVkIHsgZmFsc2UgfTsKLSAgICBib29sIG1faXNEZWZhdWx0RXZlbnRIYW5kbGVySWdub3Jl ZCB7IGZhbHNlIH07Ci0gICAgYm9vbCBtX2lzVHJ1c3RlZCB7IGZhbHNlIH07Ci0gICAgYm9vbCBt X2lzRXhlY3V0aW5nUGFzc2l2ZUV2ZW50TGlzdGVuZXIgeyBmYWxzZSB9OworICAgIEF0b21pY1N0 cmluZyBtX3R5cGU7CisKKyAgICB1bnNpZ25lZCBtX2lzSW5pdGlhbGl6ZWQgOiAxOworICAgIHVu c2lnbmVkIG1fY2FuQnViYmxlIDogMTsKKyAgICB1bnNpZ25lZCBtX2NhbmNlbGFibGUgOiAxOwor ICAgIHVuc2lnbmVkIG1fY29tcG9zZWQgOiAxOworCisgICAgdW5zaWduZWQgbV9wcm9wYWdhdGlv blN0b3BwZWQgOiAxOworICAgIHVuc2lnbmVkIG1faW1tZWRpYXRlUHJvcGFnYXRpb25TdG9wcGVk IDogMTsKKyAgICB1bnNpZ25lZCBtX3dhc0NhbmNlbGVkIDogMTsKKyAgICB1bnNpZ25lZCBtX2Rl ZmF1bHRIYW5kbGVkIDogMTsKKyAgICB1bnNpZ25lZCBtX2lzRGVmYXVsdEV2ZW50SGFuZGxlckln bm9yZWQgOiAxOworICAgIHVuc2lnbmVkIG1faXNUcnVzdGVkIDogMTsKKyAgICB1bnNpZ25lZCBt X2lzRXhlY3V0aW5nUGFzc2l2ZUV2ZW50TGlzdGVuZXIgOiAxOworCisgICAgdW5zaWduZWQgbV9l dmVudFBoYXNlIDogMjsgLy8gMTMtYml0cwogCi0gICAgUGhhc2VUeXBlIG1fZXZlbnRQaGFzZSB7 IE5PTkUgfTsKICAgICBSZWZQdHI8RXZlbnRUYXJnZXQ+IG1fY3VycmVudFRhcmdldDsKICAgICBj b25zdCBFdmVudFBhdGgqIG1fZXZlbnRQYXRoIHsgbnVsbHB0ciB9OwogICAgIFJlZlB0cjxFdmVu dFRhcmdldD4gbV90YXJnZXQ7Cg==