Bug 88684

Created attachment 151557 [details] Patch

Comment on attachment 151557 [details] Patch Attachment 151557 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13201141 New failing tests: css2.1/20110323/absolute-non-replaced-width-002.htm css2.1/20110323/absolute-non-replaced-width-001.htm WebViewTest.AutoResizeMinimumSize css2.1/20110323/absolute-non-replaced-width-003.htm css2.1/t0803-c5501-imrgn-t-00-b-ag.html css2.1/t0803-c5502-mrgn-r-01-c-a.html css2.1/t0804-c5508-ipadn-b-03-b-a.html css2.1/20110323/absolute-non-replaced-width-007.htm css2.1/20110323/absolute-non-replaced-height-002.htm css2.1/t0804-c5507-padn-r-01-c-a.html css2.1/20110323/absolute-non-replaced-width-004.htm css2.1/20110323/absolute-non-replaced-width-011.htm css2.1/20110323/absolute-non-replaced-height-009.htm css2.1/t0803-c5504-mrgn-l-01-c-a.html css2.1/20110323/absolute-non-replaced-width-012.htm css2.1/20110323/absolute-non-replaced-width-010.htm WebViewTest.AutoResizeMaxSize css2.1/t0803-c5503-imrgn-b-00-b-a.html WebViewTest.AutoResizeInBetweenSizes css2.1/20110323/absolute-non-replaced-width-005.htm css2.1/20110323/absolute-non-replaced-width-014.htm css3/zoom-coords.xhtml css2.1/20110323/absolute-non-replaced-width-013.htm css2.1/20110323/absolute-non-replaced-height-007.htm css2.1/t0803-c5505-mrgn-01-e-a.html css2.1/t0803-c5505-mrgn-03-c-ag.html css2.1/20110323/absolute-non-replaced-width-006.htm css2.1/20110323/absolute-non-replaced-width-009.htm css2.1/20110323/absolute-non-replaced-width-008.htm css2.1/t0804-c5506-padn-t-00-b-a.html

Created attachment 151563 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 151557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151557&action=review > Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:105 > + } The comment on line 88 says that the code is designed to match SimpleFontDataChromiumWin.cpp exactly. Does this patch outdate the comment? If so, please update the comment. > LayoutTests/platform/chromium/fast/text/descent-clip-in-scaled-page.html:17 > + <div>pppp</div> Please add description of the purpose of this test and what is the expectation. Such information is useful for gardeners.

Created attachment 151734 [details] Patch

Comment on attachment 151557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151557&action=review The new patch only adjusts descent when subpixel text positioning is enabled. If subpixel is not enabled, the positioning of the text might be shifted to left/up so the bottom truncation might not occur. And this also reduces the effect of existing layout tests. >> Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:105 >> + } > > The comment on line 88 says that the code is designed to match SimpleFontDataChromiumWin.cpp exactly. Does this patch outdate the comment? If so, please update the comment. Done. >> LayoutTests/platform/chromium/fast/text/descent-clip-in-scaled-page.html:17 >> + <div>pppp</div> > > Please add description of the purpose of this test and what is the expectation. Such information is useful for gardeners. Done.

Comment on attachment 151734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151734&action=review > Source/WebCore/ChangeLog:15 > + * platform/graphics/skia/SimpleFontDataSkia.cpp: Please update ChangeLog too. > Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:272 > + if (m_style.useAntiAlias == FontRenderStyle::NoPreference) > + m_style.useAntiAlias = useSkiaAntiAlias; > + > + if (!m_style.useHinting) > + m_style.hintStyle = SkPaint::kNo_Hinting; > + else if (m_style.useHinting == FontRenderStyle::NoPreference) > + m_style.hintStyle = skiaHinting; > + > + if (m_style.useBitmaps == FontRenderStyle::NoPreference) > + m_style.useBitmaps = useSkiaBitmaps; > + if (m_style.useAutoHint == FontRenderStyle::NoPreference) > + m_style.useAutoHint = useSkiaAutoHint; > + if (m_style.useSubpixelPositioning == FontRenderStyle::NoPreference) > + m_style.useSubpixelPositioning = useSkiaSubpixelPositioning; > + if (m_style.useAntiAlias == FontRenderStyle::NoPreference) > + m_style.useAntiAlias = useSkiaAntiAlias; > + if (m_style.useSubpixelRendering == FontRenderStyle::NoPreference) > + m_style.useSubpixelRendering = useSkiaSubpixelRendering; These default values are reasonable? Note that blackberry port uses Skia.

Created attachment 151988 [details] Patch

Comment on attachment 151734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151734&action=review >> Source/WebCore/ChangeLog:15 >> + * platform/graphics/skia/SimpleFontDataSkia.cpp: > > Please update ChangeLog too. Done. Sorry for missing in the last patch. >> Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:272 >> + m_style.useSubpixelRendering = useSkiaSubpixelRendering; > > These default values are reasonable? Note that blackberry port uses Skia. The patch doesn't change the default values, but only moves the default value handling code from setupPaint() into querySystemForRenderStyle() so that the new added fontRenderStyle() can get the actual styles. So it should not affect blackberry port.

Comment on attachment 151988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151988&action=review > LayoutTests/ChangeLog:9 > + * platform/chromium/fast/text/descent-clip-in-scaled-page-expected.html: Added. > + * platform/chromium/fast/text/descent-clip-in-scaled-page.html: Added. Is there something chromium-specific about this test? It seems like it should just go in the cross-platform fast/text directory. > LayoutTests/platform/chromium/fast/text/descent-clip-in-scaled-page-expected.html:14 > + if (window.layoutTestController) > + window.layoutTestController.setTextSubpixelPositioning(true); > + if (window.internals) > + window.internals.settings.setPageScaleFactor(1.7, 0, 0); The expected.html file looks exactly the same as the test HTML file. I don't understand how we're getting value from this test. Perhaps we need to use a pixel test (rather than a ref test) in this case?

Comment on attachment 151988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151988&action=review >> LayoutTests/ChangeLog:9 >> + * platform/chromium/fast/text/descent-clip-in-scaled-page.html: Added. > > Is there something chromium-specific about this test? It seems like it should just go in the cross-platform fast/text directory. The change is for ports using Skia and HarfBuzz. You reminded me, I should move it into platform/chromium-linux. >> LayoutTests/platform/chromium/fast/text/descent-clip-in-scaled-page-expected.html:14 >> + window.internals.settings.setPageScaleFactor(1.7, 0, 0); > > The expected.html file looks exactly the same as the test HTML file. I don't understand how we're getting value from this test. Perhaps we need to use a pixel test (rather than a ref test) in this case? The only difference is 'overflow: hidden' :) The test checks if the bottom of text is truncated in overflow:hidden div. If not, it should be displayed the same as in a normal div.

Created attachment 152001 [details] Patch (moved test case from platform/chromium to platform/chromium-linux because it's only applicable to chromium-linux and chromium-android)

> The change is for ports using Skia and HarfBuzz. You reminded me, I should move it into platform/chromium-linux. I understand that's what the code change is for, but the test seem applicable to all ports. > >> LayoutTests/platform/chromium/fast/text/descent-clip-in-scaled-page-expected.html:14 > >> + window.internals.settings.setPageScaleFactor(1.7, 0, 0); > > > > The expected.html file looks exactly the same as the test HTML file. I don't understand how we're getting value from this test. Perhaps we need to use a pixel test (rather than a ref test) in this case? > > The only difference is 'overflow: hidden' :) > The test checks if the bottom of text is truncated in overflow:hidden div. If not, it should be displayed the same as in a normal div. Ah, thanks. Can we add a comment to make that more obvious to dumb people like me? :)

(In reply to comment #13) > > The change is for ports using Skia and HarfBuzz. You reminded me, I should move it into platform/chromium-linux. > > I understand that's what the code change is for, but the test seem applicable to all ports. > I understand your point, but I'm afraid the test would fail on some ports as subpixel ascent/descent is not supported by all ports. For now my solution is only applicable to chromium-linux/chromium-android when subpixel text positioning is enabled. The test needs the Chromium-only layoutTestController.setSubpixelPositioning() function which only works on chromium-linux and chromium-android to force subpixel text positioning. Otherwise the test might even pass without this patch. > > >> LayoutTests/platform/chromium/fast/text/descent-clip-in-scaled-page-expected.html:14 > > >> + window.internals.settings.setPageScaleFactor(1.7, 0, 0); > > > > > > The expected.html file looks exactly the same as the test HTML file. I don't understand how we're getting value from this test. Perhaps we need to use a pixel test (rather than a ref test) in this case? > > > > The only difference is 'overflow: hidden' :) > > The test checks if the bottom of text is truncated in overflow:hidden div. If not, it should be displayed the same as in a normal div. > > Ah, thanks. Can we add a comment to make that more obvious to dumb people like me? :) (In reply to comment #13) > > The change is for ports using Skia and HarfBuzz. You reminded me, I should move it into platform/chromium-linux. > > I understand that's what the code change is for, but the test seem applicable to all ports. > > > >> LayoutTests/platform/chromium/fast/text/descent-clip-in-scaled-page-expected.html:14 > > >> + window.internals.settings.setPageScaleFactor(1.7, 0, 0); > > > > > > The expected.html file looks exactly the same as the test HTML file. I don't understand how we're getting value from this test. Perhaps we need to use a pixel test (rather than a ref test) in this case? > > > > The only difference is 'overflow: hidden' :) > > The test checks if the bottom of text is truncated in overflow:hidden div. If not, it should be displayed the same as in a normal div. > > Ah, thanks. Can we add a comment to make that more obvious to dumb people like me? :) Will do in the next patch, after we agree about where to place the test :)

> I understand your point, but I'm afraid the test would fail on some ports as subpixel ascent/descent is not supported by all ports. For now my solution is only applicable to chromium-linux/chromium-android when subpixel text positioning is enabled. The test needs the Chromium-only layoutTestController.setSubpixelPositioning() function which only works on chromium-linux and chromium-android to force subpixel text positioning. Otherwise the test might even pass without this patch. That's fine. We can mark the test as failing on other platforms until they get better and fix their bugs. I mean, it's not really a big deal where this test goes, but we typically only put tests in the platform directory if there is something intrinsically platform-specific about the test (i.e., not if it's just that other ports aren't able to pass it currently).

(In reply to comment #15) > > That's fine. We can mark the test as failing on other platforms until they get better and fix their bugs. > The main issue is about the layoutTestController.setSubpixelTextPositioning() call in the test which only exists on Chromium. If I remove the call to make the test generic, then the test won't cover the changed code because subpixel text positioning is disabled by default in DumpRenderTree.

> The main issue is about the layoutTestController.setSubpixelTextPositioning() call in the test which only exists on Chromium. If I remove the call to make the test generic, then the test won't cover the changed code because subpixel text positioning is disabled by default in DumpRenderTree. That's fine. There are lots of tests that fail on many ports due to unimplemented LayoutTestController functionality.

Created attachment 152085 [details] Patch. Make the tests generic. Will update test expectations after try build

Comment on attachment 152085 [details] Patch. Make the tests generic. Will update test expectations after try build Attachment 152085 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13200912

Comment on attachment 152085 [details] Patch. Make the tests generic. Will update test expectations after try build View in context: https://bugs.webkit.org/attachment.cgi?id=152085&action=review LGTM. Please wait formal review. > LayoutTests/fast/text/descent-clip-in-scaled-page-expected.html:17 > +<body onload="test()"> nit: Remove onload="test()" ? > LayoutTests/fast/text/descent-clip-in-scaled-page.html:18 > +<body onload="test()"> Ditto.

Created attachment 152309 [details] Patch

Comment on attachment 152085 [details] Patch. Make the tests generic. Will update test expectations after try build View in context: https://bugs.webkit.org/attachment.cgi?id=152085&action=review >> LayoutTests/fast/text/descent-clip-in-scaled-page-expected.html:17 >> +<body onload="test()"> > > nit: Remove onload="test()" ? Done. >> LayoutTests/fast/text/descent-clip-in-scaled-page.html:18 >> +<body onload="test()"> > > Ditto. Done.

Who should review this change? Looks like tony@ has review most of the changes to FontPlatformDataHarfBuzz.cpp.

Comment on attachment 152309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152309&action=review Seems reasonable to me and bashi is happy with it. > Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:104 > + descent++; > + ascent--; Nit: ++descent and --ascent

Created attachment 152339 [details] for landing

Comment on attachment 152309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152309&action=review >> Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:104 >> + ascent--; > > Nit: ++descent and --ascent Done.

Comment on attachment 152339 [details] for landing Clearing flags on attachment: 152339 Committed r122651: <http://trac.webkit.org/changeset/122651>

All reviewed patches have been landed. Closing bug.

(In reply to comment #27) > (From update of attachment 152339 [details]) > Clearing flags on attachment: 152339 > > Committed r122651: <http://trac.webkit.org/changeset/122651> The new ref test fails on Apple's Mac bots, but my guess is that's expected for ports that disable subpixel layout. Is that right?

(In reply to comment #29) > (In reply to comment #27) > > (From update of attachment 152339 [details] [details]) > > Clearing flags on attachment: 152339 > > > > Committed r122651: <http://trac.webkit.org/changeset/122651> > > The new ref test fails on Apple's Mac bots, but my guess is that's expected for ports that disable subpixel layout. Is that right? No, the test is expected to pass on all platforms. The only difference with the expected file is overflow:hidden, which shouldn't matter because there should be no overflow. Xianzhu, can you investigate?

(In reply to comment #30) > (In reply to comment #29) > > The new ref test fails on Apple's Mac bots, but my guess is that's expected for ports that disable subpixel layout. Is that right? > > No, the test is expected to pass on all platforms. The only difference with the expected file is overflow:hidden, which shouldn't matter because there should be no overflow. > > Xianzhu, can you investigate? I think in theory the bottom of text should not be truncated in any case, though my change itself only applies to chromium-linux when subpixel text positioning (a feature of Skia on linux) is enabled. If it fails on Mac, I think we should mark it as failure in expectations/skipped file and file a bug.

Filed <https://bugs.webkit.org/show_bug.cgi?id=91552>.

Created attachment 155634 [details] Bottom clipped on Android Just verified that the bug also existed without page scaling. Actually the fractional descent came from the computation of descent for a font without VDMX table, but the clipping is not very visible when the page is not scaled because only few anti-aliased pixels are clipped. When the page is scaled, the solid part of font might be clipped so it becomes visible.