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, gustavo, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Michael Catanzaro
Reported 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.
Attachments
Patch (3.50 KB, patch)
2019-02-13 15:18 PST, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Patch (3.68 KB, patch)
2019-02-23 13:32 PST, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Patch (2.64 KB, patch)
2019-02-25 06:31 PST, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Michael Catanzaro
Comment 1 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.
Michael Catanzaro
Comment 2 2019-02-13 15:18:43 PST
Created attachment 361945 [details] Patch
EWS Watchlist
Comment 3 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
Carlos Garcia Campos
Comment 4 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.
Michael Catanzaro
Comment 5 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.
Michael Catanzaro
Comment 6 2019-02-23 13:32:10 PST
Created attachment 362833 [details] Patch
Carlos Garcia Campos
Comment 7 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.
Michael Catanzaro
Comment 8 2019-02-25 06:31:24 PST
Created attachment 362898 [details] Patch
WebKit Commit Bot
Comment 9 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>
WebKit Commit Bot
Comment 10 2019-02-25 08:10:38 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.