Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years ago by Daniel Bratell Modified:
5 years ago Reviewers:
drott, eae CC:
chromium-reviews, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, Rik, blink-reviews-platform-graphics_chromium.org, dshwang, jbroman, Justin Novosad, danakj, krit, f(malita), blink-reviews, vmpstr+blinkwatch_chromium.org, Stephen Chennney, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse Ebrima as fallback font for Tifinagh in Windows.
BUG=569421
Committed: https://crrev.com/85da7a3daf73142f5aedb9e193dc4a59bb0ad0f2
Cr-Commit-Position: refs/heads/master@{#366131}
Patch Set 1 #Patch Set 2 : Now with tests. #
Total comments: 2
Patch Set 3 : More testing #
Total comments: 2
Patch Set 4 : Drop tests that could be flaky. #Patch Set 5 : Changing text used for test. #Patch Set 6 : Fixed expected #
Created: 5 years ago
Messages
Total messages: 15 (4 generated)
bratell@opera.com changed reviewers: + eae@chromium.org
Can you take a look at this? It's a very small change, but I suspect there is more to it. (Neo) Tifinagh is supported by Microsoft since Windows 7 and I see other fonts listed here that were also added with Windows 7 (Segoa right?) so I guess that is ok.
drott@chromium.org changed reviewers: + drott@chromium.org
https://codereview.chromium.org/1525653002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/text/tifinagh.html (right): https://codereview.chromium.org/1525653002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/text/tifinagh.html:10: document.write("&#x" + charCode.toString(16) + "; (U+" + Thanks for adding a test, unless there is a specific reason to use document.write here, I would suggest to use static HTML entities such as &x2d30; or placing the Tifinagh text directly and adding a <meta charset="utf-8"> in the header instead of document.write.
https://codereview.chromium.org/1525653002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/text/tifinagh.html (right): https://codereview.chromium.org/1525653002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/text/tifinagh.html:10: document.write("&#x" + charCode.toString(16) + "; (U+" + On 2015/12/16 08:52:06, drott wrote: > Thanks for adding a test, unless there is a specific reason to use > document.write here, I would suggest to use static HTML entities such as &x2d30; > or placing the Tifinagh text directly and adding a <meta charset="utf-8"> in the > header instead of document.write. This is way easier to work with. We can easily check that we have not forgotten any glyphs, that the two blocks (monospace and default font) contain the same glyphs. That the labels and the glyphs are referencing the same codepoint. Since it's pure ASCII it will also not be vulnerable to misunderstandings by editors or other tools which is particularly important when working with glyphs that not every tool/environment can render. It's also possible to quickly reuse this test for other scripts. For instance https://codereview.chromium.org/1521993008/patch/1/10002 is the same with just 2-3 lines changed. Doing the same thing manually would be much more error prone. I was going to say that it loses a bit in reviewability, but then I reconsidered. Reviewing a list of a hundred glyphs, checking that they are all there, that they use the right code point, the right label, that would be much harder than reading this script and seeing the start and end of the loop. (I actually started by modifying an existing testcase that wrote the characters literally but it was not at all easy and quite timeconsuming so I switched to a scripted loop instead).
On 2015/12/16 09:17:24, Daniel Bratell wrote: > https://codereview.chromium.org/1525653002/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/fast/text/tifinagh.html (right): > > https://codereview.chromium.org/1525653002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/text/tifinagh.html:10: document.write("&#x" > + charCode.toString(16) + "; (U+" + > On 2015/12/16 08:52:06, drott wrote: > > Thanks for adding a test, unless there is a specific reason to use > > document.write here, I would suggest to use static HTML entities such as > &x2d30; > > or placing the Tifinagh text directly and adding a <meta charset="utf-8"> in > the > > header instead of document.write. > > This is way easier to work with. We can easily check that we have not forgotten > any glyphs, that the two blocks (monospace and default font) contain the same > glyphs. That the labels and the glyphs are referencing the same codepoint. > > Since it's pure ASCII it will also not be vulnerable to misunderstandings by > editors or other tools which is particularly important when working with glyphs > that not every tool/environment can render. I'd prefer if you could at the Tifinagh alphabet or a pangram: LayoutTests/inspector-protocol/layout-fonts/languages-emoji-rare-glyphs.html (It does not necessarily need to be the full alphabet: As long as the font gets picked on Windows for USCRIPT_TIFINAGH script and some sample characters, this would be sufficient as a test in my opinion.) In this test we keep track of font/language coverage and we can have easily verifiable baselines for multiple platforms, without resorting to a pixel test. As you see, this test contains characters from the full unicode range and our tooling can handle it, so no issues there. So then, for the Windows expectation we would expect the Ebrima font selected: LayoutTests/platform/win/inspector-protocol/layout-fonts/languages-emoji-rare-glyphs-expected.txt We can check the new baseline after the rebaseline bot processed it, or we see it right away if you could add the updated Windows test expectation to this CL and let the rebaseline bot just process the other platforms, where the fallback mechanism likely chooses a different font.
drott, please take a new look. I've added a new test using the inspector framework. I've kept the glyph test though because there are so much that could potentially go wrong after the font selection and we would most likely not have any way of knowing until after a full release. I also changed it from document.write to innerHTML though I believe document.write being the more readable solution. I know document.write sucks in many ways (I've probably spent hundreds of hours on document.write related issues and implementations) but it's useful in cases like this.
Hi Daniel, On 2015/12/17 09:23:53, Daniel Bratell wrote: > drott, please take a new look. I really appreciate it, thanks for taking the the time to update the test. Test and expectations look good to me for the new test, with a license nit on the source of the sample text. > I've added a new test using the inspector framework. I've kept the glyph test > though because there are so much that could potentially go wrong after the font > selection and we would most likely not have any way of knowing until after a > full release. In the past, we've had issues with test flakiness or extra work for rebaselining from tests that test for glyph rendering of rare glyphs due to randomness in fallback behavior. For example in fast/text/midword-break-before-surrogate-pair.html ) The test results sometimes flake due to differences between bots, especially on Linux were the bot fleet has different versions of Ubuntu (and differing glyphs for the .notdef glyph) or on Mac where the font configuration differs between OS versions. I agree it would be useful to also test whether the glyphs show up on screen, but to make the testing more robust, I'd prefer if this would be backed by a web font and test the fallback mechanism separately - or we can leave the glyph table test out for now - what's your opinion? > > I also changed it from document.write to innerHTML though I believe > document.write being the more readable solution. I know document.write sucks in > many ways (I've probably spent hundreds of hours on document.write related > issues and implementations) but it's useful in cases like this. https://codereview.chromium.org/1525653002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/tifinagh.html (right): https://codereview.chromium.org/1525653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/tifinagh.html:12: <div id="tifinagh_wikipedia_text"><!-- First part of https://incubator.wikimedia.org/wiki/Wp/shi/Igudar/ⵜⵉⴼⵉⵏⴰⵖ --> ⴰⴳⴰⴷⵉⵔ ⵉⵖ ⴳⴳⵓⵜⵏ ⴰⵔ ⴰⵙⵏ ⵏⵜⵜⵉⵏⵉ ⵉⴳⵓⴷⴰⵔ ⴳⴰⵏ ⵢⴰⵜ ⵍⴱⴰⵏⴽ ⵜⴰⵎⵣⵡⴰⵔⵓⵜ ⵏⵉⵖ ⵜⴰⴱⵍⴷⵉⵜ ⴰⵔ ⴳⵉⵙ ⵜⵜⴳⴳⴰⵏ ⵎⵉⴷⴷⵏ ⵜⵓⵎⵥⵉⵏ ⵏⵖ ⵉⵔⴷⵏ ⵏⵖ ⵉⵇⵇⴰⵔⵉⴷⵏ ⴰⴳⴰⴷⵉⵔ ⵉⴳⴰ ⵍⴱⴰⵏⴽ ⵍⵍⵉ ⴷ ⴽⵓⵍⵍⵓ ⵉⵣⵡⴰⵔⵏ ⵖ ⴷⴷⵓⵏⵉⵜ ⵍⵍⴰⵏ ⵜⵓⴳⵜ ⵏ ⵉⴳⵓⴷⴰⵔ ⵖ ⵜⵙⴳⴰ ⵏ ⵙⵓⵙ Could you perhaps find a non-copyrighted sample text (perhaps there is some publicly available literature), or just use the alphabet? I don't think we can include content from Wikipedia into our tests.
drott, this might be better! https://codereview.chromium.org/1525653002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/tifinagh.html (right): https://codereview.chromium.org/1525653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/tifinagh.html:12: <div id="tifinagh_wikipedia_text"><!-- First part of https://incubator.wikimedia.org/wiki/Wp/shi/Igudar/ⵜⵉⴼⵉⵏⴰⵖ --> ⴰⴳⴰⴷⵉⵔ ⵉⵖ ⴳⴳⵓⵜⵏ ⴰⵔ ⴰⵙⵏ ⵏⵜⵜⵉⵏⵉ ⵉⴳⵓⴷⴰⵔ ⴳⴰⵏ ⵢⴰⵜ ⵍⴱⴰⵏⴽ ⵜⴰⵎⵣⵡⴰⵔⵓⵜ ⵏⵉⵖ ⵜⴰⴱⵍⴷⵉⵜ ⴰⵔ ⴳⵉⵙ ⵜⵜⴳⴳⴰⵏ ⵎⵉⴷⴷⵏ ⵜⵓⵎⵥⵉⵏ ⵏⵖ ⵉⵔⴷⵏ ⵏⵖ ⵉⵇⵇⴰⵔⵉⴷⵏ ⴰⴳⴰⴷⵉⵔ ⵉⴳⴰ ⵍⴱⴰⵏⴽ ⵍⵍⵉ ⴷ ⴽⵓⵍⵍⵓ ⵉⵣⵡⴰⵔⵏ ⵖ ⴷⴷⵓⵏⵉⵜ ⵍⵍⴰⵏ ⵜⵓⴳⵜ ⵏ ⵉⴳⵓⴷⴰⵔ ⵖ ⵜⵙⴳⴰ ⵏ ⵙⵓⵙ On 2015/12/18 11:44:52, drott wrote: > Could you perhaps find a non-copyrighted sample text (perhaps there is some > publicly available literature), or just use the alphabet? I don't think we can > include content from Wikipedia into our tests. Done.
LGTM, thanks!
The CQ bit was checked by drott@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525653002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525653002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Use Ebrima as fallback font for Tifinagh in Windows. BUG=569421 ========== to ========== Use Ebrima as fallback font for Tifinagh in Windows. BUG=569421 Committed: https://crrev.com/85da7a3daf73142f5aedb9e193dc4a59bb0ad0f2 Cr-Commit-Position: refs/heads/master@{#366131} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/85da7a3daf73142f5aedb9e193dc4a59bb0ad0f2 Cr-Commit-Position: refs/heads/master@{#366131} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
