79063 2012-02-20 18:17:48 -0800 MathML internals - code clean-up for RenderMathMLSubSup 2012-02-20 22:00:40 -0800 1 1 1 Unclassified WebKit MathML 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 dbarton webkit-unassigned eric webkit.review.bot oldest_to_newest 560513 0 dbarton 2012-02-20 18:17:48 -0800 MathML internals - code clean-up for RenderMathMLSubSup 560521 1 127890 dbarton 2012-02-20 18:27:17 -0800 Created attachment 127890 Patch 560541 2 127890 eric 2012-02-20 19:20:10 -0800 Comment on attachment 127890 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127890&action=review I think this is OK, but could be made a bit more clear. Are you willing to go another round of cleanup before moving foward? > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:-149 > - if (top->isBoxModelObject()) { > - RenderBoxModelObject* topBox = toRenderBoxModelObject(top); > - topBox->updateBoxModelInfoFromStyle(); > - } We no longer need this update? > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:151 > + if (m_kind == SubSup && m_scripts) { It would be better to early return so we didn't have to indent the whole rest of this function? > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:153 > + LayoutUnit heightDiff = (m_scripts->offsetHeight() - base->offsetHeight()) / 2; heightDiff sounds like a strange name for this half-height diff? 560544 3 eric 2012-02-20 19:20:36 -0800 Mostly we need to explain why deleting the update line is OK. 560589 4 127890 dbarton 2012-02-20 20:19:50 -0800 Comment on attachment 127890 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127890&action=review I appreciate your review & judgment and will do any changes you still want. In the next patch though this stretchToHeight() formatting code & the code in layout() will be combined and revised. >> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:-149 >> - } > > We no longer need this update? There's an old message from David Hyatt on webkit-dev saying that updateBoxModelInfoFromStyle() is an internal method to styleWillChange()/styleDidChange() and should be considered private. The supWrapper->setNeedsLayout(true) on new line 141 will cause these margins to be used, right? >> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:151 >> + if (m_kind == SubSup && m_scripts) { > > It would be better to early return so we didn't have to indent the whole rest of this function? I didn't want to rewrite too much. Also we could change someday to doing our own layout() of msub or msup also, instead of using CSS vertical-align sub/super. That way we could get the baselines of msub or msup to be the same as the baselines of subscripts & superscripts in <msubsup>s, maybe. >> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:153 >> + LayoutUnit heightDiff = (m_scripts->offsetHeight() - base->offsetHeight()) / 2; > > heightDiff sounds like a strange name for this half-height diff? This variable is going away in the next patch. I just left the name as is for now. 560590 5 127890 eric 2012-02-20 20:21:20 -0800 Comment on attachment 127890 Patch I'm very happy to approve this as a first-iteration patch. Small patches are always the best way to work, and I certainly wouldn't want to discourage you from such! Thank you for your responses and I look forward to the next patch. 560651 6 127890 webkit.review.bot 2012-02-20 22:00:36 -0800 Comment on attachment 127890 Patch Clearing flags on attachment: 127890 Committed r108302: <http://trac.webkit.org/changeset/108302> 560652 7 webkit.review.bot 2012-02-20 22:00:40 -0800 All reviewed patches have been landed. Closing bug. 127890 2012-02-20 18:27:17 -0800 2012-02-20 22:00:36 -0800 Patch bug-79063-20120220182716.patch text/plain 6677 dbarton SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwODI4MCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI1IEBACisyMDEyLTAyLTIwICBEYXZpZCBC YXJ0b24gIDxkYmFydG9uQG1hdGhzY3JpYmUuY29tPgorCisgICAgICAgIE1hdGhNTCBpbnRlcm5h bHMgLSBjb2RlIGNsZWFuLXVwIGZvciBSZW5kZXJNYXRoTUxTdWJTdXAKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTc5MDYzCisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSW4gdGhlIG5leHQgcGF0Y2gsIEkgd2ls bCBzaHJpbmsgYW5kIHJldmlzZSB0aGUgPG1zdWJzdXA+IGZvcm1hdHRpbmcgY29kZS4gVG8gbWFr ZSB0aGlzCisgICAgICAgIGVhc2llciB0byBmb2xsb3csIEkgYW0gZmlyc3QgZG9pbmcgc29tZSBz aW1wbGUgY29kZSBjbGVhbi11cC4KKworICAgICAgICBObyBuZXcgdGVzdHMuCisKKyAgICAgICAg KiByZW5kZXJpbmcvbWF0aG1sL1JlbmRlck1hdGhNTFJvdy5jcHA6CisgICAgICAgIChXZWJDb3Jl OjpSZW5kZXJNYXRoTUxSb3c6OmxheW91dCk6CisgICAgICAgICogcmVuZGVyaW5nL21hdGhtbC9S ZW5kZXJNYXRoTUxSb3cuaDoKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlck1hdGhNTFJvdzo6aXNS ZW5kZXJNYXRoTUxSb3cpOgorICAgICAgICAqIHJlbmRlcmluZy9tYXRobWwvUmVuZGVyTWF0aE1M U3ViU3VwLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlck1hdGhNTFN1YlN1cDo6c3RyZXRj aFRvSGVpZ2h0KToKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlck1hdGhNTFN1YlN1cDo6bGF5b3V0 KToKKyAgICAgICAgICAgIC0gVGhlcmUgaXMgbm8gbmVlZCB0byBpdGVyYXRlIG92ZXIgYmFzZVdy YXBwZXIncyBjaGlsZHJlbiBzaW5jZSBpdCBzaG91bGQgaGF2ZSBvbmx5CisgICAgICAgICAgICAg IG9uZSBjaGlsZCwgdGhlIGJhc2Ugb2YgdGhlIDxtc3Vic3VwPi4KKwogMjAxMi0wMi0yMCAgTWFy dGluIFJvYmluc29uICA8bXJvYmluc29uQGlnYWxpYS5jb20+CiAKICAgICAgICAgW0dUS10gV2Vi IGNvbnRlbnQgb2Z0ZW5zIHN0ZWFscyBmb2N1cyBmcm9tIG90aGVyIHdpZGdldHMKSW5kZXg6IFNv dXJjZS9XZWJDb3JlL3JlbmRlcmluZy9tYXRobWwvUmVuZGVyTWF0aE1MUm93LmNwcAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvbWF0aG1sL1JlbmRlck1hdGhNTFJvdy5j cHAJKHJldmlzaW9uIDEwODI2MykKKysrIFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9tYXRobWwv UmVuZGVyTWF0aE1MUm93LmNwcAkod29ya2luZyBjb3B5KQpAQCAtODIsMTcgKzgyLDYgQEAgdm9p ZCBSZW5kZXJNYXRoTUxSb3c6OmxheW91dCgpIAogICAgIH0KICAgICAKIH0gICAgCi0KLUxheW91 dFVuaXQgUmVuZGVyTWF0aE1MUm93OjpiYXNlbGluZVBvc2l0aW9uKEZvbnRCYXNlbGluZSwgYm9v bCBmaXJzdExpbmUsIExpbmVEaXJlY3Rpb25Nb2RlIGRpcmVjdGlvbiwgTGluZVBvc2l0aW9uTW9k ZSBsaW5lUG9zaXRpb25Nb2RlKSBjb25zdAotewotICAgIGlmIChmaXJzdENoaWxkKCkgJiYgZmly c3RDaGlsZCgpLT5pc1JlbmRlck1hdGhNTEJsb2NrKCkgJiYgbGluZVBvc2l0aW9uTW9kZSA9PSBQ b3NpdGlvbk9uQ29udGFpbmluZ0xpbmUpIHsKLSAgICAgICAgUmVuZGVyTWF0aE1MQmxvY2sqIGJs b2NrID0gdG9SZW5kZXJNYXRoTUxCbG9jayhmaXJzdENoaWxkKCkpOwotICAgICAgICBpZiAoYmxv Y2stPmlzUmVuZGVyTWF0aE1MT3BlcmF0b3IoKSkKLSAgICAgICAgICAgIHJldHVybiBibG9jay0+ eSgpICsgYmxvY2stPmJhc2VsaW5lUG9zaXRpb24oQWxwaGFiZXRpY0Jhc2VsaW5lLCBmaXJzdExp bmUsIGRpcmVjdGlvbiwgbGluZVBvc2l0aW9uTW9kZSk7Ci0gICAgfQotICAgIAotICAgIHJldHVy biBSZW5kZXJCbG9jazo6YmFzZWxpbmVQb3NpdGlvbihBbHBoYWJldGljQmFzZWxpbmUsIGZpcnN0 TGluZSwgZGlyZWN0aW9uLCBsaW5lUG9zaXRpb25Nb2RlKTsKLX0KICAgICAKIH0KIApJbmRleDog U291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL21hdGhtbC9SZW5kZXJNYXRoTUxSb3cuaAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvbWF0aG1sL1JlbmRlck1hdGhNTFJvdy5o CShyZXZpc2lvbiAxMDgyNjMpCisrKyBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvbWF0aG1sL1Jl bmRlck1hdGhNTFJvdy5oCSh3b3JraW5nIGNvcHkpCkBAIC0zNiw3ICszNiw2IEBAIGNsYXNzIFJl bmRlck1hdGhNTFJvdyA6IHB1YmxpYyBSZW5kZXJNYXQKIHB1YmxpYzoKICAgICBSZW5kZXJNYXRo TUxSb3coRWxlbWVudCopOwogICAgIHZpcnR1YWwgYm9vbCBpc1JlbmRlck1hdGhNTFJvdygpIGNv bnN0IHsgcmV0dXJuIHRydWU7IH0KLSAgICB2aXJ0dWFsIExheW91dFVuaXQgYmFzZWxpbmVQb3Np dGlvbihGb250QmFzZWxpbmUsIGJvb2wgZmlyc3RMaW5lLCBMaW5lRGlyZWN0aW9uTW9kZSwgTGlu ZVBvc2l0aW9uTW9kZSA9IFBvc2l0aW9uT25Db250YWluaW5nTGluZSkgY29uc3Q7ICAgIAogICAg IHZpcnR1YWwgdm9pZCBzdHJldGNoVG9IZWlnaHQoaW50KSB7fQogcHJvdGVjdGVkOgogICAgIHZp cnR1YWwgdm9pZCBsYXlvdXQoKTsKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9tYXRo bWwvUmVuZGVyTWF0aE1MU3ViU3VwLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9y ZW5kZXJpbmcvbWF0aG1sL1JlbmRlck1hdGhNTFN1YlN1cC5jcHAJKHJldmlzaW9uIDEwODI2MykK KysrIFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9tYXRobWwvUmVuZGVyTWF0aE1MU3ViU3VwLmNw cAkod29ya2luZyBjb3B5KQpAQCAtMTI1LDYzICsxMjUsMzkgQEAgUmVuZGVyTWF0aE1MT3BlcmF0 b3IqIFJlbmRlck1hdGhNTFN1YlN1cAogCiB2b2lkIFJlbmRlck1hdGhNTFN1YlN1cDo6c3RyZXRj aFRvSGVpZ2h0KGludCBoZWlnaHQpCiB7Ci0gICAgUmVuZGVyT2JqZWN0KiBiYXNlV3JhcHBlciA9 IGZpcnN0Q2hpbGQoKTsKLSAgICBpZiAoIWJhc2VXcmFwcGVyIHx8ICFiYXNlV3JhcHBlci0+Zmly c3RDaGlsZCgpKQotICAgICAgICByZXR1cm47Ci0gICAgCi0gICAgaWYgKGJhc2VXcmFwcGVyLT5m aXJzdENoaWxkKCkgJiYgYmFzZVdyYXBwZXItPmZpcnN0Q2hpbGQoKS0+aXNSZW5kZXJNYXRoTUxC bG9jaygpKSB7Ci0gICAgICAgIFJlbmRlck1hdGhNTEJsb2NrKiBibG9jayA9IHRvUmVuZGVyTWF0 aE1MQmxvY2soYmFzZVdyYXBwZXItPmZpcnN0Q2hpbGQoKSk7Ci0gICAgICAgIGJsb2NrLT5zdHJl dGNoVG9IZWlnaHQoc3RhdGljX2Nhc3Q8aW50PihnU3ViU3VwU3RyZXRjaCAqIGhlaWdodCkpOwor ICAgIFJlbmRlckJveE1vZGVsT2JqZWN0KiBiYXNlID0gdGhpcy0+YmFzZSgpOworICAgIGlmIChi YXNlICYmIGJhc2UtPmlzUmVuZGVyTWF0aE1MQmxvY2soKSkgeworICAgICAgICB0b1JlbmRlck1h dGhNTEJsb2NrKGJhc2UpLT5zdHJldGNoVG9IZWlnaHQoc3RhdGljX2Nhc3Q8aW50PihnU3ViU3Vw U3RyZXRjaCAqIGhlaWdodCkpOwogICAgICAgICAKICAgICAgICAgLy8gQWRqdXN0IHRoZSBzY3Jp cHQgcGxhY2VtZW50IGFmdGVyIHdlIHN0cmV0Y2gKICAgICAgICAgaWYgKGhlaWdodCA+IDAgJiYg bV9raW5kID09IFN1YlN1cCAmJiBtX3NjcmlwdHMpIHsKLSAgICAgICAgICAgIFJlbmRlck9iamVj dCogc2NyaXB0ID0gbV9zY3JpcHRzLT5maXJzdENoaWxkKCk7Ci0gICAgICAgICAgICBpZiAoc2Ny aXB0KSB7CisgICAgICAgICAgICBSZW5kZXJPYmplY3QqIHN1cFdyYXBwZXIgPSBtX3NjcmlwdHMt PmZpcnN0Q2hpbGQoKTsKKyAgICAgICAgICAgIGlmIChzdXBXcmFwcGVyKSB7CiAgICAgICAgICAg ICAgICAgLy8gQ2FsY3VsYXRlIHRoZSBzY3JpcHQgaGVpZ2h0IHdpdGhvdXQgdGhlIGNvbnRhaW5l ciBtYXJnaW5zLgotICAgICAgICAgICAgICAgIFJlbmRlck9iamVjdCogdG9wID0gc2NyaXB0Owot ICAgICAgICAgICAgICAgIGludCB0b3BIZWlnaHQgPSBnZXRCb3hNb2RlbE9iamVjdEhlaWdodCh0 b3AtPmZpcnN0Q2hpbGQoKSk7Ci0gICAgICAgICAgICAgICAgaW50IHRvcEFkanVzdCA9IHRvcEhl aWdodCAvIGdUb3BBZGp1c3REaXZpc29yOwotICAgICAgICAgICAgICAgIHRvcC0+c3R5bGUoKS0+ c2V0TWFyZ2luVG9wKExlbmd0aCgtdG9wQWRqdXN0LCBGaXhlZCkpOwotICAgICAgICAgICAgICAg IHRvcC0+c3R5bGUoKS0+c2V0TWFyZ2luQm90dG9tKExlbmd0aChoZWlnaHQgLSB0b3BIZWlnaHQg KyB0b3BBZGp1c3QsIEZpeGVkKSk7Ci0gICAgICAgICAgICAgICAgaWYgKHRvcC0+aXNCb3hNb2Rl bE9iamVjdCgpKSB7Ci0gICAgICAgICAgICAgICAgICAgIFJlbmRlckJveE1vZGVsT2JqZWN0KiB0 b3BCb3ggPSB0b1JlbmRlckJveE1vZGVsT2JqZWN0KHRvcCk7Ci0gICAgICAgICAgICAgICAgICAg IHRvcEJveC0+dXBkYXRlQm94TW9kZWxJbmZvRnJvbVN0eWxlKCk7Ci0gICAgICAgICAgICAgICAg fQotICAgICAgICAgICAgICAgIG1fc2NyaXB0cy0+c2V0TmVlZHNMYXlvdXQodHJ1ZSk7Ci0gICAg ICAgICAgICAgICAgc2V0TmVlZHNMYXlvdXQodHJ1ZSk7CisgICAgICAgICAgICAgICAgaW50IHN1 cEhlaWdodCA9IGdldEJveE1vZGVsT2JqZWN0SGVpZ2h0KHN1cFdyYXBwZXItPmZpcnN0Q2hpbGQo KSk7CisgICAgICAgICAgICAgICAgaW50IHN1cFRvcEFkanVzdCA9IHN1cEhlaWdodCAvIGdUb3BB ZGp1c3REaXZpc29yOworICAgICAgICAgICAgICAgIHN1cFdyYXBwZXItPnN0eWxlKCktPnNldE1h cmdpblRvcChMZW5ndGgoLXN1cFRvcEFkanVzdCwgRml4ZWQpKTsKKyAgICAgICAgICAgICAgICBz dXBXcmFwcGVyLT5zdHlsZSgpLT5zZXRNYXJnaW5Cb3R0b20oTGVuZ3RoKGhlaWdodCAtIHN1cEhl aWdodCArIHN1cFRvcEFkanVzdCwgRml4ZWQpKTsKKyAgICAgICAgICAgICAgICBzdXBXcmFwcGVy LT5zZXROZWVkc0xheW91dCh0cnVlKTsKICAgICAgICAgICAgIH0KICAgICAgICAgfQotICAgICAg ICAKICAgICB9CiB9CiAKLXZvaWQgUmVuZGVyTWF0aE1MU3ViU3VwOjpsYXlvdXQoKSAKK3ZvaWQg UmVuZGVyTWF0aE1MU3ViU3VwOjpsYXlvdXQoKQogewotICAgIGlmIChmaXJzdENoaWxkKCkpCi0g ICAgICAgIGZpcnN0Q2hpbGQoKS0+c2V0TmVlZHNMYXlvdXQodHJ1ZSk7Ci0gICAgaWYgKG1fc2Ny aXB0cykgCi0gICAgICAgIG1fc2NyaXB0cy0+c2V0TmVlZHNMYXlvdXQodHJ1ZSk7Ci0gICAgCiAg ICAgUmVuZGVyQmxvY2s6OmxheW91dCgpOwogICAgIAotICAgIGlmIChtX2tpbmQgPT0gU3ViU3Vw KSB7Ci0gICAgICAgIGlmIChSZW5kZXJPYmplY3QqIGJhc2VXcmFwcGVyID0gZmlyc3RDaGlsZCgp KSB7Ci0gICAgICAgICAgICBMYXlvdXRVbml0IG1heEhlaWdodCA9IDA7Ci0gICAgICAgICAgICBS ZW5kZXJPYmplY3QqIGN1cnJlbnQgPSBiYXNlV3JhcHBlci0+Zmlyc3RDaGlsZCgpOwotICAgICAg ICAgICAgd2hpbGUgKGN1cnJlbnQpIHsKLSAgICAgICAgICAgICAgICBMYXlvdXRVbml0IGhlaWdo dCA9IGdldEJveE1vZGVsT2JqZWN0SGVpZ2h0KGN1cnJlbnQpOwotICAgICAgICAgICAgICAgIGlm IChoZWlnaHQgPiBtYXhIZWlnaHQpCi0gICAgICAgICAgICAgICAgICAgIG1heEhlaWdodCA9IGhl aWdodDsKLSAgICAgICAgICAgICAgICBjdXJyZW50ID0gY3VycmVudC0+bmV4dFNpYmxpbmcoKTsK LSAgICAgICAgICAgIH0KLSAgICAgICAgICAgIExheW91dFVuaXQgaGVpZ2h0RGlmZiA9IG1fc2Ny aXB0cyA/IChtX3NjcmlwdHMtPm9mZnNldEhlaWdodCgpIC0gbWF4SGVpZ2h0KSAvIDIgOiB6ZXJv TGF5b3V0VW5pdDsKKyAgICBpZiAobV9raW5kID09IFN1YlN1cCAmJiBtX3NjcmlwdHMpIHsKKyAg ICAgICAgaWYgKFJlbmRlckJveE1vZGVsT2JqZWN0KiBiYXNlID0gdGhpcy0+YmFzZSgpKSB7Cisg ICAgICAgICAgICBMYXlvdXRVbml0IGhlaWdodERpZmYgPSAobV9zY3JpcHRzLT5vZmZzZXRIZWln aHQoKSAtIGJhc2UtPm9mZnNldEhlaWdodCgpKSAvIDI7CiAgICAgICAgICAgICBpZiAoaGVpZ2h0 RGlmZiA8IDApIAogICAgICAgICAgICAgICAgIGhlaWdodERpZmYgPSAwOworICAgICAgICAgICAg UmVuZGVyT2JqZWN0KiBiYXNlV3JhcHBlciA9IGZpcnN0Q2hpbGQoKTsKICAgICAgICAgICAgIGJh c2VXcmFwcGVyLT5zdHlsZSgpLT5zZXRQYWRkaW5nVG9wKExlbmd0aChoZWlnaHREaWZmLCBGaXhl ZCkpOwogICAgICAgICAgICAgYmFzZVdyYXBwZXItPnNldE5lZWRzTGF5b3V0KHRydWUpOworICAg ICAgICAgICAgUmVuZGVyQmxvY2s6OmxheW91dCgpOwogICAgICAgICB9Ci0gICAgICAgIHNldE5l ZWRzTGF5b3V0KHRydWUpOwotICAgICAgICBSZW5kZXJCbG9jazo6bGF5b3V0KCk7CiAgICAgfSAg ICAKIH0KIAo=