Bug 194614 – [WPE][GTK] Clean up handling of WEBKIT_FORCE_COMPLEX_TEXT

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

In WebProcessPool::platformInitializeWebProcess:

#if PLATFORM(GTK)
    // This is misnamed. It can only be used to disable complex text.
    parameters.shouldAlwaysUseComplexTextCodePath = true;
    const char* forceComplexText = getenv("WEBKIT_FORCE_COMPLEX_TEXT");
    if (forceComplexText && !strcmp(forceComplexText, "0"))
        parameters.shouldAlwaysUseComplexTextCodePath = m_alwaysUsesComplexTextCodePath;
#endif

But this doesn't make sense, for two reasons:

 (1) WEBKIT_FORCE_COMPLEX_TEXT=1 is ignored and does nothing, because complex text is used by default, which is confusing. It would be better to name this WEBKIT_DISABLE_COMPLEX_TEXT.

 (2) I would expect WEBKIT_FORCE_COMPLEX_TEXT=0 to set parameters.shouldAlwaysUseComplexTextCodePath = false, but it doesn't. The envvar just causes the code to respect m_alwaysUsesComplexTextCodePath. WebProcessPool::setAlwaysUsesComplexTextCodePath is therefore broken unless WEBKIT_FORCE_COMPLEX_TEXT=0 is specified.

[Michael Catanzaro]

Let's make WEBKIT_FORCE_COMPLEX_TEXT=0 disable complex text, and WEBKIT_FORCE_COMPLEX_TEXT=1 or any other value enable it, for both WPE and GTK.

After landing, we need to update https://trac.webkit.org/wiki/EnvironmentVariables.

[Michael Catanzaro]

Created attachment 361945 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 361945 [details]
Patch

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

> Source/WebKit/ChangeLog:16
> +        Also, move this code up to WebKitWebContext and use setAlwaysUsesComplexTextCodePath;
> +        otherwise, setAlwaysUsesComplexTextCodePath doesn't work as intended.

Why not? it has worked so far. That means we can't use it to run layout tests with complex text enabled/disabled.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:372
> +#if PLATFORM(GTK)
> +    else
> +        priv->processPool->setAlwaysUsesComplexTextCodePath(true);
> +#endif

I don't like the ifdefs in the middle of an if, I prefer to use a local variable to define the default value using ifdefs.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #4)
> Why not? it has worked so far. That means we can't use it to run layout
> tests with complex text enabled/disabled.

Oh wow, you're right.

I was trying to make setAlwaysUseComplexTextCodePath actually work. Currently, if you call setAlwaysUseComplexTextCodePath(false) then on GTK port the value will still be true. That's not good. I'll find another way.

> > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:372
> > +#if PLATFORM(GTK)
> > +    else
> > +        priv->processPool->setAlwaysUsesComplexTextCodePath(true);
> > +#endif
> 
> I don't like the ifdefs in the middle of an if, I prefer to use a local
> variable to define the default value using ifdefs.

Sure.

[Michael Catanzaro]

Created attachment 362833 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 362833 [details]
Patch

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

> Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp:77
> +    if (const char* forceComplexText = getenv("WEBKIT_FORCE_COMPLEX_TEXT"))
> +        setAlwaysUsesComplexTextCodePath(strcmp(forceComplexText, "0"));

Since we know there aren't any processes at this point, you could simply set the value of m_alwaysUsesComplexTextCodePath. You can initialize it here also when the env var is not present to avoid adding ifdefs in cross platform file.

[Michael Catanzaro]

Created attachment 362898 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 362898 [details]
Patch

Clearing flags on attachment: 362898

Committed r242042: <https://trac.webkit.org/changeset/242042>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.