Last Comment Bug 729162 - Add per-docshell observer list for private browsing transitions
: Add per-docshell observer list for private browsing transitions
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Josh Matthews [:jdm]
:
Mentors:
Depends on:
Blocks: PBnGen 722857 722942
  Show dependency treegraph
 
Reported: 2012-02-21 09:36 PST by Josh Matthews [:jdm]
Modified: 2012-04-24 18:04 PDT (History)
2 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add per-docshell privacy mode transition observers. (5.59 KB, patch)
2012-02-21 09:37 PST, Josh Matthews [:jdm]
no flags Details | Diff | Review
Add per-docshell privacy mode transition observers. (5.76 KB, patch)
2012-02-21 21:28 PST, Josh Matthews [:jdm]
bzbarsky: review+
Details | Diff | Review
Add per-docshell privacy mode transition observers. (7.44 KB, patch)
2012-04-19 20:24 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Review

Description Josh Matthews [:jdm] 2012-02-21 09:36:06 PST
To ensure that a DOMStorage clears its cache if its owning window switches privacy modes, we need some kind of immediate notification.
Comment 1 Josh Matthews [:jdm] 2012-02-21 09:37:36 PST
Created attachment 599226 [details] [diff] [review]
Add per-docshell privacy mode transition observers.
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-02-21 12:02:38 PST
Comment on attachment 599226 [details] [diff] [review]
Add per-docshell privacy mode transition observers.

Why is this going on nsILoadContext and not on the docshell?
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-02-21 12:03:18 PST
(And also, please use an observer array, so you don't have to make assumptions about the removal behavior, if any.)
Comment 4 Josh Matthews [:jdm] 2012-02-21 12:04:21 PST
Only because it seemed logical to group it with the attribute.(In reply to Boris Zbarsky (:bz) from comment #2)
> Comment on attachment 599226 [details] [diff] [review]
> Add per-docshell privacy mode transition observers.
> 
> Why is this going on nsILoadContext and not on the docshell?

Only because it seemed logical to group it with the attribute.
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-02-21 12:14:17 PST
I don't think we should do that.  If we're trying to observe on a particular window, the API should be one window or docshell, not on the load context...
Comment 6 Josh Matthews [:jdm] 2012-02-21 21:28:16 PST
Created attachment 599471 [details] [diff] [review]
Add per-docshell privacy mode transition observers.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-02-24 20:07:25 PST
Comment on attachment 599471 [details] [diff] [review]
Add per-docshell privacy mode transition observers.

>+++ b/docshell/base/nsDocShell.h
>+    nsTObserverArray<nsIWeakReference*> mPrivacyObservers;

I'm ... a little surprised that worked.  You're getting nsIWeakReference objects, putting this in the array, then dropping the only ref to them, which should by rights explode.

Please make this member nsTObserverArray<nsWeakPtr>.

>+++ b/docshell/base/nsIDocShell.idl

Rev the uuid.

>+++ b/docshell/base/nsIPrivacyTransitionObserver.idl
>\ No newline at end of file

Add one, please.

r=me with those issues fixed.  Please do NOT check in without fixing the ownership model bit!
Comment 8 Josh Matthews [:jdm] 2012-04-19 20:24:35 PDT
Created attachment 616861 [details] [diff] [review]
Add per-docshell privacy mode transition observers.
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-20 11:51:12 PDT
https://hg.mozilla.org/projects/birch/rev/6e93febee902
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-24 18:04:34 PDT
https://hg.mozilla.org/mozilla-central/rev/6e93febee902

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