110515 2013-02-21 14:57:54 -0800 Clean up computePreferredLogicalWidths functions in TableLayout subclasses 2013-02-22 11:38:56 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 110520 1 ojan ojan eae eric esprehn+autocc hyatt jchaffraix leviw ojan.autocc tony webkit.review.bot oldest_to_newest 838869 0 ojan 2013-02-21 14:57:54 -0800 Clean up computePreferredLogicalWidths functions in TableLayout subclasses 838873 1 189606 ojan 2013-02-21 15:00:41 -0800 Created attachment 189606 Patch 838927 2 tony 2013-02-21 15:36:48 -0800 Does this work with bug 109462? Maybe Julien should review this patch :) 838929 3 ojan 2013-02-21 15:39:19 -0800 (In reply to comment #2) > Does this work with bug 109462? Maybe Julien should review this patch :) As best I can tell, the two patches are totally orthogonal. 838931 4 ojan 2013-02-21 15:39:44 -0800 Also, this patch does not cause any change in behavior. It's just moving code around. 839032 5 189606 tony 2013-02-21 17:00:06 -0800 Comment on attachment 189606 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189606&action=review > Source/WebCore/rendering/AutoTableLayout.cpp:261 > - Length tableLogicalWidth = m_table->style()->logicalWidth(); > - if (tableLogicalWidth.isFixed() && tableLogicalWidth.isPositive()) { > - minWidth = max<int>(minWidth, tableLogicalWidth.value()); > - maxWidth = minWidth; > - } else if (!remainingPercent && maxNonPercent) { > - // if there was no remaining percent, maxWidth is invalid > + // if there was no remaining percent, maxWidth is invalid > + if (!remainingPercent && maxNonPercent) > maxWidth = tableMaxWidth; > - } > + > + Length tableLogicalWidth = m_table->style()->logicalWidth(); > + if (tableLogicalWidth.isFixed() && tableLogicalWidth.isPositive()) > + minWidth = maxWidth = max<int>(minWidth, tableLogicalWidth.value()); I would revert this section of the patch. It doesn't seem equally readable and it's slower. > Source/WebCore/rendering/FixedTableLayout.cpp:82 > + // FIXME: we might want to wait until we have all of the first row before computing for the first time. Nit: Capitalize the first letter of the sentence. 839143 6 ojan 2013-02-21 19:13:45 -0800 Committed r143683: <http://trac.webkit.org/changeset/143683> 839188 7 189606 jchaffraix 2013-02-21 20:51:08 -0800 Comment on attachment 189606 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189606&action=review The change is good, sorry for the delay. (In reply to comment #3) > (In reply to comment #2) > > Does this work with bug 109462? Maybe Julien should review this patch :) > > As best I can tell, the two patches are totally orthogonal. They are indeed. However you make the assumption that r130774 which cause bug 109462 is there to cover your back and properly apply min-width / max-width to the result of computePreferredLogicalWidths thus making it difficult to revert the change. > Source/WebCore/rendering/FixedTableLayout.cpp:182 > + minWidth = maxWidth = calcWidthArray() + bordersPaddingAndSpacing; WebKit style is one assignment per line (though I couldn't find an entry in our coding style). That would make this line a lot more readable IMO. > Source/WebCore/rendering/FixedTableLayout.cpp:186 > + minWidth = maxWidth = max<int>(minWidth, tableLogicalWidth.value() - bordersPaddingAndSpacing); Ditto. 839775 8 ojan 2013-02-22 11:38:56 -0800 (In reply to comment #7) > > Source/WebCore/rendering/FixedTableLayout.cpp:182 > > + minWidth = maxWidth = calcWidthArray() + bordersPaddingAndSpacing; > > WebKit style is one assignment per line (though I couldn't find an entry in our coding style). That would make this line a lot more readable IMO. Discussed this on bug 110520. 189606 2013-02-21 15:00:41 -0800 2013-02-21 20:51:08 -0800 Patch bug-110515-20130221145701.patch text/plain 5857 ojan U3VidmVyc2lvbiBSZXZpc2lvbjogMTQzNjE3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggODUzOGI5M2MzNTk3MDc5 YWM1NzBkYzg0OWIzOGQyYzFiMjI5NGFhNC4uNDE3YTIzOWM5NmI2NDU0M2Y1MWYxMzZlMGMzNzFj NzA5ZjUxM2YzZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI3IEBACisyMDEzLTAyLTIxICBPamFu IFZhZmFpICA8b2phbkBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgQ2xlYW4gdXAgY29tcHV0ZVBy ZWZlcnJlZExvZ2ljYWxXaWR0aHMgZnVuY3Rpb25zIGluIFRhYmxlTGF5b3V0IHN1YmNsYXNzZXMK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTExMDUxNQor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE5vIGNoYW5n ZSBpbiBiZWhhdmlvci4gVGhpcyBpcyBqdXN0IGEgY2xlYW51cCBpbiBwcmVwYXJhdGlvbiBmb3Ig b3RoZXIKKyAgICAgICAgcmVmYWN0b3JpbmcgdG8gdGhpcyBjb2RlLgorCisgICAgICAgICogcmVu ZGVyaW5nL0F1dG9UYWJsZUxheW91dC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpBdXRvVGFibGVM YXlvdXQ6OmNvbXB1dGVQcmVmZXJyZWRMb2dpY2FsV2lkdGhzKToKKyAgICAgICAgRml4ZWQgd2lk dGhzIGFsd2F5cyBvdmVycmlkZXMgbWF4V2lkdGguIE1ha2UgdGhlIGxvZ2ljIGEgYml0IGxlc3MK KyAgICAgICAgY29udm9sdXRlZC4KKyAgICAgICAgKiByZW5kZXJpbmcvRml4ZWRUYWJsZUxheW91 dC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpGaXhlZFRhYmxlTGF5b3V0OjpjYWxjV2lkdGhBcnJh eSk6CisgICAgICAgIE1vdmUgYSBGSVhNRSBoZXJlIGZyb20gY29tcHV0ZVByZWZlcnJlZExvZ2lj YWxXaWR0aHMuIEl0IG1ha2VzIG1vcmUgc2Vuc2UgaGVyZS4KKyAgICAgICAgKFdlYkNvcmU6OkZp eGVkVGFibGVMYXlvdXQ6OmNvbXB1dGVQcmVmZXJyZWRMb2dpY2FsV2lkdGhzKToKKyAgICAgICAg LVJlbW92ZSBvdXRkYXRlZCBvciB1bmhlbHBmdWwgY29tbWVudHMuCisgICAgICAgIC1Jc29sYXRl IHRoZSBmaXhlZCB3aWR0aCBjb2RlcGF0aCB0byBtYWtlIGl0IGEgYml0IGxlc3MgY29udm9sdXRl ZC4KKyAgICAgICAgKFdlYkNvcmU6OkZpeGVkVGFibGVMYXlvdXQ6OmxheW91dCk6CisgICAgICAg ICogcmVuZGVyaW5nL0ZpeGVkVGFibGVMYXlvdXQuaDoKKyAgICAgICAgVGhlIGFyZ3VtZW50IHRv IGNhbGNXaWR0aEFycmF5IGlzIG5ldmVyIHVzZWQuIFJlbW92ZSBpdC4KKwogMjAxMy0wMi0yMSAg QW5kcmVpIEJ1Y3VyICA8YWJ1Y3VyQGFkb2JlLmNvbT4KIAogICAgICAgICAtd2Via2l0LW1hcmdp bi1jb2xsYXBzZTogc2VwYXJhdGUgZG9lc24ndCB3b3JrIGNvcnJlY3RseSBmb3IgYmVmb3JlIG1h cmdpbnMKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9BdXRvVGFibGVMYXlv dXQuY3BwIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL0F1dG9UYWJsZUxheW91dC5jcHAKaW5k ZXggYmFjZjJhYjA0MTJjOWIyZmI2MjE0MWUyNzc4ODZlNGJhNGQ0ZDg4NC4uZThlYjk0ZDQ3YWU5 MWU0NmZmOWUwODA5NzE4YjljMzJiYzI2NGRlOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUv cmVuZGVyaW5nL0F1dG9UYWJsZUxheW91dC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcmVuZGVy aW5nL0F1dG9UYWJsZUxheW91dC5jcHAKQEAgLTI1MiwxNCArMjUyLDEzIEBAIHZvaWQgQXV0b1Rh YmxlTGF5b3V0Ojpjb21wdXRlUHJlZmVycmVkTG9naWNhbFdpZHRocyhMYXlvdXRVbml0JiBtaW5X aWR0aCwgTGF5b3V0CiAgICAgbWluV2lkdGggKz0gYm9yZGVyc1BhZGRpbmdBbmRTcGFjaW5nOwog ICAgIG1heFdpZHRoICs9IGJvcmRlcnNQYWRkaW5nQW5kU3BhY2luZzsKIAotICAgIExlbmd0aCB0 YWJsZUxvZ2ljYWxXaWR0aCA9IG1fdGFibGUtPnN0eWxlKCktPmxvZ2ljYWxXaWR0aCgpOwotICAg IGlmICh0YWJsZUxvZ2ljYWxXaWR0aC5pc0ZpeGVkKCkgJiYgdGFibGVMb2dpY2FsV2lkdGguaXNQ b3NpdGl2ZSgpKSB7Ci0gICAgICAgIG1pbldpZHRoID0gbWF4PGludD4obWluV2lkdGgsIHRhYmxl TG9naWNhbFdpZHRoLnZhbHVlKCkpOwotICAgICAgICBtYXhXaWR0aCA9IG1pbldpZHRoOwotICAg IH0gZWxzZSBpZiAoIXJlbWFpbmluZ1BlcmNlbnQgJiYgbWF4Tm9uUGVyY2VudCkgewotICAgICAg ICAvLyBpZiB0aGVyZSB3YXMgbm8gcmVtYWluaW5nIHBlcmNlbnQsIG1heFdpZHRoIGlzIGludmFs aWQKKyAgICAvLyBpZiB0aGVyZSB3YXMgbm8gcmVtYWluaW5nIHBlcmNlbnQsIG1heFdpZHRoIGlz IGludmFsaWQKKyAgICBpZiAoIXJlbWFpbmluZ1BlcmNlbnQgJiYgbWF4Tm9uUGVyY2VudCkKICAg ICAgICAgbWF4V2lkdGggPSB0YWJsZU1heFdpZHRoOwotICAgIH0KKworICAgIExlbmd0aCB0YWJs ZUxvZ2ljYWxXaWR0aCA9IG1fdGFibGUtPnN0eWxlKCktPmxvZ2ljYWxXaWR0aCgpOworICAgIGlm ICh0YWJsZUxvZ2ljYWxXaWR0aC5pc0ZpeGVkKCkgJiYgdGFibGVMb2dpY2FsV2lkdGguaXNQb3Np dGl2ZSgpKQorICAgICAgICBtaW5XaWR0aCA9IG1heFdpZHRoID0gbWF4PGludD4obWluV2lkdGgs IHRhYmxlTG9naWNhbFdpZHRoLnZhbHVlKCkpOwogfQogCiAvKgpkaWZmIC0tZ2l0IGEvU291cmNl L1dlYkNvcmUvcmVuZGVyaW5nL0ZpeGVkVGFibGVMYXlvdXQuY3BwIGIvU291cmNlL1dlYkNvcmUv cmVuZGVyaW5nL0ZpeGVkVGFibGVMYXlvdXQuY3BwCmluZGV4IDEwZGZiYzhiMTkxMDIxMWI3MWE4 YmYxNGRiMTIwYmNkZWEzMzc3YTIuLmJlOTJkMzIyYjczOTY2MWY4OWY4NGRkZmU4NzA3MWU3ZTky MjA1NzUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9GaXhlZFRhYmxlTGF5 b3V0LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvRml4ZWRUYWJsZUxheW91dC5j cHAKQEAgLTc3LDggKzc3LDkgQEAgRml4ZWRUYWJsZUxheW91dDo6Rml4ZWRUYWJsZUxheW91dChS ZW5kZXJUYWJsZSogdGFibGUpCiB7CiB9CiAKLWludCBGaXhlZFRhYmxlTGF5b3V0OjpjYWxjV2lk dGhBcnJheShpbnQpCitpbnQgRml4ZWRUYWJsZUxheW91dDo6Y2FsY1dpZHRoQXJyYXkoKQogewor ICAgIC8vIEZJWE1FOiB3ZSBtaWdodCB3YW50IHRvIHdhaXQgdW50aWwgd2UgaGF2ZSBhbGwgb2Yg dGhlIGZpcnN0IHJvdyBiZWZvcmUgY29tcHV0aW5nIGZvciB0aGUgZmlyc3QgdGltZS4KICAgICBp bnQgdXNlZFdpZHRoID0gMDsKIAogICAgIC8vIGl0ZXJhdGUgb3ZlciBhbGwgPGNvbD4gZWxlbWVu dHMKQEAgLTE3NywyNSArMTc4LDEzIEBAIGludCBGaXhlZFRhYmxlTGF5b3V0OjpjYWxjV2lkdGhB cnJheShpbnQpCiAKIHZvaWQgRml4ZWRUYWJsZUxheW91dDo6Y29tcHV0ZVByZWZlcnJlZExvZ2lj YWxXaWR0aHMoTGF5b3V0VW5pdCYgbWluV2lkdGgsIExheW91dFVuaXQmIG1heFdpZHRoKQogewot ICAgIC8vIEZJWE1FOiBUaGlzIGVudGlyZSBjYWxjdWxhdGlvbiBpcyBpbmNvcnJlY3QgZm9yIGJv dGggbWlud2lkdGggYW5kIG1heHdpZHRoLgotICAgIAotICAgIC8vIHdlIG1pZ2h0IHdhbnQgdG8g d2FpdCB1bnRpbCB3ZSBoYXZlIGFsbCBvZiB0aGUgZmlyc3Qgcm93IGJlZm9yZQotICAgIC8vIGxh eW91dGluZyBmb3IgdGhlIGZpcnN0IHRpbWUuCi0KLSAgICAvLyBvbmx5IG5lZWQgdG8gY2FsY3Vs YXRlIHRoZSBtaW5pbXVtIHdpZHRoIGFzIHRoZSBzdW0gb2YgdGhlCi0gICAgLy8gY29scy9jZWxs cyB3aXRoIGEgZml4ZWQgd2lkdGguCi0gICAgLy8KLSAgICAvLyBUaGUgbWF4aW11bSB3aWR0aCBp cyBtYXgobWluV2lkdGgsIHRhYmxlV2lkdGgpLgogICAgIGludCBib3JkZXJzUGFkZGluZ0FuZFNw YWNpbmcgPSBtX3RhYmxlLT5ib3JkZXJzUGFkZGluZ0FuZFNwYWNpbmdJblJvd0RpcmVjdGlvbigp OworICAgIG1pbldpZHRoID0gbWF4V2lkdGggPSBjYWxjV2lkdGhBcnJheSgpICsgYm9yZGVyc1Bh ZGRpbmdBbmRTcGFjaW5nOwogCi0gICAgaW50IHRhYmxlTG9naWNhbFdpZHRoID0gbV90YWJsZS0+ c3R5bGUoKS0+bG9naWNhbFdpZHRoKCkuaXNGaXhlZCgpID8gbV90YWJsZS0+c3R5bGUoKS0+bG9n aWNhbFdpZHRoKCkudmFsdWUoKSAtIGJvcmRlcnNQYWRkaW5nQW5kU3BhY2luZyA6IDA7Ci0gICAg aW50IG13ID0gY2FsY1dpZHRoQXJyYXkodGFibGVMb2dpY2FsV2lkdGgpICsgYm9yZGVyc1BhZGRp bmdBbmRTcGFjaW5nOwotCi0gICAgbWluV2lkdGggPSBtYXgobXcsIHRhYmxlTG9naWNhbFdpZHRo KTsKLSAgICBtYXhXaWR0aCA9IG1pbldpZHRoOworICAgIExlbmd0aCB0YWJsZUxvZ2ljYWxXaWR0 aCA9IG1fdGFibGUtPnN0eWxlKCktPmxvZ2ljYWxXaWR0aCgpOworICAgIGlmICh0YWJsZUxvZ2lj YWxXaWR0aC5pc0ZpeGVkKCkgJiYgdGFibGVMb2dpY2FsV2lkdGguaXNQb3NpdGl2ZSgpKQorICAg ICAgICBtaW5XaWR0aCA9IG1heFdpZHRoID0gbWF4PGludD4obWluV2lkdGgsIHRhYmxlTG9naWNh bFdpZHRoLnZhbHVlKCkgLSBib3JkZXJzUGFkZGluZ0FuZFNwYWNpbmcpOwogCi0gICAgLy8gVGhp cyBxdWlyayBpcyB2ZXJ5IHNpbWlsYXIgdG8gb25lIHRoYXQgZXhpc3RzIGluIFJlbmRlckJsb2Nr OjpjYWxjQmxvY2tQcmVmV2lkdGhzKCkuCi0gICAgLy8gSGVyZSdzIHRoZSBleGFtcGxlIGZvciB0 aGlzIG9uZToKICAgICAvKgogICAgICAgICA8dGFibGUgc3R5bGU9IndpZHRoOjEwMCU7IGJhY2tn cm91bmQtY29sb3I6cmVkIj48dHI+PHRkPgogICAgICAgICAgICAgPHRhYmxlIHN0eWxlPSJiYWNr Z3JvdW5kLWNvbG9yOmJsdWUiPjx0cj48dGQ+CkBAIC0yMjAsNyArMjA5LDcgQEAgdm9pZCBGaXhl ZFRhYmxlTGF5b3V0OjpsYXlvdXQoKQogICAgIC8vIEZJWE1FOiBJdCBpcyBwb3NzaWJsZSB0byBi ZSBjYWxsZWQgd2l0aG91dCBoYXZpbmcgcHJvcGVybHkgdXBkYXRlZCBvdXIgaW50ZXJuYWwgcmVw cmVzZW50YXRpb24uCiAgICAgLy8gVGhpcyBtZWFucyB0aGF0IG91ciBwcmVmZXJyZWQgbG9naWNh bCB3aWR0aHMgd2VyZSBub3QgcmVjb21wdXRlZCBhcyBleHBlY3RlZC4KICAgICBpZiAobkVmZkNv bHMgIT0gbV93aWR0aC5zaXplKCkpIHsKLSAgICAgICAgY2FsY1dpZHRoQXJyYXkodGFibGVMb2dp Y2FsV2lkdGgpOworICAgICAgICBjYWxjV2lkdGhBcnJheSgpOwogICAgICAgICAvLyBGSVhNRTog VGFibGUgbGF5b3V0IHNob3VsZG4ndCBtb2RpZnkgb3VyIHRhYmxlIHN0cnVjdHVyZSAoYnV0IGRv ZXMgZHVlIHRvIGNvbHVtbnMgYW5kIGNvbHVtbi1ncm91cHMpLgogICAgICAgICBuRWZmQ29scyA9 IG1fdGFibGUtPm51bUVmZkNvbHMoKTsKICAgICB9CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29y ZS9yZW5kZXJpbmcvRml4ZWRUYWJsZUxheW91dC5oIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5n L0ZpeGVkVGFibGVMYXlvdXQuaAppbmRleCAyMWVkNjVjNTFlMWI3YjIyZjFlMjc5Nzg5NDU3MzYw ODI3MGY2MGIwLi41NjA5ZThjN2I4MGJlOGJjOTVmYmMyY2Q5OTg3OGQ2ZGY2NzU2Mjc0IDEwMDY0 NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvRml4ZWRUYWJsZUxheW91dC5oCisrKyBi L1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9GaXhlZFRhYmxlTGF5b3V0LmgKQEAgLTM4LDcgKzM4 LDcgQEAgcHVibGljOgogICAgIHZpcnR1YWwgdm9pZCBsYXlvdXQoKTsKIAogcHJpdmF0ZToKLSAg ICBpbnQgY2FsY1dpZHRoQXJyYXkoaW50IHRhYmxlV2lkdGgpOworICAgIGludCBjYWxjV2lkdGhB cnJheSgpOwogCiAgICAgVmVjdG9yPExlbmd0aD4gbV93aWR0aDsKIH07Cg==