177883 2017-10-04 10:34:15 -0700 TextDecorationPainter::m_wavyOffset should be a float 2017-10-04 11:39:34 -0700 1 1 1 Unclassified WebKit Text WebKit Local Build All All RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=152587 InRadar P2 Normal --- 1 dbates dbates buildbot mmaxfield simon.fraser webkit-bug-importer zalan oldest_to_newest 1356495 0 dbates 2017-10-04 10:34:15 -0700 Currently TextDecorationPainter::m_wavyOffset should be an int. It should be a float. This is consistent with its original type when this code was extracted from InlineTextBox in the patch for bug #152587. 1356501 1 322687 dbates 2017-10-04 10:41:33 -0700 Created attachment 322687 Patch 1356502 2 buildbot 2017-10-04 10:44:00 -0700 Attachment 322687 did not pass style-queue: ERROR: Source/WebCore/rendering/TextDecorationPainter.cpp:246: 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. 1356524 3 322687 simon.fraser 2017-10-04 11:19:36 -0700 Comment on attachment 322687 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322687&action=review > Source/WebCore/rendering/TextDecorationPainter.cpp:250 > + : m_context { context } > + , m_decoration { decoration } > + , m_wavyOffset { wavyOffsetFromDecoration() } > + , m_isPrinting { renderer.document().printing() } > + , m_styles { stylesForRenderer(renderer, m_decoration, isFirstLine) } > + , m_lineStyle { isFirstLine ? renderer.firstLineStyle() : renderer.style() } Are we going to do this everywhere? It seems like a lot of churn for zero gain. > Source/WebCore/rendering/TextDecorationPainter.h:67 > TextDecoration m_decoration; > - int m_wavyOffset { 0 }; > + float m_wavyOffset; Looks like these could be re-ordered for better packing (but maybe we never heap-allocate these). 1356533 4 dbates 2017-10-04 11:32:30 -0700 (In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 322687 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322687&action=review > > > Source/WebCore/rendering/TextDecorationPainter.cpp:250 > > + : m_context { context } > > + , m_decoration { decoration } > > + , m_wavyOffset { wavyOffsetFromDecoration() } > > + , m_isPrinting { renderer.document().printing() } > > + , m_styles { stylesForRenderer(renderer, m_decoration, isFirstLine) } > > + , m_lineStyle { isFirstLine ? renderer.firstLineStyle() : renderer.style() } > > Are we going to do this everywhere? I hope so. See bug #176058. > It seems like a lot of churn for zero gain. > I found this bug by changing to uniform initializer syntax because implicit conversions are disallowed when using this syntax among other benefits. Obviously, catching the implicit conversion from float to int for m_wavyOffset is not a big gain (m_wavyOffset is always equal to 1). I suspect we may find more significant bugs caused by implicit conversion if we used uniform initializer syntax everywhere. > > Source/WebCore/rendering/TextDecorationPainter.h:67 > > TextDecoration m_decoration; > > - int m_wavyOffset { 0 }; > > + float m_wavyOffset; > > Looks like these could be re-ordered for better packing (but maybe we never > heap-allocate these). Currently TextDecorationPainter is never heap allocated, but nothing is stopping it from being heap allocated. Regardless, we should look to better pack this data structure from a good programming practice perspective. 1356542 5 322687 dbates 2017-10-04 11:37:51 -0700 Comment on attachment 322687 Patch Clearing flags on attachment: 322687 Committed r222862: <http://trac.webkit.org/changeset/222862> 1356543 6 dbates 2017-10-04 11:37:53 -0700 All reviewed patches have been landed. Closing bug. 1356544 7 webkit-bug-importer 2017-10-04 11:39:34 -0700 <rdar://problem/34816545> 322687 2017-10-04 10:41:33 -0700 2017-10-04 11:37:51 -0700 Patch bug-177883-20171004104132.patch text/plain 4592 dbates U3VidmVyc2lvbiBSZXZpc2lvbjogMjIyODU0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMWFmYmJmMjFlNWIwYzcw NjZiMzJmMjlmOGY4ZDNlYzExOTc4MTA4Ni4uZDkzNmQwZWZkOGM0MmE2NTNmNTU4OTU1MjQ1YjU1 YTc4YThmZTYwNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDE3LTEwLTA0ICBEYW5p ZWwgQmF0ZXMgIDxkYWJhdGVzQGFwcGxlLmNvbT4KKworICAgICAgICBUZXh0RGVjb3JhdGlvblBh aW50ZXI6Om1fd2F2eU9mZnNldCBzaG91bGQgYmUgYSBmbG9hdAorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTc3ODgzCisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSW4gcjE5NDQ0NyB3ZSBleHRyYWN0ZWQgdGhl IHRleHQgZGVjb3JhdGlvbiBwYWludGluZyBjb2RlIGZyb20gSW5saW5lVGV4dEJveCBpbnRvCisg ICAgICAgIFRleHREZWNvcmF0aW9uUGFpbnRlciBhbmQgY2hhbmdlZCB0aGUgZGF0YSB0eXBlIG9m IHRoZSB3YXZ5IG9mZnNldCBmcm9tIGZsb2F0IHRvIGludC4KKyAgICAgICAgV2UgdXNlIGZsb2F0 aW5nIHBvaW50IG51bWJlcnMgdGhyb3VnaG91dCB0aGUgcGFpbnRpbmcgY29kZSBhbmQgc2hvdWxk IHN0b3JlIHRoZSB3YXZ5CisgICAgICAgIG9mZnNldCBhcyBhIGZsb2F0LgorCisgICAgICAgICog cmVuZGVyaW5nL1RleHREZWNvcmF0aW9uUGFpbnRlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpU ZXh0RGVjb3JhdGlvblBhaW50ZXI6OlRleHREZWNvcmF0aW9uUGFpbnRlcik6IFVzZSBDKysgdW5p Zm9ybSBpbml0aWFsaXplcgorICAgICAgICBzeW50YXggdG8gaW5pdGlhbGl6ZSBtZW1iZXIgZmll bGRzLgorICAgICAgICAoV2ViQ29yZTo6VGV4dERlY29yYXRpb25QYWludGVyOjpwYWludFRleHRE ZWNvcmF0aW9uKTogQ2hhbmdlIGludCB0byBmbG9hdC4KKyAgICAgICAgKiByZW5kZXJpbmcvVGV4 dERlY29yYXRpb25QYWludGVyLmg6IFJlbW92ZSB1bm5lY2Vzc2FyeSBlcXVhbCBpbml0aWFsaXpl ciBmb3IgbV93YXZ5T2Zmc2V0CisgICAgICAgIGFzIHRoaXMgY2xhc3MgaGFzIGV4YWN0bHkgb25l IGNvbnN0cnVjdG9yIGFuZCBpdCBhbHdheXMgaW5pdGlhbGl6ZXMgaXQuCisKIDIwMTctMTAtMDQg IE1pZ3VlbCBHb21leiAgPG1hZ29tZXpAaWdhbGlhLmNvbT4KIAogICAgICAgICBVbnJldmlld2Vk OiBmaXggR1RLIGRlYnVnIGJ1aWxkIGFmdGVyIHIyMjI4NDEuCmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViQ29yZS9yZW5kZXJpbmcvVGV4dERlY29yYXRpb25QYWludGVyLmNwcCBiL1NvdXJjZS9XZWJD b3JlL3JlbmRlcmluZy9UZXh0RGVjb3JhdGlvblBhaW50ZXIuY3BwCmluZGV4IGNjOGFjZDA3NDJh YjU2OGY1Nzk4ZTNlNWUwMDZmNzk5MDAwODQ3ZGYuLjM2NzU2MzEyMzRkM2YzYzQ5ZjZlYzA4N2Fl ZDI5MWRkM2ZkNmUxYWEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9UZXh0 RGVjb3JhdGlvblBhaW50ZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9UZXh0 RGVjb3JhdGlvblBhaW50ZXIuY3BwCkBAIC0yNDIsMTIgKzI0MiwxMiBAQCBzdGF0aWMgU3Ryb2tl U3R5bGUgdGV4dERlY29yYXRpb25TdHlsZVRvU3Ryb2tlU3R5bGUoVGV4dERlY29yYXRpb25TdHls ZSBkZWNvcmF0aQogfQogCiBUZXh0RGVjb3JhdGlvblBhaW50ZXI6OlRleHREZWNvcmF0aW9uUGFp bnRlcihHcmFwaGljc0NvbnRleHQmIGNvbnRleHQsIFRleHREZWNvcmF0aW9uIGRlY29yYXRpb24s IGNvbnN0IFJlbmRlclRleHQmIHJlbmRlcmVyLCBib29sIGlzRmlyc3RMaW5lKQotICAgIDogbV9j b250ZXh0KGNvbnRleHQpCi0gICAgLCBtX2RlY29yYXRpb24oZGVjb3JhdGlvbikKLSAgICAsIG1f d2F2eU9mZnNldCh3YXZ5T2Zmc2V0RnJvbURlY29yYXRpb24oKSkKLSAgICAsIG1faXNQcmludGlu ZyhyZW5kZXJlci5kb2N1bWVudCgpLnByaW50aW5nKCkpCi0gICAgLCBtX3N0eWxlcyhzdHlsZXNG b3JSZW5kZXJlcihyZW5kZXJlciwgbV9kZWNvcmF0aW9uLCBpc0ZpcnN0TGluZSkpCi0gICAgLCBt X2xpbmVTdHlsZShpc0ZpcnN0TGluZSA/IHJlbmRlcmVyLmZpcnN0TGluZVN0eWxlKCkgOiByZW5k ZXJlci5zdHlsZSgpKQorICAgIDogbV9jb250ZXh0IHsgY29udGV4dCB9CisgICAgLCBtX2RlY29y YXRpb24geyBkZWNvcmF0aW9uIH0KKyAgICAsIG1fd2F2eU9mZnNldCB7IHdhdnlPZmZzZXRGcm9t RGVjb3JhdGlvbigpIH0KKyAgICAsIG1faXNQcmludGluZyB7IHJlbmRlcmVyLmRvY3VtZW50KCku cHJpbnRpbmcoKSB9CisgICAgLCBtX3N0eWxlcyB7IHN0eWxlc0ZvclJlbmRlcmVyKHJlbmRlcmVy LCBtX2RlY29yYXRpb24sIGlzRmlyc3RMaW5lKSB9CisgICAgLCBtX2xpbmVTdHlsZSB7IGlzRmly c3RMaW5lID8gcmVuZGVyZXIuZmlyc3RMaW5lU3R5bGUoKSA6IHJlbmRlcmVyLnN0eWxlKCkgfQog ewogfQogCkBAIC0zMjcsMTMgKzMyNywxMyBAQCB2b2lkIFRleHREZWNvcmF0aW9uUGFpbnRlcjo6 cGFpbnRUZXh0RGVjb3JhdGlvbihjb25zdCBUZXh0UnVuJiB0ZXh0UnVuLCBjb25zdCBGbAogICAg ICAgICAvLyBUaGVzZSBkZWNvcmF0aW9ucyBzaG91bGQgbWF0Y2ggdGhlIHZpc3VhbCBvdmVyZmxv d3MgY29tcHV0ZWQgaW4gdmlzdWFsT3ZlcmZsb3dGb3JEZWNvcmF0aW9ucygpCiAgICAgICAgIGlm IChtX2RlY29yYXRpb24gJiBUZXh0RGVjb3JhdGlvblVuZGVybGluZSkgewogICAgICAgICAgICAg Y29uc3QgaW50IG9mZnNldCA9IGNvbXB1dGVVbmRlcmxpbmVPZmZzZXQobV9saW5lU3R5bGUudGV4 dFVuZGVybGluZVBvc2l0aW9uKCksIG1fbGluZVN0eWxlLmZvbnRNZXRyaWNzKCksIG1faW5saW5l VGV4dEJveCwgdGV4dERlY29yYXRpb25UaGlja25lc3MpOwotICAgICAgICAgICAgaW50IHdhdnlP ZmZzZXQgPSBtX3N0eWxlcy51bmRlcmxpbmVTdHlsZSA9PSBUZXh0RGVjb3JhdGlvblN0eWxlV2F2 eSA/IG1fd2F2eU9mZnNldCA6IDA7CisgICAgICAgICAgICBmbG9hdCB3YXZ5T2Zmc2V0ID0gbV9z dHlsZXMudW5kZXJsaW5lU3R5bGUgPT0gVGV4dERlY29yYXRpb25TdHlsZVdhdnkgPyBtX3dhdnlP ZmZzZXQgOiAwOwogICAgICAgICAgICAgRmxvYXRQb2ludCBzdGFydCA9IGxvY2FsT3JpZ2luICsg RmxvYXRTaXplKDAsIG9mZnNldCArIHdhdnlPZmZzZXQpOwogICAgICAgICAgICAgRmxvYXRQb2lu dCBlbmQgPSBsb2NhbE9yaWdpbiArIEZsb2F0U2l6ZShtX3dpZHRoLCBvZmZzZXQgKyB3YXZ5T2Zm c2V0KTsKICAgICAgICAgICAgIHBhaW50RGVjb3JhdGlvbihUZXh0RGVjb3JhdGlvblVuZGVybGlu ZSwgbV9zdHlsZXMudW5kZXJsaW5lU3R5bGUsIG1fc3R5bGVzLnVuZGVybGluZUNvbG9yLCBzdGFy dCwgZW5kLCBvZmZzZXQpOwogICAgICAgICB9CiAgICAgICAgIGlmIChtX2RlY29yYXRpb24gJiBU ZXh0RGVjb3JhdGlvbk92ZXJsaW5lKSB7Ci0gICAgICAgICAgICBpbnQgd2F2eU9mZnNldCA9IG1f c3R5bGVzLm92ZXJsaW5lU3R5bGUgPT0gVGV4dERlY29yYXRpb25TdHlsZVdhdnkgPyBtX3dhdnlP ZmZzZXQgOiAwOworICAgICAgICAgICAgZmxvYXQgd2F2eU9mZnNldCA9IG1fc3R5bGVzLm92ZXJs aW5lU3R5bGUgPT0gVGV4dERlY29yYXRpb25TdHlsZVdhdnkgPyBtX3dhdnlPZmZzZXQgOiAwOwog ICAgICAgICAgICAgRmxvYXRQb2ludCBzdGFydCA9IGxvY2FsT3JpZ2luIC0gRmxvYXRTaXplKDAs IHdhdnlPZmZzZXQpOwogICAgICAgICAgICAgRmxvYXRQb2ludCBlbmQgPSBsb2NhbE9yaWdpbiAr IEZsb2F0U2l6ZShtX3dpZHRoLCAtd2F2eU9mZnNldCk7CiAgICAgICAgICAgICBwYWludERlY29y YXRpb24oVGV4dERlY29yYXRpb25PdmVybGluZSwgbV9zdHlsZXMub3ZlcmxpbmVTdHlsZSwgbV9z dHlsZXMub3ZlcmxpbmVDb2xvciwgc3RhcnQsIGVuZCwgMCk7CmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViQ29yZS9yZW5kZXJpbmcvVGV4dERlY29yYXRpb25QYWludGVyLmggYi9Tb3VyY2UvV2ViQ29y ZS9yZW5kZXJpbmcvVGV4dERlY29yYXRpb25QYWludGVyLmgKaW5kZXggNmUyY2E4ZDU0ODBjZWI3 ZTNjY2MwYjFmN2I2Y2YxMzdjYzU3NTg3Zi4uNDQ4M2FlMzIxYWU0MzNkY2ZhZDkxOTZlZjFlZGVj ZTkxYTJjYWE0OCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1RleHREZWNv cmF0aW9uUGFpbnRlci5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9UZXh0RGVjb3Jh dGlvblBhaW50ZXIuaApAQCAtNjQsNyArNjQsNyBAQCBwdWJsaWM6CiBwcml2YXRlOgogICAgIEdy YXBoaWNzQ29udGV4dCYgbV9jb250ZXh0OwogICAgIFRleHREZWNvcmF0aW9uIG1fZGVjb3JhdGlv bjsKLSAgICBpbnQgbV93YXZ5T2Zmc2V0IHsgMCB9OworICAgIGZsb2F0IG1fd2F2eU9mZnNldDsK ICAgICBib29sIG1faXNQcmludGluZyB7IGZhbHNlIH07CiAgICAgZmxvYXQgbV93aWR0aCB7IDAg fTsKICAgICBmbG9hdCBtX2Jhc2VsaW5lIHsgMCB9Owo=