Bug 179989 – [GTK] Some features have changed of value (13 new failures)

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

Meanwhile doing gardening I noticed that r225098 <https://trac.webkit.org/r225098> has caused 12 new failures on the GTK+ port:

The new failures:
 http/tests/misc/prefetch-purpose.html [ Timeout ]
 http/tests/misc/link-rel-prefetch-and-subresource.html [ Timeout ]
 fast/dom/HTMLLinkElement/subresource.html [ Timeout ]
 fast/dom/HTMLLinkElement/prefetch-onload.html [ Timeout ]
 fast/dom/HTMLLinkElement/prefetch-onerror.html [ Timeout ]
 fast/dom/HTMLLinkElement/prefetch.html [ Timeout ]
 fast/dom/HTMLLinkElement/link-and-subresource-test-nonexistent.html [ Timeout ]
 fast/dom/HTMLLinkElement/link-and-subresource-test.html [ Timeout ]
 fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify-inherited.html [ Failure ]
 fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html [ Failure ]
 fast/css3-text/css3-text-align-last/getComputedStyle/getComputedStyle-text-align-last-inherited.html [ Failure ]
 fast/css3-text/css3-text-align-last/getComputedStyle/getComputedStyle-text-align-last.html [ Failure ]

Before: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/4336
After: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/4335

[Carlos Alberto Lopez Perez]

This is the diff of the generated CMakeCache.txt file for the Release build (before and after) http://sprunge.us/NNVV?diff

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #1)
> This is the diff of the generated CMakeCache.txt file for the Release build
> (before and after) http://sprunge.us/NNVV?diff

Ups, i got it inverted.. this is the good one: http://sprunge.us/KOOc?diff

[Carlos Alberto Lopez Perez]

Committed r225126: <https://trac.webkit.org/changeset/225126>

[Michael Catanzaro]

I'm not surprised that the feature set we were testing is so arbitrarily different than the feature set that we release. I'm relieved we've finally gotten rid of the second set of feature defaults.

> The new failures:
>  http/tests/misc/prefetch-purpose.html [ Timeout ]
>  http/tests/misc/link-rel-prefetch-and-subresource.html [ Timeout ]
>  fast/dom/HTMLLinkElement/subresource.html [ Timeout ]
>  fast/dom/HTMLLinkElement/prefetch-onload.html [ Timeout ]
>  fast/dom/HTMLLinkElement/prefetch-onerror.html [ Timeout ]
>  fast/dom/HTMLLinkElement/prefetch.html [ Timeout ]
>  fast/dom/HTMLLinkElement/link-and-subresource-test-nonexistent.html [
> Timeout ]
>  fast/dom/HTMLLinkElement/link-and-subresource-test.html [ Timeout ]

These tests should all be skipped, because they require ENABLE_LINK_PREFETCH, which we have never supported.

>  fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-
> justify-inherited.html [ Failure ]
>  fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-
> justify.html [ Failure ]
>  fast/css3-text/css3-text-align-last/getComputedStyle/getComputedStyle-text-
> align-last-inherited.html [ Failure ]
>  fast/css3-text/css3-text-align-last/getComputedStyle/getComputedStyle-text-
> align-last.html [ Failure ]

And these tests should all be skipped because they rely on ENABLE_CSS3_TEXT, which we have never supported.

It would be interesting for someone familiar with web standards to review our feature list. I suspect there are many other low-hanging features besides these two that are probably disabled for no reason.

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #4)
> I'm not surprised that the feature set we were testing is so arbitrarily
> different than the feature set that we release. I'm relieved we've finally
> gotten rid of the second set of feature defaults.
> 
> > The new failures:
> >  http/tests/misc/prefetch-purpose.html [ Timeout ]
> >  http/tests/misc/link-rel-prefetch-and-subresource.html [ Timeout ]
> >  fast/dom/HTMLLinkElement/subresource.html [ Timeout ]
> >  fast/dom/HTMLLinkElement/prefetch-onload.html [ Timeout ]
> >  fast/dom/HTMLLinkElement/prefetch-onerror.html [ Timeout ]
> >  fast/dom/HTMLLinkElement/prefetch.html [ Timeout ]
> >  fast/dom/HTMLLinkElement/link-and-subresource-test-nonexistent.html [
> > Timeout ]
> >  fast/dom/HTMLLinkElement/link-and-subresource-test.html [ Timeout ]
> 
> These tests should all be skipped, because they require
> ENABLE_LINK_PREFETCH, which we have never supported.
> 
> >  fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-
> > justify-inherited.html [ Failure ]
> >  fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-
> > justify.html [ Failure ]
> >  fast/css3-text/css3-text-align-last/getComputedStyle/getComputedStyle-text-
> > align-last-inherited.html [ Failure ]
> >  fast/css3-text/css3-text-align-last/getComputedStyle/getComputedStyle-text-
> > align-last.html [ Failure ]
> 
> And these tests should all be skipped because they rely on ENABLE_CSS3_TEXT,
> which we have never supported.
> 

Really? Then how where this test passing before?  I think either we (partially or without knowing?) supported that or the tests are broken

[Michael Catanzaro]

The tests were passing before because the bots previously used the features defined in FeatureList.pm. Your diff shows that FeatureList.pm was enabling many unsupported features -- features that are both PRIVATE and OFF in WebKitFeatures.cmake and/or OptionsGTK.cmake -- and so tests that depended on those features were passing. But nobody is going to notice the difference except for our test bots and developers using build-webkit.

This is exactly why we wanted to remove the list of feature defaults from FeatureList.pm. Nobody was even attempting to keep it in sync with WebKitFeatures.cmake.

[Carlos Garcia Campos]

Then I guess you mean that those features were never enabled in production builds, not that they were never supported, because tests were passing when enabled.

[Michael Catanzaro]

BTW this is not to say that we *should* have link prefetch and CSS3 text disabled... just that we have always had them disabled in releases, and it's wrong for us to have been testing a different set of features than what we released with. (Except, of course, in cases where we intentionally want to test specific features that we know about. That's now solved by ENABLE_EXPERIMENTAL_FEATURES instead of FeatureList.pm.)

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #7)
> Then I guess you mean that those features were never enabled in production
> builds, not that they were never supported, because tests were passing when
> enabled.

Well that's just semantics... I would say that the definition of a supported feature is one that is either ON or PUBLIC, and all PRIVATE OFF features are "unsupported." But yes, clearly the tests had been passing before.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #7)
> Then I guess you mean that those features were never enabled in production
> builds, not that they were never supported, because tests were passing when
> enabled.

Right.. I think we should look forward to enable this features in production builds if it turns out we support them.

Certainly is shocking that we have been all this time testing a very different thing than we ship. Glad that we finally got rid of that FeatureList.pm mess

[Michael Catanzaro]

I'll just repeat what I said here:

(In reply to Michael Catanzaro from comment #4)
> It would be interesting for someone familiar with web standards to review
> our feature list. I suspect there are many other low-hanging features
> besides these two that are probably disabled for no reason.

[Michael Catanzaro]

I'm going to remove the regression tag here, since I consider this a progression rather than a regression -- we are now testing the set of features that we have been releasing -- and since I don't plan any further changes here.

[Michael Catanzaro]

I'm also changing the Timeout expectations to Skips, since it doesn't make sense for us to wait for all the tests to time out when they are not supposed to be passing anyway.

[Michael Catanzaro]

Hi WebCore experts, any idea what ENABLE_CSS3_TEXT does? Should we be enabling it? Looks like Mac does?

Regarding ENABLE_LINK_PREFETCH... I'd say we should probably turn it on, if it's working properly. Should make the web seem a bit faster.

[Manuel Rego Casasnovas]

(In reply to Michael Catanzaro from comment #14)
> Hi WebCore experts, any idea what ENABLE_CSS3_TEXT does? Should we be
> enabling it? Looks like Mac does?

On a quick look this adds support for 2 CSS properties:
"text-align-last" (https://www.w3.org/TR/css-text-3/#text-align-last-property)
and "text-justify" (https://www.w3.org/TR/css-text-3/#text-justify-property).
And some extra keywords for "text-indent" property (https://www.w3.org/TR/css-text-3/#text-indent-property).

So I'd enable it, as they seem useful for GTK+ too.

> Regarding ENABLE_LINK_PREFETCH... I'd say we should probably turn it on, if
> it's working properly. Should make the web seem a bit faster.

Yeah I guess we should enable it too.

[Manuel Rego Casasnovas]

Created attachment 334154 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 334154 [details]
Patch

I think it's enough with setting WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSS3_TEXT ON) rather than re-defining the text with WEBKIT_OPTION_DEFINE()

[Manuel Rego Casasnovas]

(In reply to Carlos Alberto Lopez Perez from comment #17)
> Comment on attachment 334154 [details]
> Patch
> 
> I think it's enough with setting
> WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSS3_TEXT ON) rather than
> re-defining the text with WEBKIT_OPTION_DEFINE()

Indeed, sorry for the mistake. Uploading a new version.

[Manuel Rego Casasnovas]

Created attachment 334155 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 334155 [details]
Patch

thanks!

[Michael Catanzaro]

Comment on attachment 334155 [details]
Patch

I'm confused why these should be GTK-specific, though.

*Maybe* some ports would want to leave link prefetch off, but surely CSS3 text should be enabled for everyone?

I would change the defaults in WebKitFeatures.cmake instead.

[Michael Catanzaro]

(For bonus points: getting rid of the ENABLE_CSS3_TEXT build option would probably be good. But I suppose there are many build options that would be good to get rid of.)

[Manuel Rego Casasnovas]

Comment on attachment 334155 [details]
Patch

After taking a deeper look, I feel like we shouldn't enable them.

At least css3-text, the implementation was only the parser.
So "-webkit-text-align-last: center" is valid with the flag enabled,
but it doesn't do anything.
I believe it'd be better to keep it disabled in that case.
The tests are not really useful as nobody is running them it seems though.
The parsing was implemented in 2012 (see bug #99439), but the tests are skipped since then in all ports.


Regarding link prefetch I have no idea, but for Mac it has been disabled
also since the beginning in 2010 (see bug #3652).

I don't know that feature, so I didn't investigate anything deeper
but it seems we shouldn't enable it either.
A code that has been untested by Mac since 2010 doesn't sound really safe to enable.

Reopen if you think we should do something different.
And sorry for the noise with the patch.

[Michael Catanzaro]

Thanks for investigating, Rego!

Some remaining work we should do here:

 * The timeouts and layout test failures should probably be moved to the global TestExpectations
 * Looks like ENABLE_CSS3_TEXT and maybe also ENABLE_LINK_PREFETCH should be removed
   * Then the corresponding tests should be removed as well

[Manuel Rego Casasnovas]

(In reply to Michael Catanzaro from comment #24)
> Some remaining work we should do here:
> 
>  * The timeouts and layout test failures should probably be moved to the
> global TestExpectations
>  * Looks like ENABLE_CSS3_TEXT and maybe also ENABLE_LINK_PREFETCH should be
> removed
>    * Then the corresponding tests should be removed as well

I believe that should be part of different bugs and not here, that's why I closed this.
But I agree it'd be nice to do that work.

So far I've moved the test failures to the generic TestExpectations file in bug #182963.

It'd be nice to check ENABLE_LINK_PREFETCH too.

Probably the discussion about removing the compilation flags,
the code and related tests should be done in webkit-dev.

[Michael Catanzaro]

I created a mailing list topic:

https://lists.webkit.org/pipermail/webkit-dev/2018-February/029881.html

And you moved the test expectations. I think further work can be done in new bugs. Thanks for the help, Rego!

[Manuel Rego Casasnovas]

JFTR, I moved the LINK_PREFETCH tests to the global TestExpectations in bug #182981.