Bug 88845 – LocaleToScriptMappingDefault.cpp should also do what scriptCodeForFontSelection does in LocaleToScriptMappingICU.cpp

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Matt Falkenhagen]

LocaleToScriptMappingICU.cpp treats certain script codes as a single script for font selection purposes. For example "Hira", "Kana", "Jpan", and "Hrkt" are all treated as "Hrkt". This way we only have to set fonts for the "Hrkt" script in Settings, and have it be used for all of the Japanese scripts.

LocaleToScriptMappingDefault.cpp should do the same thing.

[Matt Falkenhagen]

Created attachment 147042 [details]
Patch

[Matt Falkenhagen]

Uploaded a patch.

[Tony Chang]

Comment on attachment 147042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147042&action=review

Which ports use LocaleToScriptMappingICU and which use LocaleToScriptMappingDefault?

> Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp:49
> +    // Treat certain families of script codes as a single script for assigning a per-script font in Settings.
> +    static const UScriptCode japaneseScript = USCRIPT_KATAKANA_OR_HIRAGANA;
> +    static const UScriptCode koreanScript = USCRIPT_HANGUL;

Why do we use separate variables for these rather than just inlining USCRIPT_KATAKANA_OR_HIRAGANA and USCRIPT_HANGUL?

[Tony Chang]

It looks like Chromium uses Default.cpp.  Should it use ICU.cpp instead?

[Matt Falkenhagen]

(In reply to comment #3)
> (From update of attachment 147042 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147042&action=review
> 
> Which ports use LocaleToScriptMappingICU and which use LocaleToScriptMappingDefault?

Right now everyone uses Default. See <http://trac.webkit.org/changeset/110541>

> 
> > Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp:49
> > +    // Treat certain families of script codes as a single script for assigning a per-script font in Settings.
> > +    static const UScriptCode japaneseScript = USCRIPT_KATAKANA_OR_HIRAGANA;
> > +    static const UScriptCode koreanScript = USCRIPT_HANGUL;
> 
> Why do we use separate variables for these rather than just inlining USCRIPT_KATAKANA_OR_HIRAGANA and USCRIPT_HANGUL?

It seemed to have better readability this way and I thought the compiler would inline it anyway. I can change to inlining them.

[Tony Chang]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 147042 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=147042&action=review
> > 
> > Which ports use LocaleToScriptMappingICU and which use LocaleToScriptMappingDefault?
> 
> Right now everyone uses Default. See <http://trac.webkit.org/changeset/110541>

I see, so this would change font selection behavior for all ports. Is it possible to make a layout test demonstrating this change?  Is this more consistent with other browsers like Firefox, IE or Opera?

> > > Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp:49
> > > +    // Treat certain families of script codes as a single script for assigning a per-script font in Settings.
> > > +    static const UScriptCode japaneseScript = USCRIPT_KATAKANA_OR_HIRAGANA;
> > > +    static const UScriptCode koreanScript = USCRIPT_HANGUL;
> > 
> > Why do we use separate variables for these rather than just inlining USCRIPT_KATAKANA_OR_HIRAGANA and USCRIPT_HANGUL?
> 
> It seemed to have better readability this way and I thought the compiler would inline it anyway. I can change to inlining them.

I think it would be more consistent if they were all USCRIPT_.

[Tony Chang]

(In reply to comment #6)
> I see, so this would change font selection behavior for all ports. Is it possible to make a layout test demonstrating this change?  Is this more consistent with other browsers like Firefox, IE or Opera?

Oh, sorry, there is a test.  I'm still curious about the second question.  Also, are there sites currently using lang=ja-{Hira,Hrkt,Jpan,Kana}?

[Matt Falkenhagen]

I'll make the change to use USCRIPT_.

(In reply to comment #7)
> (In reply to comment #6)
> > I see, so this would change font selection behavior for all ports. Is it possible to make a layout test demonstrating this change?  Is this more consistent with other browsers like Firefox, IE or Opera?

I think the other browsers don't really look at script and just use the language. I tested on Firefox, and it uses the font preference for Japanese to render all of "lang=ja-{Hira,Hrkt,Jpan,Kana}", as this patch would do.

> 
> Oh, sorry, there is a test.  I'm still curious about the second question.  Also, are there sites currently using lang=ja-{Hira,Hrkt,Jpan,Kana}?

It seems really rare, if it happens at all. I actually only noticed this because my demo to show per-script font settings uses the script directly to show how sample text looks under different per-script font settings.

I think no one would want different settings for Hira/Hrkt/Jpan/Kana though, so they should all be rendered with the same setting.

[Matt Falkenhagen]

Created attachment 147460 [details]
Patch

[Kent Tamura]

Comment on attachment 147460 [details]
Patch

Looks ok

[Matt Falkenhagen]

Comment on attachment 147460 [details]
Patch

Thanks for the reviews.

[WebKit Review Bot]

Comment on attachment 147460 [details]
Patch

Clearing flags on attachment: 147460

Committed r120303: <http://trac.webkit.org/changeset/120303>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.