nsIContentPrefService should handle private browsing mode

RESOLVED FIXED in mozilla9

Status

()

defect
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: arno, Assigned: arno)

Tracking

({dev-doc-complete})

unspecified
mozilla9
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

8 years ago
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.
Assignee

Comment 1

8 years ago
Posted patch patch proposal (obsolete) — Splinter Review
Assignee: nobody → arno
Status: NEW → ASSIGNED
Attachment #554705 - Flags: review?(ehsan)

Comment 2

8 years ago
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+
Assignee

Comment 3

8 years ago
Posted 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:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=3c2bfc6c7739
Attachment #554705 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #554966 - Flags: review?(ehsan)

Comment 4

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

This is the wrong patch I believe!  :)
Attachment #554966 - Flags: review?(ehsan)
Assignee

Comment 5

8 years ago
Posted patch patch v1.1Splinter Review
I think this one in correct
Attachment #554966 - Attachment is obsolete: true
Attachment #557338 - Flags: review?(ehsan)

Updated

8 years ago
Attachment #557338 - Flags: review?(ehsan) → review+

Comment 6

8 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/3b04343f2382
Flags: in-testsuite+
Keywords: dev-doc-needed
Target Milestone: --- → mozilla9

Comment 7

8 years ago
http://hg.mozilla.org/mozilla-central/rev/3b04343f2382
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Assignee

Updated

8 years ago
Blocks: 684315
arno documented this:

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

It's also listed on Firefox 9 for developers.
Depends on: 762088

Updated

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