Bug 77033 - Implement overridePreference for boolean preferences in WebKitTestRunner
Summary: Implement overridePreference for boolean preferences in WebKitTestRunner
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords:
Depends on:
Blocks: 42197 73999
  Show dependency treegraph
 
Reported: 2012-01-25 11:45 PST by Caio Marcelo de Oliveira Filho
Modified: 2012-01-26 09:29 PST (History)
3 users (show)

See Also:


Attachments
Patch (17.65 KB, patch)
2012-01-25 11:52 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (22.14 KB, patch)
2012-01-25 15:03 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch for landing (22.91 KB, patch)
2012-01-26 08:09 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Marcelo de Oliveira Filho 2012-01-25 11:45:06 PST
Implement overridePreference for boolean preferences in WebKitTestRunner
Comment 1 Caio Marcelo de Oliveira Filho 2012-01-25 11:52:18 PST
Created attachment 123982 [details]
Patch
Comment 2 Alexey Proskuryakov 2012-01-25 12:09:58 PST
Comment on attachment 123982 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123982&action=review

What resets the preferences back for the next test? Please make sure that this is well understood before landing.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePrivate.h:67
>  WK_EXPORT void WKBundleOverrideXSSAuditorEnabledForTestRunner(WKBundleRef bundle, WKBundlePageGroupRef pageGroup, bool enabled);

Can this be removed now?
Comment 3 Caio Marcelo de Oliveira Filho 2012-01-25 15:03:50 PST
Created attachment 124020 [details]
Patch
Comment 4 Caio Marcelo de Oliveira Filho 2012-01-25 15:10:12 PST
(In reply to comment #2)
> (From update of attachment 123982 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123982&action=review
> 
> What resets the preferences back for the next test? Please make sure that this is well understood before landing.

Thanks for this question! I was overlooking the fact that: since properties were not changing, we were not updating the WebPreferencesStore. And worst, even if I forced an update, it would decode() before the the remove function was called.

I made a small change to the initial patch: instead of overwriting the preferences, the getter now asks the override first. Also made an explicit function that WTR should call to force the update.

> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePrivate.h:67
> >  WK_EXPORT void WKBundleOverrideXSSAuditorEnabledForTestRunner(WKBundleRef bundle, WKBundlePageGroupRef pageGroup, bool enabled);
> 
> Can this be removed now?

Removed, using WKBundleOverrideBoolPreferenceForTestRunner() instead.

Could you take another look at it?
Comment 5 Alexey Proskuryakov 2012-01-25 15:18:55 PST
Comment on attachment 124020 [details]
Patch

> I made a small change to the initial patch: instead of overwriting the preferences, the getter now asks the override first.

Thanks! I'd appreciate a detailed explanation for posterity - I think that I understand what you're saying, but only barely, and it will be more difficult in a few months. I.e., what function gets called between tests, what preferences it resets, and how that filters down to each page.
Comment 6 Caio Marcelo de Oliveira Filho 2012-01-26 08:09:27 PST
Created attachment 124118 [details]
Patch for landing
Comment 7 Caio Marcelo de Oliveira Filho 2012-01-26 08:14:02 PST
In the patch for landing I've improved the ChangeLog to explain in more detail how the preference override work.
Comment 8 WebKit Review Bot 2012-01-26 09:10:45 PST
Comment on attachment 124118 [details]
Patch for landing

Clearing flags on attachment: 124118

Committed r106005: <http://trac.webkit.org/changeset/106005>
Comment 9 WebKit Review Bot 2012-01-26 09:10:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Alexey Proskuryakov 2012-01-26 09:29:10 PST
> This clearing could be improved by creating a proper message instead of hooking
> at WebPage::preferencesDidChange().

Sounds like a good idea.