1040610 - Split prefs.js into backend and UI modules

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P3)

Description

[Daniel Gherasim]

We need to split the preferences library into two modules.

The backend code, as it's the same across products, will be in
> lib/prefs.js

The ui code, specific to firefox should live into
> firefox/lib/ui/prefs.js

Comment 1

[Barbara Miller (:galgeek)]

Attachment: bug1040610-initial-split.patch

I’ve made a quick initial attempt at this. Am I on the right track?

Comment 2

[Barbara Miller (:galgeek)]

Attachment: bug1040610-initial-split.patch

Comment 3

[Barbara Miller (:galgeek)]

Attachment: firefox-lib-ui.diff shows changes from firefox/lib/prefs.js to new firefox/lib/ui/prefs.js

Comment 4

[Barbara Miller (:galgeek)]

Attachment: lib.diff shows changes from firefox/lib/prefs.js to new lib/prefs.js

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8503411 [details] [diff] [review]
bug1040610-initial-split.patch

Review of attachment 8503411 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/ui/prefs.js
@@ +4,5 @@
> +
> +/**
> + * @fileoverview
> + * The PrefsAPI adds support for preferences related functions. 
> + * It allows to handle the preferences dialog.

You wanna update the comment. This is the UI module and not the low level API. Also please fix the trailing white-space in line 7.

@@ +232,5 @@
> +    prefModal.waitForDialog();
> +  }
> +  else {
> +    // Get the window type of the preferences window depending on the application
> +    // ??? this code shouldn't cause any problems, but does firefox/lib/ui/prefs.js need to support Thunderbird?

You can leave what we have at the moment. Just remove your comment. We will replace all this code soon for in-content preferences anyway.

::: lib/prefs.js
@@ +13,5 @@
> +
> +/**
> + * Preferences object to simplify the access to the nsIPrefBranch.
> + */
> +var preferences = {

With the separation of the UI and backend code we could get rid of this extra object, and can have all the methods and variables global. Or prevents us something from doing it beside that we have to export more entries?

@@ +41,5 @@
> +   * @return Instance of the pref service
> +   * @type nsIPrefService
> +   */
> +  get prefService() {
> +    return Services.prefs;

By also exporting Services we should not need this getter.

Comment 6

[Barbara Miller (:galgeek)]

Attachment: git diff showing new firefox/lib/ui/prefs.js and lib/prefs.js

Thanks for your feedback!

Does this look about right? I'm especially unsure of how to export Services.

Comment 7

[Barbara Miller (:galgeek)]

Attachment: shows changes from old lib/firefox/prefs.js to new lib/prefs.js

Comment 8

[Barbara Miller (:galgeek)]

Attachment: shows changes from old lib/firefox/prefs.js to new firefox/lib/ui/prefs.js

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8505082 [details] [diff] [review]
git diff showing new firefox/lib/ui/prefs.js and lib/prefs.js

Review of attachment 8505082 [details] [diff] [review]:
-----------------------------------------------------------------

Structure looks fine. Some small things to update, but otherwise you will be good to get started in updating the tests, which make use of the prefs.js module. When you do that make sure you run all the tests to verify the changes are working as expected.

::: lib/prefs.js
@@ +16,5 @@
> + *
> + * @return Instance of the preferences branch
> + * @type nsIPrefBranch
> + */
> +var prefBranch() = {

It's not clear what this is. Please directly assign the value to that variable. It should never change.

@@ +28,5 @@
> + * @type nsIPrefBranch
> + */
> +var defaultPrefBranch() = {
> +  return Services.prefs.getDefaultBranch("");
> +};

Same here as above.

@@ +36,5 @@
> + *
> + * @param {string} aPrefName
> + *        The user-set preference to clear
> + * @return False if the preference had the default value
> + * @type boolean

While rewriting this file can you combine the two return lines so we have "@returns {boolean} description" in all the cases?

@@ +130,5 @@
> +
> +// Export of variables
> +exports.prefBranch = prefBranch;
> +exports.defaultPrefBranch = defaultPrefBranch;
> +// Export object to access low level functions of nsIPrefService

Please add a blank line before the comments so blocks are separated.

Comment 10

[Barbara Miller (:galgeek)]

Attachment: git diff showing new firefox/lib/ui/prefs.js and lib/prefs.js, deleting old firefox/lib/prefs.js

Thanks for your feedback!

Is this about right for the new prefs.js files?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Please set only feedback or needinfo when you want my feedback. If you have a patch it will automatically be the feedback flag. Clearing needinfo.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8512444 [details] [diff] [review]
git diff showing new firefox/lib/ui/prefs.js and lib/prefs.js, deleting old firefox/lib/prefs.js

Review of attachment 8512444 [details] [diff] [review]:
-----------------------------------------------------------------

I think that looks fine. There are some small nits but those do not apply to a feedback request. So I would say it is time to update all the mozmill tests so that they refer to the new UI module and access the methods in the backend module directly.

Comment 13

[Barbara Miller (:galgeek)]

Thanks for your feedback!

I've made an update to firefox/lib/tests/testAddonsBlocklist.js, using:

var prefs = require("../../../lib/prefs");

but testing fails initially like this:

Test Failure | {
  "exception": {
    "message": "Module \"../firefox/lib/prefs\" not found",
    "lineNumber": 209,
    "name": "Error",
    "fileName": "resource://mozmill/stdlib/securable-module.js"
  }
}

Does "resource://mozmill/stdlib/securable-module.js" need to know about the new prefs lib/prefs.js and/or firefox/lib/ui/prefs.js before I can test, or did I make a mistake that I don't see?

Also, I wonder if it may be best to wait until Bug 1076741 lands to work much on this one, since they'll update many of the same tests.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Barbara Miller (:galgeek) from comment #13)
> I've made an update to firefox/lib/tests/testAddonsBlocklist.js, using:
> 
> var prefs = require("../../../lib/prefs");
[..]
> 
> Does "resource://mozmill/stdlib/securable-module.js" need to know about the
> new prefs lib/prefs.js and/or firefox/lib/ui/prefs.js before I can test, or
> did I make a mistake that I don't see?

How did you test this? Directly with the Mozmill command? If you took the testrun script, please use Mozmill directly. For the testrun script you would have to qref the module refactor changes first.

> Also, I wonder if it may be best to wait until Bug 1076741 lands to work
> much on this one, since they'll update many of the same tests.

This mentioned bug has nothing to do with the pref work you are doing here. Code, which will be touched, is different from that one. So I do not see a blocker why to wait.

Comment 15

[Barbara Miller (:galgeek)]

The error occurred running the test with mozmill:

$ mozmill -t firefox/lib/tests/testAddonsBlocklist.js -b /Applications/FirefoxNightly.app/

Bug 1076741 doesn't block this. It does update lots of tests, as does this bug, and bug 1076741 should be about ready to land.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I propose you upload a WIP patch with the updated library test. I can then have a look locally. Right now I do not see what's wrong.

Comment 17

[Barbara Miller (:galgeek)]

Attachment: git diff firefox/lib/tests/testAddonsBlocklist.jsbug1040610-WIP.patch

Here's a WIP patch for the failing test.

I've updated a couple of other tests and see similar failures for them.

Note that I've deleted the original prefs.js file at firefox/lib/ -- I wanted to be sure I was testing against the new libraries, not the old. But maybe that is the problem?

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Barbara, this patch misses the changes to the pref modules. You have to include them all into the patch.

Comment 19

[Barbara Miller (:galgeek)]

Attachment: patch including all WIP

I see that, in addition to updating prefs references, I've also replaced "new elementslib.ID" with findElement.ID--Andrei asked for that with bug1076741, but maybe you'd prefer to skip it for now?

Note that I've restored the old firefox/lib/prefs.js, but I'm still seeing similar test errors. I've been running from a git clone that I've been updating; Andrei has suggested I test, at least, from the mercurial repo.

Comment 20

[Andrei Eftimie]

(In reply to Barbara Miller (:galgeek) from comment #19)
> Created attachment 8516886 [details] [diff] [review]
> patch including all WIP
> 
> I see that, in addition to updating prefs references, I've also replaced
> "new elementslib.ID" with findElement.ID--Andrei asked for that with
> bug1076741, but maybe you'd prefer to skip it for now?

I was referring to only new or changed code, you don't need to go out of your way to change every instance of them (they will get changed eventually). A commit should generally follow a defined scope.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8516886 [details] [diff] [review]
patch including all WIP

Review of attachment 8516886 [details] [diff] [review]:
-----------------------------------------------------------------

Please add the feedback request when I should have a look at a patch. I will try to get to it in a couple of hours.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8516886 [details] [diff] [review]
patch including all WIP

Review of attachment 8516886 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tabs.js
@@ +42,1 @@
>  var sessionStore = require("../lib/sessionstore");

The message for the not found prefs module is indeed not that helpful given the missing stack. But to solve this you can comment out require lines step by step for a test which is failing. You will see that there are a couple of other modules left, which have not been updated yet. So that is the reason why it is failing. In your case it is mainly the sessionstore module, but also others like addons need an update.

Comment 23

[Barbara Miller (:galgeek)]

Attachment: splits prefs.js into lib/prefs.js and firefox/lib/ui/pref.js and updates affected lib files and tests

This patch should include updates to all affected files (though it does not yet remove firefox/lib/prefs.js).

There's one test failure for both functional and remote, but they're ones I've seen before locally and associated with reported bugs.

functional: http://mozmill-crowd.blargon7.com/#/functional/report/b67428b6e502c230ff85b6a4c41c5e05

remote:     http://mozmill-crowd.blargon7.com/#/remote/report/b67428b6e502c230ff85b6a4c41c7f3a

endurance:  http://mozmill-crowd.blargon7.com/#/endurance/report/b67428b6e502c230ff85b6a4c41c8447

l10n:       http://mozmill-crowd.blargon7.com/#/l10n/report/b67428b6e502c230ff85b6a4c41cbb18

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8521226 [details] [diff] [review]
splits prefs.js into lib/prefs.js and firefox/lib/ui/pref.js and updates affected lib files and tests

Review of attachment 8521226 [details] [diff] [review]:
-----------------------------------------------------------------

From inspecting the code it looks great. So clearly a feedback+. For a review I would like to see a complete patch, which should include also the cleanup of any left-over files. Once this is part of the patch I can do the review. Thanks Barbara!

Comment 25

[Barbara Miller (:galgeek)]

Attachment: splits firefox/lib/prefs.js into lib/prefs.js and firefox/lib/ui/pref.js, removes firefox/lib/prefs.js, and updates affected lib files and tests

This should be a complete patch.

There are a couple of remote test failures, both associated with bugs, but the rest are green; tests run against a patched hg repo cloned today.

functional:  http://mozmill-crowd.blargon7.com/#/functional/report/b67428b6e502c230ff85b6a4c42db863

remote:      http://mozmill-crowd.blargon7.com/#/remote/report/b67428b6e502c230ff85b6a4c42db9ed

endurance:   http://mozmill-crowd.blargon7.com/#/endurance/report/b67428b6e502c230ff85b6a4c42dc835

remote:      http://mozmill-crowd.blargon7.com/#/l10n/report/b67428b6e502c230ff85b6a4c42dd2c5

Comment 26

[Barbara Miller (:galgeek)]

Attachment: splits firefox/lib/prefs.js into lib/prefs.js and firefox/lib/ui/pref.js, removes firefox/lib/prefs.js, and updates affected lib files and tests

Thank you for all your help!

Test results are clean except for remote tests that have been failing.

functional: http://mozmill-crowd.blargon7.com/#/functional/report/5284a4eabadaf42fb824119121272304

remote: http://mozmill-crowd.blargon7.com/#/remote/report/5284a4eabadaf42fb824119121273057

endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/5284a4eabadaf42fb824119121273d0a

l10n: http://mozmill-crowd.blargon7.com/#/l10n/report/5284a4eabadaf42fb8241191212788bf

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8528519 [details] [diff] [review]
splits firefox/lib/prefs.js into lib/prefs.js and firefox/lib/ui/pref.js, removes firefox/lib/prefs.js, and updates affected lib files and tests

Review of attachment 8528519 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the update Barbara! While applying your patch I got failures for testAddons_pluginDisabledAfterRestart which has been moved recently. Also functional/testSessionStore/testRestorePreviousSession/test1.js was missed for an update. You will have to update those parts of the patch. 

For other things please see my inline comments.

::: firefox/tests/functional/restartTests/testPreferences_masterPassword/test1.js
@@ +11,5 @@
>  var tabs = require("../../../../lib/tabs");
>  var utils = require("../../../../../lib/utils");
>  
>  var browser = require("../../../../lib/ui/browser");
> +var uiprefs = require("../../../../lib/ui/prefs");

Maybe we should name the module pref-window.js. But maybe we should wait with this patch until bug 1005811 has been fixed, which is kinda a hard-blocker for us given the recent search engine changes. I don't want that you have to update the patch over and over again due to conflicts. It's indeed hard when different things have to be changed in the same area.

Would you be able to get your work done soon? If yes, we might be able to land your patch first.

::: firefox/tests/update/testDirectUpdate/testUpdate.js
@@ +50,4 @@
>  
>    // If requested force a specific update URL
>    if (persisted.update.update_url) {
> +    prefs.setPref(PREF_UPDATE_URL_OVERRIDE,persisted.update.update_url);

nit: missing blank after comma.

::: lib/addons.js
@@ +13,5 @@
>  var domUtils = require("dom-utils");
>  // Bug 1040610
>  // Prefs library needs to be refactored (general & product specific code)
>  // Include only the general library once fixed
> +var prefs = require("prefs");

You can remove the above comment given that you fixed it.

Comment 28

[Barbara Miller (:galgeek)]

Attachment: splitting prefs.js into backend and UI modules, plus test updates

Thank you for your feedback, Henrik! 

This new patch updates today's repo, with these test results...

functional: http://mozmill-crowd.blargon7.com/#/functional/report/bdaefa581a1cff14362f7944d6001f41
several failures, but don't look like they're associated with this patch

remote: http://mozmill-crowd.blargon7.com/#/remote/report/bdaefa581a1cff14362f7944d6002b61
only SafeBrowsing test fails

endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/bdaefa581a1cff14362f7944d6003309
clear

location: http://mozmill-crowd.blargon7.com/#/l10n/report/bdaefa581a1cff14362f7944d6003784
clear

If it's not helpful to land this now, no worries, I don't mind making more updates.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8530045 [details] [diff] [review]
splitting prefs.js into backend and UI modules, plus test updates

Review of attachment 8530045 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great. Thanks Barbara! Just to be sure I'm not missing something it would be great to get another pair of eyes to glance over the patch. If all is fine for Andrei, we can get this landed!

Comment 30

[Andrei Eftimie]

Comment on attachment 8530045 [details] [diff] [review]
splitting prefs.js into backend and UI modules, plus test updates

Review of attachment 8530045 [details] [diff] [review]:
-----------------------------------------------------------------

It would be great to have a move operation from the old prefs.js file.
Other than that, lgtm if all tests are passing.

::: firefox/lib/prefs.js
@@ -1,1 @@
> -/* This Source Code Form is subject to the terms of the Mozilla Public

Please make sure to rename this file to one of the new ones, not sure which would be better, prefs.js or pref-window.js, we'll want to keep as much history as possible.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Final patch (including move firefox/lib/prefs.js => lib/prefs.js)

Given Thanksgiving Barbara might not have time to update it today. So I quickly updated that nit for landing. Can you please have a look again?

Comment 32

[Andrei Eftimie]

Comment on attachment 8530266 [details] [diff] [review]
Final patch (including move firefox/lib/prefs.js => lib/prefs.js)

Review of attachment 8530266 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm, running a testrun on OSX now.

Comment 33

[Andrei Eftimie]

All green for me.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Andrei! I pushed this patch to default:

https://hg.mozilla.org/qa/mozmill-tests/rev/6b6eb2a62a7d

If all is green we can backport it after the merge to beta for Firefox 35.

Comment 35

[Teodor Druta]

Blocks Bug 1105780 for mozilla-beta branch.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Barbara, I tried to apply your patch on the mozilla-beta branch but it turned out that it causes conflicts in two files. Would you be able to update your patch for that branch?

unable to find 'firefox/tests/endurance/testPopups_OpenAndClose/test1.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file firefox/tests/endurance/testPopups_OpenAndClose/test1.js.rej
unable to find 'firefox/tests/l10n/testAccessKeys/test2.js' for patching
2 out of 2 hunks FAILED -- saving rejects to file firefox/tests/l10n/testAccessKeys/test2.js.rej
patch failed to apply

Maybe those two files do not exist on this branch.

Thanks.

Comment 37

[Barbara Miller (:galgeek)]

Yes, I'll upload an updated patch in a few minutes!

Comment 38

[Barbara Miller (:galgeek)]

Attachment: an updated patch for beta

Here's a patch for beta.

Tests are clear for functional, remote, l10n, and all but one endurance. I couldn't find a bug report for the failing endurance test, but it's failed twice for me today, and it doesn't look like it's related to this patch.

functional: http://mozmill-crowd.blargon7.com/#/functional/report/eaa287eb6543546f0b0d98e1f041a608

remote: http://mozmill-crowd.blargon7.com/#/remote/report/eaa287eb6543546f0b0d98e1f041ac00

endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/eaa287eb6543546f0b0d98e1f041b6b5

l10n: http://mozmill-crowd.blargon7.com/#/l10n/report/eaa287eb6543546f0b0d98e1f041d422

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8532639 [details] [diff] [review]
an updated patch for beta

Review of attachment 8532639 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine. Thanks for updating the patch that quickly.

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed the patch on beta as:
https://hg.mozilla.org/qa/mozmill-tests/rev/6654b3be59a0

Thanks Barbara!

Comment 41

[Teodor Druta]

Will this patch land into mozilla-release as well ?

Comment 42

[Barbara Miller (:galgeek)]

The patch for the current beta doesn't apply cleanly to mozilla-release.

Please let me know if I should provide another patch for mozilla-release.

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The release is out and we do not expect something else. Also I'm not tempted to change the release branch for that. We should never do large changes on that branch for the remaining time but stabilize the code via the beta branch. Is there any reason you want to have that for release?

Comment 44

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #43)
> Is there any reason you want to have that for release?

I was just asking to know for which branches I should work on Bug 1105780, which is a blocker for this bug, due to differences in the functionality of different Firefox branches. Thanks !

Comment 45

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We should not spend time on release for that. Lets make all correct for down to beta.