Last Comment Bug 679784 - nsIContentPrefService should handle private browsing mode
: nsIContentPrefService should handle private browsing mode
Status: RESOLVED FIXED
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Preferences (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla9
Assigned To: arno renevier
:
Mentors:
Depends on: 575830 762088
Blocks: 526828 684107 684315
  Show dependency treegraph
 
Reported: 2011-08-17 10:38 PDT by arno renevier
Modified: 2013-04-12 17:52 PDT (History)
3 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch proposal (38.30 KB, patch)
2011-08-21 03:20 PDT, arno renevier
ehsan: review+
Details | Diff | Splinter Review
patch v1.1 (6.48 KB, patch)
2011-08-22 14:25 PDT, arno renevier
no flags Details | Diff | Splinter Review
patch v1.1 (38.40 KB, patch)
2011-08-31 15:00 PDT, arno renevier
ehsan: review+
Details | Diff | Splinter Review

Description arno renevier 2011-08-17 10:38:35 PDT
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 2011-08-21 03:20:23 PDT
Created attachment 554705 [details] [diff] [review]
patch proposal
Comment 2 :Ehsan Akhgari 2011-08-22 13:09:12 PDT
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 2011-08-22 14:25:16 PDT
Created attachment 554966 [details] [diff] [review]
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 :Ehsan Akhgari 2011-08-31 14:48:06 PDT
Comment on attachment 554966 [details] [diff] [review]
patch v1.1

This is the wrong patch I believe!  :)
Comment 5 arno renevier 2011-08-31 15:00:03 PDT
Created attachment 557338 [details] [diff] [review]
patch v1.1

I think this one in correct
Comment 8 Eric Shepherd [:sheppy] 2011-09-07 14:48:08 PDT
arno documented this:

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

It's also listed on Firefox 9 for developers.

Note You need to log in before you can comment on or make changes to this bug.