Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
263363005:
Initial patch for FontVariant, variant like HalfWidth should take Complex Path (Closed)
|
Created:
6 years, 7 months ago by h.joshi Modified:
6 years, 4 months ago CC:
blink-reviews, jamesr, krit, dsinclair, jbroman, danakj, Rik, Stephen Chennney, pdr., rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@emoji_android Visibility:
Public. |
DescriptionInitial patch for FontVariant, this is to support HalfWidth,
ThirdWidth, QuarterWidth and will take complex path
while rendering.
BUG=350404
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180241
Patch Set 1 #
Total comments: 2
Patch Set 2 : Adding test case based on testharness.js #
Total comments: 7
Patch Set 3 : Updated test case #
Total comments: 2
Patch Set 4 : Comment fix #Patch Set 5 : Updating test case #
Total comments: 16
Patch Set 6 : Comment fix and adding Font file #
Total comments: 20
Patch Set 7 : Updated Test case #Patch Set 8 : Rebase files #Patch Set 9 : Updating Test Expectation #Patch Set 10 : Updating Test Expectation #Messages
Total messages: 62 (0 generated)
Initial patch, PTAL
The code itself looks alright to me, with a nit about the comment. Note that I am not an owner yet, and I can only give a preliminary review. Please provide a test cases which explicitly exercises several example (as mentioned in my previous comment on the bug: Several combinations of wide digits, like 5, and narrow digits, like 1, etc. Pairs, triples, and combinations of 4 digits etc. Can you try looking for an open source licensed font which has width variants so that we see at least three of the cases hit? Regular + two smaller ones? We need a permissive license for inclusion in the repository. How about Droid Sans? Or the Ubuntu fonts? You probably have better chances finding one when you search for fonts which supports CJK, since the combined rendering is mostly used in vertical text. Google Fonts might make the search easier. https://codereview.chromium.org/263363005/diff/1/Source/platform/fonts/harfbu... File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right): https://codereview.chromium.org/263363005/diff/1/Source/platform/fonts/harfbu... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:534: // to keep compiler happy Please remove this comment.
Will check Droid Sans and in case that does not work will try to find some open source font and create new test case https://codereview.chromium.org/263363005/diff/1/Source/platform/fonts/harfbu... File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right): https://codereview.chromium.org/263363005/diff/1/Source/platform/fonts/harfbu... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:534: // to keep compiler happy Okey, will remove this in next patch On 2014/05/09 07:52:18, Dominik Röttsches wrote: > Please remove this comment.
Not able to find any ope source font which has all three variants present, found one Adobe Open Source project font but that was only having Half Width. Droid Sans does not have any variant present. Would require some help or suggestion to write better test case so that this patch can be landed. On 2014/05/09 08:48:25, h.joshi wrote: > Will check Droid Sans and in case that does not work will try to find some open > source font and create new test case > > https://codereview.chromium.org/263363005/diff/1/Source/platform/fonts/harfbu... > File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right): > > https://codereview.chromium.org/263363005/diff/1/Source/platform/fonts/harfbu... > Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:534: // to keep compiler happy > Okey, will remove this in next patch > > On 2014/05/09 07:52:18, Dominik Röttsches wrote: > > Please remove this comment.
On 2014/05/10 01:50:26, h.joshi wrote: > Not able to find any ope source font which has all three variants present, found > one Adobe Open Source project font but that was only having Half Width. Droid > Sans does not have any variant present. > > Would require some help or suggestion to write better test case so that this > patch can be landed. In terms of the test case, I imagine something like: You could test and measure in JS (and create a test based on testharness.js) the vertical length of a text block to see whether a numeral pair, triplet or group of four respectively is broken into two blocks vertically, or squeezed into one, as we would expect, then pass when it fits in one block, and fail if it doesn't. In terms of the font used: Tricky indeed. Any of those marked with [F] here perhaps? http://en.wikipedia.org/wiki/List_of_CJK_fonts One other option is to create a font yourself, just with digits and their reduced width substitutions: I briefly looked at Hiragino Mincho Pro in FontForge and there are the qwid and twid substitions in it (if you right click, then go to glyph properties on one of the digits, and check the substituions table). But it seems like FontForge can't edit this one. However, there might be a way to create a test font in FontForge which contains such tables. This would allow testing with a custom font on multiple platforms when the font is added to content_shell (like https://codereview.chromium.org/206253006/). Or you might find another tool which allows you to do that. If it's not at all possible to create a custom one: For each platform, you could try to find a font which has at least one feature (half width, third with, quarter width) that's reliably available on the bots, then write a test case like sketched above and reference the system font in CSS. If such a font can't be found on Linux, I imagine this is probably possible for Windows at least. And on Mac as soon as we have HarfBuzz there (=>Hiragino). Would be great if see such a test case like above at least pass for all width cases on one platform, if it partially fails on others (i.e. fails for third width and quarter width case if the font doesn't support it, but half width works), we can store a corresponding platform specific expected text result in LayoutTests.
In the link shared I found "ipaexm.ttf" under "IPA Minchō", can we put this font under "WebKit/LayoutTests/resources" and use the same in our test case as "typeface" by "font-face" property? Pls Suggest
Similar is used in "font-face-woff.html" (LayoutTests/fast/css/font-face-woff.html), font "ipaexm.ttf" has HalfWidth property present. On 2014/05/12 09:58:42, h.joshi wrote: > In the link shared I found "ipaexm.ttf" under "IPA Minchō", can we put this font > under "WebKit/LayoutTests/resources" and use the same in our test case as > "typeface" by "font-face" property? > Pls Suggest
On 2014/05/12 10:03:29, h.joshi wrote: > Similar is used in "font-face-woff.html" > (LayoutTests/fast/css/font-face-woff.html), font "ipaexm.ttf" has HalfWidth > property present. If you don't find a better one with the other features - as I explained - yes, you can use this one. I am not sure the Half Width table is getting through the font sanitizer. Try it out. Happy to review a patch with a test, as I outlined before.
Added new patch with test case based on testharness.js, new to using testharness and facing few problem require some help. When running test case with "content_shell" it shows "PASS" on screen "PASS Testing that webkit-text-combine is applied", but with running with "run-webkit-test" script it shows "assert failed" command prompt, not able to get what went wrong here. Need suggestion to fix this.
File "CSSHWOrientationTest.ttf " is taken from "http://sourceforge.net/projects/csso9ntestfonts.adobe/" it has Half Width variant information present, it does not have actual numeric glyphs, all glyphs are drawn as triangles.
Thanks for the update. However, in my opinion, the patch needs a lot more work. The font you found seems to be a good start. Please reconsider my suggestions on how to write the test, which I sent in earlier messages. Currently, you're only testing one case, "90". How about differently sized digits (1 vs 5), different lengths of digit combinations: 1 digits, pairs, triplets, groups of 4 etc. I am not sure you intended to upload this for review. If you consider it work in progress, please indicate it as such. I can't debug the different result for you between run-webkit-tests and content_shell. You might want to add additional asserts to see where your failure happens, or run it as a pixel test to see the rendering when run in run-webkit-tests. https://codereview.chromium.org/263363005/diff/40001/LayoutTests/fast/text/fo... File LayoutTests/fast/text/fontWidthVariant.html (right): https://codereview.chromium.org/263363005/diff/40001/LayoutTests/fast/text/fo... LayoutTests/fast/text/fontWidthVariant.html:1: <!DOCTYPE HTML> Please change the file name to be consistent with surrounding file names. Lower case with hyphens. The patch is missing expected result for the new test case. https://codereview.chromium.org/263363005/diff/40001/LayoutTests/fast/text/fo... LayoutTests/fast/text/fontWidthVariant.html:10: src: url(../../resources/CSSHWOrientationTest.ttf) format("truetype"); Please take a look at LayoutTests/third_party/Libertine for an example on how to add a new third party font. Make a new separate patch about adding the font, please add dpranke as reviewer. You need to add a license file and a readme file with justification why the font is needed. https://codereview.chromium.org/263363005/diff/40001/LayoutTests/fast/text/fo... LayoutTests/fast/text/fontWidthVariant.html:32: function updateQuery() { Why "updateQuery()?" - Please think about better naming. https://codereview.chromium.org/263363005/diff/40001/LayoutTests/fast/text/fo... LayoutTests/fast/text/fontWidthVariant.html:36: var theCSSprop_1 = spanElement.offsetHeight; Please clean up code before uploading to make it easier for reviewers to understand what you're trying to do. 1) Remove commented code. 2) Remove debugging code. 3) Fix indentation. The logic is indeed one way to test whether there is a difference between the height before and after applying the class. But we don't know into how many blocks. Also, you're only testing one combination of digits. Please follow my instructions on how to design this test more closely. https://codereview.chromium.org/263363005/diff/40001/LayoutTests/fast/text/fo... LayoutTests/fast/text/fontWidthVariant.html:47: "New query doesn't match, property not applied.") Needs a better assert failure description. The property is applied, but its rendering is incorrect - is probably what you mean here. https://codereview.chromium.org/263363005/diff/40001/LayoutTests/fast/text/fo... LayoutTests/fast/text/fontWidthVariant.html:58: <span>This test verifies that webkit-text-combin is applied.</span> Please spell the CSS property correctly, including the hyphen. What we're testing is not exactly that the property is applied, but that the applied property renders visually correctly. Testing whether the property is applied would mean that we're testing CSS style resolution etc. But we know that this part works. What we're really testing here is whether the rendering is correct. For this, I don't think you need a dynamic test that compares the property before and after. You can apply the -webkit-text-combine CSS from the start, and then just measure the height and compare it to the font size. Here: If it's closer to 24 * 2 = 48, we know it's broken into two boxes. https://codereview.chromium.org/263363005/diff/40001/LayoutTests/fast/text/fo... LayoutTests/fast/text/fontWidthVariant.html:61: 文字<span id="variantCombine">90</span>年 Do we need the filler text before and after the "90"?
On 2014/05/12 09:58:42, h.joshi wrote: > In the link shared I found "ipaexm.ttf" under "IPA Minchō", can we put this font > under "WebKit/LayoutTests/resources" and use the same in our test case as > "typeface" by "font-face" property? Which Link? Which features does this font have? Half, Third, Quarter?
@Dominik: Yes this patch was "work in progress", will make sure to mention this in future patches. Now we are using "CSSHWOrientationTest.ttf" which has Half-width" feature. I was using "ipaexm.ttf" previously but that was of 10MB and git did not allowed to upload so search and shifted to "CSSHWOrientationTest.ttf" file. Final aim is to have test case as u suggested, I am new to testharness.js so facing problem.
On 2014/05/13 09:01:33, h.joshi wrote: > Now we are using "CSSHWOrientationTest.ttf" which has Half-width" feature. I was > using "ipaexm.ttf" previously [...] May I ask again - where did you find IPA Mincho - what's its license, and what features does it have with regards to half, third, quarter widht, etc? You mentioned, you provided a link to it, but I did not find such a link. Thanks.
In one of your previous comment, link "http://en.wikipedia.org/wiki/List_of_CJK_fonts" was shared. In this link we have "IPA Minchō" marked with [F] (so must be free) and then we have download link for this font. This font has Half-Width only as for other same no change in width was observed.
@Dominik: I have started to work to make suggested test case , after going though all the discussion I have one doubt. With this patch Font Width variant is working now in Blink, should we first create a test case to show correct rendering/output which will test if property is correctly applied or not. Pls Suggest.
On 2014/05/14 06:11:10, h.joshi wrote: > @Dominik: I have started to work to make suggested test case , after going > though all the discussion I have one doubt. > With this patch Font Width variant is working now in Blink, should we first > create a test case to show correct rendering/output which will test if property > is correctly applied or not. > Pls Suggest. I don't understand your question - could you rephrase it? My intention was to distinguish "property is applied" vs. "rendering is correct". By "property is applied", I mean: Does the CSS parsing and style resolution stage correctly attribute the span element with the -webkit-text-combine property. This is not the issue here, so we don't need to test this. Second "rendering is correct" - is where the problem lies: The applied property is not correctly taken into account in layout - this is what this patch, and this is what the text should be about.
Update: Font "CSSHWOrientationTest.ttf" has only Half-width and on using more than 2 digits rendering of digits does not happen in single line. From output font seems to be having same width for all glyphs, I am not very sure but seems to make a test case with more than 2 digits we need some font which supports third-width and quater-width. Pls guide me if test case with more than 2 digits can be made with current font we are using.
On 2014/05/14 07:38:58, h.joshi wrote: > Update: > Font "CSSHWOrientationTest.ttf" has only Half-width and on using more than 2 > digits rendering of digits does not happen in single line. > From output font seems to be having same width for all glyphs, I am not very > sure but seems to make a test case with more than 2 digits we need some font > which supports third-width and quater-width. > Pls guide me if test case with more than 2 digits can be made with current font > we are using. In any event, you would want to test more cases, especially corner cases. If you have three digits, do they break into the next block, etc. - Does it fit 3 digits into one block if it's a narrow one, etc. Your test case so far only tests one case.
Yes, I also want to make a good test case the only problem is font files unavailability due to which test case is not able to test more cases. As shared in previous comment current font has only half-width feature and all glyphs of equal width so if we have 3 digits in test case then property is not applied and all 3 digit are rendered vertically. If we can find font which has third-width and quarter-width then 3 or 4 digits will combine. One option which I see is adding platform based font family names, but not sure what to write or test for platform specific fonts, I do not have Windows system so will not be able to test myself.
What I was trying to say: Even if you have only half-width, your test needs to be extended to more cases, not just the standard "90". Yes, if we only have half-width, you would only expect one block, 2 digits. But what happens for narrow digits, like a "1" - does the code maybe squeeze more digits into one block, etc. What happens for 1 digit, what happens for three, etc.
For "But what happens for narrow digits, like a "1" - does the code maybe squeeze more digits into one block", with current "CSSHWOrientationTest" font all digits has same width so does not matter we use 1 or 9/0, this is test font so Adobe seems to have made width of all glyphs equal. One problem I am facing is, when running webkit test script "run-webkit-tests", it seems to be printing garbage values in output and due to that compare fails with expected text file, looking into this.
To check more cases, we need new font with all Width variants and glyphs with different widths. I feel to upload current test case with one test case and then if we find some other good font file make more test case and upload later. Pls let me know your view on this.
On 2014/05/15 11:47:11, h.joshi wrote: > For "But what happens for narrow digits, like a "1" - does the code maybe > squeeze more digits into one block", with current "CSSHWOrientationTest" font > all digits has same width so does not matter we use 1 or 9/0, this is test font > so Adobe seems to have made width of all glyphs equal. > I feel to upload current test case with one test case and then > if we find some other good font file make more test case and upload later. My comment was a suggestion to extend the test case and how to write a better test case: If you already know the behaviour you're describing, it's a good step: Make the test case show it. Put those cases into the test. Put variants of digits in it, and assert the same behaviour. Put triples of digits in the test case and assert that they are split into two blocks. A test case that just tests the trivial case which is expected to work does not really exercise corner cases. Think about exposing failures in the code. Write a test case which is in a way an adversary to the right behaviour of the code. Try to make it fail in unexpected ways by playing around with various situations. The indentation in your test is still not fixed, I assume you're mixing tabs and spaces. spanElement is an unused variable. theCSSprop_1 is an unsuitable variable name.
https://codereview.chromium.org/263363005/diff/60001/LayoutTests/fast/text/fo... File LayoutTests/fast/text/font-variant-width.html (right): https://codereview.chromium.org/263363005/diff/60001/LayoutTests/fast/text/fo... LayoutTests/fast/text/font-variant-width.html:46: assert_true(( (theCSSprop_1 >= 24) && (theCSSprop_1 < (2 * 24))), "Numbers laid out in one line."); Why >=24? The condition should be 24, right? You're testing: "A height which is somewhere between one and two lines".
https://codereview.chromium.org/263363005/diff/60001/LayoutTests/fast/text/fo... File LayoutTests/fast/text/font-variant-width.html (right): https://codereview.chromium.org/263363005/diff/60001/LayoutTests/fast/text/fo... LayoutTests/fast/text/font-variant-width.html:46: assert_true(( (theCSSprop_1 >= 24) && (theCSSprop_1 < (2 * 24))), "Numbers laid out in one line."); Thank you for notifying this, will make changes to condition and also to variable name in next patch.
Patch updated with test case suggested changes and font file moved to https://codereview.chromium.org/291103003/ patch
Comments #22 and #25 have not been addressed.
https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... File LayoutTests/fast/text/font-variant-width-expected.txt (right): https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width-expected.txt:2: This is a testharness.js-based test. As far as I know, tests using testharness.js do not need a corresponding -expected.txt file https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... File LayoutTests/fast/text/font-variant-width.html (right): https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:1: <!DOCTYPE HTML> lowercase "html" https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:4: <style type="text/css"> Why this style tag? https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:46: assert_true(( propertyHeight == 24), "Numbers laid out in one line."); assert_equals(propertyHeight, 24, "Numbers are laid out in one line."); https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:47: }, "Numbers exected to be laid out in one line"); s/exected/expected https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:50: </head> bad indentation https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:52: <body onload="load(); done();"> bad indentation https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:54: <div id="log"></div> bad identation
https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... File LayoutTests/fast/text/font-variant-width-expected.txt (right): https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width-expected.txt:2: This is a testharness.js-based test. Webkit layout script is trying to compare with expected file, expected file is present for other similar test cases like font-ligatures-linebreak.html.
https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... File LayoutTests/fast/text/font-variant-width.html (right): https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:1: <!DOCTYPE HTML> Will make changes. https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:4: <style type="text/css"> Will remove this https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:46: assert_true(( propertyHeight == 24), "Numbers laid out in one line."); Okey, will make suggested changes. https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:47: }, "Numbers exected to be laid out in one line"); Okey, will make suggested changes. https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:50: </head> Some issue with my editor, will fix indentation. https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:52: <body onload="load(); done();"> Some issue with my editor, will fix indentation. https://codereview.chromium.org/263363005/diff/120001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:54: <div id="log"></div> Some issue with my editor, will fix indentation.
New patch with suggested changes and adding Font File to this patch ("Dirk
Pranke" suggested to include
file in same patch in review of https://codereview.chromium.org/291103003)
Does not look good to me. Your test does not pass on Linux. I'd suggest to use the bots or ask a committer to tick the box to test your patch on the bots. After this review was basically stalled since I see you incorporating only some of the comments and feedback I gave you, I took the time to bascially re-do your test. Take a look at https://github.com/drott/blink-crosswalk/commit/ee6a2a86152d1e8fcfa066db1284b... and feel free to incorporate the changes and new revision of the test into your next upload to this CL. I commented on your test to show you what improvements I would like to have seen. It took me perhaps 40 minutes to do it properly and to demonstrate what I was expecting. In general, you will have better chances of getting your changes landed if you work thoroughly and make the reviewer's life easier. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... File LayoutTests/fast/text/font-variant-width-expected.txt (right): https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width-expected.txt:9: The test fails on my Linux system with your patch applied, see http://jsfiddle.net/n9A63/ for the failure. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... File LayoutTests/fast/text/font-variant-width.html (right): https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:3: <head> No <title>, no charset. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:11: -webkit-writing-mode: vertical-rl; Only make the relevant parts vertical, so that the testharness log is easier to read. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:15: div { Why div specific CSS? This can be collapsed with a new class for just the vertical testing. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:22: .combine { Cleanup redundant CSS, why combine1, combine2, etc.? https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:53: var propertyHeight = document.getElementById('variantCombine').offsetHeight; Loop over properties, instead of manually repeating code. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:58: assert_equals(propertyHeight, 24, "Numbers laid out in one line."); Try to declare constants separately, don't use magic numbers. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:66: assert_equals(propertyHeight2, 24*4, "Numbers laid out in more than one line."); This test fails on Linux. Test expectation is inaccurate. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:72: <body onload="load(); done();"> Rename load() to something more meaningful, put done() into test function for clarity. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:73: <div><span id="variantCombine" class="combine">90</span></div> Using class and id is redundant. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:76: <div id="log"></div> Not enough test cases. Still missing cases that I suggested several times.
Thank you for taking time and creating test case to help me understand things. Will follow suggestions in all future test case patches. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... File LayoutTests/fast/text/font-variant-width.html (right): https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:3: <head> Will add both to test case. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:11: -webkit-writing-mode: vertical-rl; Okey https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:15: div { New to JS and did not knew we can do something like "document.getElementsByClassName("combine")[testId].getBoundingClientRect().height" which you have used in your test case and get height/width for all test cases in loop. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:22: .combine { New to JS and did not knew we can do something like "document.getElementsByClassName("combine")[testId].getBoundingClientRect().height" which you have used in your test case and get height/width for all test cases in loop. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:53: var propertyHeight = document.getElementById('variantCombine').offsetHeight; Got idea from your test case. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:58: assert_equals(propertyHeight, 24, "Numbers laid out in one line."); Okey, will follow this in future also. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:66: assert_equals(propertyHeight2, 24*4, "Numbers laid out in more than one line."); It is showing "Pass" in my Linux system, let me try to reproduce this on different Linux box. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:72: <body onload="load(); done();"> Okey. https://codereview.chromium.org/263363005/diff/140001/LayoutTests/fast/text/f... LayoutTests/fast/text/font-variant-width.html:73: <div><span id="variantCombine" class="combine">90</span></div> Okey, will follow test case you shared.
Behdad or eae are the correct reviewers. Thank you Chris for helping h.joshi to navigate the patch process.
lgtm
@Emil: Pls review this patch Thank you Behdad
On 2014/07/23 17:49:53, h.joshi wrote: > @Emil: Pls review this patch > Thank you Behdad Dominik has started reviewing it and has/had objections. You need to make sure he is happy with it before asking for someone else to review it.
okey from Eric last comment I thought only you and Behdad need to review, sorry for confusion. @Dominik: Pls review patch
I was not able to test the patch. It currently does not apply cleanly (Font.cpp part needs to be rebased.) I was using $ git cl patch 263363005 to try and download and apply your patch but there seems to be an issue with the binary font upload. Could you double check whether the font file was upload correctly? Also, please apply for permission to use the bots, Chris Dumez or other Samsung committers might be able to help you with that. Then test your patches on all relevant platform bots in order to catch issues like the above. Please file a bug describing that this issue needs a font to test ThirdWidth and QuarterWidth, add the Cr-Blink-Fonts tag and CC me on the issue. Eric or dpranke can okay the inclusion of the font, reviewing the license and README.chromium. I will comment once the patch is rebased and I can successfully test it.
Thanks for the rebase- I was able to test the patch now when using the .tar.bz2 download. I tried again with $ git cl patch and there seems to be an issue with the tool. > Please file a bug describing that this issue needs a font to test ThirdWidth and > QuarterWidth, add the Cr-Blink-Fonts tag and CC me on the issue. Please follow _all_ given review comments. LGTM on condition the bug I mentioned gets filed before landing. Thanks for the patch.
@Dominik: thank you for LGTM Planned to file bug for font today, was busy with some other bug so could not do it with rebasing. Initially https://codereview.chromium.org/291103003/ was created to add new font file, You are also in the reviewer list. Then "dpranke" suggested to move that here so that we have record that font files are for this bug.
@Dominik: Bug "http://crbug.com/403211" is filed for Font addition.
The CQ bit was checked by h.joshi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/h.joshi@samsung.com/263363005/210001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...) android_blink_compile_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...) android_chromium_gn_compile_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...) blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...) linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...) mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/40961) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46182)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
On 2014/08/13 04:34:04, h.joshi wrote: > @Dominik: Bug "http://crbug.com/403211" is filed for Font addition. This is not what I meant. You are adding a font in this CL for HalfWidth, this case is covered. I asked you to file a bug describing that this patch needs better test coverage for the ThirdWidth and QuarterWidth cases.
@Dominik: Okey, I mixed your statement with this CL. Once this is patch is uploaded, will re-search to find some open source font which has requited features which can be used to write test cases.
The CQ bit was checked by h.joshi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/h.joshi@samsung.com/263363005/230001
On 2014/08/13 12:47:56, h.joshi wrote: > @Dominik: Okey, I mixed your statement with this CL. > Once this is patch is uploaded, will re-search to find some open source > font which has requited features which can be used to write test cases. I focus the efforts on actually authoring one.
> I focus the efforts on actually authoring one. Sry, I meant: I *would* suggest focussing your efforts on authoring one, e.g. using FontForge or https://github.com/behdad/fonttools
Sure, I had this is mind to use FontTools to modify things. Will surely start looking more into thsi.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
The CQ bit was checked by h.joshi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/h.joshi@samsung.com/263363005/230001
Message was sent while issue was closed.
Committed patchset #10 (230001) as 180241 |
