| Summary: | [WK2] WebKitTestRunner should enable <style scoped> support for testing | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Zan Dobersek <zan> | ||||||||||||||
| Component: | WebKitGTK | Assignee: | Zan Dobersek <zan> | ||||||||||||||
| Status: | RESOLVED INVALID | ||||||||||||||||
| Severity: | Normal | CC: | benjamin, koivisto, mrobinson, philn, pnormand, sam, svillar, xan.lopez | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Zan Dobersek
2012-09-26 04:23:31 PDT
Created attachment 166422 [details]
Patch
Comment on attachment 166422 [details] Patch Attachment 166422 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14078838 Comment on attachment 166422 [details] Patch Attachment 166422 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14074764 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). Created attachment 180752 [details]
Rebased patch
Comment on attachment 180752 [details]
Rebased patch
Shouldn't this patch also unskip some tests?
Created attachment 180776 [details]
Updated patch
Also removes failure expectations from the gtk-wk2 TestExpectations file.
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.
Since this doesn't just touch GTK code, taking off the [GTK] prefix. This patch seems to add a declaration for WKPreferencesGetStyleScopedEnabled but no implementation. Created attachment 180950 [details]
Patch
Comment on attachment 180950 [details] Patch Attachment 180950 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15599282 Comment on attachment 180950 [details] Patch Attachment 180950 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15576013 Comment on attachment 180950 [details] Patch Attachment 180950 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15584457 Created attachment 180951 [details]
Patch
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? (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. > > 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.
(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. 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. Created attachment 183225 [details]
Patch
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.
Style scoped support is gone for the moment. |