Split prefs.js into backend and UI modules

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Daniel Gherasim, Assigned: galgeek, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(2 attachments, 14 obsolete attachments)

206.25 KB, patch
Andrei Eftimie
: review+
Details | Diff | Splinter Review
203.01 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
OS: Linux → All
Hardware: x86 → All
(Reporter)

Updated

3 years ago
Blocks: 1079736
(Assignee)

Comment 1

3 years ago
Created attachment 8503407 [details] [diff] [review]
bug1040610-initial-split.patch

I’ve made a quick initial attempt at this. Am I on the right track?
Attachment #8503407 - Flags: feedback?(hskupin)
(Assignee)

Comment 2

3 years ago
Created attachment 8503411 [details] [diff] [review]
bug1040610-initial-split.patch
Attachment #8503407 - Attachment is obsolete: true
Attachment #8503407 - Flags: feedback?(hskupin)
Attachment #8503411 - Flags: feedback?(hskupin)
(Assignee)

Comment 3

3 years ago
Created attachment 8503513 [details]
firefox-lib-ui.diff shows changes from firefox/lib/prefs.js to new firefox/lib/ui/prefs.js
(Assignee)

Comment 4

3 years ago
Created attachment 8503514 [details]
lib.diff shows changes from firefox/lib/prefs.js to new lib/prefs.js
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.
Attachment #8503411 - Flags: feedback?(hskupin) → feedback+
(Assignee)

Comment 6

3 years ago
Created attachment 8505082 [details] [diff] [review]
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.
Attachment #8503411 - Attachment is obsolete: true
Attachment #8505082 - Flags: feedback?(hskupin)
Attachment #8505082 - Flags: checkin?
Flags: needinfo?(hskupin)
(Assignee)

Comment 7

3 years ago
Created attachment 8505083 [details]
shows changes from old lib/firefox/prefs.js to new lib/prefs.js
Attachment #8503514 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Created attachment 8505084 [details]
shows changes from old lib/firefox/prefs.js to new firefox/lib/ui/prefs.js
Attachment #8503513 - Attachment is obsolete: true
Assignee: nobody → galgeek
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.
Attachment #8505082 - Flags: feedback?(hskupin)
Attachment #8505082 - Flags: feedback+
Attachment #8505082 - Flags: checkin?
(Assignee)

Comment 10

3 years ago
Created attachment 8512444 [details] [diff] [review]
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?
Attachment #8505082 - Attachment is obsolete: true
Attachment #8505083 - Attachment is obsolete: true
Attachment #8512444 - Flags: feedback?(hskupin)
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.
Flags: needinfo?(hskupin)
Mentor: hskupin@gmail.com
Status: NEW → ASSIGNED
Whiteboard: [lang=js]
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.
Attachment #8512444 - Flags: feedback?(hskupin) → feedback+
(Assignee)

Comment 13

3 years ago
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.
(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.
(Assignee)

Comment 15

3 years ago
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.
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.
(Assignee)

Comment 17

3 years ago
Created attachment 8516182 [details] [diff] [review]
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?
Barbara, this patch misses the changes to the pref modules. You have to include them all into the patch.
(Assignee)

Comment 19

3 years ago
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?

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.
Attachment #8505084 - Attachment is obsolete: true
Attachment #8512444 - Attachment is obsolete: true
Attachment #8516182 - Attachment is obsolete: true

Comment 20

3 years ago
(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 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.
Attachment #8516886 - Flags: feedback?(hskupin)
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.
Attachment #8516886 - Flags: feedback?(hskupin) → feedback+
(Assignee)

Comment 23

3 years ago
Created 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

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
Attachment #8516886 - Attachment is obsolete: true
Attachment #8521226 - Flags: review?(hskupin)
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!
Attachment #8521226 - Flags: review?(hskupin) → feedback+
(Assignee)

Comment 25

3 years ago
Created attachment 8521543 [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

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
Attachment #8521226 - Attachment is obsolete: true
Attachment #8521543 - Flags: review?(hskupin)
(Assignee)

Updated

3 years ago
Attachment #8521543 - Flags: review?(hskupin)
(Assignee)

Comment 26

3 years ago
Created 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

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
Attachment #8521543 - Attachment is obsolete: true
Attachment #8528519 - Flags: review?(hskupin)
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.
Attachment #8528519 - Flags: review?(hskupin) → review-
(Assignee)

Comment 28

3 years ago
Created attachment 8530045 [details] [diff] [review]
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.
Attachment #8528519 - Attachment is obsolete: true
Attachment #8530045 - Flags: review?(hskupin)
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!
Attachment #8530045 - Flags: review?(hskupin)
Attachment #8530045 - Flags: review?(andrei.eftimie)
Attachment #8530045 - Flags: review+

Comment 30

3 years ago
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.
Attachment #8530045 - Flags: review?(andrei.eftimie) → review+
Created attachment 8530266 [details] [diff] [review]
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?
Attachment #8530045 - Attachment is obsolete: true
Attachment #8530266 - Flags: review?(andrei.eftimie)

Comment 32

3 years ago
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.
Attachment #8530266 - Flags: review?(andrei.eftimie) → review+

Comment 33

3 years ago
All green for me.
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.
status-firefox35: --- → affected
status-firefox36: --- → fixed

Comment 35

3 years ago
Blocks Bug 1105780 for mozilla-beta branch.
Blocks: 1105780
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.
Flags: needinfo?(galgeek)
(Assignee)

Comment 37

3 years ago
Yes, I'll upload an updated patch in a few minutes!
Flags: needinfo?(galgeek)
(Assignee)

Comment 38

3 years ago
Created attachment 8532639 [details] [diff] [review]
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
Attachment #8532639 - Flags: review?(hskupin)
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.
Attachment #8532639 - Flags: review?(hskupin) → review+
Landed the patch on beta as:
https://hg.mozilla.org/qa/mozmill-tests/rev/6654b3be59a0

Thanks Barbara!
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox35: affected → fixed
Resolution: --- → FIXED

Comment 41

3 years ago
Will this patch land into mozilla-release as well ?
(Assignee)

Comment 42

3 years ago
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.
Flags: needinfo?(hskupin)
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?
Flags: needinfo?(hskupin)

Comment 44

3 years ago
(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 !
We should not spend time on release for that. Lets make all correct for down to beta.
You need to log in before you can comment on or make changes to this bug.