Bug 97655 – [WK2] WebKitTestRunner should enable <style scoped> support for testing

• Component: WebKitGTK
• Status: RESOLVED INVALID

[Zan Dobersek]

Currently the <style scoped> support is enabled by default at compile-time, but should also be enabled at runtime in WebKitTestRunner. The feature was enabled (along with proper support in DumpRenderTree) in r129604.
http://trac.webkit.org/changeset/129604

[Zan Dobersek]

Created attachment 166422 [details]
Patch

[Build Bot]

Comment on attachment 166422 [details]
Patch

Attachment 166422 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14078838

[Build Bot]

Comment on attachment 166422 [details]
Patch

Attachment 166422 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14074764

[Zan Dobersek]

The build failures indicate the forwarding header for RuntimeEnabledFeatures.h isn't generated on mac and win platforms. Don't really know if this is a real problem or just a build system fluke (i.e. whether it would require a clean build on those two platforms when the patch lands).

[Zan Dobersek]

Created attachment 180752 [details]
Rebased patch

[Martin Robinson]

Comment on attachment 180752 [details]
Rebased patch

Shouldn't this patch also unskip some tests?

[Zan Dobersek]

Created attachment 180776 [details]
Updated patch

Also removes failure expectations from the gtk-wk2 TestExpectations file.

[Martin Robinson]

Comment on attachment 180776 [details]
Updated patch

Okay. This looks good to me, though I don't understand how these tests pass for other WebKit2 ports. It would be good to have the final review done by one of the core WK2 maintainers.

[Sam Weinig]

Since this doesn't just touch GTK code, taking off the [GTK] prefix.

[Sam Weinig]

This patch seems to add a declaration for WKPreferencesGetStyleScopedEnabled but no implementation.

[Zan Dobersek]

Created attachment 180950 [details]
Patch

[Early Warning System Bot]

Comment on attachment 180950 [details]
Patch

Attachment 180950 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15599282

[Build Bot]

Comment on attachment 180950 [details]
Patch

Attachment 180950 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15576013

[EFL EWS Bot]

Comment on attachment 180950 [details]
Patch

Attachment 180950 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15584457

[Zan Dobersek]

Created attachment 180951 [details]
Patch

[Benjamin Poulain]

I don't know anything about the state of STYLE_SCOPED's implementation so correct me if I am wrong.

I suppose StyleScoped is a RuntimeEnabledFeatures because the feature is not ready(?). As soon as it will be in a good state, we can remove all that jazz and just enable it by default.

If that is the case, can't you just hack something in GTK's injected bundle instead of extending WKPreferences?

Why do you want to enable a experimental feature? Are you planning to work on it?

[Zan Dobersek]

(In reply to comment #16)
> I don't know anything about the state of STYLE_SCOPED's implementation so correct me if I am wrong.
> 
> I suppose StyleScoped is a RuntimeEnabledFeatures because the feature is not ready(?). As soon as it will be in a good state, we can remove all that jazz and just enable it by default.
> 
> If that is the case, can't you just hack something in GTK's injected bundle instead of extending WKPreferences?

I see, it doesn't make any sense to expose API for enabling a feature that might not be compiled at all. There might be other features in a similar position as well.

I'll concoct something that performs the required through the injected bundle.

> Why do you want to enable a experimental feature? Are you planning to work on it?

No, I'm trying to provide test coverage in WKTR much like it's provided in Gtk's DRT since the feature is enabled in development builds.

[Benjamin Poulain]

> > Why do you want to enable a experimental feature? Are you planning to work on it?
> 
> No, I'm trying to provide test coverage in WKTR much like it's provided in Gtk's DRT since the feature is enabled in development builds.

"Because WebKit1 has it" is not really the answer I was expecting. :)
Why is it enabled on GTK's DRT? If you are not actually working on the feature, or deploying it, it might just be causing some unnecessary pain.

[Zan Dobersek]

(In reply to comment #18)
> > > Why do you want to enable a experimental feature? Are you planning to work on it?
> > 
> > No, I'm trying to provide test coverage in WKTR much like it's provided in Gtk's DRT since the feature is enabled in development builds.
> 
> "Because WebKit1 has it" is not really the answer I was expecting. :)
> Why is it enabled on GTK's DRT? If you are not actually working on the feature, or deploying it, it might just be causing some unnecessary pain.

Enabling and testing the feature while it's in the implementation process gives valuable test coverage and change feedback to people who are doing the implementation. The pain of managing the test expectations does not occur that often, but I believe is appreciated by the implementors.

It also saves some time and stress when the feature is deemed stable enough to enable it by default.

[Benjamin Poulain]

Comment on attachment 180951 [details]
Patch

> Enabling and testing the feature while it's in the implementation process gives valuable test coverage and change feedback to people who are doing the implementation. The pain of managing the test expectations does not occur that often, but I believe is appreciated by the implementors.
> 
> It also saves some time and stress when the feature is deemed stable enough to enable it by default.

Fair enough. If you don't mind the rebaselines :)

I r- this one until you look at a simpler way to enable this for GTK. Feel free to r? back if needed.

[Zan Dobersek]

Created attachment 183225 [details]
Patch

[Andreas Kling]

Comment on attachment 183225 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Martin Robinson]

Style scoped support is gone for the moment.