Closed
Bug 1367813
Opened 7 years ago
Closed 7 years ago
Remove readUserPref API?
Categories
(Core :: Preferences: Backend, enhancement, P3)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: milan, Assigned: benjamin)
References
Details
Attachments
(1 file)
See bug 789945 comment 94 - some add-ons use readUsePref API, this may cause a main thread synchronized pref file save.
Assignee | ||
Updated•7 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Reporter | ||
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8879611 [details] Bug 1367813 - 1) Add telemetry for prefs.js not existing or being corrupted, and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save https://reviewboard.mozilla.org/r/150952/#review155742 ::: modules/libpref/Preferences.cpp:97 (Diff revision 1) > static NS_DEFINE_CID(kZipReaderCID, NS_ZIPREADER_CID); > > void > Preferences::DirtyCallback() > { > + if (!XRE_IsParentProcess()) { We could just not hook up the callback function in Preferences::Init by not calling PREF_SetDirtyCallback() when we're not in the parent process? ::: modules/libpref/nsIPrefService.idl:122 (Diff revision 1) > + * @param aFile The file to be read. > + * > + * @throws Error File failed to read or contained invalid data. > + * @note This method is intended for internal unit testing only! > + */ > + void readUserPrefsFromFile(in nsIFile aFile); Minor naming thing; this now feels more like "importPrefsFromFile" rather than "read..." as we maintain no connection to the file afterwards. No big deal either way. ::: modules/libpref/test/unit/test_dirtyPrefs.js (Diff revision 1) > let userBranch = ps.getBranch(""); > > let prefFile = do_get_profile(); > prefFile.append("prefs.js"); > > - // dirty flag only applies to the default pref file save, not all of them, Isn't this test now going to modify the default preference file? Is that OK for the other tests? ::: modules/libpref/test/unit/test_libPrefs.js:322 (Diff revision 1) > > // load a preexisting pref file > let prefFile = do_get_file("data/testPref.js"); > - ps.readUserPrefs(prefFile); > + ps.readUserPrefsFromFile(prefFile); > + > + do_print("before throw tests"); Assuming this and the other do_print are not needed in the version we'd land?
Attachment #8879611 -
Flags: review?(milan) → review+
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → benjamin
Assignee | ||
Comment 3•7 years ago
|
||
> We could just not hook up the callback function in Preferences::Init by not > calling PREF_SetDirtyCallback() when we're not in the parent process? We could, but I'd like to try adding an assert in the future and so I'd prefer to keep this explicit. > > - // dirty flag only applies to the default pref file save, not all of them, > > Isn't this test now going to modify the default preference file? Is that OK > for the other tests? It is, and I checked that should be ok for the test. It's an xpcshell test so the profile is a throwaway directory. > Assuming this and the other do_print are not needed in the version we'd land? Ah yeah, leftover debugging code.
Assignee | ||
Comment 4•7 years ago
|
||
Comment on attachment 8879611 [details] Bug 1367813 - 1) Add telemetry for prefs.js not existing or being corrupted, and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save rweiss for data review. This is type 1 data.
Attachment #8879611 -
Flags: review?(rweiss)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8879611 -
Attachment description: , and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save → , and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save changed preferences to that location.
Attachment #8879611 -
Flags: review?(rweiss)
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8879611 [details] Bug 1367813 - 1) Add telemetry for prefs.js not existing or being corrupted, and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save https://reviewboard.mozilla.org/r/150952/#review156770 In accordance with the Data Collection categories on the wiki (https://wiki.mozilla.org/Firefox/Data_Collection), I assert that this is Type 1 data, and thus eligible for default on collection in release.
Attachment #8879611 -
Flags: review?(rweiss) → review+
Pushed by bsmedberg@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0ee0f75a30 1) Add telemetry for prefs.js not existing or being corrupted, and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save changed preferences to that location. r=milan data-r=rweiss
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4f0ee0f75a30
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 9•7 years ago
|
||
Backed out in https://hg.mozilla.org/mozilla-central/rev/6fed2516a664ecb9c2793b9beb0f081bc5833764 because something in that push made "dom/filesystem/tests/test_worker_basic.html | Something when wrong" permaorange on Windows opt, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=109432905&repo=mozilla-inbound
Status: RESOLVED → REOPENED
status-firefox56:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla56 → ---
Comment 10•7 years ago
|
||
Pushed by bsmedberg@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/66aba634f34d 1) Add telemetry for prefs.js not existing or being corrupted, and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save changed preferences to that location. r=milan data-r=rweiss
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/66aba634f34d
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•