Bug 81329 - Fallback to common script when per-script font setting is the empty string
Summary: Fallback to common script when per-script font setting is the empty string
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matt Falkenhagen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-16 03:51 PDT by Matt Falkenhagen
Modified: 2012-03-18 23:58 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.95 KB, patch)
2012-03-16 04:00 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Patch (4.74 KB, patch)
2012-03-18 22:07 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Patch (4.48 KB, patch)
2012-03-18 22:25 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Falkenhagen 2012-03-16 03:51:51 PDT
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.
Comment 1 Matt Falkenhagen 2012-03-16 04:00:21 PDT
Created attachment 132242 [details]
Patch
Comment 2 Kent Tamura 2012-03-18 19:24:27 PDT
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.
Comment 3 Matt Falkenhagen 2012-03-18 22:07:23 PDT
Created attachment 132544 [details]
Patch
Comment 4 Matt Falkenhagen 2012-03-18 22:09:20 PDT
(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.
Comment 5 Kent Tamura 2012-03-18 22:18:17 PDT
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();
Comment 6 Matt Falkenhagen 2012-03-18 22:25:42 PDT
Created attachment 132545 [details]
Patch
Comment 7 Kent Tamura 2012-03-18 22:39:08 PDT
Comment on attachment 132545 [details]
Patch

ok
Comment 8 WebKit Review Bot 2012-03-18 23:58:36 PDT
Comment on attachment 132545 [details]
Patch

Clearing flags on attachment: 132545

Committed r111157: <http://trac.webkit.org/changeset/111157>
Comment 9 WebKit Review Bot 2012-03-18 23:58:41 PDT
All reviewed patches have been landed.  Closing bug.