On Wed, Sep 26, 2012 at 2:35 PM, Brady Eidson <[email protected]> wrote:
> > On Sep 26, 2012, at 2:05 PM, Adam Barth <[email protected]> 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 <[email protected]> wrote: >> >>> >>> On Sep 26, 2012, at 1:48 PM, Ryosuke Niwa <[email protected]> wrote: >>> >>> On Wed, Sep 26, 2012 at 1:44 PM, Simon Fraser <[email protected]>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. > 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 [email protected] http://lists.webkit.org/mailman/listinfo/webkit-dev
