Bug 179989

Summary: [GTK] Some features have changed of value (13 new failures)
Product: WebKit Reporter: Carlos Alberto Lopez Perez <clopez>
Component: WebKitGTKAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cgarcia, jfernandez, mcatanzaro, rego, svillar
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=180004
Bug Depends on:    
Bug Blocks: 177338    
Attachments:
Description Flags
Patch
none
Patch clopez: review+

Description Carlos Alberto Lopez Perez 2017-11-23 17:10:33 PST
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
Comment 1 Carlos Alberto Lopez Perez 2017-11-23 17:15:39 PST
This is the diff of the generated CMakeCache.txt file for the Release build (before and after) http://sprunge.us/NNVV?diff
Comment 2 Carlos Alberto Lopez Perez 2017-11-23 17:18:43 PST
(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
Comment 3 Carlos Alberto Lopez Perez 2017-11-23 17:26:04 PST
Committed r225126: <https://trac.webkit.org/changeset/225126>
Comment 4 Michael Catanzaro 2017-11-24 03:57:18 PST
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.
Comment 5 Carlos Alberto Lopez Perez 2017-11-24 04:02:42 PST
(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
Comment 6 Michael Catanzaro 2017-11-24 04:23:07 PST
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.
Comment 7 Carlos Garcia Campos 2017-11-24 04:25:35 PST
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.
Comment 8 Michael Catanzaro 2017-11-24 04:26:17 PST
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.)
Comment 9 Michael Catanzaro 2017-11-24 04:28:32 PST
(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.
Comment 10 Carlos Alberto Lopez Perez 2017-11-24 05:25:10 PST
(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
Comment 11 Michael Catanzaro 2017-11-24 06:47:05 PST
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.
Comment 12 Michael Catanzaro 2017-11-27 08:35:04 PST
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.
Comment 13 Michael Catanzaro 2017-12-04 13:11:51 PST
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.
Comment 14 Michael Catanzaro 2018-02-13 20:27:50 PST
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.
Comment 15 Manuel Rego Casasnovas 2018-02-19 03:48:21 PST
(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.
Comment 16 Manuel Rego Casasnovas 2018-02-19 05:13:03 PST
Created attachment 334154 [details]
Patch
Comment 17 Carlos Alberto Lopez Perez 2018-02-19 05:21:32 PST
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()
Comment 18 Manuel Rego Casasnovas 2018-02-19 05:25:36 PST
(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.
Comment 19 Manuel Rego Casasnovas 2018-02-19 05:28:03 PST
Created attachment 334155 [details]
Patch
Comment 20 Carlos Alberto Lopez Perez 2018-02-19 05:42:21 PST
Comment on attachment 334155 [details]
Patch

thanks!
Comment 21 Michael Catanzaro 2018-02-19 06:06:41 PST
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.
Comment 22 Michael Catanzaro 2018-02-19 06:08:31 PST
(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.)
Comment 23 Manuel Rego Casasnovas 2018-02-19 06:32:46 PST
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.
Comment 24 Michael Catanzaro 2018-02-19 06:53:19 PST
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
Comment 25 Manuel Rego Casasnovas 2018-02-20 04:46:17 PST
(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.
Comment 26 Michael Catanzaro 2018-02-20 11:31:02 PST
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!
Comment 27 Manuel Rego Casasnovas 2018-02-20 14:05:45 PST
JFTR, I moved the LINK_PREFETCH tests to the global TestExpectations in bug #182981.