Bug 194614

Summary: [WPE][GTK] Clean up handling of WEBKIT_FORCE_COMPLEX_TEXT
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, bugs-noreply, cgarcia, commit-queue, ews-watchlist, gns, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Michael Catanzaro 2019-02-13 14:49:30 PST
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.
Comment 1 Michael Catanzaro 2019-02-13 15:03:05 PST
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.
Comment 2 Michael Catanzaro 2019-02-13 15:18:43 PST
Created attachment 361945 [details]
Patch
Comment 3 EWS Watchlist 2019-02-13 15:19:49 PST
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
Comment 4 Carlos Garcia Campos 2019-02-14 00:58:00 PST
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.
Comment 5 Michael Catanzaro 2019-02-14 10:00:47 PST
(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.
Comment 6 Michael Catanzaro 2019-02-23 13:32:10 PST
Created attachment 362833 [details]
Patch
Comment 7 Carlos Garcia Campos 2019-02-25 02:07:31 PST
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.
Comment 8 Michael Catanzaro 2019-02-25 06:31:24 PST
Created attachment 362898 [details]
Patch
Comment 9 WebKit Commit Bot 2019-02-25 08:10:37 PST
Comment on attachment 362898 [details]
Patch

Clearing flags on attachment: 362898

Committed r242042: <https://trac.webkit.org/changeset/242042>
Comment 10 WebKit Commit Bot 2019-02-25 08:10:38 PST
All reviewed patches have been landed.  Closing bug.