Bug 81329 – Fallback to common script when per-script font setting is the empty string

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Matt Falkenhagen]

Currently, when a per-script font setting is not set, we fall back to the default, common script font setting. For example, standardFontFamily(USCRIPT_ARABIC) will return standardFontFamily(USCRIPT_COMMON) if setStandardFontFamily("Font", USCRIPT_ARABIC) was never called.

However, there is no way to unset a font setting, so once a per-script font setting is set, it can never fallback to the common script font setting. It'd be useful to allow this, so it can be possible for a user to clear their font setting for Arabic script and just use their setting for the default script. One way to do this is by treating the empty string the same as an unset setting.

[Matt Falkenhagen]

Created attachment 132242 [details]
Patch

[Kent Tamura]

Comment on attachment 132242 [details]
Patch

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

> Source/WebCore/page/Settings.cpp:67
> -    if (it != fontMap.end())
> +    if (it != fontMap.end() && !it->second.isEmpty())

This is ok, but can we remove the map entry when an empty font family name is set? It will reduce memory usage.

[Matt Falkenhagen]

Created attachment 132544 [details]
Patch

[Matt Falkenhagen]

(In reply to comment #2)
> (From update of attachment 132242 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132242&action=review
> 
> > Source/WebCore/page/Settings.cpp:67
> > -    if (it != fontMap.end())
> > +    if (it != fontMap.end() && !it->second.isEmpty())
> 
> This is ok, but can we remove the map entry when an empty font family name is set? It will reduce memory usage.

Thanks, uploaded revised patch.

[Kent Tamura]

Comment on attachment 132544 [details]
Patch

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

> Source/WebCore/page/Settings.cpp:71
> +    if (family.isEmpty()) {
> +        if (it != fontMap.end()) {
> +            fontMap.remove(it);
> +            changed = true;
> +        }
> +    } else if (it == fontMap.end() || it->second != family) {
> +        fontMap.set(static_cast<int>(script), family);
> +        changed = true;
> +    }
> +
> +    if (changed)
> +        page->setNeedsRecalcStyleInAllFrames();

The variable 'changed' is not needed.
We prefer early-return.  The code should be:

if (family.isEmpty()) {
    if (it == fontMap.end())
        return;
    fontMap.remove(it);
} else if (it != fontMap.end() && it->second == family)
    return;
else
    fontMap.set(...);
page->setNeedsRecalcStyleInAllFrames();

[Matt Falkenhagen]

Created attachment 132545 [details]
Patch

[Kent Tamura]

Comment on attachment 132545 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 132545 [details]
Patch

Clearing flags on attachment: 132545

Committed r111157: <http://trac.webkit.org/changeset/111157>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.