Remove readUserPref API?

RESOLVED FIXED in Firefox 56

Status

()

Core
Preferences: Backend
P3
normal
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: milan, Assigned: Benjamin Smedberg)

Tracking

unspecified
mozilla56
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

See bug 789945 comment 94 - some add-ons use readUsePref API, this may cause a main thread synchronized pref file save.
(Assignee)

Updated

11 months ago
Priority: -- → P3
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Blocks: 981818
(Reporter)

Comment 2

11 months 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

11 months ago
Assignee: nobody → benjamin
(Assignee)

Comment 3

11 months 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

11 months 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

11 months ago
Attachment #8879611 - Attachment description: 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 → 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 …
Attachment #8879611 - Flags: review?(rweiss)

Comment 6

11 months 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+

Comment 7

11 months ago
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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4f0ee0f75a30
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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

11 months 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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/66aba634f34d
Status: REOPENED → RESOLVED
Last Resolved: 11 months ago11 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

6 months ago
Blocks: 1420311
You need to log in before you can comment on or make changes to this bug.