Last Comment Bug 722942 - nsPluginHost and nsNPAPIPluginInstance use Private Browsing global state to set plugin properties
: nsPluginHost and nsNPAPIPluginInstance use Private Browsing global state to s...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Josh Matthews [:jdm] (on vacation until Dec 5)
:
: Benjamin Smedberg [:bsmedberg]
Mentors:
Depends on: 729162 748752
Blocks: PBnGen
  Show dependency treegraph
 
Reported: 2012-01-31 17:16 PST by Josh Matthews [:jdm] (on vacation until Dec 5)
Modified: 2012-04-25 06:57 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Obtain private browsing status from document of plugin owner, and watch private mode transitions on a per-instance basis. (14.94 KB, patch)
2012-04-22 23:44 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
jaas: review-
Details | Diff | Splinter Review
Obtain private browsing status from document of plugin owner, and watch private mode transitions on a per-instance basis. (12.98 KB, patch)
2012-04-23 21:51 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
jaas: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (on vacation until Dec 5) 2012-01-31 17:16:10 PST
The PB service is going away. Instead, we might be able to get away with setting the property based on the closest docshell when the plugin is created.
Comment 1 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-01-31 18:48:43 PST
This could probably cover nsNPAPIPlugin's use of the service for getting the value of NPNVprivateModeBool as well.
Comment 2 :Ehsan Akhgari 2012-03-22 19:12:42 PDT
Josh, do you know how we can get our hands on a window/document/docshell/loading context in these two classes?
Comment 3 Josh Aas 2012-03-26 09:23:43 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> Josh, do you know how we can get our hands on a
> window/document/docshell/loading context in these two classes?

Not off the top of my head, I'd start by using mxr to find relevant class usage in "dom/plugins/base". Ping me on irc to talk this through if you need more help, the more specific the question the better.
Comment 4 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-04-15 21:39:05 PDT
Ehsan, it looks like we can obtain the plugin's element (http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.cpp#1299 or http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.cpp#2149), that suggests to me we can walk up to find the document and docshell, etc. Josh, does that sounds workable?
Comment 5 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-04-15 21:44:55 PDT
nsPluginHost can switch to using the observers from bug 729162 and can pass the value to nsNPAPIPluginInstance.
Comment 6 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-04-15 22:13:39 PDT
Actually it's not quite that easy - the plugin host can't be in charge of being the observer; each instance has to be instead. We can probably get the plugin instance owner, get the document from that, and get a docshell from that.
Comment 7 Josh Aas 2012-04-17 04:04:41 PDT
(In reply to Josh Matthews [:jdm] from comment #6)
> Actually it's not quite that easy - the plugin host can't be in charge of
> being the observer; each instance has to be instead. We can probably get the
> plugin instance owner, get the document from that, and get a docshell from
> that.

Sounds reasonable.
Comment 8 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-04-22 23:44:37 PDT
Created attachment 617401 [details] [diff] [review]
Obtain private browsing status from document of plugin owner, and watch private mode transitions on a per-instance basis.

test_privatemode.xul continues to pass with these changes.
Comment 9 Josh Aas 2012-04-23 10:33:52 PDT
Comment on attachment 617401 [details] [diff] [review]
Obtain private browsing status from document of plugin owner, and watch private mode transitions on a per-instance basis.

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

I was hoping to fully decomtaminate nsNPAPIPluginInstance some day (it's down to just nsISupports now), this is a step in the other direction. We typically use a peer xpcom object (nsPluginInstanceOwner) to handle events and notifications like this. Can you consider making the nsPluginInstanceOwner the listener instead of nsNPAPIPluginInstance? That should also remove some ugly casts from the patch.

Minusing for the move to nsPluginInstanceOwner, happy to hear objections and reconsider though.

(FYI, I'm sort of hoping to combine nsPluginInstanceOwner and nsNPAPIPluginInstance some day, but for now I want to try to decom nsNPAPIPluginInstance to simplify things.)

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +157,5 @@
>  
> +  nsCOMPtr<nsPIDOMWindow> domWindow = GetDOMWindow();
> +  NS_ENSURE_STATE(domWindow);
> +  nsCOMPtr<nsIDocShell> docShell = domWindow->GetDocShell();
> +  docShell->AddWeakPrivacyTransitionObserver(this);

Is docShell guaranteed to be non-null here?
Comment 10 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-04-23 21:51:58 PDT
Created attachment 617779 [details] [diff] [review]
Obtain private browsing status from document of plugin owner, and watch private mode transitions on a per-instance basis.
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2012-04-25 01:31:33 PDT
Comment on attachment 617779 [details] [diff] [review]
Obtain private browsing status from document of plugin owner, and watch private mode transitions on a per-instance basis.

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

::: dom/plugins/base/nsNPAPIPlugin.cpp
@@ +2136,5 @@
>    }
>  
>    case NPNVprivateModeBool: {
> +    nsCOMPtr<nsIDocument> doc = GetDocumentFromNPP(npp);
> +    nsCOMPtr<nsPIDOMWindow> domwindow = do_QueryInterface(doc);

This will always be null.
Comment 14 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-04-25 06:54:52 PDT
Ah, thanks. I'll file a followup.

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