Bug 180029

Created attachment 327624 [details] Patch This patch demonstrates how mspace-units.html fails for zoomlevel=3. This gives a way to write a test for this bug.

@Minsheng: Did you get a chance to look into this? I suspect it should be relatively easy to address since it's just one function to modify and the test is already almost written ;-)

Created attachment 331279 [details] patch

Comment on attachment 331279 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=331279&action=review > LayoutTests/ChangeLog:10 > + * mathml/presentation/mspace-units-expected.html: Remove the call to setPageZoomFactor(). mspace-units does not have setPageZoomFactor() and I believe the test should actually *not* be modified as it is not affected by this patch (and please don't do whitespace only change as that makes the history more complicated). > LayoutTests/mathml/presentation/mspace-units-with-zoom-expected.html:8 > + } That's interesting and seems better than using window.internals. It's the only difference you make with respect to the mspace-units.html test, right? > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:173 > + Mmh, that seems complicated. Why don't you just do "* style.effectiveZoom()" in the 6 cases below? That would make things more readable.

(In reply to Frédéric Wang (:fredw) from comment #4) > Comment on attachment 331279 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331279&action=review > > > LayoutTests/ChangeLog:10 > > + * mathml/presentation/mspace-units-expected.html: Remove the call to setPageZoomFactor(). > > mspace-units does not have setPageZoomFactor() and I believe the test should > actually *not* be modified as it is not affected by this patch (and please > don't do whitespace only change as that makes the history more complicated). > > > LayoutTests/mathml/presentation/mspace-units-with-zoom-expected.html:8 > > + } > > That's interesting and seems better than using window.internals. It's the > only difference you make with respect to the mspace-units.html test, right? Oops! I mistook the file you provided on this page with the one we have in the repository. Note that the old test does not check the unit "mu". I agree you with that a significant change to whitespace is not history-friendly, something I have not thought of before. So I will submit a separate patch to amend that test later. For this patch, I will only include the new mspace-units-with-zoom. And yes, other than the CSS property "zoom" and the check for "mu", there is nothing new. > > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:173 > > + > > Mmh, that seems complicated. Why don't you just do "* style.effectiveZoom()" > in the 6 cases below? That would make things more readable. I had a hard time deciding between code duplication and readability. If we are using Rust or other switch-as-expression languages, I will try to argue. However, since I am duplicating lots of "break" anyway, I agree with your proposal.

Comment on attachment 331279 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=331279&action=review >>> LayoutTests/ChangeLog:10 >>> + * mathml/presentation/mspace-units-expected.html: Remove the call to setPageZoomFactor(). >> >> mspace-units does not have setPageZoomFactor() and I believe the test should actually *not* be modified as it is not affected by this patch (and please don't do whitespace only change as that makes the history more complicated). > > Oops! I mistook the file you provided on this page with the one we have in the repository. > > Note that the old test does not check the unit "mu". I agree you with that a significant change to whitespace is not history-friendly, something I have not thought of before. So I will submit a separate patch to amend that test later. For this patch, I will only include the new mspace-units-with-zoom. > > And yes, other than the CSS property "zoom" and the check for "mu", there is nothing new. OK, I did not notice your addition of "mu" (that's another reason not to do whitespace-only changes as that makes review much harder). Note that "mu" is not a CSS property nor a MathML unit. It's just an internal property based on LaTeX to represent MathML's namedspace (see Source/WebCore/mathml/MathMLElement.h). So I don't think you need to test it but if you really want to, you should use namespaces like "verythickmathspace" not "mu".

(In reply to Frédéric Wang (:fredw) from comment #6) > Comment on attachment 331279 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331279&action=review > > >>> LayoutTests/ChangeLog:10 > >>> + * mathml/presentation/mspace-units-expected.html: Remove the call to setPageZoomFactor(). > >> > >> mspace-units does not have setPageZoomFactor() and I believe the test should actually *not* be modified as it is not affected by this patch (and please don't do whitespace only change as that makes the history more complicated). > > > > Oops! I mistook the file you provided on this page with the one we have in the repository. > > > > Note that the old test does not check the unit "mu". I agree you with that a significant change to whitespace is not history-friendly, something I have not thought of before. So I will submit a separate patch to amend that test later. For this patch, I will only include the new mspace-units-with-zoom. > > > > And yes, other than the CSS property "zoom" and the check for "mu", there is nothing new. > > OK, I did not notice your addition of "mu" (that's another reason not to do > whitespace-only changes as that makes review much harder). Note that "mu" is > not a CSS property nor a MathML unit. It's just an internal property based > on LaTeX to represent MathML's namedspace (see > Source/WebCore/mathml/MathMLElement.h). So I don't think you need to test it > but if you really want to, you should use namespaces like > "verythickmathspace" not "mu". I will not add it then.

Created attachment 331282 [details] Patch

Note that I reorder the switch-block to put physical units together. I hope this could improve readability a bit.

Comment on attachment 331282 [details] Patch Yes, I think that's better. Thanks!

Comment on attachment 331282 [details] Patch Clearing flags on attachment: 331282 Committed r226936: <https://trac.webkit.org/changeset/226936>

All reviewed patches have been landed. Closing bug.

<rdar://problem/36495027>