Re: [webkit-dev] Settings and Preferences in layout tests
Brady Eidson beid...@apple.com writes:
On Sep 26, 2012, at 2:46 PM, Tony Chang t...@chromium.org wrote:
I suppose we're biased in Mac-land where the mechanism originated,
but the API is simply a single string based API that only had to
be implemented once.
Your comment leads me to understand that at least one other port
doesn't have simple string based preferences.
[...]
I think for all the major ports, they are simple string based
preferences. However, adding a new overridePreference call means
updating 5 WebKit API interfaces (Mac, Win, Chromium, GTK+, QT), and
updating 5 DRTs and 1 WTR. Compared to updating a single
internal.settings implementation, this is a lot of work.
I think you misunderstand what I meant.
The Mac DRT implementation of overridePreference has the following
implementation:
void TestRunner::overridePreference(JSStringRef key, JSStringRef
value)
{
RetainPtrCFStringRef keyCF(AdoptCF,
JSStringCopyCFString(kCFAllocatorDefault, key));
NSString *keyNS = (NSString *)keyCF.get();
RetainPtrCFStringRef valueCF(AdoptCF,
JSStringCopyCFString(kCFAllocatorDefault, value));
NSString *valueNS = (NSString *)valueCF.get();
[[WebPreferences standardPreferences]
_setPreferenceForTestWithValue:valueNS forKey:keyNS];
}
This works for any preference; Even a new one that has never been
twiddled in a regression test before.
The situation seems to be improving as more ports move towards WebKit2,
where adding new preferences to overridePreferences is normally a matter
of touching a (few) file(s) in Source/WebKit2/Shared.
However, for chromium and ports using DRT this does not help much, as
the Mac code ends up being the exception: all the other DRTs (chromium,
blackberry, Qt, EFL, GTK+) implement some variation of a big switch
mapping that string into an API of some sort, so even if the preference
already exists in the WebKit layer one needs to add more code to DRT
simply to call that.
And sometimes there's even more than one way of achieving the same
thing, such as changing the WebKitDisplayImagesKey preference or
calling testRunner.disableImageLoading().
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] Settings and Preferences in layout tests
We have a lot of tests that poke internal settings, via testRunner: testRunner.setFrameFlatteningEnabled(true); or window.internals: internals.settings.setPageScaleFactor(0.5, 0, 0); and some that poke preferences, like: testRunner.overridePreference(WebKitUsesPageCachePreferenceKey, 1); First, direct calls on testRunner that just set preferences should be migrated to internals.settings or testRunner.overridePreference calls, I think (I don't know if either is preferred). Secondly, I see no guarantee that these settings and preferences are reset to their default values before the next test, and we don't seem to have a consistent strategy about how to do this. For internal settings, we have InternalSettings::Backup methods. However, there's no enforcement that when changing a setting in a test for the first time, developers also add code to reset it in InternalSettings. I looked at testRunner.overridePreference(), and it doesn't appear to reset the value at the end of the test. So I think we need clearer rules here. I suggest: * testRunner methods that just set preferences should migrate to internals.settings or testRunner.overridePreference * we should choose between internals.settings or testRunner.overridePreference if that makes sense. * we should enforce a policy that patches adding a settings/prefs toggle should, if necessary, add code to reset between tests. Simon ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Settings and Preferences in layout tests
On Wed, Sep 26, 2012 at 1:44 PM, Simon Fraser simon.fra...@apple.comwrote: We have a lot of tests that poke internal settings, via testRunner: testRunner.setFrameFlatteningEnabled(true); or window.internals: internals.settings.setPageScaleFactor(0.5, 0, 0); and some that poke preferences, like: testRunner.overridePreference(WebKitUsesPageCachePreferenceKey, 1); First, direct calls on testRunner that just set preferences should be migrated to internals.settings or testRunner.overridePreference calls, I think (I don't know if either is preferred). I support the idea of unifying the approaches and just use internals.settings. However, the last time I checked, Alexey had some concerns about using internals due to settings may not be properly propagated to WebKit2 layer. Has this concern been addressed? Secondly, I see no guarantee that these settings and preferences are reset to their default values before the next test, and we don't seem to have a consistent strategy about how to do this. That sounds really bad. We should fix that. For internal settings, we have InternalSettings::Backup methods. However, there's no enforcement that when changing a setting in a test for the first time, developers also add code to reset it in InternalSettings. I looked at testRunner.overridePreference(), and it doesn't appear to reset the value at the end of the test. So I think we need clearer rules here. I suggest: * testRunner methods that just set preferences should migrate to internals.settings or testRunner.overridePreference * we should choose between internals.settings or testRunner.overridePreference if that makes sense. * we should enforce a policy that patches adding a settings/prefs toggle should, if necessary, add code to reset between tests. Sounds good to me. I'm surprised that the third point isn't already enforced. It seems like that'll be a review failure. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Settings and Preferences in layout tests
On Sep 26, 2012, at 1:48 PM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Sep 26, 2012 at 1:44 PM, Simon Fraser simon.fra...@apple.com wrote: First, direct calls on testRunner that just set preferences should be migrated to internals.settings or testRunner.overridePreference calls, I think (I don't know if either is preferred). I support the idea of unifying the approaches and just use internals.settings. However, the last time I checked, Alexey had some concerns about using internals due to settings may not be properly propagated to WebKit2 layer. Has this concern been addressed? In general I prefer the overridePreference() calls whenever they exist. internals.settings are not exposed in any real-world product whereas preferences exist in each platform's WebKit-layer API that they expose to their embedders in some form. ~Brady ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Settings and Preferences in layout tests
On Sep 26, 2012, at 1:44 PM, Simon Fraser simon.fra...@apple.com wrote: * we should choose between internals.settings or testRunner.overridePreference if that makes sense. I've recently been working on a new API and I've discovered that internals.settings and testRunner.overridePreference go through different layers—internals.settings goes right to WebCore, whereas testRunner.overridePreference goes through WebKit1 first. As my API only went through WebKit2 at first, I was unable to use testRunner.overridePreference. Moreover, I needed to use testRunner.overridePreference separately from internals.settings to test that the WebKit1 API that I'd added worked. Although I'm not fond of having multiple ways into the settings, I feel like the two methods have separate use cases. Jeffrey ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Settings and Preferences in layout tests
[re-sent from the proper address] On Wed, Sep 26, 2012 at 2:00 PM, Adam Barth abarth@nowhere wrote: On Wed, Sep 26, 2012 at 1:53 PM, Brady Eidson beid...@apple.com wrote: On Sep 26, 2012, at 1:48 PM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Sep 26, 2012 at 1:44 PM, Simon Fraser simon.fra...@apple.comwrote: First, direct calls on testRunner that just set preferences should be migrated to internals.settings or testRunner.overridePreference calls, I think (I don't know if either is preferred). I support the idea of unifying the approaches and just use internals.settings. However, the last time I checked, Alexey had some concerns about using internals due to settings may not be properly propagated to WebKit2 layer. Has this concern been addressed? In general I prefer the overridePreference() calls whenever they exist. internals.settings are not exposed in any real-world product whereas preferences exist in each platform's WebKit-layer API that they expose to their embedders in some form. The main downside of overridePreference is that it requires that you expose an API for twiddling the preference on every port. That can lead to us exposing unneeded APIs (making them harder to remove) and to a bunch of port-specific code in an otherwise port-independent patch. IMHO, we should prefer InternalSettings unless we need to test the WebKit-layer code. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Settings and Preferences in layout tests
I would agree with Adam, and the more we can move to window.internals, the less technical debt we incur with each new DRT feature. I would love to see overridePreferences go away (or only be used for preferences which need to test the WebKit-side plumbing). TestExpectation files on all ports are full of: # unskip these tests when we add obscure-drt-feature-x http://trac.webkit.org/browser/trunk/LayoutTests/platform/wk2/Skipped#L107 http://trac.webkit.org/browser/trunk/LayoutTests/platform/wk2/Skipped#L209 http://trac.webkit.org/browser/trunk/LayoutTests/platform/wk2/Skipped#L247 http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/TestExpectations#L115 http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/TestExpectations#L948 http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/TestExpectations#L958 as just a few examples. :) I didn't even look at the less-well-funded ports. :) On Wed, Sep 26, 2012 at 4:05 PM, Adam Barth aba...@webkit.org wrote: [re-sent from the proper address] On Wed, Sep 26, 2012 at 2:00 PM, Adam Barth abarth@nowhere wrote: On Wed, Sep 26, 2012 at 1:53 PM, Brady Eidson beid...@apple.com wrote: On Sep 26, 2012, at 1:48 PM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Sep 26, 2012 at 1:44 PM, Simon Fraser simon.fra...@apple.com wrote: First, direct calls on testRunner that just set preferences should be migrated to internals.settings or testRunner.overridePreference calls, I think (I don't know if either is preferred). I support the idea of unifying the approaches and just use internals.settings. However, the last time I checked, Alexey had some concerns about using internals due to settings may not be properly propagated to WebKit2 layer. Has this concern been addressed? In general I prefer the overridePreference() calls whenever they exist. internals.settings are not exposed in any real-world product whereas preferences exist in each platform's WebKit-layer API that they expose to their embedders in some form. The main downside of overridePreference is that it requires that you expose an API for twiddling the preference on every port. That can lead to us exposing unneeded APIs (making them harder to remove) and to a bunch of port-specific code in an otherwise port-independent patch. IMHO, we should prefer InternalSettings unless we need to test the WebKit-layer code. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Settings and Preferences in layout tests
On Sep 26, 2012, at 2:05 PM, Adam Barth aba...@webkit.org wrote: [re-sent from the proper address] On Wed, Sep 26, 2012 at 2:00 PM, Adam Barth abarth@nowhere wrote: On Wed, Sep 26, 2012 at 1:53 PM, Brady Eidson beid...@apple.com wrote: On Sep 26, 2012, at 1:48 PM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Sep 26, 2012 at 1:44 PM, Simon Fraser simon.fra...@apple.com wrote: First, direct calls on testRunner that just set preferences should be migrated to internals.settings or testRunner.overridePreference calls, I think (I don't know if either is preferred). I support the idea of unifying the approaches and just use internals.settings. However, the last time I checked, Alexey had some concerns about using internals due to settings may not be properly propagated to WebKit2 layer. Has this concern been addressed? In general I prefer the overridePreference() calls whenever they exist. internals.settings are not exposed in any real-world product whereas preferences exist in each platform's WebKit-layer API that they expose to their embedders in some form. The main downside of overridePreference is that it requires that you expose an API for twiddling the preference on every port. That can lead to us exposing unneeded APIs (making them harder to remove) and to a bunch of port-specific code in an otherwise port-independent patch. IMHO, we should prefer InternalSettings unless we need to test the WebKit-layer code. I suppose we're biased in Mac-land where the mechanism originated, but the API is simply a single string based API that only had to be implemented once. Your comment leads me to understand that at least one other port doesn't have simple string based preferences. Of course to add *any* new internal setting new code must be added specifically for that setting... Of course that code only has to be added once for all platforms… I would argue it's not a clear cut decision either way. ~Brady Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Settings and Preferences in layout tests
On a related note, there is a gradual movement to pass more command line flags along with each test to DRT during run-webkit-tests (to toggle between pixel tests and non, change timeout values, etc.), and being able to ensure we reset the state to the default between each test is kinda necessary for this to work. -- Dirk On Wed, Sep 26, 2012 at 2:00 PM, Eric Seidel e...@webkit.org wrote: When overridePreferences was added back in the day: http://trac.webkit.org/changeset/39212 The contract was that DRT would call resetDefaults between tests. I don't know what the current status of resetToDefaults implementations in other ports or in non-WK1 architectures. On Wed, Sep 26, 2012 at 3:53 PM, Brady Eidson beid...@apple.com wrote: On Sep 26, 2012, at 1:48 PM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Sep 26, 2012 at 1:44 PM, Simon Fraser simon.fra...@apple.com wrote: First, direct calls on testRunner that just set preferences should be migrated to internals.settings or testRunner.overridePreference calls, I think (I don't know if either is preferred). I support the idea of unifying the approaches and just use internals.settings. However, the last time I checked, Alexey had some concerns about using internals due to settings may not be properly propagated to WebKit2 layer. Has this concern been addressed? In general I prefer the overridePreference() calls whenever they exist. internals.settings are not exposed in any real-world product whereas preferences exist in each platform's WebKit-layer API that they expose to their embedders in some form. ~Brady ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Settings and Preferences in layout tests
On Sep 26, 2012, at 2:14 PM, Eric Seidel e...@webkit.org wrote: TestExpectation files on all ports are full of: # unskip these tests when we add obscure-drt-feature-x http://trac.webkit.org/browser/trunk/LayoutTests/platform/wk2/Skipped#L107 http://trac.webkit.org/browser/trunk/LayoutTests/platform/wk2/Skipped#L209 http://trac.webkit.org/browser/trunk/LayoutTests/platform/wk2/Skipped#L247 I believe these will go away with a single implementation of overridePreference(). http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/TestExpectations#L115 http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/TestExpectations#L948 http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/TestExpectations#L958 But am not so sure here. I would agree with Adam, and the more we can move to window.internals, the less technical debt we incur with each new DRT feature. I would love to see overridePreferences go away (or only be used for preferences which need to test the WebKit-side plumbing). DRT/WKTR are important not only in that they test WebCore but also in that they test the WebKit they embed. This seems like a short sided conclusion based on convenience. ~Brady as just a few examples. :) I didn't even look at the less-well-funded ports. :) On Wed, Sep 26, 2012 at 4:05 PM, Adam Barth aba...@webkit.org wrote: [re-sent from the proper address] On Wed, Sep 26, 2012 at 2:00 PM, Adam Barth abarth@nowhere wrote: On Wed, Sep 26, 2012 at 1:53 PM, Brady Eidson beid...@apple.com wrote: On Sep 26, 2012, at 1:48 PM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Sep 26, 2012 at 1:44 PM, Simon Fraser simon.fra...@apple.com wrote: First, direct calls on testRunner that just set preferences should be migrated to internals.settings or testRunner.overridePreference calls, I think (I don't know if either is preferred). I support the idea of unifying the approaches and just use internals.settings. However, the last time I checked, Alexey had some concerns about using internals due to settings may not be properly propagated to WebKit2 layer. Has this concern been addressed? In general I prefer the overridePreference() calls whenever they exist. internals.settings are not exposed in any real-world product whereas preferences exist in each platform's WebKit-layer API that they expose to their embedders in some form. The main downside of overridePreference is that it requires that you expose an API for twiddling the preference on every port. That can lead to us exposing unneeded APIs (making them harder to remove) and to a bunch of port-specific code in an otherwise port-independent patch. IMHO, we should prefer InternalSettings unless we need to test the WebKit-layer code. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Settings and Preferences in layout tests
On Wed, Sep 26, 2012 at 2:35 PM, Brady Eidson beid...@apple.com wrote: On Sep 26, 2012, at 2:05 PM, Adam Barth aba...@webkit.org wrote: [re-sent from the proper address] On Wed, Sep 26, 2012 at 2:00 PM, Adam Barth abarth@nowhere wrote: On Wed, Sep 26, 2012 at 1:53 PM, Brady Eidson beid...@apple.com wrote: On Sep 26, 2012, at 1:48 PM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Sep 26, 2012 at 1:44 PM, Simon Fraser simon.fra...@apple.comwrote: First, direct calls on testRunner that just set preferences should be migrated to internals.settings or testRunner.overridePreference calls, I think (I don't know if either is preferred). I support the idea of unifying the approaches and just use internals.settings. However, the last time I checked, Alexey had some concerns about using internals due to settings may not be properly propagated to WebKit2 layer. Has this concern been addressed? In general I prefer the overridePreference() calls whenever they exist. internals.settings are not exposed in any real-world product whereas preferences exist in each platform's WebKit-layer API that they expose to their embedders in some form. The main downside of overridePreference is that it requires that you expose an API for twiddling the preference on every port. That can lead to us exposing unneeded APIs (making them harder to remove) and to a bunch of port-specific code in an otherwise port-independent patch. IMHO, we should prefer InternalSettings unless we need to test the WebKit-layer code. I suppose we're biased in Mac-land where the mechanism originated, but the API is simply a single string based API that only had to be implemented once. Your comment leads me to understand that at least one other port doesn't have simple string based preferences. Of course to add *any* new internal setting new code must be added specifically for that setting... Of course that code only has to be added once for all platforms… I think for all the major ports, they are simple string based preferences. However, adding a new overridePreference call means updating 5 WebKit API interfaces (Mac, Win, Chromium, GTK+, QT), and updating 5 DRTs and 1 WTR. Compared to updating a single internal.settings implementation, this is a lot of work. In addition to being a lot of work, it increases the size of each WebKit API even if the particular port doesn't want/need to expose the feature. tony ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Settings and Preferences in layout tests
On Wed, Sep 26, 2012 at 2:35 PM, Brady Eidson beid...@apple.com wrote: On Sep 26, 2012, at 2:05 PM, Adam Barth aba...@webkit.org wrote: [re-sent from the proper address] On Wed, Sep 26, 2012 at 2:00 PM, Adam Barth abarth@nowhere wrote: On Wed, Sep 26, 2012 at 1:53 PM, Brady Eidson beid...@apple.com wrote: On Sep 26, 2012, at 1:48 PM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Sep 26, 2012 at 1:44 PM, Simon Fraser simon.fra...@apple.comwrote: First, direct calls on testRunner that just set preferences should be migrated to internals.settings or testRunner.overridePreference calls, I think (I don't know if either is preferred). I support the idea of unifying the approaches and just use internals.settings. However, the last time I checked, Alexey had some concerns about using internals due to settings may not be properly propagated to WebKit2 layer. Has this concern been addressed? In general I prefer the overridePreference() calls whenever they exist. internals.settings are not exposed in any real-world product whereas preferences exist in each platform's WebKit-layer API that they expose to their embedders in some form. The main downside of overridePreference is that it requires that you expose an API for twiddling the preference on every port. That can lead to us exposing unneeded APIs (making them harder to remove) and to a bunch of port-specific code in an otherwise port-independent patch. IMHO, we should prefer InternalSettings unless we need to test the WebKit-layer code. I suppose we're biased in Mac-land where the mechanism originated, but the API is simply a single string based API that only had to be implemented once. Your comment leads me to understand that at least one other port doesn't have simple string based preferences. Of course to add *any* new internal setting new code must be added specifically for that setting... Of course that code only has to be added once for all platforms… I would argue it's not a clear cut decision either way. I'm curious whether you've added something to overridePreferences recently. It's a ton of port-specific work. For example, here's the changed that added WebKitCSSGridLayoutEnabled: http://trac.webkit.org/changeset/117613. It had to change these files: Source/WebKit/chromium/public/WebSettings.h Source/WebKit/chromium/src/WebSettingsImpl.cpp Source/WebKit/chromium/src/WebSettingsImpl.h Source/WebKit/mac/WebView/WebPreferenceKeysPrivate.h Source/WebKit/mac/WebView/WebPreferences.mm Source/WebKit/mac/WebView/WebPreferencesPrivate.h Source/WebKit/mac/WebView/WebView.mm Source/WebKit2/Shared/WebPreferencesStore.h Source/WebKit2/UIProcess/API/C/WKPreferences.cpp Source/WebKit2/UIProcess/API/C/WKPreferencesPrivate.h Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp Source/WebKit2/WebProcess/WebPage/WebPage.cpp Tools/DumpRenderTree/chromium/LayoutTestController.cpp Tools/DumpRenderTree/chromium/WebPreferences.cpp Tools/DumpRenderTree/chromium/WebPreferences.h ... and even then, the patch only made it work in Chromium and apple-mac! By contrast, here's a patch that adds something to InternalSettings: http://trac.webkit.org/changeset/124372: Source/WebCore/testing/InternalSettings.cpp Source/WebCore/testing/InternalSettings.h Source/WebCore/testing/InternalSettings.idl Source/WebKit/chromium/public/WebSettings.h Source/WebKit/chromium/src/WebSettingsImpl.cpp Source/WebKit/chromium/src/WebSettingsImpl.h ... and now it we can run the tests on all ports. The API for twiddling the preference is exposed only on Chromium, which is the only port that wishes to expose this setting at this time. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Settings and Preferences in layout tests
On Sep 26, 2012, at 2:46 PM, Tony Chang t...@chromium.org wrote:
On Wed, Sep 26, 2012 at 2:35 PM, Brady Eidson beid...@apple.com wrote:
On Sep 26, 2012, at 2:05 PM, Adam Barth aba...@webkit.org wrote:
[re-sent from the proper address]
On Wed, Sep 26, 2012 at 2:00 PM, Adam Barth abarth@nowhere wrote:
On Wed, Sep 26, 2012 at 1:53 PM, Brady Eidson beid...@apple.com wrote:
On Sep 26, 2012, at 1:48 PM, Ryosuke Niwa rn...@webkit.org wrote:
On Wed, Sep 26, 2012 at 1:44 PM, Simon Fraser simon.fra...@apple.com
wrote:
First, direct calls on testRunner that just set preferences should be
migrated to internals.settings or testRunner.overridePreference calls, I
think (I don't know if either is preferred).
I support the idea of unifying the approaches and just use
internals.settings. However, the last time I checked, Alexey had some
concerns about using internals due to settings may not be properly
propagated to WebKit2 layer. Has this concern been addressed?
In general I prefer the overridePreference() calls whenever they exist.
internals.settings are not exposed in any real-world product whereas
preferences exist in each platform's WebKit-layer API that they expose to
their embedders in some form.
The main downside of overridePreference is that it requires that you expose
an API for twiddling the preference on every port. That can lead to us
exposing unneeded APIs (making them harder to remove) and to a bunch of
port-specific code in an otherwise port-independent patch.
IMHO, we should prefer InternalSettings unless we need to test the
WebKit-layer code.
I suppose we're biased in Mac-land where the mechanism originated, but the
API is simply a single string based API that only had to be implemented
once.
Your comment leads me to understand that at least one other port doesn't have
simple string based preferences.
Of course to add *any* new internal setting new code must be added
specifically for that setting...
Of course that code only has to be added once for all platforms…
I think for all the major ports, they are simple string based preferences.
However, adding a new overridePreference call means updating 5 WebKit API
interfaces (Mac, Win, Chromium, GTK+, QT), and updating 5 DRTs and 1 WTR.
Compared to updating a single internal.settings implementation, this is a lot
of work.
I think you misunderstand what I meant.
The Mac DRT implementation of overridePreference has the following
implementation:
void TestRunner::overridePreference(JSStringRef key, JSStringRef value)
{
RetainPtrCFStringRef keyCF(AdoptCF,
JSStringCopyCFString(kCFAllocatorDefault, key));
NSString *keyNS = (NSString *)keyCF.get();
RetainPtrCFStringRef valueCF(AdoptCF,
JSStringCopyCFString(kCFAllocatorDefault, value));
NSString *valueNS = (NSString *)valueCF.get();
[[WebPreferences standardPreferences]
_setPreferenceForTestWithValue:valueNS forKey:keyNS];
}
This works for any preference; Even a new one that has never been twiddled in
a regression test before.
For example in http://trac.webkit.org/changeset/127956 we added a new test that
twiddled the WebKitStorageBlockingPolicy preference and we didn't need to
change any DRT Mac code to accomplish this.
Compared to adding a single implementation to internal.settings, this was *NO*
additional work.
In addition to being a lot of work, it increases the size of each WebKit API
even if the particular port doesn't want/need to expose the feature.
If a port doesn't want the feature, it can't be tested and needs to be skipped
anyways, right?
~Brady
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Settings and Preferences in layout tests
On Sep 26, 2012, at 4:13 PM, Brady Eidson beid...@apple.com wrote:
On Sep 26, 2012, at 2:46 PM, Tony Chang t...@chromium.org wrote:
On Wed, Sep 26, 2012 at 2:35 PM, Brady Eidson beid...@apple.com wrote:
On Sep 26, 2012, at 2:05 PM, Adam Barth aba...@webkit.org wrote:
[re-sent from the proper address]
On Wed, Sep 26, 2012 at 2:00 PM, Adam Barth abarth@nowhere wrote:
On Wed, Sep 26, 2012 at 1:53 PM, Brady Eidson beid...@apple.com wrote:
On Sep 26, 2012, at 1:48 PM, Ryosuke Niwa rn...@webkit.org wrote:
On Wed, Sep 26, 2012 at 1:44 PM, Simon Fraser simon.fra...@apple.com
wrote:
First, direct calls on testRunner that just set preferences should be
migrated to internals.settings or testRunner.overridePreference calls, I
think (I don't know if either is preferred).
I support the idea of unifying the approaches and just use
internals.settings. However, the last time I checked, Alexey had some
concerns about using internals due to settings may not be properly
propagated to WebKit2 layer. Has this concern been addressed?
In general I prefer the overridePreference() calls whenever they exist.
internals.settings are not exposed in any real-world product whereas
preferences exist in each platform's WebKit-layer API that they expose to
their embedders in some form.
The main downside of overridePreference is that it requires that you expose
an API for twiddling the preference on every port. That can lead to us
exposing unneeded APIs (making them harder to remove) and to a bunch of
port-specific code in an otherwise port-independent patch.
IMHO, we should prefer InternalSettings unless we need to test the
WebKit-layer code.
I suppose we're biased in Mac-land where the mechanism originated, but the
API is simply a single string based API that only had to be implemented
once.
Your comment leads me to understand that at least one other port doesn't
have simple string based preferences.
Of course to add *any* new internal setting new code must be added
specifically for that setting...
Of course that code only has to be added once for all platforms…
I think for all the major ports, they are simple string based preferences.
However, adding a new overridePreference call means updating 5 WebKit API
interfaces (Mac, Win, Chromium, GTK+, QT), and updating 5 DRTs and 1 WTR.
Compared to updating a single internal.settings implementation, this is a
lot of work.
I think you misunderstand what I meant.
The Mac DRT implementation of overridePreference has the following
implementation:
void TestRunner::overridePreference(JSStringRef key, JSStringRef value)
{
RetainPtrCFStringRef keyCF(AdoptCF,
JSStringCopyCFString(kCFAllocatorDefault, key));
NSString *keyNS = (NSString *)keyCF.get();
RetainPtrCFStringRef valueCF(AdoptCF,
JSStringCopyCFString(kCFAllocatorDefault, value));
NSString *valueNS = (NSString *)valueCF.get();
[[WebPreferences standardPreferences]
_setPreferenceForTestWithValue:valueNS forKey:keyNS];
}
This works for any preference; Even a new one that has never been twiddled
in a regression test before.
For example in http://trac.webkit.org/changeset/127956 we added a new test
that twiddled the WebKitStorageBlockingPolicy preference and we didn't need
to change any DRT Mac code to accomplish this.
Compared to adding a single implementation to internal.settings, this was
*NO* additional work.
But is there code to undo this pref change for subsequent tests?
Simon
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Settings and Preferences in layout tests
On Sep 26, 2012, at 4:43 PM, Adam Barth aba...@webkit.org wrote: Maybe a better solution is auto-generate all this boilerplate code? If we had a Settings.in file, we could generate all the port-specific code (and maybe even much of Settings.h/cpp) automatically. Then all of these patches would be one-liners and work correctly on every port. I would be all for that. I'm fuzzy on how the WebCore::Settings boiler plate would correctly apply to all WebKit ports in general, but I can see how it would easily solve our use case. ~Brady Adam On Wed, Sep 26, 2012 at 4:28 PM, Brady Eidson beid...@apple.com wrote: On Sep 26, 2012, at 4:15 PM, Simon Fraser simon.fra...@apple.com wrote: On Sep 26, 2012, at 4:13 PM, Brady Eidson beid...@apple.com wrote: This works for any preference; Even a new one that has never been twiddled in a regression test before. For example in http://trac.webkit.org/changeset/127956 we added a new test that twiddled the WebKitStorageBlockingPolicy preference and we didn't need to change any DRT Mac code to accomplish this. Compared to adding a single implementation to internal.settings, this was *NO* additional work. But is there code to undo this pref change for subsequent tests? I looked into the mechanism that does this. On Sep 26, 2012, at 1:44 PM, Simon Fraser simon.fra...@apple.com wrote: I looked at testRunner.overridePreference(), and it doesn't appear to reset the value at the end of the test. That happens elsewhere, in: static void resetDefaultsToConsistentValues() Indeed, each individual pref is currently managed with unique API calls here, and the example I provided of WebKitStorageBlockingPolicy leaks. However, the key/value based preference mechanism can easily be augmented within DRT in a general way that will fix this and all future key/value preference usage. That change would only have to happen once per port (assuming the port supports key/value based prefs) Brady ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Settings and Preferences in layout tests
On Wed, Sep 26, 2012 at 4:51 PM, Brady Eidson beid...@apple.com wrote: On Sep 26, 2012, at 4:43 PM, Adam Barth aba...@webkit.org wrote: Maybe a better solution is auto-generate all this boilerplate code? If we had a Settings.in file, we could generate all the port-specific code (and maybe even much of Settings.h/cpp) automatically. Then all of these patches would be one-liners and work correctly on every port. I would be all for that. I'm fuzzy on how the WebCore::Settings boiler plate would correctly apply to all WebKit ports in general, but I can see how it would easily solve our use case. I mean all the port-specific boilerplate in http://trac.webkit.org/changeset/117613 as well. I don't have time to do this myself right now, but I'm happy to point folks in the right direction. Adam On Wed, Sep 26, 2012 at 4:28 PM, Brady Eidson beid...@apple.com wrote: On Sep 26, 2012, at 4:15 PM, Simon Fraser simon.fra...@apple.com wrote: On Sep 26, 2012, at 4:13 PM, Brady Eidson beid...@apple.com wrote: This works for any preference; Even a new one that has never been twiddled in a regression test before. For example in http://trac.webkit.org/changeset/127956 we added a new test that twiddled the WebKitStorageBlockingPolicy preference and we didn't need to change any DRT Mac code to accomplish this. Compared to adding a single implementation to internal.settings, this was *NO* additional work. But is there code to undo this pref change for subsequent tests? I looked into the mechanism that does this. On Sep 26, 2012, at 1:44 PM, Simon Fraser simon.fra...@apple.com wrote: I looked at testRunner.overridePreference(), and it doesn't appear to reset the value at the end of the test. That happens elsewhere, in: static void resetDefaultsToConsistentValues() Indeed, each individual pref is currently managed with unique API calls here, and the example I provided of WebKitStorageBlockingPolicy leaks. However, the key/value based preference mechanism can easily be augmented within DRT in a general way that will fix this and all future key/value preference usage. That change would only have to happen once per port (assuming the port supports key/value based prefs) Brady ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
