Closed
Bug 1364714
Opened 8 years ago
Closed 7 years ago
Update to Freetype 2.8
Categories
(Core :: Graphics: Text, defect, P3)
Core
Graphics: Text
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: ionnv, Assigned: jfkthame)
References
()
Details
(Whiteboard: [gfx-noted])
Attachments
(15 files, 1 obsolete file)
2.54 MB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
1.40 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
961 bytes,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
2.19 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
887 bytes,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
1.51 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
1.68 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
1.35 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
3.03 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
3.22 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
1.28 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
2.28 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
1.78 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
3.41 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
14.90 KB,
patch
|
RyanVM
:
review+
|
Details | Diff | Splinter Review |
https://www.freetype.org/index.html#news
https://sourceforge.net/projects/freetype/files/freetype2/2.8/
"FreeType 2.8 has been released. CFF2 support and OpenType variation font handling is now complete; the auto-hinter now understands 25 more scripts, for example N'Ko and Tifinagh.
See here for a list of changes; noteworthy bug fixes are the handling of TrueType fonts: unhinted loading didn't work as expected, and the light auto-hinter used incorrect metrics."
Comment 1•8 years ago
|
||
Also worth noting:
- CVE-2017-8105, CVE-2017-8287: Older FreeType versions have out-of-bounds writes caused by heap-based buffer overflows related to Type 1 fonts.
Not sure how severe that is for Firefox, though.
Assignee: nobody → ryanvm
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 2•8 years ago
|
||
Linux looks fine, Android less so.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddece01e2f4dca6317cc134d6f1ed8629c5d7b93
Attachment #8867512 -
Flags: feedback?(jfkthame)
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> Also worth noting:
> - CVE-2017-8105, CVE-2017-8287: Older FreeType versions have out-of-bounds
> writes caused by heap-based buffer overflows related to Type 1 fonts.
>
> Not sure how severe that is for Firefox, though.
Doesn't sound like a big concern; Type 1 fonts aren't supported via @font-face, so they wouldn't be a potential attack vector for Firefox.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #2)
> Linux looks fine, Android less so.
:-(
Looking at some of the failures, I think many of them are happening because we're getting a change in the default line-height for some fonts -- in particular, for our default sans-serif -- perhaps related to a fix for "the light auto-hinter used incorrect metrics".
In many cases, this indicates the tests are making fragile assumptions and should be fixed to be more robust, but we should also make sure we understand what the effect of the change really is, and whether it's OK.
Comment 4•8 years ago
|
||
Sorry, I'm not going to have time to wade through the Android reftest issues.
Assignee: ryanvm → nobody
Status: ASSIGNED → NEW
Updated•7 years ago
|
Whiteboard: [gfx-noted]
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 5•7 years ago
|
||
The majority of the reftest failures here occur because the new freetype version gives us a slightly greater 'normal' line-height for the Clear Sans font used as the default sans-serif on Android. The difference is quite marginal: with current Firefox on my Nexus tablet, line-height:normal computes to 21.5px, and with the updated freetype, it computes to 22px. (Exact results may vary depending on device resolution, because IIRC we "snap" ascent/descent metrics to device pixels.)
So the change does not appear to have any adverse impact on actual web content, AFAICS; but a number of reftests are fragile enough that it results in test failures. Explicitly setting line-height where necessary can make these more robust.
There are also a few cases where already-fuzzy tests just need an adjustment to the fuzz annotations due to changes in the antialiasing/rendering.
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8898410 -
Flags: review?(dholbert)
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8898412 -
Flags: review?(dholbert)
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8898413 -
Flags: review?(dholbert)
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8898414 -
Flags: review?(dholbert)
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8898415 -
Flags: review?(dholbert)
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8898416 -
Flags: review?(dholbert)
Assignee | ||
Comment 12•7 years ago
|
||
Attachment #8898417 -
Flags: review?(dholbert)
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8898418 -
Flags: review?(dholbert)
Assignee | ||
Comment 14•7 years ago
|
||
Attachment #8898419 -
Flags: review?(dholbert)
Assignee | ||
Comment 15•7 years ago
|
||
Attachment #8898420 -
Flags: review?(dholbert)
Assignee | ||
Comment 16•7 years ago
|
||
Attachment #8898421 -
Flags: review?(dholbert)
Comment 17•7 years ago
|
||
Comment on attachment 8898410 [details] [diff] [review]
Fix fragile reference case for multicol-gap-001 test
Review of attachment 8898410 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/reftests/w3c-css/received/css-multicol-1/multicol-gap-001-ref.xht
@@ +5,5 @@
> <link rel="author" title="Opera Software ASA" href="http://www.opera.com/" />
> <link rel="reviewer" title="Gérard Talbot" href="http://www.gtalbot.org/BrowserBugsSection/css21testsuite/" /> <!-- 2013-08-05 -->
> <meta name="flags" content="image" />
> <style type="text/css"><![CDATA[
> + div {margin: 1.25em; line-height: 1;}
We're not really supposed to edit reftests in the "received" subdirectory, because our edits will be lost/stomped-on when we re-sync.
Better to mark the test as fuzzy/failing in the reftest manifest, and (optionally) submit a fix upstream, I think?
Attachment #8898410 -
Flags: review?(dholbert) → review-
Assignee | ||
Comment 18•7 years ago
|
||
Attachment #8898422 -
Flags: review?(dholbert)
Comment 19•7 years ago
|
||
Comment on attachment 8898412 [details] [diff] [review]
Set explicit line-height on background-position-vrl-018-ref file for better reliability
Review of attachment 8898412 [details] [diff] [review]:
-----------------------------------------------------------------
(previous comment applies to this tweak as well)
Attachment #8898412 -
Flags: review?(dholbert) → review-
Assignee | ||
Comment 20•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #17)
> Comment on attachment 8898410 [details] [diff] [review]
> Fix fragile reference case for multicol-gap-001 test
>
> Review of attachment 8898410 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: layout/reftests/w3c-css/received/css-multicol-1/multicol-gap-001-ref.xht
> @@ +5,5 @@
> > <link rel="author" title="Opera Software ASA" href="http://www.opera.com/" />
> > <link rel="reviewer" title="Gérard Talbot" href="http://www.gtalbot.org/BrowserBugsSection/css21testsuite/" /> <!-- 2013-08-05 -->
> > <meta name="flags" content="image" />
> > <style type="text/css"><![CDATA[
> > + div {margin: 1.25em; line-height: 1;}
>
> We're not really supposed to edit reftests in the "received" subdirectory,
> because our edits will be lost/stomped-on when we re-sync.
>
> Better to mark the test as fuzzy/failing in the reftest manifest, and
> (optionally) submit a fix upstream, I think?
This has been merged upstream already, so we'll get it when we next take an update from there. So applying the patch here is just an interim measure.
https://github.com/w3c/web-platform-tests/commit/cb90869af81f0cd7d184a44137c62ce73741b00e
Assignee | ||
Comment 21•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #19)
> Comment on attachment 8898412 [details] [diff] [review]
> Set explicit line-height on background-position-vrl-018-ref file for better
> reliability
>
> Review of attachment 8898412 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> (previous comment applies to this tweak as well)
For this one, I just opened a PR upstream, awaiting review there.
But we can mark them as failing or fuzzy for now rather than applying the fixes locally, if you prefer.
Comment 22•7 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #20)
> This has been merged upstream already, so we'll get it when we next take an
> update from there. So applying the patch here is just an interim measure.
>
> https://github.com/w3c/web-platform-tests/commit/cb90869af81f0cd7d184a44137c62ce73741b00e
Ah, OK - thanks! Could you link to this commit (or the merged pull request, https://github.com/w3c/web-platform-tests/pull/6879 ) in the extended commit message, with a note about the fact that this fix has already happened upstream?
(In reply to Jonathan Kew (:jfkthame) from comment #21)
> (In reply to Daniel Holbert [:dholbert] from comment #19)
> > Comment on attachment 8898412 [details] [diff] [review]
> > Set explicit line-height on background-position-vrl-018-ref file for better
> > reliability
> For this one, I just opened a PR upstream, awaiting review there.
Gotcha - indeed, looks like this is https://github.com/w3c/web-platform-tests/pull/6924 . As with the other patch, please link to this in the commit message.
> But we can mark them as failing or fuzzy for now rather than applying the fixes locally, if you prefer.
I'm fine with taking these fixes as long as they're happening upstream too.
Comment 23•7 years ago
|
||
Comment on attachment 8898410 [details] [diff] [review]
Fix fragile reference case for multicol-gap-001 test
Review of attachment 8898410 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with a link to upstream pull request or commit, in the extended commit message.
Attachment #8898410 -
Flags: review- → review+
Comment 24•7 years ago
|
||
Comment on attachment 8898412 [details] [diff] [review]
Set explicit line-height on background-position-vrl-018-ref file for better reliability
Review of attachment 8898412 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with a link to upstream pull request or commit, in the extended commit message.
Updated•7 years ago
|
Attachment #8898412 -
Flags: review- → review+
Updated•7 years ago
|
Attachment #8898413 -
Flags: review?(dholbert) → review+
Comment 25•7 years ago
|
||
Comment on attachment 8898414 [details] [diff] [review]
Adjust fuzzy() annotation for box-decoration reftest
Review of attachment 8898414 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/reftests/css-break/reftest.list
@@ +1,5 @@
> default-preferences pref(layout.css.box-decoration-break.enabled,true)
>
> == box-decoration-break-1.html box-decoration-break-1-ref.html
> fuzzy(1,20) fuzzy-if(skiaContent,1,700) fuzzy-if(webrender,4-4,3273-3273) == box-decoration-break-with-inset-box-shadow-1.html box-decoration-break-with-inset-box-shadow-1-ref.html
> +fuzzy(16,460) fuzzy-if(Android,10,3673) fuzzy-if(skiaContent,57,1330) fuzzy-if(styloVsGecko,2,1410) == box-decoration-break-with-outset-box-shadow-1.html box-decoration-break-with-outset-box-shadow-1-ref.html # Bug 1386543
Do you have a recent Try run that shows the mismatch here? (I tried the link from comment 2, but the reftest-analyzer "graph" links don't work because it seems the .log files are now 404.)
It looks like the change here is from...
fuzzy-if(skiaContent,57,374)
...to
fuzzy-if(skiaContent,57,1330)
And that's a big enough regression in nontrivially-mismatching-pixels that I'm curious what the mismatch actually looks like in practice...
Updated•7 years ago
|
Flags: needinfo?(jfkthame)
Even better, would you mind running an update from upstream? We don't have a process for doing that regularly, though we should...
(Though if it's a mess of annotation, that might be another story...)
Comment 27•7 years ago
|
||
Comment on attachment 8898414 [details] [diff] [review]
Adjust fuzzy() annotation for box-decoration reftest
Review of attachment 8898414 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/reftests/css-break/reftest.list
@@ +1,5 @@
> default-preferences pref(layout.css.box-decoration-break.enabled,true)
>
> == box-decoration-break-1.html box-decoration-break-1-ref.html
> fuzzy(1,20) fuzzy-if(skiaContent,1,700) fuzzy-if(webrender,4-4,3273-3273) == box-decoration-break-with-inset-box-shadow-1.html box-decoration-break-with-inset-box-shadow-1-ref.html
> +fuzzy(16,460) fuzzy-if(Android,10,3673) fuzzy-if(skiaContent,57,1330) fuzzy-if(styloVsGecko,2,1410) == box-decoration-break-with-outset-box-shadow-1.html box-decoration-break-with-outset-box-shadow-1-ref.html # Bug 1386543
Also -- it looks like the failure here was on Android, and this manifest line has annotations for *both* "Android" *and* "skiaContent". Current mozilla-central has fuzzy-if(Android,10,3673) fuzzy-if(skiaContent,57,374)
Is there a reason you went for adjusting the skiaContent annotation, rather than the Android one? (IIRC, "skiaContent" is true on multiple platforms, and it seems like a shame to reduce the strictness on multiple platforms, for what seems to be an Android-specific issue per comment 2...)
Assignee | ||
Comment 28•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #25)
> Comment on attachment 8898414 [details] [diff] [review]
> Adjust fuzzy() annotation for box-decoration reftest
> Do you have a recent Try run that shows the mismatch here? (I tried the link
> from comment 2, but the reftest-analyzer "graph" links don't work because it
> seems the .log files are now 404.)
The logs from https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d1e561d09b55bc81fcf5695a7617064344bdf7c are still available, it looks like. I think you'll find all these failures there.
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 29•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #27)
> Comment on attachment 8898414 [details] [diff] [review]
> Is there a reason you went for adjusting the skiaContent annotation, rather
> than the Android one? (IIRC, "skiaContent" is true on multiple platforms,
> and it seems like a shame to reduce the strictness on multiple platforms,
> for what seems to be an Android-specific issue per comment 2...)
IIRC, I first adjusted the Android annotation, but then it appeared that the Skia one must be overriding it in our current configuration, so I switched to doing that one instead. But I guess the better option is probably to re-order them so that the Android annotation comes last; I'll give that a try and check that it works as expected.
Comment 30•7 years ago
|
||
That makes sense -- I've definitely been bitten by annoying one-upsmanship between fails-if/fuzzy-if annotations in the past. I forget the precedence rules, but I wouldn't be surprised if a reordering is needed.
(In reply to Jonathan Kew (:jfkthame) from comment #28)
> The logs from
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d1e561d09b55bc81fcf5695a7617064344bdf7c are still
> available, it looks like. I think you'll find all these failures there.
Thanks! But -- in that try run "R7", I'm seeing box-decoration-break-with-outset-box-shadow-1.html having "max difference: 57, number of differing pixels: 400" (which is only slightly-worse than our current fuzzy-if(skiaContent,57,374)). I'd r+ a patch that bumps 374 to 400 there.
But your box-decoration patch was bumping the pixels to 1330 (much more than 400). Maybe that was a mistake? Or maybe sometimes there's a larger difference?
Here's a link to the Try run with that job selected:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d1e561d09b55bc81fcf5695a7617064344bdf7c&selectedJob=121051154
Comment 31•7 years ago
|
||
Comment on attachment 8898415 [details] [diff] [review]
Modify text to avoid possibly-overflowing descender in last line of orthogonal float
Review of attachment 8898415 [details] [diff] [review]:
-----------------------------------------------------------------
r=me on this possibly-overflowing descender one (makes sense -- sneaky sideways-and-protruding "y" character)
Attachment #8898415 -
Flags: review?(dholbert) → review+
Comment 32•7 years ago
|
||
Comment on attachment 8898416 [details] [diff] [review]
Add fuzzy() annotation to mathml-mmultiscript test to allow for changed freetype rendering
Review of attachment 8898416 [details] [diff] [review]:
-----------------------------------------------------------------
r=me on this mathml one, with a followup filed & noted on the reftest.list line.
::: layout/reftests/mathml/reftest.list
@@ +204,5 @@
> == opentype-fraction-dynamic-linethickness.html opentype-fraction-dynamic-linethickness-ref.html
> == operator-1.xhtml operator-1-ref.xhtml
> == scriptshift-1.xhtml scriptshift-1-ref.xhtml
> == number-size-1.xhtml number-size-1-ref.xhtml
> +fuzzy-if(skiaContent,1,80) fuzzy-if(Android,255,105) skip-if(winWidget) == multiscripts-1.html multiscripts-1-ref.html # Windows: bug 1314684
Hmm... I guess this is fine, though "fuzzy" with color-channel=255 and a nontrivial number of pixels sounds more like a failure than fuzziness.
I think this is really a test-bug, actually! The test run is https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d1e561d09b55bc81fcf5695a7617064344bdf7c&selectedJob=121051123 and here's my observation:
- The mismatch is on the msup / msubsup line, the first "AAAbbb" (of two) is missing a 1px stripe of of red background at the bottom. (The testcase is missing that stripe, but the reference case has it.)
- I see a similar difference between the testcase and reference if I view them in Firefox Nightly & zoom in with full-page-zoom as much as Firefox allows me to.
Could you file a bug on this mismatch and mention it in the comment at the end of the line here?
Attachment #8898416 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 33•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #30)
> But your box-decoration patch was bumping the pixels to 1330 (much more than
> 400). Maybe that was a mistake? Or maybe sometimes there's a larger
> difference?
That'll be the difference I got when running the tests locally (on a higher-resolution device than we use in automation), so I picked the value that silenced the failure on a local run as well. We could stay with the lower value based on what tryserver shows, though, and accept a greater tendency for the test to fail locally depending on people's configurations. (It would be far from the only such test!) WDYT?
Assignee | ||
Comment 34•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #30)
> (In reply to Jonathan Kew (:jfkthame) from comment #28)
> > The logs from
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d1e561d09b55bc81fcf5695a7617064344bdf7c are still
> > available, it looks like. I think you'll find all these failures there.
>
> Thanks! But -- in that try run "R7", I'm seeing
> box-decoration-break-with-outset-box-shadow-1.html having "max difference:
> 57, number of differing pixels: 400" (which is only slightly-worse than our
> current fuzzy-if(skiaContent,57,374)). I'd r+ a patch that bumps 374 to 400
> there.
>
> But your box-decoration patch was bumping the pixels to 1330 (much more than
> 400). Maybe that was a mistake? Or maybe sometimes there's a larger
> difference?
FWIW, I tried running this locally (Nexus 10) *without* the freetype update, and I get
REFTEST TEST-UNEXPECTED-FAIL | http://192.168.1.65:8888/tests/layout/reftests/css-break/box-decoration-break-with-outset-box-shadow-1.html == http://192.168.1.65:8888/tests/layout/reftests/css-break/box-decoration-break-with-outset-box-shadow-1-ref.html | image comparison, max difference: 57, number of differing pixels: 1239
So that's already a lot more than the 374 in the current annotation, and not much less than the 1330 I get with the updated freetype.
Assignee | ||
Comment 35•7 years ago
|
||
So this version, which re-orders the annotations and then actually *reduces* the number of fuzzy pixels in the Android annotation, seems to be sufficient in local testing. (I'll double-check on tryserver before any of this lands, though.)
Attachment #8898857 -
Flags: review?(dholbert)
Assignee | ||
Updated•7 years ago
|
Attachment #8898414 -
Attachment is obsolete: true
Attachment #8898414 -
Flags: review?(dholbert)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee | ||
Comment 36•7 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #26)
> Even better, would you mind running an update from upstream? We don't have
> a process for doing that regularly, though we should...
>
> (Though if it's a mess of annotation, that might be another story...)
Once I got the import script to run successfully, it didn't look too bad at all. I've filed it separately as bug 1391717.
This also led me to wonder how much duplication we have between tests we're importing here, and what we run under testing/web-platform. (Maybe you know the answer?) Rather than proceed down that rabbit-hole right now, I filed bug 1391730.
Comment 37•7 years ago
|
||
Comment on attachment 8898417 [details] [diff] [review]
Adjust fuzzy() annotation for table-background test to allow for change in freetype's rendering
Review of attachment 8898417 [details] [diff] [review]:
-----------------------------------------------------------------
r=me
Attachment #8898417 -
Flags: review?(dholbert) → review+
Comment 38•7 years ago
|
||
Comment on attachment 8898418 [details] [diff] [review]
Adjust fuzzy() annotation for pre-line-1 test to account for change in freetype's font rendering
Review of attachment 8898418 [details] [diff] [review]:
-----------------------------------------------------------------
r=me
Attachment #8898418 -
Flags: review?(dholbert) → review+
Comment 39•7 years ago
|
||
Comment on attachment 8898419 [details] [diff] [review]
Increase line-height to allow for default font with larger metrics
Review of attachment 8898419 [details] [diff] [review]:
-----------------------------------------------------------------
Commit message nit:
> Bug 1364714 - Increase line-height to allow for default font with larger metrics.
s/line-height/line-height in some ruby reftests/
Attachment #8898419 -
Flags: review?(dholbert) → review+
Comment 40•7 years ago
|
||
Comment on attachment 8898420 [details] [diff] [review]
Adjust dimensions in fixed-table-layout-010 tests to allow for increased font metrics without disrupting layout of test elements
Review of attachment 8898420 [details] [diff] [review]:
-----------------------------------------------------------------
r=me
Attachment #8898420 -
Flags: review?(dholbert) → review+
Comment 41•7 years ago
|
||
Comment on attachment 8898421 [details] [diff] [review]
Avoid potential unwanted scrollbar when rendering test file
Review of attachment 8898421 [details] [diff] [review]:
-----------------------------------------------------------------
Commit message nit:
> Bug 1364714 - Avoid potential unwanted scrollbar when rendering test file.
This commit message is bit too vague -- it's unclear from this description whether it's a code change vs. a test change. Maybe use something like: "Adjust reftest text-emphasis-line-height-001z.html to avoid potential unwanted scrollbar"?
r=me with some sort of clarification there.
::: layout/reftests/w3c-css/submitted/text-decor-3/text-emphasis-line-height-001z.html
@@ +10,5 @@
> textarea {
> all: inherit;
> width: 100%;
> + height: 10em; /* ensure there's plenty of height even if the default font is quite tall,
> + to avoid risk of a vertical scrollbar showing up */
s/showing up/showing up inside textarea/
(At first, I interpreted this as talking about a scrollbar on the *viewport*, and I was confused.)
Attachment #8898421 -
Flags: review?(dholbert) → review+
Comment 42•7 years ago
|
||
Comment on attachment 8898422 [details] [diff] [review]
Adjust fuzzy() annotations on async-scrollbar tests
Review of attachment 8898422 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/apz/test/reftest/reftest.list
@@ +4,3 @@
> fuzzy-if(Android,4,5) skip-if(!Android) pref(apz.allow_zooming,true) == async-scrollbar-1-h.html async-scrollbar-1-h-ref.html
> fuzzy-if(Android,3,5) skip-if(!Android) pref(apz.allow_zooming,true) == async-scrollbar-1-vh.html async-scrollbar-1-vh-ref.html
> +fuzzy-if(Android,43,8) skip-if(!Android) pref(apz.allow_zooming,true) == async-scrollbar-1-v-rtl.html async-scrollbar-1-v-rtl-ref.html
This change -- from fuzzy-if(Android,1,2) to fuzzy-if(Android,43,8) -- is quite large. The old annotation was extremely subtle (a rounding error on 2 pixels), whereas the new annotation is arguably a test failure (8 pixels that differ by quite a lot). Looking at the reftest snapshot, the scrollbar is simply at a different position between the testcase & reference case -- it's 1px lower in the reference case than in the testcase, I think (and this produces 4px of mismatch at the top and 4px at the bottom, from places where one file is painting the scrollbar edges and the other is not).
If this gets triggered by this freetype update, maybe this test just depends on font metrics in a way that it should not? If there's no obvious easy fix, could you file a followup on this and CC someone who's worked on these tests?
(This is for both async-scrollbar-1-v.html and async-scrollbar-1-v-rtl.html)
Attachment #8898422 -
Flags: review?(dholbert) → review+
Comment 43•7 years ago
|
||
Comment on attachment 8898857 [details] [diff] [review]
Adjust fuzzy() annotation for box-decoration reftest
Review of attachment 8898857 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/reftests/css-break/reftest.list
@@ +1,5 @@
> default-preferences pref(layout.css.box-decoration-break.enabled,true)
>
> == box-decoration-break-1.html box-decoration-break-1-ref.html
> fuzzy(1,20) fuzzy-if(skiaContent,1,700) fuzzy-if(webrender,4-4,3273-3273) == box-decoration-break-with-inset-box-shadow-1.html box-decoration-break-with-inset-box-shadow-1-ref.html
> +fuzzy(16,460) fuzzy-if(skiaContent,57,374) fuzzy-if(Android,57,1330) fuzzy-if(styloVsGecko,2,1410) == box-decoration-break-with-outset-box-shadow-1.html box-decoration-break-with-outset-box-shadow-1-ref.html # Bug 1386543
Please add a note in the extended commit message to note that you're re-ordering the annotations (and briefly why -- i.e. that the old Android annotation had become suppressed/ignored effectively due to clashing with the more general & higher-priority-due-to-ordering skiaContent annotation, or whatever)
Attachment #8898857 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 44•7 years ago
|
||
Sigh... I thought we were about ready to land this, with all the android reftest issues addressed one way or another. But tryserver indicates[1] that we also get a build failure in pdfium code (which has been added to the tree -- see bug 1368948 -- since the original try run in comment 2 was done).
This occurs because pdfium relies on an internal freetype function ft_get_adobe_glyph_index(), and changes in freetype 2.8 mean this is no longer publicly visible even when the (private) header file is included, unless the including file #defines a specific symbol.
There's already an upstream issue[2] related to this, so I expect something will be worked out between the pdfium and freetype projects, but for the time being we'll need to patch around it locally.
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a2bf2b0cf87ae8d64083b2c65f63977ec5f7e0e&group_state=expanded&selectedJob=124518641
[2] https://bugs.chromium.org/p/pdfium/issues/detail?id=733
Assignee | ||
Comment 45•7 years ago
|
||
As noted above, this will probably be superseded once pdfium's issue #733 is fixed upstream, but for the time being this should be sufficient to fix the build failure.
Attachment #8899434 -
Flags: review?(jwatt)
Assignee | ||
Comment 46•7 years ago
|
||
To avoid defining freetype's psnames data twice (due to pdfium including the internal header), we should cherry-pick this recent freetype commit (upstream commit message '[psnames] Really fix issue #49949'), which separates the preprocessor symbol controlling definitions from declarations. This allows pdfium to #include the header with DEFINE_PS_TABLES so that it gets access to the function it needs, without duplicating the definition of all the data.
Attachment #8899436 -
Flags: review?(ryanvm)
Assignee | ||
Comment 47•7 years ago
|
||
Comment on attachment 8867512 [details] [diff] [review]
update freetype to version 2.8
rs=me for the import of freetype 2.8 here, which looks fine (though it can't actually land until we have all the associated test fixups and other patches ready to go with it).
Attachment #8867512 -
Flags: feedback?(jfkthame) → review+
Updated•7 years ago
|
Attachment #8899436 -
Flags: review?(ryanvm) → review+
Assignee | ||
Comment 48•7 years ago
|
||
Try run with the two patches from comment 46 and comment 47 added, to check that we can now build on Windows:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cd0c939fc19ca1aa0edf6fa588cf2d123159dd8
Updated•7 years ago
|
Attachment #8899434 -
Flags: review?(jwatt) → review+
Comment 49•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #42)
> ::: gfx/layers/apz/test/reftest/reftest.list
> @@ +4,3 @@
> > fuzzy-if(Android,4,5) skip-if(!Android) pref(apz.allow_zooming,true) == async-scrollbar-1-h.html async-scrollbar-1-h-ref.html
> > fuzzy-if(Android,3,5) skip-if(!Android) pref(apz.allow_zooming,true) == async-scrollbar-1-vh.html async-scrollbar-1-vh-ref.html
> > +fuzzy-if(Android,43,8) skip-if(!Android) pref(apz.allow_zooming,true) == async-scrollbar-1-v-rtl.html async-scrollbar-1-v-rtl-ref.html
>
> This change -- from fuzzy-if(Android,1,2) to fuzzy-if(Android,43,8) -- is
> quite large. The old annotation was extremely subtle (a rounding error on 2
> pixels), whereas the new annotation is arguably a test failure (8 pixels
> that differ by quite a lot). Looking at the reftest snapshot, the scrollbar
> is simply at a different position between the testcase & reference case --
> it's 1px lower in the reference case than in the testcase, I think (and this
> produces 4px of mismatch at the top and 4px at the bottom, from places where
> one file is painting the scrollbar edges and the other is not).
I see bug 1392259 was filed for this, which is great. It's clear that the freetype update doesn't actually break async scrollbar dragging; this reftest must be getting affected accidentally. To ensure that the now-large fuzzy value doesn't paper over future failures though, I'd like to see the fuzzy-if annotation use precise ranges, i.e. fuzzy-if(Android,43-43,8-8). That way if any future change affects this reftest behaviour even by a pixel we can re-examine it to ensure the behaviour hasn't been broken.
Assignee | ||
Comment 50•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d653fc499c9b9c30b16c8ffd090f0f3553a41cfb
Bug 1364714 - Adjust fuzzy() annotation for box-decoration reftest. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/40e902ed7dbe983d096fa7d6218db29152119d8b
Bug 1364714 - Fix fragile reference case for multicol-gap-001 test. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a297dc40177e8a2face062375e2ff4e22a693af0
Bug 1364714 - Set explicit line-height on background-position-vrl-018-ref file for better reliability. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/b02a1d99e6030a631091c6ed15681f79ba779a1f
Bug 1364714 - Set explicit line-height in position-relative tests, to avoid fragile dependence on default font's metrics. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/7634973c46ade547944ce62415cbfdfde20adca2
Bug 1364714 - Modify text to avoid possibly-overflowing descender in last line of orthogonal float. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa5a89aac5ea25ea64c36d2fc50254f60814850a
Bug 1364714 - Add fuzzy() annotation to mathml-mmultiscript test to allow for changed freetype rendering. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd5a2d0ea09febca69e415c931697a445c65a45
Bug 1364714 - Adjust fuzzy() annotation for table-background test to allow for change in freetype's rendering. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/80ca8faf1dac1f487946ad6564769a042e4b5b09
Bug 1364714 - Adjust fuzzy() annotation for pre-line-1 test to account for change in freetype's font rendering. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b915e42b36d92cb47494cb85c5447e8526b09d1
Bug 1364714 - Increase line-height in some ruby reftests to allow for default font with larger metrics. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f758a7673d3d87cfaf815ba793998051631ff7d
Bug 1364714 - Adjust dimensions in fixed-table-layout-010 tests to allow for increased font metrics without disrupting layout of test elements. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0cf94cb8666e16dad764d0b49d79e3188c3bc6e
Bug 1364714 - Adjust reftest text-emphasis-line-height-001z.html to avoid potential unwanted scrollbar. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e26a8c07caee0885fef4d1359e1d0b37c69b8db
Bug 1364714 - Adjust fuzzy() annotations on async-scrollbar tests. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff188112de35e14dacd836293e477f5f70ca79c5
Bug 1364714 - Patch PDFium for compatibility with freetype 2.8, where the ft_get_adobe_glyph_index function is guarded by DEFINE_PS_TABLES. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3dda4fe9dae7d8afafd7d705a698e845c8a12fc
Bug 1364714 - Update Freetype to version 2.8. r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f52db2530d3c55240ce9535be3a8d94d287a37a
Bug 1364714 - Cherry-pick commit c87fec0299533fee9b20505a4955c7f2f5c5bed7 from upstream freetype, to avoid bloating the build with duplicate PSname data tables due to PDFium's use of ft_get_adobe_glyph_index. r=ryanvm
Assignee | ||
Comment 51•7 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #49)
> I see bug 1392259 was filed for this, which is great. It's clear that the
> freetype update doesn't actually break async scrollbar dragging; this
> reftest must be getting affected accidentally. To ensure that the now-large
> fuzzy value doesn't paper over future failures though, I'd like to see the
> fuzzy-if annotation use precise ranges, i.e. fuzzy-if(Android,43-43,8-8).
> That way if any future change affects this reftest behaviour even by a pixel
> we can re-examine it to ensure the behaviour hasn't been broken.
Sorry, I saw this comment right after pushing the patches above. :( I'll push a trivial follow-up to tighten the fuzzy-if as requested.
Assignee | ||
Comment 52•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b00b74e9d34bc219ac3c70fedb7efd79c78268ce
Bug 1364714 followup, use precise fuzzy-if() range in APZ scroll reftest annotations. r=me
Comment 53•7 years ago
|
||
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b00b74e9d34b
followup, use precise fuzzy-if() range in APZ scroll reftest annotations. r=me
Comment 54•7 years ago
|
||
Thanks!
I had to back these out for android reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=124579672&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/bccc691242c5460e58c6aac1bd3fd63d15037b4a
Flags: needinfo?(jfkthame)
Comment 56•7 years ago
|
||
Backout by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/0267fb03043a
Backed out 16 changesets for android reftest failures in out-of-bounds-selectedindex.html a=backout
Assignee | ||
Comment 57•7 years ago
|
||
Sigh.... I thought I had enough tryserver coverage to be safe (e.g. comment 48), but apparently the android fuzziness on this test is debug-only, and it increased by one pixel. :( Re-landing with the annotation adjusted accordingly.
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 58•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6cd9dc49d54e193186c28fbe000da3630ca7e9
Bug 1364714 - (re-landing) Update Freetype to version 2.8, and fix/annotate tests affected by changes to font metrics & rasterization. r=dholbert,jwatt,ryanvm,jfkthame
Comment 59•7 years ago
|
||
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6cd9dc49d5
(re-landing) Update Freetype to version 2.8, and fix/annotate tests affected by changes to font metrics & rasterization. r=dholbert,jwatt,ryanvm,jfkthame
Comment 60•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 61•7 years ago
|
||
Improvements noticed. Thanks for landing these!
== Change summary for alert #8900 (as of August 21 2017 13:48 UTC) ==
Improvements:
2% compiler warnings summary windows2012-64 pgo 154.00 -> 151.00
2% compiler warnings summary windows2012-64 opt 154.00 -> 151.00
2% compiler warnings summary windows2012-64-noopt debug 160.00 -> 157.00
2% compiler warnings summary windows2012-64 debug 160.00 -> 157.00
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8900
You need to log in
before you can comment on or make changes to this bug.
Description
•