183285 2018-03-02 04:55:32 -0800 [GTK] Switch to use always complex text code path 2019-10-28 21:15:54 -0700 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=167557 https://bugs.webkit.org/show_bug.cgi?id=203544 Gtk P2 Normal --- 1 cgarcia webkit-unassigned bugs-noreply mcatanzaro oldest_to_newest 1403199 0 cgarcia 2018-03-02 04:55:32 -0800 Now that we have branched for 2.20, it's a good time to try using complex text path always. We can simply force it for GTK+ port and see how it works for the whole release cycle and if we don't notice any issues or performance regressions we release 2.22 with complex text path forced. We can add a debug env variable to switch back to auto without having to recompile. We should also keep the auto mode for the layout tests to avoid having to rebaseline a lot of tests. After 2.22 is released we can make a final decision and remove the env variable. 1403200 1 334892 cgarcia 2018-03-02 05:00:11 -0800 Created attachment 334892 Patch 1403228 2 334892 mcatanzaro 2018-03-02 08:30:26 -0800 Comment on attachment 334892 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334892&action=review > Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:96 > + const char* forceComplexText = getenv("WEBKIT_FORCE_COMPLEX_TEXT"); The semantics of this are backwards. Should be either WEBKIT_FORCE_SIMPLE_TEXT=1 or WEBKIT_DISABLE_COMPLEX_TEXT=1. Let's not check for 0. > Tools/ChangeLog:8 > + Keep the auto mode for the layout tests to avoid having to rebaseline a lot of tests. I don't think this is a great idea... now we are not testing the code that users are actually running anymore. I would do a rebaseline at the same time. 1403230 3 cgarcia 2018-03-02 08:37:16 -0800 (In reply to Michael Catanzaro from comment #2) > Comment on attachment 334892 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=334892&action=review > > > Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:96 > > + const char* forceComplexText = getenv("WEBKIT_FORCE_COMPLEX_TEXT"); > > The semantics of this are backwards. > > Should be either WEBKIT_FORCE_SIMPLE_TEXT=1 or WEBKIT_DISABLE_COMPLEX_TEXT=1. This doesn't force simple text and it doesn't disable complex text either. It either forces complex text or leaves the current auto where complex or simple is used depending on the context. > Let's not check for 0. I don't see why. > > Tools/ChangeLog:8 > > + Keep the auto mode for the layout tests to avoid having to rebaseline a lot of tests. > > I don't think this is a great idea... now we are not testing the code that > users are actually running anymore. I would do a rebaseline at the same time. The default is auto, so complex test is still tested when required. Most of the changes that we will have to rebaseline are in text that doesn't affect the test at all, like text explaining what the test does, for example. The idea is to try it our during the next release cycle in the unstable releases, and once we make a decision we do the rebaseline. I don't want to waste the time doing a huge rebaseline to roll out the patch tomorrow. 1403231 4 cgarcia 2018-03-02 08:38:01 -0800 Note that my main concern is performance, and our layout tests don't help with that, but real usage does. 1403239 5 cgarcia 2018-03-02 08:59:35 -0800 This is not different than GTK_OVERLAY_SCROLLING, for example, that defaults to enabled when not present or != "0" is used. You need to export GTK_OVERLAY_SCROLLING=0 to disable it. 1403243 6 mcatanzaro 2018-03-02 09:06:05 -0800 (In reply to Carlos Garcia Campos from comment #3) > This doesn't force simple text and it doesn't disable complex text either. > It either forces complex text or leaves the current auto where complex or > simple is used depending on the context. Good point. Other options: WEBKIT_DO_NOT_FORCE_COMPLEX_TEXT=1 WEBKIT_ALLOW_SIMPLE_TEXT=1 > > Let's not check for 0. > > I don't see why. It's very confusing to have an environment variable that only works when set to 0. > Most of > the changes that we will have to rebaseline are in text that doesn't affect > the test at all, like text explaining what the test does, for example. The > idea is to try it our during the next release cycle in the unstable > releases, and once we make a decision we do the rebaseline. I don't want to > waste the time doing a huge rebaseline to roll out the patch tomorrow. Fair enough, as long as you're confident you won't forget to complete this before 2.22. One more thing. The most likely reason to revert this change would be if we notice performance problems. But we are not actually going to notice performance problems, not without real perf testing. We do have a perf bot that I never look at, but it won't be able to test this change because the environment variable will be set in the test runner. How do you plan to evaluate the performance impact? 1403244 7 mcatanzaro 2018-03-02 09:07:43 -0800 (In reply to Carlos Garcia Campos from comment #5) > This is not different than GTK_OVERLAY_SCROLLING, for example, that defaults > to enabled when not present or != "0" is used. You need to export > GTK_OVERLAY_SCROLLING=0 to disable it. True, but that seems bad to me. Let's not copy that example, when it is *simple* to avoid. ;) WEBKIT_ALLOW_SIMPLE_TEXT=1 is fine, right? It's not a big deal, since we don't plan to keep this environment variable all the way until 2.22, but it would be nicer IMO. 1403245 8 mcatanzaro 2018-03-02 09:09:03 -0800 (In reply to Michael Catanzaro from comment #6) > How do you plan to evaluate the performance impact? Myles needs to do similar testing in https://bugs.webkit.org/show_bug.cgi?id=178960#c11, so we could ask him for advice. 1403524 9 cgarcia 2018-03-04 02:18:24 -0800 (In reply to Michael Catanzaro from comment #6) > (In reply to Carlos Garcia Campos from comment #3) > > This doesn't force simple text and it doesn't disable complex text either. > > It either forces complex text or leaves the current auto where complex or > > simple is used depending on the context. > > Good point. > > Other options: > > WEBKIT_DO_NOT_FORCE_COMPLEX_TEXT=1 > WEBKIT_ALLOW_SIMPLE_TEXT=1 > > > > Let's not check for 0. > > > > I don't see why. > > It's very confusing to have an environment variable that only works when set > to 0. I don't thin it's confusing and it's consistent with other env vars like force AC where we also check for 0, the only difference is that in this case the default is disabled instead of enabled. > > Most of > > the changes that we will have to rebaseline are in text that doesn't affect > > the test at all, like text explaining what the test does, for example. The > > idea is to try it our during the next release cycle in the unstable > > releases, and once we make a decision we do the rebaseline. I don't want to > > waste the time doing a huge rebaseline to roll out the patch tomorrow. > > Fair enough, as long as you're confident you won't forget to complete this > before 2.22. Sure. > One more thing. The most likely reason to revert this change would be if we > notice performance problems. But we are not actually going to notice > performance problems, not without real perf testing. We do have a perf bot > that I never look at, but it won't be able to test this change because the > environment variable will be set in the test runner. How do you plan to > evaluate the performance impact? I'm not worried about a performance regression, it's not a problem if complex text is a bit slower. My concern is only if performance i really noticeable and affects real usage. If we think a particular website is too slow, we can try disabling the complex text force and see if there's any significant difference. 1403525 10 cgarcia 2018-03-04 02:19:37 -0800 We can run the perf tests with complex text forced, note that WTR doesn't override the variable if present. 1413952 11 cgarcia 2018-04-12 00:02:06 -0700 Committed r230559: <https://trac.webkit.org/changeset/230559> 334892 2018-03-02 05:00:11 -0800 2018-03-04 08:48:58 -0800 Patch wkgtk-complex-text.diff text/plain 3329 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9DaGFu Z2VMb2cKaW5kZXggYjY2NmE1MTVhYzIuLjUwY2NiNGNhZjViIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViS2l0L0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2ViS2l0L0NoYW5nZUxvZwpAQCAtMSwzICsx LDIwIEBACisyMDE4LTAzLTAyICBDYXJsb3MgR2FyY2lhIENhbXBvcyAgPGNnYXJjaWFAaWdhbGlh LmNvbT4KKworICAgICAgICBbR1RLXSBTd2l0Y2ggdG8gdXNlIGFsd2F5cyBjb21wbGV4IHRleHQg Y29kZSBwYXRoCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xODMyODUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBOb3cgdGhhdCB3ZSBoYXZlIGJyYW5jaGVkIGZvciAyLjIwLCBpdCdzIGEgZ29vZCB0aW1lIHRv IHRyeSB1c2luZyBjb21wbGV4IHRleHQgcGF0aCBhbHdheXMuIFdlIGNhbiBzaW1wbHkgZm9yY2UK KyAgICAgICAgaXQgZm9yIEdUSysgcG9ydCBhbmQgc2VlIGhvdyBpdCB3b3JrcyBmb3IgdGhlIHdo b2xlIHJlbGVhc2UgY3ljbGUgYW5kIGlmIHdlIGRvbid0IG5vdGljZSBhbnkgaXNzdWVzIG9yCisg ICAgICAgIHBlcmZvcm1hbmNlIHJlZ3Jlc3Npb25zIHdlIHJlbGVhc2UgMi4yMiB3aXRoIGNvbXBs ZXggdGV4dCBwYXRoIGZvcmNlZC4gQSBkZWJ1ZyBlbnYgdmFyaWFibGUgaXMgYWRkZWQgdG8gc3dp dGNoCisgICAgICAgIGJhY2sgdG8gYXV0byB3aXRob3V0IGhhdmluZyB0byByZWNvbXBpbGUuIEFm dGVyIDIuMjIgaXMgcmVsZWFzZWQgd2UgY2FuIG1ha2UgYSBmaW5hbCBkZWNpc2lvbiBhbmQgcmVt b3ZlIHRoZSBlbnYKKyAgICAgICAgdmFyaWFibGUuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvZ3Rr L1dlYlByb2Nlc3NQb29sR3RrLmNwcDoKKyAgICAgICAgKFdlYktpdDo6V2ViUHJvY2Vzc1Bvb2w6 OnBsYXRmb3JtSW5pdGlhbGl6ZVdlYlByb2Nlc3MpOiBGb3JjZSBjb21wbGV4IHRleHQgY29kZSBw YXRoIHVubGVzcworICAgICAgICBXRUJLSVRfRk9SQ0VfQ09NUExFWF9URVhUIGlzIHByZXNlbnQg YW5kIHNldCB0byAwLgorCiAyMDE4LTAzLTAyICBDYXJsb3MgR2FyY2lhIENhbXBvcyAgPGNnYXJj aWFAaWdhbGlhLmNvbT4KIAogICAgICAgICBBdXRvbWF0aW9uOiBjbGlja2luZyBvbiBhIGRpc2Fi bGVkIG9wdGlvbiBlbGVtZW50IHNob3VsZG4ndCBwcm9kdWNlIGFuIGVycm9yCmRpZmYgLS1naXQg YS9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9ndGsvV2ViUHJvY2Vzc1Bvb2xHdGsuY3BwIGIvU291 cmNlL1dlYktpdC9VSVByb2Nlc3MvZ3RrL1dlYlByb2Nlc3NQb29sR3RrLmNwcAppbmRleCAzNzcx ZGM1NThiNS4uMDhhZDNlZjFlM2IgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNz L2d0ay9XZWJQcm9jZXNzUG9vbEd0ay5jcHAKKysrIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3Mv Z3RrL1dlYlByb2Nlc3NQb29sR3RrLmNwcApAQCAtOTIsNiArOTIsMTEgQEAgdm9pZCBXZWJQcm9j ZXNzUG9vbDo6cGxhdGZvcm1Jbml0aWFsaXplV2ViUHJvY2VzcyhXZWJQcm9jZXNzQ3JlYXRpb25Q YXJhbWV0ZXJzJgogICAgIHBhcmFtZXRlcnMubWVtb3J5Q2FjaGVEaXNhYmxlZCA9IG1fbWVtb3J5 Q2FjaGVEaXNhYmxlZCB8fCBjYWNoZU1vZGVsKCkgPT0gQ2FjaGVNb2RlbERvY3VtZW50Vmlld2Vy OwogICAgIHBhcmFtZXRlcnMucHJveHlTZXR0aW5ncyA9IG1fbmV0d29ya1Byb3h5U2V0dGluZ3M7 CiAKKyAgICBwYXJhbWV0ZXJzLnNob3VsZEFsd2F5c1VzZUNvbXBsZXhUZXh0Q29kZVBhdGggPSB0 cnVlOworICAgIGNvbnN0IGNoYXIqIGZvcmNlQ29tcGxleFRleHQgPSBnZXRlbnYoIldFQktJVF9G T1JDRV9DT01QTEVYX1RFWFQiKTsKKyAgICBpZiAoZm9yY2VDb21wbGV4VGV4dCAmJiAhc3RyY21w KGZvcmNlQ29tcGxleFRleHQsICIwIikpCisgICAgICAgIHBhcmFtZXRlcnMuc2hvdWxkQWx3YXlz VXNlQ29tcGxleFRleHRDb2RlUGF0aCA9IG1fYWx3YXlzVXNlc0NvbXBsZXhUZXh0Q29kZVBhdGg7 CisKICNpZiBVU0UoR1NUUkVBTUVSKQogICAgIHBhcmFtZXRlcnMuZ3N0cmVhbWVyT3B0aW9ucyA9 IFdlYkNvcmU6OmV4dHJhY3RHU3RyZWFtZXJPcHRpb25zRnJvbUNvbW1hbmRMaW5lKCk7CiAjZW5k aWYKZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBiL1Rvb2xzL0NoYW5nZUxvZwppbmRleCAw YzE5NzBjZThkNy4uNWFlNDRiNTIwMzUgMTAwNjQ0Ci0tLSBhL1Rvb2xzL0NoYW5nZUxvZworKysg Yi9Ub29scy9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNSBAQAorMjAxOC0wMy0wMiAgQ2FybG9zIEdh cmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CisKKyAgICAgICAgW0dUS10gU3dpdGNo IHRvIHVzZSBhbHdheXMgY29tcGxleCB0ZXh0IGNvZGUgcGF0aAorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTgzMjg1CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgS2VlcCB0aGUgYXV0byBtb2RlIGZvciB0aGUg bGF5b3V0IHRlc3RzIHRvIGF2b2lkIGhhdmluZyB0byByZWJhc2VsaW5lIGEgbG90IG9mIHRlc3Rz LgorCisgICAgICAgICogV2ViS2l0VGVzdFJ1bm5lci9ndGsvbWFpbi5jcHA6CisgICAgICAgICht YWluKTogU2V0IFdFQktJVF9GT1JDRV9DT01QTEVYX1RFWFQgdG8gMCB1bmxlc3MgaXQncyBhbHJl YWR5IHByZXNlbnQgaW4gdGhlIGVudmlyb25tZW50LgorCiAyMDE4LTAzLTAxICBZb3Vlbm4gRmFi bGV0ICA8eW91ZW5uQGFwcGxlLmNvbT4KIAogICAgICAgICBBZGQgQVBJIHRlc3QgdG8gdmFsaWRh dGUgc2V0dGluZyBvZiBzZXJ2aWNlIHdvcmtlciBhbmQgY2FjaGUgc3RvcmFnZSBkaXJlY3Rvcmll cwpkaWZmIC0tZ2l0IGEvVG9vbHMvV2ViS2l0VGVzdFJ1bm5lci9ndGsvbWFpbi5jcHAgYi9Ub29s cy9XZWJLaXRUZXN0UnVubmVyL2d0ay9tYWluLmNwcAppbmRleCA3OTQ3Y2U1NWNkNS4uZTI5MTUy NTNmMTggMTAwNjQ0Ci0tLSBhL1Rvb2xzL1dlYktpdFRlc3RSdW5uZXIvZ3RrL21haW4uY3BwCisr KyBiL1Rvb2xzL1dlYktpdFRlc3RSdW5uZXIvZ3RrL21haW4uY3BwCkBAIC0zMiw2ICszMiw4IEBA CiAKIGludCBtYWluKGludCBhcmdjLCBjaGFyKiogYXJndikKIHsKKyAgICBnX3NldGVudigiV0VC S0lUX0ZPUkNFX0NPTVBMRVhfVEVYVCIsICIwIiwgRkFMU0UpOworCiAgICAgZ3RrX2luaXQoJmFy Z2MsICZhcmd2KTsKIAogICAgIEdSZWZQdHI8R1B0ckFycmF5PiBsYW5ndWFnZXMgPSBhZG9wdEdS ZWYoZ19wdHJfYXJyYXlfbmV3KCkpOwo=