DumpRenderTree needs a way to override settings on a per-test basis Something like this: if (window.layoutTestController) layoutTestController.overrideSetting("WhateverYourSettingName", "YES"); or this: if (window.settingsController) settingsController.overrideSetting("WhateverYourSettingName", "YES"); This would allow us to test all sorts of things that we can't today (maybe even disabling javascript!?).
Created attachment 23056 [details] Possible patch to issue 20534 Here is a possible patch to this issue. There are some questions that still remain, however. This patch allows a layout test to override a default WebPreference value at test time. For instance: layoutTestController.overridePreference("WebKitJavaScriptEnabled", false); will turn javascript off for the rest of the test. Several decisions/observations I made: 1. To keep things simple, I only developed this patch to set or unset boolean flags. It would be possible to add other methods that set string or integer values, but it would either require IWebPreferences.idl to handle classes it probably shouldn't (CFPropertyListRef), or multiple methods in LayoutTestController to accept multiple input data types. Both of those options seemed less than appealing to me, but I'm hoping for feedback about what to do there. 2. Overriding the flag seems to only work once per layout test -- that is, you cannot disable javascript, try to do some javascript work, then re-enable javascript. I'm thinking that this is an effect of layoutTestController, and not the new overridePreference method. 3. The key values used in the call (i.e. "WebKitJavaScriptEnabled") are drawn directly from WebPreferences. So if there is a key in the dictionary with a boolean value set for it, then it can be set. I added multiple erroneous calls in the layout test to illustrate that passing bad input to overridePreference would not crash the test. What else should be done here?
Comment on attachment 23056 [details] Possible patch to issue 20534 It would need to reset preferences after every test in order to be useful. Otherwise you run any test after this one, and they'll all fail. :)
Aha, I see. I thought that the LayoutTestController reset the preferences automagically, but I was wrong. I'll upload another patch that does reset them after each test.
Comment on attachment 23056 [details] Possible patch to issue 20534 r- due to corrupting other tests. Also, I'm not sure disabling JS is the best test. But I'm not sure I have a better suggestion atm.
Created attachment 23105 [details] Possible patch to issue 20534 Ok, here's the patch with additional support for resetting default values after each test. Changes from earlier patch: 1. DumpRenderTree now calls a method on IWebPreferences called "resetToDefaults". 2. Added "resetToDefaults" method to WebPreferences. "resetToDefaults" will go through all of the preferences set in its internal dictionary and replace them with the default values, but _ONLY_ if some value was overridden. If no value was overridden, this method simply returns. 3. Changed overridePreference to take a string value instead of just boolean. Although starting simple with just boolean support seemed right initially, I didn't take into account that any positive string value (such as "true", "yes", "lfaijweofij") would be interpreted as true. Any empty value (such as false, 0, maybe ""?) would be interpreted as false. So this way, the method can now support overriding both boolean values and string values in the preferences. 4. More layout tests were added. One was added to test the overriding of string values. Another was added specifically to test that preferences were being reset after every value. But wait, there's more! WebPreferences resets after every test by default. But now you can also specifically override a flag to tell WebPreferences NOT to reset. That is: layoutTestController.overridePreference("WebKitDefaultPreferencesOverridden", false); will make WebPreferences think that it hasn't been overriden, and so it won't reset until you call: layoutTestController.overridePreference("WebKitDefaultPreferencesOverridden", true); This means that you can specifically override a preference in the first test of a long series of tests, tell the preferences not to reset after that test, then tell the preferences to reset after the last test needing the overrides. So you don't need to override the same preference over and over in a long line of tests. ....Whether this is useful or not is up for debate :) Thanks to Eric for his feedback on this!
Comment on attachment 23105 [details] Possible patch to issue 20534 This patch is windows only... an improved patch is in the works...
Created attachment 23328 [details] Improved possible patch to issue 20534 Here is a version of the patch that also provides Mac support.
(In reply to comment #5) > But wait, there's more! WebPreferences resets after every test by default. > But now you can also specifically override a flag to tell WebPreferences NOT to > reset. That is: > layoutTestController.overridePreference("WebKitDefaultPreferencesOverridden", > false); > will make WebPreferences think that it hasn't been overriden, and so it won't > reset until you call: > layoutTestController.overridePreference("WebKitDefaultPreferencesOverridden", > true); > > This means that you can specifically override a preference in the first test of > a long series of tests, tell the preferences not to reset after that test, then > tell the preferences to reset after the last test needing the overrides. So > you don't need to override the same preference over and over in a long line of > tests. ....Whether this is useful or not is up for debate :) I don't think we want this. I can lead to bad tests that can't be run individually. It is true that we run the tests mostly in order, but each test should also be able to run individually. Also if a new unsuspecting test is added in the future and ends up sorting in the middle of a group that has settings disabled, that would be a pain!
(In reply to comment #1) > This patch allows a layout test to override a default WebPreference value at > test time. For instance: > > layoutTestController.overridePreference("WebKitJavaScriptEnabled", false); > > will turn javascript off for the rest of the test. Are you sure this fully works? In general I think the pages needs to be reloaded to disable JavaScript (at least fully).
Comment on attachment 23328 [details] Improved possible patch to issue 20534 You can't change WebPreferences.h (Mac) since that is a public framework header. Any changes to those public headers need formal API review first. The correct change is to put things in WebPreferencesPrivate.h, until they can someday be made public. r- because of this. I didn't review the rest.
Created attachment 23484 [details] Improved possible fix to bug 20534 I've modified this patch to have overridePreference and resetToDefaults in WebPreferencesPrivate instead of WebPreferences. re: testing...I thought it was a neat quirk of the patch, but now I can see that using it would probably be unwise, since all tests should be able to be run independently. Thanks for the review & the feedback!
Comment on attachment 23484 [details] Improved possible fix to bug 20534 Looks fine. Glenn and I talked about possibly having DumpRenderTree maintain the state of whether the preferences were "overridden" instead of extending the preferences object to know this. DRT doesn't currently have a clean way for layout test controller to record this information, and there is some (small) possibility that some other client of preferences might want to be able to check if any preferences have been changed from the defaults or to reset to the defaults in an efficient way, so he's left that added functionality in the patch. There were a couple c-style casts which could have been static_cast, but not enough to justify another round of patch.
Comment on attachment 23484 [details] Improved possible fix to bug 20534 I think the new functions added to WebPreferences should have some explicit comments near their declarations stating more precisely what their purpose is (i.e., these are for DRT only). + CFStringRef keyref = CFStringCreateWithCharactersNoCopy(0, (UniChar*)_wcsdup(key), (CFIndex)_tcslen(key), kCFAllocatorMalloc); This string is being leaked. Passing in kCFAllocatorMalloc here is wrong. This will cause CF to call free(key) when keyref is finalized. + setStringValue(keyref, value); Indentation here looks funny. + BSTR val = stringValueForKey(static_cast<CFStringRef>(keys[i])); val is being leaked here. It also isn't used at all. + wstring wKey = jsStringRefToWString(key); + wstring wFlag = jsStringRefToWString(flag); + + BSTR keyBSTR = SysAllocStringLen((OLECHAR*)wKey.c_str(), wKey.length()); + BSTR flagBSTR = SysAllocStringLen((OLECHAR*)wFlag.c_str(), wFlag.length()); You can use JSStringCopyBSTR here (from JSStringRefBSTR.h) instead. r- so that the leaks and free() problem above can be fixed.
Comment on attachment 23484 [details] Improved possible fix to bug 20534 + CFStringRef keyref = CFStringCreateWithCharactersNoCopy(0, (UniChar*)_wcsdup(key), (CFIndex)_tcslen(key), kCFAllocatorMalloc); Also, that should be wcslen, not _tcslen.
T(In reply to comment #14) > (From update of attachment 23484 [details] [edit]) > + CFStringRef keyref = CFStringCreateWithCharactersNoCopy(0, > (UniChar*)_wcsdup(key), (CFIndex)_tcslen(key), kCFAllocatorMalloc); > > Also, that should be wcslen, not _tcslen. Thank you Adam for the review.
Created attachment 24198 [details] Improved possible patch to issue 20534 Ok, the latest patch should clean up these issues. >I think the new functions added to WebPreferences should have some explicit >comments near their declarations stating more precisely what their purpose is >(i.e., these are for DRT only). Done, added comments to WebPreferences.h describing that these functions are used in DumpRenderTree testing only. >+ CFStringRef keyref = CFStringCreateWithCharactersNoCopy(0, >(UniChar*)_wcsdup(key), (CFIndex)_tcslen(key), kCFAllocatorMalloc); > >This string is being leaked. >Passing in kCFAllocatorMalloc here is wrong. This will cause CF to call >free(key) when keyref is finalized. Oops. I'll admit my ignorance about kCFAllocatorMalloc here. I've changed this to kCFAllocatorNull, since the caller should take care of the cleanup. I debated using kCFAllocatorDefault, but I didn't want WebPreferences to get overzealous in cleaning up memory it didn't allocate. I also changed one of the c-style casts to a static_cast. I tried changing both, by my compiler didn't know of a good cast between a wide character and an unsigned short. So I left this as a c-style cast for now (it is done this way elsewhere in WebPreferences). >+ setStringValue(keyref, value); > >Indentation here looks funny. Fixed this. >+ BSTR val = stringValueForKey(static_cast<CFStringRef>(keys[i])); > >val is being leaked here. It also isn't used at all. Yep, removed this. A remnant of another way I was looping through values here. >+ wstring wKey = jsStringRefToWString(key); >+ wstring wFlag = jsStringRefToWString(flag); >+ >+ BSTR keyBSTR = SysAllocStringLen((OLECHAR*)wKey.c_str(), wKey.length()); >+ BSTR flagBSTR = SysAllocStringLen((OLECHAR*)wFlag.c_str(), >wFlag.length()); > >You can use JSStringCopyBSTR here (from JSStringRefBSTR.h) instead. Aha -- that made this much easier than allocating more memory. I had to add an include for JSStringRefBSTR. Thanks for the review!
Comment on attachment 24198 [details] Improved possible patch to issue 20534 1190 CFStringRef keyref = CFStringCreateWithCharactersNoCopy(0, (UniChar*)_wcsdup(key), static_cast<CFIndex>(wcslen(key)), kCFAllocatorNull); You're leaking keyref. I also see now that you were using _wcsdup, so in fact kCFAllocatorMalloc was correct (sorry for misleading you!). But I think we should just let CF handle the memory allocation, like this: RetainPtr<CFStringRef> keyRef(AdoptCF, CFStringCreateWithCharacters(0, key, wcslen(key))); (This will also fix the leak of keyRef by putting RetainPtr in charge of releasing it). 1209 for (int i = 0; i < count; ++i) { 1210 setValueForKey(static_cast<CFStringRef>(keys[i]), values[i]); 1211 } The curly braces here should be removed. 323 // This method is meant for overriding preferences for tests run 324 // with DumpRenderTree only. 325 virtual HRESULT STDMETHODCALLTYPE overridePreference( 326 /* [in] */ BSTR key, 327 /* [in] */ BSTR value); 328 329 // This method is meant for resetting overridden preferences for tests run 330 // with DumpRenderTree only. 331 virtual HRESULT STDMETHODCALLTYPE resetToDefaults(); Thanks for adding these comments. You should also put them in IWebPreferencesPrivate.idl and WebPreferencesPrivate.h. 645 prefsPrivate->setAuthorAndUserStylesEnabled(TRUE); 646 prefsPrivate->resetToDefaults(); What is the effect of the call to resetToDefaults on setAuthorAndUserStylesEnabled? I think this is quite close! r- so that we can fix the remaining memory issues. Thanks for working on this!
(In reply to comment #16) > I also changed one of the c-style casts to a static_cast. I tried changing > both, by my compiler didn't know of a good cast between a wide character and an > unsigned short. I think reinterpret_cast is what you want for this.
Created attachment 24204 [details] Improved possible patch to issue 20534 >You're leaking keyref. I also see now that you were using _wcsdup, so in fact >kCFAllocatorMalloc was correct (sorry for misleading you!). But I think we >should just let CF handle the memory allocation, like this: > >RetainPtr<CFStringRef> keyRef(AdoptCF, CFStringCreateWithCharacters(0, key, >wcslen(key))); > >(This will also fix the leak of keyRef by putting RetainPtr in charge of >releasing it). Ah, much easier this way :) I added the line you suggested with one change: a reinterpret_cast was necessary to cast the BSTR to UniChar*, so thanks for that tip! >The curly braces here should be removed. Removed. >Thanks for adding these comments. You should also put them in >IWebPreferencesPrivate.idl and WebPreferencesPrivate.h. Added the same comments to both places. I changed the previous style of comment that I used in WebPreferencesPrivate to the style the rest of the file used (just "//"s). The only other place I can see comments being useful is in all the LayoutTestController files, but I decided against it since the LayoutTestController is only for running tests anyway (...or is it?) >What is the effect of the call to resetToDefaults on >setAuthorAndUserStylesEnabled? Since the default is the same as what is being set on this line, the net effect is zero. *But* since the default for this setting could change in the future, I've flipped these lines so the reset call won't clobber this setting. The modified Mac version of DumpRenderTree works this way, so it makes sense for consistency. Thanks for your help!!
Comment on attachment 24204 [details] Improved possible patch to issue 20534 1060 [self _postPreferencesChangesNotification]; 1061 [self _setBoolValue:false forKey:WebKitDefaultPreferencesOverridden]; On Windows you perform these operations in the opposite order. Which one is better? 1190 RetainPtr<CFStringRef> keyRef(AdoptCF, CFStringCreateWithCharacters(0, reinterpret_cast<UniChar*>(key), wcslen(key))); I just remembered that 0 is a valid BSTR. What should be our behavior when 0 is passed in? Currently I believe we will crash. Using SysStringLen instead of wcslen would help, but I don't think we should be passing 0 for the characters pointer to CFStringCreateWithCharacters. Probably we should just bail out with E_FAIL in this case. 335 controller->overridePreference(key.get(), JSValueToStringCopy(context, arguments[1], exception)); It looks like you're leaking the second parameter to overridePreference here. Sorry for not catching these on the last go-round. But once these are fixed we should be able to land this!
Created attachment 24232 [details] Improved possible patch to issue 20534 >On Windows you perform these operations in the opposite order. Which one is >better? The way I was doing this on Windows is probably better, because it avoids a race condition. The changes notification should happen after the flag gets reset, so that any listeners who care about the flag being reset will see the correct state. So, I flipped these lines on the Mac implementation of this. > 1190 RetainPtr<CFStringRef> keyRef(AdoptCF, >CFStringCreateWithCharacters(0, reinterpret_cast<UniChar*>(key), wcslen(key))); > >I just remembered that 0 is a valid BSTR. What should be our behavior when 0 is >passed in? Currently I believe we will crash. Using SysStringLen instead of >wcslen would help, but I don't think we should be passing 0 for the characters >pointer to CFStringCreateWithCharacters. Probably we should just bail out with >E_FAIL in this case. So I'm not totally sure this is the right way to do this, but I added this check: if (!key || SysStringLen(key) == 0) return E_FAIL; I figured that the BSTR could be either 0 (like you said), or an empty string (the case in which it is non-zero but empty). So I added a bailout on either one, since we probably don't want empty string key. It doesn't seem to have this type of problem on the Mac version, since it just passes the pointer through...no allocation of memory needed. > 335 controller->overridePreference(key.get(), JSValueToStringCopy(context, >arguments[1], exception)); > >It looks like you're leaking the second parameter to overridePreference here. > Ah, yes...good catch. I got the key RetainPtr right, but not the value. This has been changed so both are made into RetainPtrs before being passed in. Please let me know if you find anything else that should be fixed! Thanks for all of your feedback.
(In reply to comment #21) > So I'm not totally sure this is the right way to do this, but I added this > check: > > if (!key || SysStringLen(key) == 0) > return E_FAIL; > > I figured that the BSTR could be either 0 (like you said), or an empty string > (the case in which it is non-zero but empty). So I added a bailout on either > one, since we probably don't want empty string key. Agreed, we should treat empty the same as null. Passing null to SysStringLen should work just fine, so the first half of your || isn't needed, but it's not terrible to thave it there, either. > It doesn't seem to have > this type of problem on the Mac version, since it just passes the pointer > through...no allocation of memory needed. Mac and Windows are doing the same thing here: DRT allocates the string, then passes a pointer to the string in to WebKit. So maybe Mac needs a similar check, or maybe not, depending on how the code handles null/empty strings.
Comment on attachment 24232 [details] Improved possible patch to issue 20534 1190 if (!key || SysStringLen(key) == 0) 1191 return E_FAIL; We prefer to use ! instead of ==0. We also need the same check for value. And as mentioned above, the null-check isn't necessary. 331 JSRetainPtr<JSStringRef> key(Adopt, JSValueToStringCopy(context, arguments[0], exception)); 332 ASSERT(!*exception); 333 JSRetainPtr<JSStringRef> value(Adopt, JSValueToStringCopy(context, arguments[1], exception)); If you're going to assert about one you should assert about both. So close!
Created attachment 24237 [details] Improved possible patch to issue 20534 > 1190 if (!key || SysStringLen(key) == 0) > 1191 return E_FAIL; > >We prefer to use ! instead of ==0. We also need the same check for value. And >as mentioned above, the null-check isn't necessary. Changed. I added value to the check and used ! instead: if (!SysStringLen(key) || !SysStringLen(value)) return E_FAIL; > 331 JSRetainPtr<JSStringRef> key(Adopt, JSValueToStringCopy(context, >arguments[0], exception)); > 332 ASSERT(!*exception); > 333 JSRetainPtr<JSStringRef> value(Adopt, JSValueToStringCopy(context, >arguments[1], exception)); > >If you're going to assert about one you should assert about both. I wasn't sure whether it was better to try to assert twice (after trying to create each one), or just once after they're both attempted. I went with the former, since it is probably better not to try to create the JSStringRef to value if the creation of the JSStringRef to key failed. Other functions in this class use multiple asserts as well, so this should be consistent. Thanks! We're almost there, I think.
Adam, can you take a look at Glenn's latest patch? Thank you :-)
Comment on attachment 24237 [details] Improved possible patch to issue 20534 > +HRESULT STDMETHODCALLTYPE WebPreferences::overridePreference(BSTR key, BSTR value) > +{ > + if (!SysStringLen(key) || !SysStringLen(value)) > + return E_FAIL; > + RetainPtr<CFStringRef> keyRef(AdoptCF, CFStringCreateWithCharacters(0, reinterpret_cast<UniChar*>(key), wcslen(key))); We should use SysStringLen instead of wcslen when dealing with BSTRs, since it is O(1) as opposed to O(n). r=me even if we keep using wcslen. If you upload a new version that uses SysStringLen I'll r+ that, too. Thanks for all the revisions!
Created attachment 24915 [details] Possible fix for issue 20534 Here's a version with SysStringLen instead of wcslen. Thanks for going through all the revisions with me!
Comment on attachment 24915 [details] Possible fix for issue 20534 r=me
Glenn, please post an updated patch. This one no longer applies cleanly.
Created attachment 25486 [details] An updated patch for 20534 Here is a revived version of this patch that should apply cleanly. It looks like the conflicts came from several new methods were added to IWebPreferencesPrivate, and the references to WebPreferencesPrivate in DumpRenderTree having been moved. Fortunately, this reduces the number of lines changed by this patch :)
Comment on attachment 25486 [details] An updated patch for 20534 > Index: WebKit/mac/ChangeLog > =================================================================== > --- WebKit/mac/ChangeLog (revision 37402) > +++ WebKit/mac/ChangeLog (working copy) > @@ -1,3 +1,16 @@ > +2008-10-08 Glenn Wilson <gwilson@google.com> > + > + Added new methods for overriding default WebPreference values > + and for resetting preferences to their defaults. (bug 20534) It would be nice to add the URL of the bug in here (and the other ChangeLogs), too. > + [preferences setEditableLinkBehavior:WebKitEditableLinkOnlyLiveWithShiftKey]; Why was this line added? I don't know why I didn't question it before!
Created attachment 25496 [details] Improved possible patch for issue 20534 >> Why was this line added? I don't know why I didn't question it before! Strange. This line may have been part of the resetWebViewTo... method in the past, and was removed in a revision since the initial version of the patch. Or maybe a copy & paste error. Anyways, it shouldn't be in there, so I removed it. Good catch! Also, rather than just referring to the issue numbers in the ChangeLogs, I replaced them with links directly to the issues. Thanks for the review!
Comment on attachment 25496 [details] Improved possible patch for issue 20534 r=me
Committing to http://svn.webkit.org/repository/webkit/trunk ... A LayoutTests/security/override-preferences-2-expected.txt A LayoutTests/security/override-preferences-2.html A LayoutTests/security/override-preferences-expected.txt A LayoutTests/security/override-preferences.html A LayoutTests/security/override-zzz-reset-expected.txt A LayoutTests/security/override-zzz-reset.html M WebKit/mac/ChangeLog M WebKit/mac/WebView/WebPreferenceKeysPrivate.h M WebKit/mac/WebView/WebPreferences.mm M WebKit/mac/WebView/WebPreferencesPrivate.h M WebKit/win/ChangeLog M WebKit/win/Interfaces/IWebPreferencesPrivate.idl M WebKit/win/WebPreferenceKeysPrivate.h M WebKit/win/WebPreferences.cpp M WebKit/win/WebPreferences.h M WebKitTools/ChangeLog M WebKitTools/DumpRenderTree/LayoutTestController.cpp M WebKitTools/DumpRenderTree/LayoutTestController.h M WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm M WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm M WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp M WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp Committed r39212 A couple issues in the commit. one of the changelogs was missing a bug url, added. Another one had the review not as the top-most line, fixed. 2 files had tabs in them (which causes the commit to fail), removed I saw some leaks reported when I ran the layout tests. I assumed those were not related to this patch, and landed anyway. If I'm wrong, I've got a roll-out ahead of me. :) Thanks for the patch!
This patch was rolled out in r39277 because it caused many test failures and assertion failures. Also, calling init again from resetToDefaults is a pretty bad idea.
Comment on attachment 25496 [details] Improved possible patch for issue 20534 r- this rolled out patch.
Created attachment 26328 [details] Revised patch to issue 20534 Here is a revised patch that may address this issue. resetToDefaults in WebPreferences.mm was calling init, which is definitely bad. I didn't know what other interactions init would have, so hopefully I know better now. I modified resetToDefaults to work much like the Win version of WebPreferences...it gets a dictionary of all the default values, loops through each entry, and resets each key/value pair to the default. To do this, I had to break out the default dictionary into its own method that just returns the dict, and have "initialize" call this method. This seemed acceptable, since this is not an instance method, no interface changes were necessary. I didn't want to break it out, but using NSUserDefaults::standardUserDefaults as the default preference value 'repository' didn't seem to work. (Possibly because WebPreferences::initialize is never called when running layout tests?) Eric, I also tried to incorporate some of the cleanup you did elsewhere -- removing all the tabs I could find, including reviewer lines in ChangeLogs, etc. Let me know what others I missed so I can clean them up for later iterations. Thanks for the feedback, and let me know what else should be improved here.
Created attachment 26524 [details] Improved patch to issue 20534 Here is an improved version of the previous patch. This version does not use Objective-C 2.0 behavior (specifically, for(in) loops) in WebPreferences. Eric and I were talking last night on IRC about a different approach to this patch -- specifically keeping all overridden values in a separate dictionary, so there is no risk of any overridden values being saved to disk. I'm hoping to experiment with this in another patch.
Created attachment 26874 [details] Alternate patch to issue 20534 Here is an alternate patch to this that implements the ideas that Eric and I discussed a few weeks back. This patch adds a new dictionary to WebPreferencesPrivate that holds overridden values. When valueForKey looks to access a value, it first checks the overrides to see if there is an overridden value for this particular key. If so, it uses that key, if not, it goes about its normal business. overridePreference sets a value in this new dictionary, and resetToDefaults merely empties this new dictionary of all values. This version of the patch has several advantages over previous versions: - all overridden values are kept separate from default values. If there's ever a crash, these won't be saved anywhere - resetToDefaults and overridePreference are significantly simpler - it requires no flag-setting to indicate an overridden value Please take a look and let me know what can be improved!
Created attachment 27296 [details] Alternate patch to issue 20534 Here's a modified version of the override preference patch that does not have any resetToDefault method in WebPreferences. Instead, this relies on DumpRenderTree to reset preferences properly between tests. DumpRenderTree.mm was setting preference values in two places -- setDefaultsToConsistentValueForTesting and also resetWebViewToConsistentState. setDefaults was being called once when DRT starts, and resetWebView between every test. Since each layout test assumes that its preferences should be set to their default 'testing' values when it is run, I consolidated all of the web preference resetting to setDefaults and changed resetWebView to call setDefaults. This patch assumes that if a preference is meant to have a default 'testing' value, it will always be reset in DumpRenderTree. Thus, if a preference is added in the future that will need to be reset between tests, setDefaults should be modified to reset the value of the preference. I originally wanted to destroy and re-create the instance of WebPreference between tests. This is equivalent to resetting the values to their 'testing state' and getting the rest of the preferences reset as a side effect. However, the preference values that should be used for testing can often be different than the default values in WebPreferences. Moreover, if a preference must have a default testing state, it should be clearly stated as such in DumpRenderTree::setDefaultsToConsistentValueForTesting. In short, I felt that just re-instantiating WebPreferences "hides" the desired 'testing state' of these preferences. What do you think? What can I improve?
Comment on attachment 24237 [details] Improved possible patch to issue 20534 Silly bugzilla, clearing r+ to remove it from the commit queue.
Comment on attachment 24915 [details] Possible fix for issue 20534 Silly bugzilla, clearing r+ to remove it from the commit queue.
*** Bug 22816 has been marked as a duplicate of this bug. ***
Comment on attachment 27296 [details] Alternate patch to issue 20534 One minor comment: +- (void)setPreference:(NSString *)key stringValue:(NSString *)value Private ObjC methods on API classes should start with an underscore. Otherwise looks good, assuming it doesn't break the tests now. Please resubmit with that minor change.
Created attachment 32572 [details] Fixed and cleaned patch. I'd like to help move it forward. I've merged the patch and changed the method naming per Maciej's suggestion. It runs all the tests fine (on Mac).
Glen and I talked and I'll take this to complete. I have a couple of tests I want to write which need a back-forward page cache enabled :-) This will allow to override preferences for a single test while not affecting thousands of others. I built it on Windows and found at least one issue. Removing r? for now, will prepare updated patch.
Created attachment 33495 [details] Hopefully the final cut. I think this can be the final version. I've fixed windows DRT to not break the tests following the one with changed preferences. Now both mac and windows versions of DRT have 'resetDefaultsToConsistentValues()' functin which is called before each test and initializes preferences to the regular DRT 'defaults'. I also moved all the initialization of such 'defaults' into this single function. Run tests on both mac and windows. On windows (Vista) not all tests pass for me even before this patch but at least the set of failed tests is the same before and after.
Comment on attachment 33495 [details] Hopefully the final cut. > + * security/override-preferences-2-expected.txt: > + * security/override-preferences-2.html: Verifies overridePreverence("WebKitDefaultFontSize", "24") > + * security/override-preferences-expected.txt: > + * security/override-preferences.html: Verifies overridePreference("WebKitJavaScriptEnabled", false). > + * security/override-zzz-reset-expected.txt: > + * security/override-zzz-reset.html: Because of 'zzz' this test will run after the tests above and > + verify that override of preferences does not 'spill' to the subsequent tests in a batch. The added tests are missing from this patch. security/ seems like a strange place to put them. Maybe we need a new directory for tests of the test harness? > +- (void)_setPreference:(NSString *)key stringValue:(NSString *)value > +{ > + NSString *_key = KEY(key); > + [self _setStringValue:value forKey:_key]; > +} This is wrong. _setStringValue:forKey: also passes the key through KEY(), so you'll end up with the preferences identifier prepended twice. _setPreference:stringValue: is not a very Cocoa-y name. The obvious name would be _setStringValue:forKey:. Maybe we should just expose that method through WebPreferencesPrivate.h? Or maybe we can come up with a name that indicates this is for testing purposes only. I don't think it's super-important to have similar names on Mac and Windows for this. > @@ -70,6 +70,10 @@ interface IWebPreferencesPrivate : IUnknown > HRESULT setFontSmoothingContrast([in] float contrast); > HRESULT fontSmoothingContrast([out, retval] float* contrast); > > + // This method is meant for overriding preferences for tests run > + // with DumpRenderTree only. > + HRESULT setPreference([in] BSTR key, [in] BSTR value); > + > HRESULT isWebSecurityEnabled([out, retval] BOOL* enabled); > HRESULT setWebSecurityEnabled([in] BOOL enabled); Please add the new function at the end of the interface, to avoid the risk of breaking nightly builds. > +HRESULT STDMETHODCALLTYPE WebPreferences::setPreference(BSTR key, BSTR value) > +{ There's no need to have STDMETHODCALLTYPE on the implementation. It's only needed on the declaration. > + LayoutTestController* controller = reinterpret_cast<LayoutTestController*>(JSObjectGetPrivate(thisObject)); static_cast is preferable here. > +void LayoutTestController::overridePreference(JSStringRef key, JSStringRef flag) > +{ > + RetainPtr<CFStringRef> keyCF(AdoptCF, JSStringCopyCFString(kCFAllocatorDefault, key)); > + NSString *keyNS = (NSString *)keyCF.get(); > + > + RetainPtr<CFStringRef> flagCF(AdoptCF, JSStringCopyCFString(kCFAllocatorDefault, flag)); > + NSString *flagNS = (NSString *)flagCF.get(); I think we tend to use 0 instead of kCFAllocatorDefault, though either is fine. > +static void resetDefaultsToConsistentValues(IWebPreferences* preferences) > +{ > +#ifdef USE_MAC_FONTS > + BSTR standardFamily = SysAllocString(TEXT("Times")); > + BSTR fixedFamily = SysAllocString(TEXT("Courier")); > + BSTR sansSerifFamily = SysAllocString(TEXT("Helvetica")); > + BSTR cursiveFamily = SysAllocString(TEXT("Apple Chancery")); > + BSTR fantasyFamily = SysAllocString(TEXT("Papyrus")); > +#else > + BSTR standardFamily = SysAllocString(TEXT("Times New Roman")); > + BSTR fixedFamily = SysAllocString(TEXT("Courier New")); > + BSTR sansSerifFamily = SysAllocString(TEXT("Arial")); > + BSTR cursiveFamily = SysAllocString(TEXT("Comic Sans MS")); // Not actually cursive, but it's what IE and Firefox use. > + BSTR fantasyFamily = SysAllocString(TEXT("Times New Roman")); > +#endif > + > + preferences->setStandardFontFamily(standardFamily); > + preferences->setFixedFontFamily(fixedFamily); > + preferences->setSerifFontFamily(standardFamily); > + preferences->setSansSerifFontFamily(sansSerifFamily); > + preferences->setCursiveFontFamily(cursiveFamily); > + preferences->setFantasyFontFamily(fantasyFamily); > + > + SysFreeString(standardFamily); > + SysFreeString(fixedFamily); > + SysFreeString(sansSerifFamily); > + SysFreeString(cursiveFamily); > + SysFreeString(fantasyFamily); You could make the font names be leaked static BSTRs. > + preferences->resetDefaultsToConsistentValues(); This won't compile. resetDefaultsToConsistentValues() isn't a member of IWebPreferences.
Created attachment 33859 [details] Patch with changes after review Updated per aroben's coments. Thanks for review! (In reply to comment #48) > The added tests are missing from this patch. > > security/ seems like a strange place to put them. Maybe we need a new directory > for tests of the test harness? Moved to LayoutTests/fast/harness. Indeed a new place is better. > > +- (void)_setPreference:(NSString *)key stringValue:(NSString *)value > > +{ > > + NSString *_key = KEY(key); > > + [self _setStringValue:value forKey:_key]; > > +} > > This is wrong. _setStringValue:forKey: also passes the key through KEY(), so > you'll end up with the preferences identifier prepended twice. Done. > _setPreference:stringValue: is not a very Cocoa-y name. The obvious name would > be _setStringValue:forKey:. Maybe we should just expose that method through > WebPreferencesPrivate.h? Or maybe we can come up with a name that indicates > this is for testing purposes only. I don't think it's super-important to have > similar names on Mac and Windows for this. Changed name to - (void)_setPreferenceForTestWithValue:(NSString *)value forKey:(NSString *)key; Hope it's a better name. Exposing setStringValue feels strange because it is part of a bunch of similar functions. > > + HRESULT setPreference([in] BSTR key, [in] BSTR value); > > + > > HRESULT isWebSecurityEnabled([out, retval] BOOL* enabled); > > HRESULT setWebSecurityEnabled([in] BOOL enabled); > > Please add the new function at the end of the interface, to avoid the risk of > breaking nightly builds. Fixed. Also renamed it to setPreferenceForTest to match closer the Mac version. > > +HRESULT STDMETHODCALLTYPE WebPreferences::setPreference(BSTR key, BSTR value) > > There's no need to have STDMETHODCALLTYPE on the implementation. It's only > needed on the declaration. Fixed. > > + LayoutTestController* controller = reinterpret_cast<LayoutTestController*>(JSObjectGetPrivate(thisObject)); > > static_cast is preferable here. Done. > > > +void LayoutTestController::overridePreference(JSStringRef key, JSStringRef flag) > > +{ > > + RetainPtr<CFStringRef> keyCF(AdoptCF, JSStringCopyCFString(kCFAllocatorDefault, key)); > > + NSString *keyNS = (NSString *)keyCF.get(); > > + > > + RetainPtr<CFStringRef> flagCF(AdoptCF, JSStringCopyCFString(kCFAllocatorDefault, flag)); > > + NSString *flagNS = (NSString *)flagCF.get(); > > I think we tend to use 0 instead of kCFAllocatorDefault, though either is fine. In the same file (LayoutTestControllerMac.mm) both are used but 0 once and kCFAllocatorDefault multiple times so decided to leave kCFAllocatorDefault. I can replace all with 0... > > + SysFreeString(standardFamily); > > + SysFreeString(fixedFamily); > > + SysFreeString(sansSerifFamily); > > + SysFreeString(cursiveFamily); > > + SysFreeString(fantasyFamily); > > You could make the font names be leaked static BSTRs. Done. > > + preferences->resetDefaultsToConsistentValues(); > > This won't compile. resetDefaultsToConsistentValues() isn't a member of > IWebPreferences. Oops. Fixed.
Created attachment 33995 [details] Same patch with ChangeLog files updated and GTK and Wx changes. Same patch with ChangeLog files updated and GTK and Wx changes to prevent build break. Also, added new tests to Skipped list on GTK until GTK DRT implements preference override.
Comment on attachment 33995 [details] Same patch with ChangeLog files updated and GTK and Wx changes. it seems aroben has the most context here. You should try and track him down over irc and ask him to take another quick look at this.
(In reply to comment #51) > it seems aroben has the most context here. You should try and track him down > over irc and ask him to take another quick look at this. Thanks! I'm trying to do just that. I didn't see him on IRC for a while, perhaps on vacation or something... Will keep looking :-)
(In reply to comment #52) > (In reply to comment #51) > > it seems aroben has the most context here. You should try and track him down > > over irc and ask him to take another quick look at this. > > Thanks! I'm trying to do just that. I didn't see him on IRC for a while, > perhaps on vacation or something... Will keep looking :-) Email works well.
(In reply to comment #49) > (In reply to comment #48) > > > +void LayoutTestController::overridePreference(JSStringRef key, JSStringRef flag) > > > +{ > > > + RetainPtr<CFStringRef> keyCF(AdoptCF, JSStringCopyCFString(kCFAllocatorDefault, key)); > > > + NSString *keyNS = (NSString *)keyCF.get(); > > > + > > > + RetainPtr<CFStringRef> flagCF(AdoptCF, JSStringCopyCFString(kCFAllocatorDefault, flag)); > > > + NSString *flagNS = (NSString *)flagCF.get(); > > > > I think we tend to use 0 instead of kCFAllocatorDefault, though either is fine. > > In the same file (LayoutTestControllerMac.mm) both are used but 0 once and > kCFAllocatorDefault multiple times so decided to leave kCFAllocatorDefault. I > can replace all with 0... Matching the existing style (even if the existing style is muddled) seems good.
Comment on attachment 33995 [details] Same patch with ChangeLog files updated and GTK and Wx changes. Sorry for the long delay in reviewing this -- I was away on vacation. The new tests are missing spaces between "if" and "(", and around "=", in a few places. > +++ b/WebKitTools/DumpRenderTree/LayoutTestController.h > @@ -52,6 +52,7 @@ public: > bool isCommandEnabled(JSStringRef name); > void keepWebHistory(); > void notifyDone(); > + void overridePreference(JSStringRef key, JSStringRef flag); I think "value" would be a clearer name for the second parameter. r=me
Fixed style in tests as suggested, renamed 'flag' to 'value' and also added new tests to Qt Skipped list. Landed: http://trac.webkit.org/changeset/47015
Reverted in http://trac.webkit.org/changeset/47017 Caused many Windows Layout tests to fail.
The failed layout tests were due to incorrect merge... re-landed in http://trac.webkit.org/changeset/47039, and fixed the issue with ShouldPaintNativeControls setting on Windows inhttp://trac.webkit.org/changeset/47043