155406 2016-03-13 04:38:32 -0700 MathMLMencloseElement style should not depend on rendering 2016-07-07 08:42:44 -0700 1 1 1 Unclassified WebKit MathML WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 155298 1 koivisto webkit-unassigned commit-queue darin dbarton fred.wang mrobinson oldest_to_newest 1174179 0 koivisto 2016-03-13 04:38:32 -0700 Style shouldn't depend on rendering. 1174180 1 273889 koivisto 2016-03-13 04:43:05 -0700 Created attachment 273889 patch 1174181 2 fred.wang 2016-03-13 04:52:49 -0700 See also the complete refactoring in bug 155019, that will probably make this unnecessary. 1174184 3 koivisto 2016-03-13 05:57:27 -0700 Yeah, looks much better. We could probably just disable mathml/presentation/menclose-notation-attribute-change-value.html until that lands. 1174185 4 fred.wang 2016-03-13 06:03:33 -0700 (In reply to comment #3) > Yeah, looks much better. We could probably just disable > mathml/presentation/menclose-notation-attribute-change-value.html until that > lands. Yes, I guess we can do it either way. Just disabling that test is probably the easiest way so that it does not block the work on bug 155298. 1174226 5 273889 darin 2016-03-13 14:31:44 -0700 Comment on attachment 273889 patch View in context: https://bugs.webkit.org/attachment.cgi?id=273889&action=review > Source/WebCore/mathml/MathMLMencloseElement.cpp:134 > String closingBrace(")", String::ConstructFromLiteral); > TextRun run(closingBrace); We do not need a String to construct a TextRun, which takes a StringView. We can write something more like this: LChar rightParenthesis = ')'; TextRun run({ &rightParenthesis, 1 }); I took the liberty of calling this what the Unicode standard calls it, rather than calling it "closing brace", which seems like it could be the name of some other characters. > Source/WebCore/mathml/MathMLMencloseElement.cpp:136 > + const FontCascade& font = parentStyle.fontCascade(); > + padding.appendNumber(font.width(run)); I don’t think we need a local variable when we are just using it once. In fact, I would write this: LChar rightParenthesis = ')'; padding.appendNumber(parentStyle.fontCascade().width(TextRun{ &rightParenthesis, 1 })); Might need an extra type name or set of curly brackets in there, but something like that would work. The version with more lines of code seems no more clear to me. > Source/WebCore/mathml/MathMLMencloseElement.cpp:143 > +String MathMLMencloseElement::longDivLeftPadding() const This could return a const String& instead of a String to avoid unneeded reference count churn. 1174227 6 darin 2016-03-13 14:32:14 -0700 Oh, I see, this code is going away. Oops, my comments are overkill. 1208627 7 fred.wang 2016-07-07 08:42:44 -0700 Resolving since this was fixed by https://trac.webkit.org/changeset/199980 273889 2016-03-13 04:43:05 -0700 2016-03-13 13:08:09 -0700 patch mathml-longdivleftpadding.patch text/plain 3646 koivisto SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE5ODA3OCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIxIEBACisyMDE2LTAzLTEzICBBbnR0aSBL b2l2aXN0byAgPGFudHRpQGFwcGxlLmNvbT4KKworICAgICAgICBNYXRoTUxNZW5jbG9zZUVsZW1l bnQgc3R5bGUgc2hvdWxkIG5vdCBkZXBlbmQgb24gcmVuZGVyaW5nCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNTU0MDYKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIG1hdGhtbC9NYXRoTUxNZW5jbG9zZUVs ZW1lbnQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6TWF0aE1MTWVuY2xvc2VFbGVtZW50OjpNYXRo TUxNZW5jbG9zZUVsZW1lbnQpOgorICAgICAgICAoV2ViQ29yZTo6TWF0aE1MTWVuY2xvc2VFbGVt ZW50OjpjcmVhdGUpOgorICAgICAgICAoV2ViQ29yZTo6TWF0aE1MTWVuY2xvc2VFbGVtZW50Ojpj b2xsZWN0U3R5bGVGb3JQcmVzZW50YXRpb25BdHRyaWJ1dGUpOgorICAgICAgICAoV2ViQ29yZTo6 TWF0aE1MTWVuY2xvc2VFbGVtZW50OjpyZXNvbHZlQ3VzdG9tU3R5bGUpOgorCisgICAgICAgICAg ICBNYWtlIHRoZSBsb25nRGl2TGVmdFBhZGRpbmcgc3RyaW5nIGluIHRoZSBjdXN0b20gc3R5bGUg Y2FsbGJhY2sgaW5zdGVhZCBvZiBwb2tpbmcgdGhlIHJlbmRlciB0cmVlLgorCisgICAgICAgIChX ZWJDb3JlOjpNYXRoTUxNZW5jbG9zZUVsZW1lbnQ6OmxvbmdEaXZMZWZ0UGFkZGluZyk6CisgICAg ICAgICogbWF0aG1sL01hdGhNTE1lbmNsb3NlRWxlbWVudC5oOgorCiAyMDE2LTAzLTEyICBNeWxl cyBDLiBNYXhmaWVsZCAgPG1tYXhmaWVsZEBhcHBsZS5jb20+CiAKICAgICAgICAgW0NvY29hXSBS ZW1vdmUgdHlwZWRlZiBmcm9tIE5TU2Nyb2xsZXJJbXAgdG8gU2Nyb2xsYmFyUGFpbnRlcgpJbmRl eDogU291cmNlL1dlYkNvcmUvbWF0aG1sL01hdGhNTE1lbmNsb3NlRWxlbWVudC5jcHAKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gU291cmNlL1dlYkNvcmUvbWF0aG1sL01hdGhNTE1lbmNsb3NlRWxlbWVudC5jcHAJ KHJldmlzaW9uIDE5ODA2MSkKKysrIFNvdXJjZS9XZWJDb3JlL21hdGhtbC9NYXRoTUxNZW5jbG9z ZUVsZW1lbnQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zMiw2ICszMiw3IEBACiAjaW5jbHVkZSAi UmVuZGVyRWxlbWVudC5oIgogI2luY2x1ZGUgIlJlbmRlck1hdGhNTE1lbmNsb3NlLmgiCiAjaW5j bHVkZSAiUmVuZGVyT2JqZWN0LmgiCisjaW5jbHVkZSAiU3R5bGVSZXNvbHZlci5oIgogI2luY2x1 ZGUgIlRleHRSdW4uaCIKICNpbmNsdWRlIDx3dGYvUmVmLmg+CiAjaW5jbHVkZSA8d3RmL3RleHQv U3RyaW5nQnVpbGRlci5oPgpAQCAtNDMsNiArNDQsNyBAQCBNYXRoTUxNZW5jbG9zZUVsZW1lbnQ6 Ok1hdGhNTE1lbmNsb3NlRWxlCiAgICAgOiBNYXRoTUxJbmxpbmVDb250YWluZXJFbGVtZW50KHRh Z05hbWUsIGRvY3VtZW50KQogICAgICwgbV9pc1JhZGljYWxWYWx1ZShmYWxzZSkKIHsKKyAgICBz ZXRIYXNDdXN0b21TdHlsZVJlc29sdmVDYWxsYmFja3MoKTsKIH0KIAogUmVmPE1hdGhNTE1lbmNs b3NlRWxlbWVudD4gTWF0aE1MTWVuY2xvc2VFbGVtZW50OjpjcmVhdGUoY29uc3QgUXVhbGlmaWVk TmFtZSYgdGFnTmFtZSwgRG9jdW1lbnQmIGRvY3VtZW50KQpAQCAtMTI1LDE5ICsxMjcsMjIgQEAg dm9pZCBNYXRoTUxNZW5jbG9zZUVsZW1lbnQ6OmNvbGxlY3RTdHlsZQogICAgICAgICBNYXRoTUxJ bmxpbmVDb250YWluZXJFbGVtZW50Ojpjb2xsZWN0U3R5bGVGb3JQcmVzZW50YXRpb25BdHRyaWJ1 dGUobmFtZSwgdmFsdWUsIHN0eWxlKTsKIH0KIAotCi1TdHJpbmcgTWF0aE1MTWVuY2xvc2VFbGVt ZW50Ojpsb25nRGl2TGVmdFBhZGRpbmcoKSBjb25zdAorT3B0aW9uYWw8RWxlbWVudFN0eWxlPiBN YXRoTUxNZW5jbG9zZUVsZW1lbnQ6OnJlc29sdmVDdXN0b21TdHlsZShSZW5kZXJTdHlsZSYgcGFy ZW50U3R5bGUsIFJlbmRlclN0eWxlKikKIHsKICAgICBTdHJpbmdCdWlsZGVyIHBhZGRpbmc7CiAg ICAgU3RyaW5nIGNsb3NpbmdCcmFjZSgiKSIsIFN0cmluZzo6Q29uc3RydWN0RnJvbUxpdGVyYWwp OwogICAgIFRleHRSdW4gcnVuKGNsb3NpbmdCcmFjZSk7Ci0gICAgTm9kZSogbm9kZSA9IHBhcmVu dE5vZGUoKTsKLSAgICBpZiAobm9kZSAmJiBub2RlLT5yZW5kZXJlcigpKSB7Ci0gICAgICAgIGNv bnN0IEZvbnRDYXNjYWRlJiBmb250ID0gbm9kZS0+cmVuZGVyZXIoKS0+c3R5bGUoKS5mb250Q2Fz Y2FkZSgpOwotICAgICAgICBwYWRkaW5nLmFwcGVuZE51bWJlcihmb250LndpZHRoKHJ1bikpOwot ICAgICAgICBwYWRkaW5nLmFwcGVuZExpdGVyYWwoInB4Iik7Ci0gICAgfQotICAgIHJldHVybiBw YWRkaW5nLnRvU3RyaW5nKCk7CisgICAgY29uc3QgRm9udENhc2NhZGUmIGZvbnQgPSBwYXJlbnRT dHlsZS5mb250Q2FzY2FkZSgpOworICAgIHBhZGRpbmcuYXBwZW5kTnVtYmVyKGZvbnQud2lkdGgo cnVuKSk7CisgICAgcGFkZGluZy5hcHBlbmRMaXRlcmFsKCJweCIpOworICAgIG1fbG9uZ0Rpdkxl ZnRQYWRkaW5nID0gcGFkZGluZy50b1N0cmluZygpOworCisgICAgcmV0dXJuIE51bGxvcHQ7Cit9 CisKK1N0cmluZyBNYXRoTUxNZW5jbG9zZUVsZW1lbnQ6OmxvbmdEaXZMZWZ0UGFkZGluZygpIGNv bnN0Cit7CisgICAgcmV0dXJuIG1fbG9uZ0RpdkxlZnRQYWRkaW5nOwogfQogCiB9CkluZGV4OiBT b3VyY2UvV2ViQ29yZS9tYXRobWwvTWF0aE1MTWVuY2xvc2VFbGVtZW50LmgKPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gU291cmNlL1dlYkNvcmUvbWF0aG1sL01hdGhNTE1lbmNsb3NlRWxlbWVudC5oCShyZXZpc2lv biAxOTgwNjEpCisrKyBTb3VyY2UvV2ViQ29yZS9tYXRobWwvTWF0aE1MTWVuY2xvc2VFbGVtZW50 LmgJKHdvcmtpbmcgY29weSkKQEAgLTQ1LDkgKzQ1LDExIEBAIHByaXZhdGU6CiAgICAgYm9vbCBp c1ByZXNlbnRhdGlvbkF0dHJpYnV0ZShjb25zdCBRdWFsaWZpZWROYW1lJikgY29uc3Qgb3ZlcnJp ZGU7CiAgICAgdm9pZCBjb2xsZWN0U3R5bGVGb3JQcmVzZW50YXRpb25BdHRyaWJ1dGUoY29uc3Qg UXVhbGlmaWVkTmFtZSYsIGNvbnN0IEF0b21pY1N0cmluZyYsIE11dGFibGVTdHlsZVByb3BlcnRp ZXMmKSBvdmVycmlkZTsKICAgICB2b2lkIGZpbmlzaFBhcnNpbmdDaGlsZHJlbigpIG92ZXJyaWRl OworICAgIE9wdGlvbmFsPEVsZW1lbnRTdHlsZT4gcmVzb2x2ZUN1c3RvbVN0eWxlKFJlbmRlclN0 eWxlJiBwYXJlbnRTdHlsZSwgUmVuZGVyU3R5bGUqIHNoYWRvd0hvc3RTdHlsZSkgb3ZlcnJpZGU7 CiAKICAgICBWZWN0b3I8U3RyaW5nPiBtX25vdGF0aW9uVmFsdWVzOwogICAgIGJvb2wgbV9pc1Jh ZGljYWxWYWx1ZTsKKyAgICBTdHJpbmcgbV9sb25nRGl2TGVmdFBhZGRpbmc7CiB9OwogCiB9Cg==