Closed Bug 723002 Opened 9 years ago Closed 8 years ago

ContentPrefService uses global Private Browsing state to make decisions

Categories

(Toolkit :: Preferences, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: jdm, Assigned: jdm)

References

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(1 file, 3 obsolete files)

The global state is going away. Either we need to modify the interface to pass in whether to use private or public storage, or we need to get access to a window's docshell from inside the relevant calls.
I think after your last comment, a lot of things have been done. @Josh and others, what needs to be done here ?
This one I still don't have a good plan for. It's a global service, so we probably need to add a load context parameter to all relevant methods in the interface. I wonder if we could create a new interface that has an Init method that takes a load context and duplicates the rest of nsIContentPrefService, forwarding the context to all calls. It would make updating callers quite easy...
I think Josh is right on in comment 2.
I am interested in working on this.
Assignee: nobody → josh
/me assumes that Josh is a novice bugzilla user  :P
Assignee: josh → saurabhanandiit
Nope, I actually started working on this with Saurabh's blessing.
Assignee: saurabhanandiit → josh
Depends on: 722995
Try run for 4f25f1af982e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4f25f1af982e
Results (out of 132 total builds):
    exception: 6
    success: 83
    warnings: 43
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-4f25f1af982e
Try run for fd83d66ae564 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fd83d66ae564
Results (out of 133 total builds):
    exception: 5
    success: 109
    warnings: 19
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-fd83d66ae564
Whiteboard: [waiting on bug 722995]
Attachment #638112 - Attachment is obsolete: true
Comment on attachment 663764 [details] [diff] [review]
Determine privacy status from provided nsILoadContext in ContentPrefService.

I would appreciate your thoughts on this. I can't properly put it through its paces until bug 722995 stops mysteriously destroying mochitest-browser-chrome, but feedback would be useful.
Attachment #663764 - Flags: review?(ehsan)
Blocks: mobilepb
Comment on attachment 663764 [details] [diff] [review]
Determine privacy status from provided nsILoadContext in ContentPrefService.

Review of attachment 663764 [details] [diff] [review]:
-----------------------------------------------------------------

I agree that we need to have both sets of APIs, the one where you pass the load context to individual methods and the one that you specify it once for the lifetime of the contentprefs object you're interfacing, but I think that having two XPCOM interfaces this way is going to be very confusing.  Of all of the callsites which use init(), there is only one of them which is C++, so I suggest to modify the nsIContentPrefService to accept a load context for all of its methods (and of course document what that parameter is and where callers should get it from) and then create a js module named something like ContentPrefWrapper which basically does what your current ContentPrefInstance object does, and do not expose that to C++.  You can document the JS module as a simpler interface for the case where the caller wants to call multiple methods on the object.
Attachment #663764 - Flags: review?(ehsan)
CCing Drew to keep him on the loop here.
Attachment #663764 - Attachment is obsolete: true
Attachment #674949 - Attachment is obsolete: true
Comment on attachment 675020 [details] [diff] [review]
Determine privacy status from provided nsILoadContext in ContentPrefService.

Review of attachment 675020 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/privatebrowsing/test/browser/global/browser_privatebrowsing_DownloadLastDirWithCPS.js
@@ +113,5 @@
>  
>      { // check clearHistory removes all data
>        clearHistory();
>        is(gDownloadLastDir.file, null, "clearHistory removes all data");
> +      is(Services.contentPrefs.hasPref(uri1, "browser.download.lastDir", null), false, "LastDir preference should be absent", null);

Nit: is takes three arguments.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +452,2 @@
>  {
> +  NS_PRECONDITION(aDoc, "aDoc is null"); 

Nit: trailing space.

@@ +456,5 @@
> +  nsIURI* docURI = aDoc->GetDocumentURI();
> +  NS_PRECONDITION(docURI, "docURI is null");
> +
> +  nsCOMPtr<nsISupports> container = aDoc->GetContainer();
> +  nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(container);

You should be able to QI aDoc->GetContainer() directly.

@@ +462,3 @@
>    // Attempt to get the CPS, if it's not present we'll just return
>    nsCOMPtr<nsIContentPrefService> contentPrefService =
> +    do_GetService("@mozilla.org/content-pref/service;1");

No need for this change.

@@ +502,5 @@
>    }
>  
>    // Attempt to get the CPS, if it's not present we'll just return
>    nsCOMPtr<nsIContentPrefService> contentPrefService =
> +    do_GetService("@mozilla.org/content-pref/service;1");

No need for this either.

@@ +521,5 @@
>      return NS_ERROR_OUT_OF_MEMORY;
>    prefValue->SetAsAString(unicodePath);
> +
> +  nsCOMPtr<nsISupports> container = aDoc->GetContainer();
> +  nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(container);

Same as above.

::: editor/composer/src/nsEditorSpellCheck.cpp
@@ +108,5 @@
>    NS_ADDREF(*aURI);
>    return NS_OK;
>  }
>  
> +static nsILoadContext*

Return Already_AddRefed.  Otherwise the object may die when you return it.

@@ +119,5 @@
> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> +  NS_ENSURE_TRUE(doc, nullptr);
> +
> +  nsCOMPtr<nsISupports> container = doc->GetContainer();
> +  nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(container);

Nit: QI in one go please.

::: services/common/preferences.js
@@ +428,5 @@
>     * @private
>     */
>    get _contentPrefSvc() {
>      let contentPrefSvc = Cc["@mozilla.org/content-pref/service;1"].
> +                         createInstance(Ci.nsIContentPrefService);

Why is this needed?

::: toolkit/components/contentprefs/ContentPrefInstance.jsm
@@ +1,1 @@
> +'use strict';

Nit: forgot the MPL2 line.

::: toolkit/components/contentprefs/tests/unit/test_contentPrefs.js
@@ +24,5 @@
>    {
>      let dbFile = ContentPrefTest.deleteDatabase();
>  
>      let cps = Cc["@mozilla.org/content-pref/service;1"].
> +          createInstance(Ci.nsIContentPrefService);

Nit: you're just ruining the indentation here.  ;-)

::: toolkit/mozapps/downloads/DownloadLastDir.jsm
@@ +49,5 @@
>        case "browser:purge-session-history":
>          gDownloadLastDirFile = null;
>          if (Services.prefs.prefHasUserValue(LAST_DIR_PREF))
>            Services.prefs.clearUserPref(LAST_DIR_PREF);
> +        Services.contentPrefs.removePrefsByName(LAST_DIR_PREF, {usePrivateBrowsing: false});

Nit: please add a comment to explain what this does.
Attachment #675020 - Flags: review?(ehsan) → review+
Whiteboard: [waiting on bug 722995]
https://hg.mozilla.org/mozilla-central/rev/53b97b4ec554
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Blocks: 806735
Blocks: 806734
Blocks: 807407
You need to log in before you can comment on or make changes to this bug.