nsPluginHost and nsNPAPIPluginInstance use Private Browsing global state to set plugin properties

RESOLVED FIXED in mozilla15

Status

()

Core
Plug-ins
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

Trunk
mozilla15
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

5 years ago
This could probably cover nsNPAPIPlugin's use of the service for getting the value of NPNVprivateModeBool as well.
Josh, do you know how we can get our hands on a window/document/docshell/loading context in these two classes?

Comment 3

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

Comment 4

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

Comment 5

5 years ago
nsPluginHost can switch to using the observers from bug 729162 and can pass the value to nsNPAPIPluginInstance.
Depends on: 729162
(Assignee)

Comment 6

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

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

Updated

5 years ago
Assignee: nobody → josh
(Assignee)

Comment 8

5 years ago
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.
Attachment #617401 - Flags: review?(joshmoz)
(Assignee)

Updated

5 years ago
Whiteboard: [needs review]

Comment 9

5 years ago
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?
Attachment #617401 - Flags: review?(joshmoz) → review-
(Assignee)

Comment 10

5 years ago
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.
Attachment #617779 - Flags: review?(joshmoz)
(Assignee)

Updated

5 years ago
Attachment #617401 - Attachment is obsolete: true

Updated

5 years ago
Attachment #617779 - Flags: review?(joshmoz) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [needs review] → [needs landing]
https://hg.mozilla.org/projects/birch/rev/e18846bc6b20
Whiteboard: [needs landing]
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/e18846bc6b20
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
(Assignee)

Comment 14

5 years ago
Ah, thanks. I'll file a followup.
(Assignee)

Updated

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