As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Bug 685628 - mpadded: height/depth should apply to logical metrics
 Summary: mpadded: height/depth should apply to logical metrics
 Status: RESOLVED FIXED [mentor=fredw][lang=c++] helpwanted Core Components MathML (show other bugs) Trunk All All P5 normal (vote) mozilla28 Xuan Hu Anthony Jones (:kentuckyfriedtakahe, :k17e) mathml-in-mathjax Show dependency tree / graph

Reported: 2011-09-08 11:59 PDT by Frédéric Wang (:fredw)
Modified: 2013-11-19 10:30 PST (History)
5 users (show)
ryanvm: in‑testsuite+
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Attachments
Proposed Patch for Bug 685628 - mpadded: height/depth should apply to logical metrics (574 bytes, patch)
2012-05-13 12:21 PDT, Anjor
fred.wang: feedback+
Details | Diff | Splinter Review
First trial for bug #685628 (6.52 KB, patch)
2012-07-20 08:17 PDT, Xuan Hu
no flags Details | Diff | Splinter Review
Trial Patch (4.33 KB, patch)
2012-07-20 09:06 PDT, Xuan Hu
fred.wang: review-
fred.wang: feedback+
Details | Diff | Splinter Review
Bug 685628: Make mpadded apply height/depth to logical metrics. r=fredw,karlt (6.35 KB, patch)
2012-07-23 02:16 PDT, Xuan Hu
fred.wang: review-
fred.wang: feedback+
Details | Diff | Splinter Review
Bug 685628: Make mpadded apply height/depth to logical metrics. r=fredw f=karlt (6.33 KB, patch)
2012-07-23 05:19 PDT, Xuan Hu
fred.wang: review+
karlt: feedback+
Details | Diff | Splinter Review
685628-5.patch (6.31 KB, patch)
2012-07-24 04:31 PDT, Xuan Hu
fred.wang: review+
Details | Diff | Splinter Review
sixth try for bug fix 685628 (8.17 KB, patch)
2013-04-26 06:28 PDT, Xuan Hu
no flags Details | Diff | Splinter Review
bug-685628-fix-7 (11.53 KB, patch)
2013-04-29 02:28 PDT, Xuan Hu
fred.wang: feedback+
Details | Diff | Splinter Review
Diff of reftests (3.28 KB, text/plain)
2013-08-25 03:43 PDT, Xuan Hu
no flags Details
Diff of reftests - 2 (3.28 KB, text/plain)
2013-08-25 12:17 PDT, Xuan Hu
i: feedback?
Details
bug-685628-fix-8.patch (11.34 KB, patch)
2013-08-26 11:45 PDT, Xuan Hu
karlt: review+
Details | Diff | Splinter Review
bug-685628-fix-9.patch (11.34 KB, patch)
2013-11-11 09:59 PST, Xuan Hu
no flags Details | Diff | Splinter Review

 Frédéric Wang (:fredw) 2011-09-08 11:59:07 PDT In nsMathMLmpaddedFrame::Place, the height and depth of the mpadded element corresponds to ink metrics mBoundingMetrics.ascent and mBoundingMetrics.descent. It may be preferable to consider the logical metrics aDesiredSize.ascent and aDesiredSize.height - aDesiredSize.ascent instead. Anjor 2012-05-13 12:21:18 PDT Created attachment 623514 [details] [diff] [review] Proposed Patch for Bug 685628 - mpadded: height/depth should apply to logical metrics Frédéric Wang (:fredw) 2012-05-14 01:41:24 PDT Comment on attachment 623514 [details] [diff] [review] Proposed Patch for Bug 685628 - mpadded: height/depth should apply to logical metrics Review of attachment 623514 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for looking at this, Anjor. mpadded is described here: http://www.w3.org/TR/MathML/chapter3.html#presm.mpadded And our two kinds of metrics "logical" and "ink" discussed here: https://bugzilla.mozilla.org/show_bug.cgi?id=557474#c18 nsMathMLmpaddedFrame::Place has essentially three steps 1) Initialize nscoord height, depth with the inner frame metrics. 2) Calling UpdateValue to update these values. For example if has an attribute height="200%", then height should become twice larger. 3) Update the outer frame metrics with the new values. Currently your patch modifies 1) to use logical metrics instead of ink metrics. You have to do the same work for steps 2) and 3). I also suspect that you'll have to add an argument "const nsHTMLReflowMetrics& aDesiredSize" to the UpdateValue function in order to pass the logical metrics and use them to set the "scaler" variable. Frédéric Wang (:fredw) 2012-07-19 03:52:21 PDT Assigning to Hu Xuan. Xuan Hu 2012-07-20 08:17:59 PDT Created attachment 644348 [details] [diff] [review] First trial for bug #685628 A trial for bug fix as well as some redundant spaces removed. Frédéric Wang (:fredw) 2012-07-20 08:29:48 PDT (In reply to Xuan Hu from comment #4) > Created attachment 644348 [details] [diff] [review] > First trial for bug #685628 > > A trial for bug fix as well as some redundant spaces removed. Thank you, I'll look into this when I have time. FYI, Mozilla does not generally accept whitespace changes and clearly not when they are mixed with other changes of the code (that makes diff less friendly to review or to read in the mercurial history and these kinds of changes are more likely to conflict with others). Some of the lines in the MathML directory does not always respect Mozilla's coding style, but we leave them as they are unless we really modify the code. Xuan Hu 2012-07-20 09:06:19 PDT Created attachment 644361 [details] [diff] [review] Trial Patch Patch Updated removing changes related to whitespace. Frédéric Wang (:fredw) 2012-07-20 11:17:38 PDT Comment on attachment 644361 [details] [diff] [review] Trial Patch Review of attachment 644361 [details] [diff] [review]: ----------------------------------------------------------------- Thank you, that seems good. Can you please follow the instructions here: https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F https://developer.mozilla.org/En/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment I don't think your change will affect the horizontal metrics because mBoundingMetrics.width == aDesiredSize.width AFAIK. So you only need to write reftests for the vertical metrics. The idea is that some characters like _ have different ink (mBoundingMetrics) and logical metrics (aDesiredSize). Here the ink metrics is only the space occupied by the bar while the logical metrics is the whole rectangle occupied by the character. I think you can try to consider the rectangle $@2$ 1) with various values "10height" or "10depth" for @1 and various characters (like _, X etc) for @2. I guess the width should not depend on the character @2. 2) for a fixed width @1 (say "100px") and various @2. I guess the actual height+depth of the rectangle should not depend on the character @2. Currently, the results of 1) and 2) depend on @2. ::: layout/mathml/nsMathMLmpaddedFrame.cpp @@ +428,5 @@ > width - initialWidth - lspace : lspace; > > aDesiredSize.ascent += dy; > aDesiredSize.width = mBoundingMetrics.width; > + aDesiredSize.height += dy + depth - aDesiredSize.height + aDesiredSize.ascent; This is not correct because aDesiredSize.ascent has already been updated two lines above. Anyway, what we did here was forcing mBoundingMetrics.ascent = height mBoundingMetrics.descent = depth and updating aDesiredSize to keep the deltas between the old aDesiredSize/mBoundingMetrics vertical metrics. What we want now is forcing aDesiredSize to have the specified vertical metrics height/depth. Not sure what we should do for mBoundingMetrics... either set them to the same size or leave them alone. Karl? Xuan Hu 2012-07-23 02:16:45 PDT Created attachment 644874 [details] [diff] [review] Bug 685628: Make mpadded apply height/depth to logical metrics. r=fredw,karlt Bug fix according to fredw's suggestion. Still working on reftest. If there's any further problems, feel free to contact me. Frédéric Wang (:fredw) 2012-07-23 04:57:03 PDT Comment on attachment 644874 [details] [diff] [review] Bug 685628: Make mpadded apply height/depth to logical metrics. r=fredw,karlt Review of attachment 644874 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/mathml/nsMathMLmpaddedFrame.cpp @@ +428,5 @@ > width - initialWidth - lspace : lspace; > > aDesiredSize.ascent += dy; > aDesiredSize.width = mBoundingMetrics.width; > + aDesiredSize.height += depth - aDesiredSize.height + aDesiredSize.ascent; This is equivalent to the simpler syntax: aDesiredSize.height = depth + aDesiredSize.ascent; Xuan Hu 2012-07-23 05:19:56 PDT Created attachment 644900 [details] [diff] [review] Bug 685628: Make mpadded apply height/depth to logical metrics. r=fredw f=karlt Sorry for the silly error. Karl Tomlinson (back Feb 1 :karlt) 2012-07-23 20:37:17 PDT Comment on attachment 644900 [details] [diff] [review] Bug 685628: Make mpadded apply height/depth to logical metrics. r=fredw f=karlt (In reply to Frédéric Wang (:fredw) from comment #7) > What we want now is forcing aDesiredSize to have the specified vertical > metrics height/depth. Not sure what we should do for mBoundingMetrics... > either set them to the same size or leave them alone. Karl? If the height and/or depth are absolute, then it probably makes sense to set mBoundingMetrics to the same as aDesiredSize. If the change is relative, then it would be best to make the same relative change to mBoundingMetrics and aDesiredSize. It may be somewhat more complicated to handle both, so I think that can be considered a future improvement, and the patch is OK as is if Frédéric is fine with that. Frédéric Wang (:fredw) 2012-07-24 00:54:29 PDT Comment on attachment 644900 [details] [diff] [review] Bug 685628: Make mpadded apply height/depth to logical metrics. r=fredw f=karlt Review of attachment 644900 [details] [diff] [review]: ----------------------------------------------------------------- OK that sounds good. BTW, see also comment 557474 comment 18 for more information about ink/logical metrics. That could help you for reftests. ::: layout/mathml/nsMathMLmpaddedFrame.cpp @@ +422,5 @@ > mBoundingMetrics.width = width; > mBoundingMetrics.rightBearing = mBoundingMetrics.width; > } > > + nscoord dy = height - aDesiredSize.ascent; I think this variable is no longer needed. Just setting aDesiredSize.ascent = height below will work. r=me with this change. Xuan Hu 2012-07-24 04:31:17 PDT Created attachment 645256 [details] [diff] [review] 685628-5.patch Frédéric Wang (:fredw) 2012-07-24 06:33:40 PDT Comment on attachment 645256 [details] [diff] [review] 685628-5.patch Thanks. If you need to discuss a bit more reftests, don't hesitate. Xuan Hu 2012-08-16 02:37:04 PDT (In reply to Frédéric Wang (:fredw) from comment #7) > > I don't think your change will affect the horizontal metrics because > mBoundingMetrics.width == aDesiredSize.width AFAIK. So you only need to > write reftests for the vertical metrics. The idea is that some characters > like _ have different ink (mBoundingMetrics) and logical metrics > (aDesiredSize). Here the ink metrics is only the space occupied by the bar > while the logical metrics is the whole rectangle occupied by the character. > I think you can try to consider the rectangle > > $> > > @2 > > >$ > > 1) with various values "10height" or "10depth" for @1 and various characters > (like _, X etc) for @2. I guess the width should not depend on the character > @2. > 2) for a fixed width @1 (say "100px") and various @2. I guess the actual > height+depth of the rectangle should not depend on the character @2. > > Currently, the results of 1) and 2) depend on @2. > hi, Fredw, As you suggested in comment 7, I want to have two reftests about mpadded. The first is about the standalone width and the second is about the height. But I feel it hard to judge the equality between the specific dimension. For example, the two mpadded elements have same width but different height. After looking the doc of reftests, I found "script" as may achieve this but I don't ensure whether is proper to do so. Is there any alternative methods? Wish your further suggestion. Xuan Hu 2012-08-16 03:18:25 PDT Thx for karl's explanation. I found the reason of my mistake. I will be back after some progress. Frédéric Wang (:fredw) 2013-04-06 04:10:49 PDT Xuan, are you still working on this bug? Xuan Hu 2013-04-06 04:19:43 PDT Hi Fredw： Sorry, I have completely forget this. I want to finish it if there is no others instead. It's that ok? Frédéric Wang (:fredw) 2013-04-06 04:22:47 PDT (In reply to Xuan Hu from comment #18) > I want to finish it if there is no others instead. > It's that ok? Sure. I just wanted to be sure, but glad to see you're still interested. Josh Matthews [:jdm] 2013-04-24 12:15:22 PDT Xuan, could you confirm you're still working on this? Xuan Hu 2013-04-26 06:28:47 PDT Created attachment 742339 [details] [diff] [review] sixth try for bug fix 685628 Sorry for the late coming patch. Just busy with school matters as well as appling for GSoC 2013 If there is any problem. feel free to tell me. :-) Josh Matthews [:jdm] 2013-04-26 06:40:28 PDT Comment on attachment 742339 [details] [diff] [review] sixth try for bug fix 685628 Review of attachment 742339 [details] [diff] [review]: ----------------------------------------------------------------- I'll let Fred handle this, since I know nothing about this code. Xuan Hu 2013-04-29 02:28:54 PDT Created attachment 742969 [details] [diff] [review] bug-685628-fix-7 Some minor error fixed compared to last uploaded patch. Should work now both with compile and reftests. If there is anything wrong, feel free to tell me. :-) Frédéric Wang (:fredw) 2013-05-11 09:16:20 PDT Now I'm wondering if that's a good idea to make width depends on height. That's a bit in conflict with the CSS model and I'm not sure mpadded works very well at the moment. So could you consider something like height="10height" instead? I think for "X" and "_" depth = 0, so you only want to consider the height. E.g $_$ Can you check if the test fails before and passes after. Frédéric Wang (:fredw) 2013-07-28 00:18:02 PDT Comment on attachment 742969 [details] [diff] [review] bug-685628-fix-7 See comment 24. Frédéric Wang (:fredw) 2013-07-28 00:26:37 PDT So my point was that you can keep mpadded-9.html but replace -8 and -7 by tests where depth or height are expressed in term of width, height or depth ; or where width is expressed in term of width (not in term of height or depth). For example the test I gave in comment 24 (height in term of height) but you may try to find other tests. Xuan Hu 2013-08-11 00:02:46 PDT OK, I will deal with it when I have time. Currently I am busy with another GSoC project. Sorry for the delay. :-( Xuan Hu 2013-08-25 03:43:07 PDT Created attachment 795138 [details] Diff of reftests Hi, fredw, From the above comments, I think there should be three reftests. - height in term of height should not depend on the characters - height in term of width should not depend on the characters - width in term of width should not depend on the characters By following these three items I created the reftests as in the attachments. But I feel a little confused about the usage of reftests, cause with the reftest.list  == mpadded-7.html mpadded-7-ref.html == mpadded-8.html mpadded-8-ref.html == mpadded-9.html mpadded-9-ref.html  all test failed with following log information:  0:07.77 reftest failed: 0:07.77 REFTEST TEST-UNEXPECTED-FAIL | WARNING: USE_WIDGET_LAYERS disabled 0:07.77 reference to undefined property options.bytesREFTEST TEST-UNEXPECTED-FAIL | file:///Users/huxuan/Code/mozilla-central/layout/reftests/mathml/mpadded-7.html | image comparison (==), max difference: 255, number of differing pixels: 9190 0:07.77 REFTEST TEST-UNEXPECTED-FAIL | file:///Users/huxuan/Code/mozilla-central/layout/reftests/mathml/mpadded-8.html | image comparison (==), max difference: 255, number of differing pixels: 7008 0:07.77 REFTEST TEST-UNEXPECTED-FAIL | file:///Users/huxuan/Code/mozilla-central/layout/reftests/mathml/mpadded-9.html | image comparison (==), max difference: 255, number of differing pixels: 2541  I am not sure where is wrong, wish you can give me some help. Frédéric Wang (:fredw) 2013-08-25 04:40:05 PDT I guess for 8 and 9, you want something to compare monospace characters like "|" and "_" that clearly have different ink widths but (I think) equal logical widths. Your tests are supposed to fail before your C++ patch is applied and to pass after it is applied (so be sure to have your C++ patch applied and to have built Firefox again before running the reftests). You can compare the results "by eye": open two mpadded-*.html and mpadded-*-ref.html into two Firefox tabs and switch between these two tabs. You should not see any difference. You can also open source/layout/tools/reftest/reftest-analyzer.xhtml and copy and paste the test results into it. This can help you to see the differences. It's possible that the width/height of the characters used in your tests are not exactly equal and the difference will become even larger when multiplied by 100. In that case the reftests won't work :-( Xuan Hu 2013-08-25 12:17:26 PDT Created attachment 795179 [details] Diff of reftests - 2 Hi, fredw, Sorry for the silly error, I just forget to rebuild firefox before reftests. :-) I have successfully make the reftests work as the diff in the attachment. But I still have two questions, The first one is that reftests still report some error with following log:  0:07.32 REFTEST INFO | runreftest.py | Running tests: end. 0:07.34 reftest failed: 0:07.34 REFTEST TEST-UNEXPECTED-FAIL | WARNING: USE_WIDGET_LAYERS disabled 0:07.34 make: *** [reftest] Error 1  The second one is that in reftest -8 & -9, character _ is wider than X and |, which means when I change one of the character to _, corresponding reftest will fail. It's so strange, isn't it? Frédéric Wang (:fredw) 2013-08-25 12:57:51 PDT (In reply to Xuan Hu from comment #30) > 0:07.32 REFTEST INFO | runreftest.py | Running tests: end. > 0:07.34 reftest failed: > 0:07.34 REFTEST TEST-UNEXPECTED-FAIL | WARNING: USE_WIDGET_LAYERS disabled > 0:07.34 make: *** [reftest] Error 1 Are you sure this error comes from the mpadded-7? This seems unrelated to your test, something like a compilation option disabled on your set up... >  > > The second one is that in reftest -8 & -9, character _ is wider than X > and |, which means when I change one of the character to _, > corresponding reftest will fail. It's so strange, isn't it? Yes, that's weird. I was not quite sure that it would be the case but I expected that monospace fonts use the same logical widths for all characters. I think you can keep '|' and 'X' if that works. It would be interesting to send your patches to the TryServer to see if the tests pass on all platforms: https://wiki.mozilla.org/Build:TryServer Karl Tomlinson (back Feb 1 :karlt) 2013-08-25 17:23:43 PDT (In reply to Xuan Hu from comment #30) > 0:07.32 REFTEST INFO | runreftest.py | Running tests: end. > 0:07.34 reftest failed: > 0:07.34 REFTEST TEST-UNEXPECTED-FAIL | WARNING: USE_WIDGET_LAYERS disabled > 0:07.34 make: *** [reftest] Error 1 I don't think you need to worry about that. It might be something like your screen not having enough lines to run the tests in a particular way, but that is not important for these tests. Xuan Hu 2013-08-26 11:45:32 PDT Created attachment 795562 [details] [diff] [review] bug-685628-fix-8.patch This is the new version of patch. If there is anything wrong, feel free to tell me. Xuan Hu 2013-08-26 11:48:36 PDT Hi, fredw (In reply to Frédéric Wang (:fredw) from comment #31) > It would be interesting to send your patches to the TryServer to see if the > tests pass on all platforms: https://wiki.mozilla.org/Build:TryServer I tried to use TryServer according to the wiki page, but I found that I have no permission to it. The log just looks like the following:  \$ hg push -f ssh://hg.mozilla.org/try/ pushing to ssh://hg.mozilla.org/try/ remote: Permission denied (publickey). abort: no suitable response from remote hg!  Xuan Hu 2013-08-27 20:20:18 PDT Hi, fredw & karl I have apply for level 1 access of try server via Bug 910070. Need one voucher - any other user with level 2 or above access. Wish you can help me. :-) Frédéric Wang (:fredw) 2013-09-15 00:47:11 PDT Xuan Hu: did you try to send your patch to the try server? Xuan Hu 2013-10-09 02:53:18 PDT Hi, Fred Wang I have subbmit the patch to try server The result can be found here: https://tbpl.mozilla.org/?tree=Try&rev=9dccfd92fcfc Builds and logs can be found here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/i@huxuan.org-9dccfd92fcfc There seems to have some errors, any suggestions for debuging that? Just feel a little hard to start off. Frédéric Wang (:fredw) 2013-10-11 14:08:06 PDT (In reply to Xuan Hu from comment #37) > There seems to have some errors, any suggestions for debuging that? Just > feel a little hard to start off. These errors don't seem to be related to your changes. Karl Tomlinson (back Feb 1 :karlt) 2013-10-20 17:41:59 PDT Comment on attachment 795562 [details] [diff] [review] bug-685628-fix-8.patch Thank you for your contribution here. Frédéric Wang (:fredw) 2013-10-26 01:46:33 PDT Xuan, can you please finish the work here (check that the patch still applies on trunk and set 'checkin needed')? Xuan Hu 2013-11-11 09:59:07 PST Created attachment 830264 [details] [diff] [review] bug-685628-fix-9.patch Hi, fredw, I created a new patch after apply previous version of patch. It seems to work fine and it should be in checkin needed. If there is any problem, feel free to tell me. Frédéric Wang (:fredw) 2013-11-19 00:33:54 PST @Xuan: If you didn't do any other change, please make "obsolete" all but the latest version and change the keyword to checkin needed. Frédéric Wang (:fredw) 2013-11-19 01:37:46 PST I don't have level access 3 to commit the patch. Xuan Hu 2013-11-19 01:40:01 PST Hi, fredw. Sorry for misunderstanding your mean by keyword checkin needed, thx for your help. Ryan VanderMeulen [:RyanVM] 2013-11-19 06:32:48 PST https://hg.mozilla.org/integration/mozilla-inbound/rev/07b7146e0b7d Ryan VanderMeulen [:RyanVM] 2013-11-19 10:30:35 PST https://hg.mozilla.org/mozilla-central/rev/07b7146e0b7d`

 Note You need to log in before you can comment on or make changes to this bug.