The default bug view has changed. See this FAQ.

nsIContentPrefService should handle private browsing mode

RESOLVED FIXED in mozilla9

Status

()

Toolkit
Preferences
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: arno renevier, Assigned: arno renevier)

Tracking

({dev-doc-complete})

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 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

6 years ago
Created attachment 554705 [details] [diff] [review]
patch proposal
Assignee: nobody → arno
Status: NEW → ASSIGNED
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+
(Assignee)

Comment 3

6 years ago
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
Attachment #554705 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
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)
(Assignee)

Comment 5

6 years ago
Created attachment 557338 [details] [diff] [review]
patch v1.1

I think this one in correct
Attachment #554966 - Attachment is obsolete: true
Attachment #557338 - Flags: review?(ehsan)

Updated

6 years ago
Attachment #557338 - Flags: review?(ehsan) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/3b04343f2382
Flags: in-testsuite+
Keywords: dev-doc-needed
Target Milestone: --- → mozilla9
Blocks: 684107
http://hg.mozilla.org/mozilla-central/rev/3b04343f2382
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 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.
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 575830
Depends on: 762088

Updated

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