The default bug view has changed. See this FAQ.

ContentPrefService uses global Private Browsing state to make decisions

RESOLVED FIXED in mozilla19

Status

()

Toolkit
Preferences
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

({addon-compat, dev-doc-needed})

unspecified
mozilla19
x86
Mac OS X
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Comment 2

5 years ago
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)

Updated

5 years ago
Assignee: nobody → josh
/me assumes that Josh is a novice bugzilla user  :P
Assignee: josh → saurabhanandiit
(Assignee)

Comment 6

5 years ago
Nope, I actually started working on this with Saurabh's blessing.
Assignee: saurabhanandiit → josh
(Assignee)

Updated

5 years ago
Depends on: 722995
(Assignee)

Comment 7

5 years ago
Created attachment 638112 [details] [diff] [review]
Determine privacy status from provided nsILoadContext in ContentPrefService.

WIP

Comment 8

5 years ago
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

Comment 9

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

Updated

5 years ago
Whiteboard: [waiting on bug 722995]
(Assignee)

Comment 10

5 years ago
Created attachment 663764 [details] [diff] [review]
Determine privacy status from provided nsILoadContext in ContentPrefService.
(Assignee)

Updated

5 years ago
Attachment #638112 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
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)

Updated

5 years ago
Keywords: addon-compat, dev-doc-needed
(Assignee)

Updated

5 years ago
Blocks: 794502
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.
(Assignee)

Comment 14

5 years ago
Created attachment 674949 [details] [diff] [review]
Determine privacy status from provided nsILoadContext in ContentPrefService.
(Assignee)

Updated

5 years ago
Attachment #663764 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=626684f29501
(Assignee)

Comment 16

5 years ago
Created attachment 675020 [details] [diff] [review]
Determine privacy status from provided nsILoadContext in ContentPrefService.

Now passing tests.
Attachment #675020 - Flags: review?(ehsan)
(Assignee)

Updated

5 years ago
Attachment #674949 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=4ec71db7cafb
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+
(Assignee)

Comment 19

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=43ce00f8cb9e
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/53b97b4ec554
(Assignee)

Updated

5 years ago
Whiteboard: [waiting on bug 722995]
https://hg.mozilla.org/mozilla-central/rev/53b97b4ec554
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

5 years ago
Blocks: 806735

Updated

5 years ago
Blocks: 806734

Updated

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