Summary: | [Chromium] Vertical positions are off for some Arabic glyphs on Linux | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenichi Ishibashi <bashi> | ||||||||
Component: | Text | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bashi, commit-queue, evan, jshin, tony | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
URL: | http://crbug.com/76458 | ||||||||||
Attachments: |
|
Description
Kenichi Ishibashi
2011-04-22 02:08:16 PDT
Created attachment 90682 [details]
Patch V0
Comment on attachment 90682 [details] Patch V0 View in context: https://bugs.webkit.org/attachment.cgi?id=90682&action=review This patch looks perfect. I only have very minor comments. > LayoutTests/platform/chromium-linux/fast/text/arabic-vertical-offset.html:7 > + src: url(data:font/opentype;base64,AAEAAAAQAQAABAAARkZUTVqB2JMAAAZoAAAAHEdERUYAOQAQAAAFuAAAACZHUE9TtHzC6QAABgAAAABmR1NVQkR2THUAAAXgAAAAIE9TLzJ8FWiUAAABiAAAAFZjbWFwB9EI6gAAAfQAAAFKY3Z0IAAhAnkAAANAAAAABGdhc3D//wADAAAFsAAAAAhnbHlmCNrWUwAAA1AAAACMaGVhZPax+w0AAAEMAAAANmhoZWEGOgOZAAABRAAAACRobXR4DQQAwwAAAeAAAAASbG9jYQBiAJoAAANEAAAADG1heHAASQA5AAABaAAAACBuYW1ldi8ayAAAA9wAAAGVcG9zdCbbGCcAAAV0AAAAOgABAAAAAQAAriJ80l8PPPUAiwPoAAAAAMnW2/EAAAAAydbb8QAh/6wDRAKaAAAACAACAAAAAAAAAAEAAAKa/6wAWgPoAAAAAANEAAEAAAAAAAAAAAAAAAAAAAAEAAEAAAAFAAgAAgAAAAAAAgAAAAEAAQAAAEAALgAAAAAAAQPoAfQABQAAAooCvAAAAIwCigK8AAAB4AAxAQIAAAIABgkAAAAAAAAAACAAAAAAAAAAAAAAAAAAUGZFZABABgYG4QMg/zgAWgKaAFQAAAABAAAAAAAAA+gAIQAAAAAD6AAAA+gAogFMAAAAAAADAAAAAwAAABwAAQAAAAAARAADAAEAAAAcAAQAKAAAAAYABAABAAIGBgbh//8AAAYGBuH///n9+SMAAQAAAAAAAAAAAQYAAAEAAAAAAAAAAQIAAAACAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAhAnkAAAAqACoAKgA4AEYAAgAhAAABKgKaAAMABwAusQEALzyyBwQA7TKxBgXcPLIDAgDtMgCxAwAvPLIFBADtMrIHBgH8PLIBAgDtMjMRIREnMxEjIQEJ6MfHApr9ZiECWAAAAQCiAPsDRAFRAAMAABMhFSGiAqL9XgFRVgAAAAEBTP+sAkwAqAADAAAlIRUhAUwBAP8AqPwAAAAAAA4ArgABAAAAAAAAAAAAAgABAAAAAAABAAUADwABAAAAAAACAAYAIwABAAAAAAADACEAbgABAAAAAAAEAAUAnAABAAAAAAAFABAAxAABAAAAAAAGAAUA4QADAAEECQAAAAAAAAADAAEECQABAAoAAwADAAEECQACAAwAFQADAAEECQADAEIAKgADAAEECQAEAAoAkAADAAEECQAFACAAogADAAEECQAGAAoA1QAAAAB0AGUAcwB0ADEAAHRlc3QxAABNAGUAZABpAHUAbQAATWVkaXVtAABGAG8AbgB0AEYAbwByAGcAZQAgADIALgAwACAAOgAgAHQAZQBzAHQAMQAgADoAIAAyADIALQA0AC0AMgAwADEAMQAARm9udEZvcmdlIDIuMCA6IHRlc3QxIDogMjItNC0yMDExAAB0AGUAcwB0ADEAAHRlc3QxAABWAGUAcgBzAGkAbwBuACAAMAAwADEALgAwADAAMAAgAABWZXJzaW9uIDAwMS4wMDAgAAB0AGUAcwB0ADEAAHRlc3QxAAAAAAACAAAAAAAA/4MAMgAAAAEAAAAAAAAAAAAAAAAAAAAAAAUAAAABAAIBAgEDBkdseXBoMQZHbHlwaDIAAAAAAAH//wACAAEAAAAOAAAAHgAAAAAAAgACAAMAAwABAAQABAADAAQAAAACAAAAAAABAAAACgAcAB4AAURGTFQACAAEAAAAAP//AAAAAAAAAAEAAAAKAB4ALAABREZMVAAIAAQAAAAA//8AAQAAAAFtYXJrAAgAAAABAAAAAQAEAAQAAAABAAgAAQAcABYAAQAiAAwAAQAEAAEB9AIeAAEAAQADAAEAAQAEAAEAAAAGAAEBzAAoAAAAAAABAAAAAMbULpkAAAAAydbSQAAAAADJ1te5); Did you create this font yourself? (I am worried about licensing) > Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:64 > + ComplexTextController(const TextRun&, unsigned, unsigned, const Font*); Is it necessary to take the startingY as an argument since the users always want 0? (Now that I look, maybe both of these unsigned arguments aren't needed; they might be left over from previous refactorings.) Comment on attachment 90682 [details] Patch V0 View in context: https://bugs.webkit.org/attachment.cgi?id=90682&action=review >> LayoutTests/platform/chromium-linux/fast/text/arabic-vertical-offset.html:7 >> + src: url(data:font/opentype;base64,AAEAAAAQAQAABAAARkZUTVqB2JMAAAZoAAAAHEdERUYAOQAQAAAFuAAAACZHUE9TtHzC6QAABgAAAABmR1NVQkR2THUAAAXgAAAAIE9TLzJ8FWiUAAABiAAAAFZjbWFwB9EI6gAAAfQAAAFKY3Z0IAAhAnkAAANAAAAABGdhc3D//wADAAAFsAAAAAhnbHlmCNrWUwAAA1AAAACMaGVhZPax+w0AAAEMAAAANmhoZWEGOgOZAAABRAAAACRobXR4DQQAwwAAAeAAAAASbG9jYQBiAJoAAANEAAAADG1heHAASQA5AAABaAAAACBuYW1ldi8ayAAAA9wAAAGVcG9zdCbbGCcAAAV0AAAAOgABAAAAAQAAriJ80l8PPPUAiwPoAAAAAMnW2/EAAAAAydbb8QAh/6wDRAKaAAAACAACAAAAAAAAAAEAAAKa/6wAWgPoAAAAAANEAAEAAAAAAAAAAAAAAAAAAAAEAAEAAAAFAAgAAgAAAAAAAgAAAAEAAQAAAEAALgAAAAAAAQPoAfQABQAAAooCvAAAAIwCigK8AAAB4AAxAQIAAAIABgkAAAAAAAAAACAAAAAAAAAAAAAAAAAAUGZFZABABgYG4QMg/zgAWgKaAFQAAAABAAAAAAAAA+gAIQAAAAAD6AAAA+gAogFMAAAAAAADAAAAAwAAABwAAQAAAAAARAADAAEAAAAcAAQAKAAAAAYABAABAAIGBgbh//8AAAYGBuH///n9+SMAAQAAAAAAAAAAAQYAAAEAAAAAAAAAAQIAAAACAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAhAnkAAAAqACoAKgA4AEYAAgAhAAABKgKaAAMABwAusQEALzyyBwQA7TKxBgXcPLIDAgDtMgCxAwAvPLIFBADtMrIHBgH8PLIBAgDtMjMRIREnMxEjIQEJ6MfHApr9ZiECWAAAAQCiAPsDRAFRAAMAABMhFSGiAqL9XgFRVgAAAAEBTP+sAkwAqAADAAAlIRUhAUwBAP8AqPwAAAAAAA4ArgABAAAAAAAAAAAAAgABAAAAAAABAAUADwABAAAAAAACAAYAIwABAAAAAAADACEAbgABAAAAAAAEAAUAnAABAAAAAAAFABAAxAABAAAAAAAGAAUA4QADAAEECQAAAAAAAAADAAEECQABAAoAAwADAAEECQACAAwAFQADAAEECQADAEIAKgADAAEECQAEAAoAkAADAAEECQAFACAAogADAAEECQAGAAoA1QAAAAB0AGUAcwB0ADEAAHRlc3QxAABNAGUAZABpAHUAbQAATWVkaXVtAABGAG8AbgB0AEYAbwByAGcAZQAgADIALgAwACAAOgAgAHQAZQBzAHQAMQAgADoAIAAyADIALQA0AC0AMgAwADEAMQAARm9udEZvcmdlIDIuMCA6IHRlc3QxIDogMjItNC0yMDExAAB0AGUAcwB0ADEAAHRlc3QxAABWAGUAcgBzAGkAbwBuACAAMAAwADEALgAwADAAMAAgAABWZXJzaW9uIDAwMS4wMDAgAAB0AGUAcwB0ADEAAHRlc3QxAAAAAAACAAAAAAAA/4MAMgAAAAEAAAAAAAAAAAAAAAAAAAAAAAUAAAABAAIBAgEDBkdseXBoMQZHbHlwaDIAAAAAAAH//wACAAEAAAAOAAAAHgAAAAAAAgACAAMAAwABAAQABAADAAQAAAACAAAAAAABAAAACgAcAB4AAURGTFQACAAEAAAAAP//AAAAAAAAAAEAAAAKAB4ALAABREZMVAAIAAQAAAAA//8AAQAAAAFtYXJrAAgAAAABAAAAAQAEAAQAAAABAAgAAQAcABYAAQAiAAwAAQAEAAEB9AIeAAEAAQADAAEAAQAEAAEAAAAGAAEBzAAoAAAAAAABAAAAAMbULpkAAAAAydbSQAAAAADJ1te5); > > Did you create this font yourself? (I am worried about licensing) Is it possible to load the font from disk? We normally put extra test files in the resources subdirectory (for example, platform/chromium-linux-fast/text/resources/). Hi Evan, Thank you for taking a look of this patch. (In reply to comment #2) > Did you create this font yourself? (I am worried about licensing) Yes, I've created it with FontForge from scratch so I think there is no licensing issue. I'll add an explanation of it. > > Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:64 > > + ComplexTextController(const TextRun&, unsigned, unsigned, const Font*); > > Is it necessary to take the startingY as an argument since the users always want 0? (Now that I look, maybe both of these unsigned arguments aren't needed; they might be left over from previous refactorings.) I might have wrong idea, but I couldn't find other way to pass the starting position to ComplexTextController. As far as I checked, the starting point is not always zero at FontLinux::drawComplexText(). Hi Tony, (In reply to comment #3) > Is it possible to load the font from disk? We normally put extra test files in the resources subdirectory (for example, platform/chromium-linux-fast/text/resources/). Thank you letting me know that. I'll put the font on resources directory in the next patch. Comment on attachment 90682 [details]
Patch V0
I think the change is fine, but please update the ChangeLog to say that you created the font and split the font into a separate file in resources/.
Created attachment 91057 [details]
Patch V1
Created attachment 91058 [details]
Thai script rendering result
Hi,
I've updated the patch to address Tony's comments. Thanks!
BTW, I noticed that the rendering results of some Thai scripts on Chromium (w/this patch) and Firefox are slightly different. Chromium w/this patch renders some marks a bit higher than Firefox. The attached image shows the rendering results of text U+0E17 U+0E4D U+0E2B U+0E49 (I don't understand Thai language so the text might be meaningless). This difference comes from the shaping result (the return value of HB_ShapeItem()) and I'm not sure this is correct rendering.
Regards,
See https://bugs.webkit.org/show_bug.cgi?id=53637 for some more background on a recent bug we had with getting a font metric wrong that caused glyph positions to be subtly off. I think we should land this patch, since we know it is correct for at least Arabic, and worry about the Thai issue separately if it's an issue at all. I know that Firefox usually has correct rendering so it is probably worth looking into, but until we fixed the above bug (which we only fixed a few months ago), Thai rendered completely wrong. So I don't worry about upsetting our nonexistent Thai userbase. :) Comment on attachment 91057 [details]
Patch V1
Thank you for review, Tony! Could you cq+?
Hi Evan, I get it. Thank you for the information! (In reply to comment #9) > See https://bugs.webkit.org/show_bug.cgi?id=53637 for some more background on a recent bug we had with getting a font metric wrong that caused glyph positions to be subtly off. > > I think we should land this patch, since we know it is correct for at least Arabic, and worry about the Thai issue separately if it's an issue at all. I know that Firefox usually has correct rendering so it is probably worth looking into, but until we fixed the above bug (which we only fixed a few months ago), Thai rendered completely wrong. So I don't worry about upsetting our nonexistent Thai userbase. :) Comment on attachment 91057 [details] Patch V1 Clearing flags on attachment: 91057 Committed r85013: <http://trac.webkit.org/changeset/85013> All reviewed patches have been landed. Closing bug. |