Closed Bug 679784 Opened 13 years ago Closed 13 years ago

nsIContentPrefService should handle private browsing mode


(Toolkit :: Preferences, defect)

Not set





(Reporter: arno, Assigned: arno)



(Keywords: dev-doc-complete)


(1 file, 2 obsolete files)

Currently, nsHTMLInputElement uses content preferences service (bug #536567). But, it also manages private browsing mode: when in private browsing mode, it stores the preferences in memory. So, it has first to observe NS_PRIVATE_BROWSING_SWITCH_TOPIC, then it must have two code paths: one for private mode, the other for normal mode. The private mode path also duplicates a part of nsIContentPrefService.setPref (by getting the group's uri).

Then, the same thing will have to be done for the spellChecker in #678842
So, it would be nice that content pref service manages private browsing mode. Here is how this is handled in nsHTMLInputElement.cpp

- when setting a pref in private mode, store the association uri/pref in an in-memory hashtable.
- when getting a pref in private mode, get it from in-memory hashtable.
- when leaving the private mode, clear the memory storage.
This allow content preferences service to run correctly during a private browsing session, and assures that no data is kept afterwards.

This would also improve full zoom content pref handling because currently, no zoom value is restored inside a private browsing session. Also, it would make future uses of content pref services easier, and make sure an extension using content pref service does not leak browsing informations accidentally.
Attached patch patch proposal (obsolete) — Splinter Review
Assignee: nobody → arno
Attachment #554705 - Flags: review?(ehsan)
Comment on attachment 554705 [details] [diff] [review]
patch proposal

Review of attachment 554705 [details] [diff] [review]:

Looks really good!

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +602,4 @@
>      return this._selectPrefsByName(aName);
>    },
> +  // or boolean to indicate if we are in private browsing mode

Nit: s/or //

::: toolkit/components/contentprefs/tests/unit/test_bug248970.js
@@ +91,5 @@
>        do_check_eq(cp.getPref(uri1, pref_name), zoomA_new);
>        // exit private browsing mode
>        pb.privateBrowsingEnabled = false;
> +      // make sure Zoom-A change has not persisted
> +      do_check_eq(cp.getPref(uri1, pref_name), zoomA);

Can you also make sure that the Zoom-B value is not persisted here?
Attachment #554705 - Flags: review?(ehsan) → review+
Attached patch patch v1.1 (obsolete) — Splinter Review
updated patch to fix mentioned issues.
By the way, I forgot to mention that previous patch has built and passed tests successfully on try server:
Attachment #554705 - Attachment is obsolete: true
Attachment #554966 - Flags: review?(ehsan)
Comment on attachment 554966 [details] [diff] [review]
patch v1.1

This is the wrong patch I believe!  :)
Attachment #554966 - Flags: review?(ehsan)
Attached patch patch v1.1Splinter Review
I think this one in correct
Attachment #554966 - Attachment is obsolete: true
Attachment #557338 - Flags: review?(ehsan)
Attachment #557338 - Flags: review?(ehsan) → review+
Flags: in-testsuite+
Keywords: dev-doc-needed
Target Milestone: --- → mozilla9
Blocks: 684107
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 684315
arno documented this:

It's also listed on Firefox 9 for developers.
Depends on: 762088
Blocks: 526828
You need to log in before you can comment on or make changes to this bug.