Closed
Bug 723002
Opened 12 years ago
Closed 12 years ago
ContentPrefService uses global Private Browsing state to make decisions
Categories
(Toolkit :: Preferences, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: jdm, Assigned: jdm)
References
Details
(Keywords: addon-compat, dev-doc-needed)
Attachments
(1 file, 3 obsolete files)
81.96 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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•12 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...
Comment 4•12 years ago
|
||
I am interested in working on this.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → josh
Comment 5•12 years ago
|
||
/me assumes that Josh is a novice bugzilla user :P
Assignee: josh → saurabhanandiit
Assignee | ||
Comment 6•12 years ago
|
||
Nope, I actually started working on this with Saurabh's blessing.
Assignee: saurabhanandiit → josh
Assignee | ||
Comment 7•12 years ago
|
||
WIP
Comment 8•12 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•12 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•12 years ago
|
Whiteboard: [waiting on bug 722995]
Assignee | ||
Comment 10•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #638112 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 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•12 years ago
|
Keywords: addon-compat,
dev-doc-needed
Comment 12•12 years ago
|
||
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)
Comment 13•12 years ago
|
||
CCing Drew to keep him on the loop here.
Assignee | ||
Comment 14•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #663764 -
Attachment is obsolete: true
Assignee | ||
Comment 15•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=626684f29501
Assignee | ||
Updated•12 years ago
|
Attachment #674949 -
Attachment is obsolete: true
Assignee | ||
Comment 17•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=4ec71db7cafb
Comment 18•12 years ago
|
||
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•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=43ce00f8cb9e
Assignee | ||
Comment 20•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/53b97b4ec554
Assignee | ||
Updated•12 years ago
|
Whiteboard: [waiting on bug 722995]
Comment 21•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/53b97b4ec554
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•