103450 2012-11-27 12:53:40 -0800 [CSS Exclusions] Add helper functions for converting floats to LayoutUnits 2012-12-17 09:12:01 -0800 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 giles_joplin giles_joplin bjonesbe eric ojan webkit.review.bot oldest_to_newest 777142 0 giles_joplin 2012-11-27 12:53:40 -0800 When a float logicalTop value is converted to a LayoutUnit its necessary to use LayoutUnit::fromFloatCeil(). This ensures that we're snapping to a value that's inside the ExclusionShape. Similarly, to convert a logicalBottom value from float to LayoutUnit we use LayoutUnit::fromFloatFloor(). See https://bugs.webkit.org/show_bug.cgi?id=100996. These conversionsares likely to be needed in several places, for example see https://bugs.webkit.org/show_bug.cgi?id=103327, so moving them to an aptly named pair of helper functions would be useful. 785138 1 178057 giles_joplin 2012-12-06 12:14:22 -0800 Created attachment 178057 Patch 785240 2 bjonesbe 2012-12-06 14:03:55 -0800 Since these are private methods, I don't see how this accomplishes the goal of code reuse. As it is, it seems like the layer of indirection only serves to make things harder to read. Am I missing something? 785296 3 giles_joplin 2012-12-06 14:45:21 -0800 (In reply to comment #2) > Since these are private methods, I don't see how this accomplishes the goal of code reuse. As it is, it seems like the layer of indirection only serves to make things harder to read. Am I missing something? At the moment they're only used by ExclusionShapeInsideInfo methods so private scope is adequate. If we find that they're more broadly needed, it's easy to expand their scope. 785304 4 bjonesbe 2012-12-06 14:48:48 -0800 Then why add them now as opposed to when they are more widely needed?(In reply to comment #3) > (In reply to comment #2) > > Since these are private methods, I don't see how this accomplishes the goal of code reuse. As it is, it seems like the layer of indirection only serves to make things harder to read. Am I missing something? > > At the moment they're only used by ExclusionShapeInsideInfo methods so private scope is adequate. If we find that they're more broadly needed, it's easy to expand their scope. Then why add them now as opposed to when they are more widely needed? 785306 5 giles_joplin 2012-12-06 14:51:03 -0800 (In reply to comment #4) > Then why add them now as opposed to when they are more widely needed?(In reply to comment #3) > > (In reply to comment #2) > > > Since these are private methods, I don't see how this accomplishes the goal of code reuse. As it is, it seems like the layer of indirection only serves to make things harder to read. Am I missing something? > > > > At the moment they're only used by ExclusionShapeInsideInfo methods so private scope is adequate. If we find that they're more broadly needed, it's easy to expand their scope. > > Then why add them now as opposed to when they are more widely needed? Because they clarify and isolate the use of LayoutUnit::fromFloatCeil() and LayoutUnit::fromFloatFloor(). 785337 6 178057 krit 2012-12-06 15:33:44 -0800 Comment on attachment 178057 Patch I am not convinced that this is faster or better. What is the context of this change? 785343 7 giles_joplin 2012-12-06 15:43:50 -0800 (In reply to comment #6) > (From update of attachment 178057 [details]) > I am not convinced that this is faster or better. What is the context of this change? Previously the implementation contained three occurrences of an idiom similar to this: // Use fromFloatCeil() to ensure that the returned LayoutUnit value is within the shape's bounds. LayoutUnit newLineTop = LayoutUnit::fromFloatCeil(floatNewLineTop); The comment was necessary because the code alone is somewhat obscure. Replacing this idiom with an inline with a suggestive name makes the code a little more readable and obviates replicating the comment at each site. 792759 8 178057 krit 2012-12-17 08:48:46 -0800 Comment on attachment 178057 Patch r=me 792773 9 178057 webkit.review.bot 2012-12-17 09:11:58 -0800 Comment on attachment 178057 Patch Clearing flags on attachment: 178057 Committed r137914: <http://trac.webkit.org/changeset/137914> 792774 10 webkit.review.bot 2012-12-17 09:12:01 -0800 All reviewed patches have been landed. Closing bug. 178057 2012-12-06 12:14:22 -0800 2012-12-17 09:11:57 -0800 Patch tmp.patch text/plain 4961 giles_joplin ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA0OTgzZjJhLi5kOGExYTcwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjkg QEAKKzIwMTItMTItMDYgIEhhbnMgTXVsbGVyICA8aG11bGxlckBhZG9iZS5jb20+CisKKyAgICAg ICAgW0NTUyBFeGNsdXNpb25zXSBBZGQgaGVscGVyIGZ1bmN0aW9ucyBmb3IgY29udmVydGluZyBm bG9hdHMgdG8gTGF5b3V0VW5pdHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTEwMzQ1MAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIFdoZW4gYSBmbG9hdCBsb2dpY2FsVG9wIHZhbHVlIGlzIGNvbnZlcnRlZCB0 byBhIExheW91dFVuaXQgaXQncyBuZWNlc3NhcnkgdG8KKyAgICAgICAgdXNlIExheW91dFVuaXQ6 OmZyb21GbG9hdENlaWwoKSB0byBlbnN1cmUgdGhhdCB3ZSdyZSBzbmFwcGluZyB0byBhIHZhbHVl IHRoYXQncworICAgICAgICBpbnNpZGUgdGhlIEV4Y2x1c2lvblNoYXBlLiAgU2ltaWxhcmx5LCB0 byBjb252ZXJ0IGEgbG9naWNhbEJvdHRvbSB2YWx1ZSBmcm9tCisgICAgICAgIGZsb2F0IHRvIExh eW91dFVuaXQgd2UgdXNlIExheW91dFVuaXQ6OmZyb21GbG9hdEZsb29yKCkuICBBZGRlZCBhIHBh aXIgb2YgcHJpdmF0ZQorICAgICAgICBFeGxjdXNpb25TaGFwZUluc2lkZUluZm8gbWV0aG9kcyB0 aGF0IGRvIHRoZSBjb252ZXJzaW9ucyBhbmQgcmVmYWN0b3JlZCBleGlzdGluZworICAgICAgICBj b2RlIHRvIHVzZSB0aGVtLgorCisgICAgICAgIFRoaXMgaXMganVzdCBhIGNsZWFudXAuICBObyBu ZXcgdGVzdHMgYXJlIG5lZWRlZCwgdGhlIGV4aXN0aW5nIHRlc3RzIGNvdmVyCisgICAgICAgIHRo ZXNlIGNoYW5nZXMuCisKKyAgICAgICAgKiByZW5kZXJpbmcvRXhjbHVzaW9uU2hhcGVJbnNpZGVJ bmZvLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkV4Y2x1c2lvblNoYXBlSW5zaWRlSW5mbzo6YWRq dXN0TG9naWNhbExpbmVUb3ApOiBVc2UgdGhlIG5ldyBmbG9hdExvZ2ljYWxUb3BUb0xheW91dFVu aXQoKSBtZXRob2QuCisgICAgICAgICogcmVuZGVyaW5nL0V4Y2x1c2lvblNoYXBlSW5zaWRlSW5m by5oOgorICAgICAgICAoV2ViQ29yZTo6RXhjbHVzaW9uU2hhcGVJbnNpZGVJbmZvOjpzaGFwZUxv Z2ljYWxUb3ApOiBVc2UgdGhlIG5ldyBmbG9hdExvZ2ljYWxUb3BUb0xheW91dFVuaXQoKSBtZXRo b2QuCisgICAgICAgIChXZWJDb3JlOjpFeGNsdXNpb25TaGFwZUluc2lkZUluZm86OnNoYXBlTG9n aWNhbEJvdHRvbSk6IFVzZSB0aGUgbmV3IGZsb2F0TG9naWNhbEJvdHRvbVRvTGF5b3V0VW5pdCgp IG1ldGhvZC4KKyAgICAgICAgKEV4Y2x1c2lvblNoYXBlSW5zaWRlSW5mbyk6CisgICAgICAgIChX ZWJDb3JlOjpFeGNsdXNpb25TaGFwZUluc2lkZUluZm86OmZsb2F0TG9naWNhbFRvcFRvTGF5b3V0 VW5pdCk6IENvbnZlcnQgYSBmbG9hdCB0byBhIExheW91dFVuaXQgd2l0aCBMYXlvdXRVbml0Ojpm cm9tRmxvYXRDZWlsKCkuCisgICAgICAgIChXZWJDb3JlOjpFeGNsdXNpb25TaGFwZUluc2lkZUlu Zm86OmZsb2F0TG9naWNhbEJvdHRvbVRvTGF5b3V0VW5pdCk6IENvbnZlcnQgYSBmbG9hdCB0byBh IExheW91dFVuaXQgd2l0aCBMYXlvdXRVbml0Ojpmcm9tRmxvYXRGbG9vcigpLgorCiAyMDEyLTEy LTA1ICBBZGFtIEtsZWluICA8YWRhbWtAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJlbW92ZSBn eXAgY29uZmlnIGZvciBpbmNvbXBsZXRlIGFuZCB1bnVzZWQgQXBwbGUgTWFjIGd5cCBidWlsZApk aWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL0V4Y2x1c2lvblNoYXBlSW5zaWRl SW5mby5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvRXhjbHVzaW9uU2hhcGVJbnNpZGVJ bmZvLmNwcAppbmRleCBiOGM0OWE4Li5lNzYwOTg5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9yZW5kZXJpbmcvRXhjbHVzaW9uU2hhcGVJbnNpZGVJbmZvLmNwcAorKysgYi9Tb3VyY2UvV2Vi Q29yZS9yZW5kZXJpbmcvRXhjbHVzaW9uU2hhcGVJbnNpZGVJbmZvLmNwcApAQCAtMTI1LDggKzEy NSw3IEBAIGJvb2wgRXhjbHVzaW9uU2hhcGVJbnNpZGVJbmZvOjphZGp1c3RMb2dpY2FsTGluZVRv cChmbG9hdCBtaW5TZWdtZW50V2lkdGgpCiAKICAgICBmbG9hdCBmbG9hdE5ld0xpbmVUb3A7CiAg ICAgaWYgKG1fc2hhcGUtPmZpcnN0SW5jbHVkZWRJbnRlcnZhbExvZ2ljYWxUb3AobV9saW5lVG9w LCBGbG9hdFNpemUobWluU2VnbWVudFdpZHRoLCBtX2xpbmVIZWlnaHQpLCBmbG9hdE5ld0xpbmVU b3ApKSB7Ci0gICAgICAgIC8vIFVzZSBmcm9tRmxvYXRDZWlsKCkgdG8gZW5zdXJlIHRoYXQgdGhl IHJldHVybmVkIExheW91dFVuaXQgdmFsdWUgaXMgd2l0aGluIHRoZSBzaGFwZSdzIGJvdW5kcy4K LSAgICAgICAgTGF5b3V0VW5pdCBuZXdMaW5lVG9wID0gTGF5b3V0VW5pdDo6ZnJvbUZsb2F0Q2Vp bChmbG9hdE5ld0xpbmVUb3ApOworICAgICAgICBMYXlvdXRVbml0IG5ld0xpbmVUb3AgPSBmbG9h dExvZ2ljYWxUb3BUb0xheW91dFVuaXQoZmxvYXROZXdMaW5lVG9wKTsKICAgICAgICAgaWYgKG5l d0xpbmVUb3AgPiBtX2xpbmVUb3ApIHsKICAgICAgICAgICAgIG1fbGluZVRvcCA9IG5ld0xpbmVU b3A7CiAgICAgICAgICAgICByZXR1cm4gdHJ1ZTsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3Jl L3JlbmRlcmluZy9FeGNsdXNpb25TaGFwZUluc2lkZUluZm8uaCBiL1NvdXJjZS9XZWJDb3JlL3Jl bmRlcmluZy9FeGNsdXNpb25TaGFwZUluc2lkZUluZm8uaAppbmRleCAxZjExY2YxLi4xZDczMjNm IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvRXhjbHVzaW9uU2hhcGVJbnNp ZGVJbmZvLmgKKysrIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL0V4Y2x1c2lvblNoYXBlSW5z aWRlSW5mby5oCkBAIC02Niw4ICs2NiwxNiBAQCBwdWJsaWM6CiAgICAgc3RhdGljIHZvaWQgcmVt b3ZlRXhjbHVzaW9uU2hhcGVJbnNpZGVJbmZvRm9yUmVuZGVyQmxvY2soY29uc3QgUmVuZGVyQmxv Y2sqKTsKICAgICBzdGF0aWMgYm9vbCBpc0V4Y2x1c2lvblNoYXBlSW5zaWRlSW5mb0VuYWJsZWRG b3JSZW5kZXJCbG9jayhjb25zdCBSZW5kZXJCbG9jayopOwogCi0gICAgTGF5b3V0VW5pdCBzaGFw ZUxvZ2ljYWxUb3AoKSBjb25zdCB7IHJldHVybiBzaGFwZUxvZ2ljYWxCb3VuZHNZKCk7IH0KLSAg ICBMYXlvdXRVbml0IHNoYXBlTG9naWNhbEJvdHRvbSgpIGNvbnN0IHsgcmV0dXJuIHNoYXBlTG9n aWNhbEJvdW5kc01heFkoKTsgfQorICAgIExheW91dFVuaXQgc2hhcGVMb2dpY2FsVG9wKCkgY29u c3QKKyAgICB7CisgICAgICAgIEFTU0VSVChtX3NoYXBlKTsKKyAgICAgICAgcmV0dXJuIGZsb2F0 TG9naWNhbFRvcFRvTGF5b3V0VW5pdChtX3NoYXBlLT5zaGFwZUxvZ2ljYWxCb3VuZGluZ0JveCgp LnkoKSk7CisgICAgfQorICAgIExheW91dFVuaXQgc2hhcGVMb2dpY2FsQm90dG9tKCkgY29uc3QK KyAgICB7CisgICAgICAgIEFTU0VSVChtX3NoYXBlKTsKKyAgICAgICAgcmV0dXJuIGZsb2F0TG9n aWNhbEJvdHRvbVRvTGF5b3V0VW5pdChtX3NoYXBlLT5zaGFwZUxvZ2ljYWxCb3VuZGluZ0JveCgp Lm1heFkoKSk7CisgICAgfQogICAgIGJvb2wgbGluZU92ZXJsYXBzU2hhcGVCb3VuZHMoKSBjb25z dCB7IHJldHVybiBtX2xpbmVUb3AgPCBzaGFwZUxvZ2ljYWxCb3R0b20oKSAmJiBtX2xpbmVUb3Ag KyBtX2xpbmVIZWlnaHQgPj0gc2hhcGVMb2dpY2FsVG9wKCk7IH0KIAogICAgIGJvb2wgaGFzU2Vn bWVudHMoKSBjb25zdApAQCAtOTgsMTkgKzEwNiw5IEBAIHB1YmxpYzoKIHByaXZhdGU6CiAgICAg RXhjbHVzaW9uU2hhcGVJbnNpZGVJbmZvKFJlbmRlckJsb2NrKik7CiAKLSAgICBpbmxpbmUgTGF5 b3V0VW5pdCBzaGFwZUxvZ2ljYWxCb3VuZHNZKCkgY29uc3QKLSAgICB7Ci0gICAgICAgIEFTU0VS VChtX3NoYXBlKTsKLSAgICAgICAgLy8gVXNlIGZyb21GbG9hdENlaWwoKSB0byBlbnN1cmUgdGhh dCB0aGUgcmV0dXJuZWQgTGF5b3V0VW5pdCB2YWx1ZSBpcyB3aXRoaW4gdGhlIHNoYXBlJ3MgYm91 bmRzLgotICAgICAgICByZXR1cm4gTGF5b3V0VW5pdDo6ZnJvbUZsb2F0Q2VpbChtX3NoYXBlLT5z aGFwZUxvZ2ljYWxCb3VuZGluZ0JveCgpLnkoKSk7Ci0gICAgfQotCi0gICAgaW5saW5lIExheW91 dFVuaXQgc2hhcGVMb2dpY2FsQm91bmRzTWF4WSgpIGNvbnN0Ci0gICAgewotICAgICAgICBBU1NF UlQobV9zaGFwZSk7Ci0gICAgICAgIC8vIFVzZSBmcm9tRmxvYXRGbG9vcigpIHRvIGVuc3VyZSB0 aGF0IHRoZSByZXR1cm5lZCBMYXlvdXRVbml0IHZhbHVlIGlzIHdpdGhpbiB0aGUgc2hhcGUncyBi b3VuZHMuCi0gICAgICAgIHJldHVybiBMYXlvdXRVbml0Ojpmcm9tRmxvYXRGbG9vcihtX3NoYXBl LT5zaGFwZUxvZ2ljYWxCb3VuZGluZ0JveCgpLm1heFkoKSk7Ci0gICAgfQorICAgIC8vIFVzZSBj ZWlsIGFuZCBmbG9vciB0byBlbnN1cmUgdGhhdCB0aGUgcmV0dXJuZWQgTGF5b3V0VW5pdCB2YWx1 ZSBpcyB3aXRoaW4gdGhlIHNoYXBlJ3MgYm91bmRzLgorICAgIExheW91dFVuaXQgZmxvYXRMb2dp Y2FsVG9wVG9MYXlvdXRVbml0KGZsb2F0IGxvZ2ljYWxUb3ApIGNvbnN0IHsgcmV0dXJuIExheW91 dFVuaXQ6OmZyb21GbG9hdENlaWwobG9naWNhbFRvcCk7IH0KKyAgICBMYXlvdXRVbml0IGZsb2F0 TG9naWNhbEJvdHRvbVRvTGF5b3V0VW5pdChmbG9hdCBsb2dpY2FsQm90dG9tKSBjb25zdCB7IHJl dHVybiBMYXlvdXRVbml0Ojpmcm9tRmxvYXRGbG9vcihsb2dpY2FsQm90dG9tKTsgfQogCiAgICAg UmVuZGVyQmxvY2sqIG1fYmxvY2s7CiAgICAgT3duUHRyPEV4Y2x1c2lvblNoYXBlPiBtX3NoYXBl Owo=