1164124 - Create a 'using_prefs' context manager

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

We have pref setting code scattered throughout the marionette runner and tests; we should provide an easier way of dealing with these.

Let's implement a 'with_pref' context manager that would set a pref and restore it at exit, similar to the 'with_permission' feature requested in bug 1161606.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: MozReview Request: Bug 1164124 - Add using_prefs context manager. r=jgriffin

Bug 1164124 - Add using_prefs context manager. r=jgriffin

Add get_pref(), set_pref(), set_prefs() to make manipulate preferences
easier.

enforce_gecko_prefs() did the similar job as set_prefs(), but it will
restart the browser if a preference to be set are different from what is
already set in the system. Not all gecko preferences require a restart
to work. Using set_prefs() should make testing faster. See bug 1048554.

Comment 2

[Jonathan Griffin (:jgriffin)]

https://reviewboard.mozilla.org/r/20573/#review18447

::: testing/marionette/driver/marionette_driver/marionette.py:953
(Diff revision 1)
> +                    case Services.prefs.PREF_INVALID:

There's a slight mismatch here between get_pref and set_pref.  If get_pref is called on a pref which doesn't exist, it will return None.  But if you pass None to set_pref, it will thrown an error.  Instead, set_pref should probably delete the pref (via clearUserValue) in this case.

Otherwise, there's no way to use this context manager to temporarily set a pref which may not have already had a value.

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8666574 [details]
MozReview Request: Bug 1164124 - Add using_prefs context manager. r=jgriffin

Bug 1164124 - Add using_prefs context manager. r=jgriffin

Add get_pref(), set_pref(), set_prefs() to make manipulate preferences
easier.

enforce_gecko_prefs() did the similar job as set_prefs(), but it will
restart the browser if a preference to be set are different from what is
already set in the system. Not all gecko preferences require a restart
to work. Using set_prefs() should make testing faster. See bug 1048554.

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

https://reviewboard.mozilla.org/r/20573/#review18447

> There's a slight mismatch here between get_pref and set_pref.  If get_pref is called on a pref which doesn't exist, it will return None.  But if you pass None to set_pref, it will thrown an error.  Instead, set_pref should probably delete the pref (via clearUserValue) in this case.
> 
> Otherwise, there's no way to use this context manager to temporarily set a pref which may not have already had a value.

In patch set 2, the pref will be cleared in set_pref if the value to be set in None. Unit test is also updated to ensure that.

Comment 5

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8666574 [details]
MozReview Request: Bug 1164124 - Add using_prefs context manager. r=jgriffin

https://reviewboard.mozilla.org/r/20575/#review18571

Thanks, looks good.

Comment 6

[Jonathan Griffin (:jgriffin)]

https://reviewboard.mozilla.org/r/20575/#review18573

Thanks, looks good.

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Thank you for the review.

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8666574 [details]
MozReview Request: Bug 1164124 - Add using_prefs context manager. r=jgriffin

Bug 1164124 - Add using_prefs context manager. r=jgriffin

Add get_pref(), set_pref(), set_prefs() to make manipulate preferences
easier.

enforce_gecko_prefs() did the similar job as set_prefs(), but it will
restart the browser if a preference to be set are different from what is
already set in the system. Not all gecko preferences require a restart
to work. Using set_prefs() should make testing faster. See bug 1048554.

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

https://reviewboard.mozilla.org/r/20575/#review18697

::: testing/marionette/driver/marionette_driver/marionette.py:968
(Diff revision 3)
> +        with self.using_context(self.CONTEXT_CHROME):

In patch set3, use CONTEXT_CHROME in set_pref and clear_pref to fix try fail in Mn-e10s

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0db5f3c2b00451cc7acab6bfb42f4eb93ce88074
Bug 1164124 - Add using_prefs context manager. r=jgriffin

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0db5f3c2b004