679784 - nsIContentPrefService should handle private browsing mode

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[arno renevier]

Hi,
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.

Comment 1

[arno renevier]

Attachment: patch proposal

Comment 2

[(no longer active)]

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?

Comment 3

[arno renevier]

Attachment: patch v1.1

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:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=3c2bfc6c7739

Comment 4

[(no longer active)]

Comment on attachment 554966 [details] [diff] [review]
patch v1.1

This is the wrong patch I believe!  :)

Comment 5

[arno renevier]

Attachment: patch v1.1

I think this one in correct

Comment 6

[(no longer active)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3b04343f2382

Comment 7

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/3b04343f2382

Comment 8

[Eric Shepherd [:sheppy]]

arno documented this:

https://developer.mozilla.org/en/Using_content_preferences#private-browsing

It's also listed on Firefox 9 for developers.