224050 2021-04-01 08:56:15 -0700 Rename RenderBox::shouldIgnoreAspectRatio 2021-04-30 08:56:57 -0700 1 1 1 Unclassified WebKit Layout and Rendering WebKit Nightly Build Unspecified Unspecified NEW InRadar P2 Normal --- 1 zalan zalan bfulgham changseok esprehn+autocc ews-watchlist glenn kondapallykalyan pdr rbuis simon.fraser webkit-bug-importer zalan oldest_to_newest 1746046 0 zalan 2021-04-01 08:56:15 -0700 This function is about whether the aspect ratio value is applicable and set to a value other than auto and not whether we should ignore the aspect ratio (and we ignore the aspect ratio value for some other reasons) 1746049 1 424900 zalan 2021-04-01 09:00:39 -0700 Created attachment 424900 Patch 1746062 2 424900 simon.fraser 2021-04-01 09:26:21 -0700 Comment on attachment 424900 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424900&action=review > Source/WebCore/rendering/RenderBox.cpp:5098 > +static inline bool hasAspectRatio(const RenderBox& renderer) This name is worse than the old one. It should be more like canUseAspectRatio() or aspectRatioApplies(). 1746080 3 zalan 2021-04-01 09:59:44 -0700 (In reply to Simon Fraser (smfr) from comment #2) > Comment on attachment 424900 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424900&action=review > > > Source/WebCore/rendering/RenderBox.cpp:5098 > > +static inline bool hasAspectRatio(const RenderBox& renderer) > > This name is worse than the old one. It should be more like > canUseAspectRatio() or aspectRatioApplies(). The function name certainly can be improved but it is definitely not as misleading as the initial name. 1746088 4 zalan 2021-04-01 10:15:52 -0700 (In reply to zalan from comment #3) > (In reply to Simon Fraser (smfr) from comment #2) > > Comment on attachment 424900 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=424900&action=review > > > > > Source/WebCore/rendering/RenderBox.cpp:5098 > > > +static inline bool hasAspectRatio(const RenderBox& renderer) > > > > This name is worse than the old one. It should be more like > > canUseAspectRatio() or aspectRatioApplies(). > The function name certainly can be improved but it is definitely not as > misleading as the initial name. This was supposed to be just an extended version of RenderStyle::hasAspectRatio() taking the box type into account. 1746098 5 simon.fraser 2021-04-01 10:54:36 -0700 (In reply to zalan from comment #4) > (In reply to zalan from comment #3) > > (In reply to Simon Fraser (smfr) from comment #2) > > > Comment on attachment 424900 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=424900&action=review > > > > > > > Source/WebCore/rendering/RenderBox.cpp:5098 > > > > +static inline bool hasAspectRatio(const RenderBox& renderer) > > > > > > This name is worse than the old one. It should be more like > > > canUseAspectRatio() or aspectRatioApplies(). > > The function name certainly can be improved but it is definitely not as > > misleading as the initial name. > This was supposed to be just an extended version of > RenderStyle::hasAspectRatio() taking the box type into account. hasAspectRatio(const RenderBox& renderer) sounds like it's asking "does this box have aspect ratio", by which we really mean "does this box have dimensions controlled by the aspect-ratio property: But aspect-ratio has all these other constraints, like non-auto dimensions. I think it's different from transform in that way, so following hasTransform() is confusing. 1746224 6 zalan 2021-04-01 13:33:55 -0700 (In reply to Simon Fraser (smfr) from comment #5) > (In reply to zalan from comment #4) > > (In reply to zalan from comment #3) > > > (In reply to Simon Fraser (smfr) from comment #2) > > > > Comment on attachment 424900 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=424900&action=review > > > > > > > > > Source/WebCore/rendering/RenderBox.cpp:5098 > > > > > +static inline bool hasAspectRatio(const RenderBox& renderer) > > > > > > > > This name is worse than the old one. It should be more like > > > > canUseAspectRatio() or aspectRatioApplies(). > > > The function name certainly can be improved but it is definitely not as > > > misleading as the initial name. > > This was supposed to be just an extended version of > > RenderStyle::hasAspectRatio() taking the box type into account. > > hasAspectRatio(const RenderBox& renderer) sounds like it's asking "does this > box have aspect ratio", by which we really mean "does this box have > dimensions controlled by the aspect-ratio property: But aspect-ratio has all > these other constraints, like non-auto dimensions. I think it's different > from transform in that way, so following hasTransform() is confusing. I agree it's not the same and that's why I moved it out of the class (to reduce visibility/misuse) I find it difficult to come up with a meaningful/proper name. 1748310 7 webkit-bug-importer 2021-04-08 08:57:13 -0700 <rdar://problem/76403114> 1755694 8 rbuis 2021-04-30 08:56:57 -0700 I like the idea of the patch. Maybe something like canApplyAspectRatio? 424900 2021-04-01 09:00:39 -0700 2021-04-01 10:00:01 -0700 Patch bug-224050-20210401090039.patch text/plain 2940 zalan U3VidmVyc2lvbiBSZXZpc2lvbjogMjc1MzYxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZTBhMGVlMGMxZDliMTQ5 ODUwMWIzODk1ZmQzMjI3ZWFkMjA4OGI2MS4uMDU4MjhlNWE0ZmY5NzA0MmEzZGI5MGY5N2ZjYjI4 MjMwNDlhZmUwYSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDIxLTA0LTAxICBaYWxh biBCdWp0YXMgIDx6YWxhbkBhcHBsZS5jb20+CisKKyAgICAgICAgUmVuYW1lIFJlbmRlckJveDo6 c2hvdWxkSWdub3JlQXNwZWN0UmF0aW8KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTIyNDA1MAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIFRoaXMgZnVuY3Rpb24gaXMgYWJvdXQgd2hldGhlciB0aGUgYXNwZWN0 IHJhdGlvIHByb3BlcnR5IGlzIGFwcGxpY2FibGUgYW5kIGhhcyBhIG5vbi1hdXRvIHZhbHVlCisg ICAgICAgIChhbmQgc29tZXRpbWVzIHdlIGlnbm9yZSB0aGlzIHZhbHVlIGZvciBzb21lIG90aGVy IHJlYXNvbnMpLgorCisgICAgICAgICogcmVuZGVyaW5nL1JlbmRlckJveC5jcHA6CisgICAgICAg IChXZWJDb3JlOjpoYXNBc3BlY3RSYXRpbyk6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJCb3g6 OnNob3VsZENvbXB1dGVMb2dpY2FsSGVpZ2h0RnJvbUFzcGVjdFJhdGlvIGNvbnN0KToKKyAgICAg ICAgKFdlYkNvcmU6OlJlbmRlckJveDo6c2hvdWxkQ29tcHV0ZUxvZ2ljYWxXaWR0aEZyb21Bc3Bl Y3RSYXRpbyBjb25zdCk6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJCb3g6OnNob3VsZElnbm9y ZUFzcGVjdFJhdGlvIGNvbnN0KTogRGVsZXRlZC4KKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVy Qm94Lmg6CisKIDIwMjEtMDQtMDEgIE1hcmsgTGFtICA8bWFyay5sYW1AYXBwbGUuY29tPgogCiAg ICAgICAgIEZpeCBzb21lIG1pc3NpbmcgZXhjZXB0aW9uIGNoZWNrcyBpbiBIVE1MTWVkaWFFbGVt ZW50IG1ldGhvZHMuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVy Qm94LmNwcCBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJCb3guY3BwCmluZGV4IDRk NzVhY2YzNGVhMDUzYjY5YjZlM2NkMGM0OGEyNTRiZDAwMDVhYTAuLmZhOTkwN2RkYjIwMzM3ZDc5 MmRjNWEzYThiMDQ5MWZlNGQxYzQ0ZjMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3JlbmRl cmluZy9SZW5kZXJCb3guY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJC b3guY3BwCkBAIC01MDk1LDE0ICs1MDk1LDE0IEBAIHZvaWQgUmVuZGVyQm94OjphcHBseVRvcExl ZnRMb2NhdGlvbk9mZnNldFdpdGhGbGlwcGluZyhMYXlvdXRQb2ludCYgcG9pbnQpIGNvbnN0CiAg ICAgcG9pbnQubW92ZShyZWN0LngoKSwgcmVjdC55KCkpOwogfQogCi1ib29sIFJlbmRlckJveDo6 c2hvdWxkSWdub3JlQXNwZWN0UmF0aW8oKSBjb25zdAorc3RhdGljIGlubGluZSBib29sIGhhc0Fz cGVjdFJhdGlvKGNvbnN0IFJlbmRlckJveCYgcmVuZGVyZXIpCiB7Ci0gICAgcmV0dXJuICFzdHls ZSgpLmhhc0FzcGVjdFJhdGlvKCkgfHwgaXNUYWJsZVBhcnQoKTsKKyAgICByZXR1cm4gcmVuZGVy ZXIuc3R5bGUoKS5oYXNBc3BlY3RSYXRpbygpICYmICFyZW5kZXJlci5pc1RhYmxlUGFydCgpOwog fQogCiBib29sIFJlbmRlckJveDo6c2hvdWxkQ29tcHV0ZUxvZ2ljYWxIZWlnaHRGcm9tQXNwZWN0 UmF0aW8oKSBjb25zdAogewotICAgIGlmIChzaG91bGRJZ25vcmVBc3BlY3RSYXRpbygpKQorICAg IGlmICghaGFzQXNwZWN0UmF0aW8oKnRoaXMpKQogICAgICAgICByZXR1cm4gZmFsc2U7CiAKICAg ICBpZiAoc2hvdWxkQ29tcHV0ZUxvZ2ljYWxXaWR0aEZyb21Bc3BlY3RSYXRpb0FuZEluc2V0cygp KQpAQCAtNTExNCw3ICs1MTE0LDcgQEAgYm9vbCBSZW5kZXJCb3g6OnNob3VsZENvbXB1dGVMb2dp Y2FsSGVpZ2h0RnJvbUFzcGVjdFJhdGlvKCkgY29uc3QKIAogYm9vbCBSZW5kZXJCb3g6OnNob3Vs ZENvbXB1dGVMb2dpY2FsV2lkdGhGcm9tQXNwZWN0UmF0aW8oKSBjb25zdAogewotICAgIGlmIChz aG91bGRJZ25vcmVBc3BlY3RSYXRpbygpKQorICAgIGlmICghaGFzQXNwZWN0UmF0aW8oKnRoaXMp KQogICAgICAgICByZXR1cm4gZmFsc2U7CiAKICAgICBhdXRvIGlzUmVzb2x2YWJsZVBlcmNlbnRh Z2VIZWlnaHQgPSBbJl0gewpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1Jl bmRlckJveC5oIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckJveC5oCmluZGV4IGM1 YThkOGZjOTU3ZGRlMzkwNjJjMDNlYTFmYWY3YzZkY2FjY2U1MTIuLjI2NTMxNGQwMGQ3MWUwNzU3 NDNmOGU1OGQ2YTNlNjBmZDM5NDg2ZmEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3JlbmRl cmluZy9SZW5kZXJCb3guaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQm94 LmgKQEAgLTcwMSw3ICs3MDEsNiBAQCBwcm90ZWN0ZWQ6CiAKICAgICB2b2lkIGluY3JlbWVudFZp c3VhbGx5Tm9uRW1wdHlQaXhlbENvdW50SWZOZWVkZWQoY29uc3QgSW50U2l6ZSYpOwogCi0gICAg Ym9vbCBzaG91bGRJZ25vcmVBc3BlY3RSYXRpbygpIGNvbnN0OwogICAgIGJvb2wgc2hvdWxkQ29t cHV0ZUxvZ2ljYWxXaWR0aEZyb21Bc3BlY3RSYXRpbygpIGNvbnN0OwogICAgIGJvb2wgc2hvdWxk Q29tcHV0ZUxvZ2ljYWxXaWR0aEZyb21Bc3BlY3RSYXRpb0FuZEluc2V0cygpIGNvbnN0OwogICAg IExheW91dFVuaXQgY29tcHV0ZUxvZ2ljYWxXaWR0aEZyb21Bc3BlY3RSYXRpbyhSZW5kZXJGcmFn bWVudENvbnRhaW5lciogPSBudWxscHRyKSBjb25zdDsK