Last Comment Bug 723002 - ContentPrefService uses global Private Browsing state to make decisions
: ContentPrefService uses global Private Browsing state to make decisions
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Toolkit
Classification: Components
Component: Preferences (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla19
Assigned To: Josh Matthews [:jdm] (on vacation until Dec 5)
:
: Neil Deakin
Mentors:
Depends on: 722995
Blocks: PBnGen mobilepb 806734 806735 807407
  Show dependency treegraph
 
Reported: 2012-01-31 23:02 PST by Josh Matthews [:jdm] (on vacation until Dec 5)
Modified: 2012-11-03 13:30 PDT (History)
7 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Determine privacy status from provided nsILoadContext in ContentPrefService. (66.00 KB, patch)
2012-06-30 07:50 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
no flags Details | Diff | Splinter Review
Determine privacy status from provided nsILoadContext in ContentPrefService. (64.51 KB, patch)
2012-09-22 15:10 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
no flags Details | Diff | Splinter Review
Determine privacy status from provided nsILoadContext in ContentPrefService. (82.58 KB, patch)
2012-10-24 19:46 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
no flags Details | Diff | Splinter Review
Determine privacy status from provided nsILoadContext in ContentPrefService. (81.96 KB, patch)
2012-10-25 01:33 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
ehsan: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (on vacation until Dec 5) 2012-01-31 23:02:58 PST
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.
Comment 1 Saurabh Anand [:sawrubh] 2012-06-15 14:21:26 PDT
I think after your last comment, a lot of things have been done. @Josh and others, what needs to be done here ?
Comment 2 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-06-18 12:43:29 PDT
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...
Comment 3 :Ehsan Akhgari 2012-06-18 21:14:21 PDT
I think Josh is right on in comment 2.
Comment 4 Saurabh Anand [:sawrubh] 2012-06-20 11:22:21 PDT
I am interested in working on this.
Comment 5 :Ehsan Akhgari 2012-06-26 18:46:45 PDT
/me assumes that Josh is a novice bugzilla user  :P
Comment 6 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-06-26 19:45:02 PDT
Nope, I actually started working on this with Saurabh's blessing.
Comment 7 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-06-30 07:50:19 PDT
Created attachment 638112 [details] [diff] [review]
Determine privacy status from provided nsILoadContext in ContentPrefService.

WIP
Comment 8 Mozilla RelEng Bot 2012-06-30 11:15:21 PDT
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 Mozilla RelEng Bot 2012-06-30 16:00:24 PDT
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
Comment 10 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-22 15:10:03 PDT
Created attachment 663764 [details] [diff] [review]
Determine privacy status from provided nsILoadContext in ContentPrefService.
Comment 11 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-26 08:34:55 PDT
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.
Comment 12 :Ehsan Akhgari 2012-10-01 08:19:23 PDT
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.
Comment 13 :Ehsan Akhgari 2012-10-01 08:19:53 PDT
CCing Drew to keep him on the loop here.
Comment 14 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-24 19:46:43 PDT
Created attachment 674949 [details] [diff] [review]
Determine privacy status from provided nsILoadContext in ContentPrefService.
Comment 15 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-24 19:47:10 PDT
https://tbpl.mozilla.org/?tree=Try&rev=626684f29501
Comment 16 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-25 01:33:06 PDT
Created attachment 675020 [details] [diff] [review]
Determine privacy status from provided nsILoadContext in ContentPrefService.

Now passing tests.
Comment 17 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-25 01:33:25 PDT
https://tbpl.mozilla.org/?tree=Try&rev=4ec71db7cafb
Comment 18 :Ehsan Akhgari 2012-10-25 06:22:15 PDT
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.
Comment 19 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-25 12:23:24 PDT
https://tbpl.mozilla.org/?tree=Try&rev=43ce00f8cb9e
Comment 20 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-29 13:23:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/53b97b4ec554
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-10-30 08:28:50 PDT
https://hg.mozilla.org/mozilla-central/rev/53b97b4ec554

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