90256 - Can't script Fullpage Plugins

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Jazmin Jonson]

Since the user can have customized menus with Netscape6,
plugin vendors want to integrate the plugin menus with
browsers menus. However since we cannot script the
Fullpage Plugin through the DOM, we cannot get access
to the plugin nor call any of it's methods from the
XUL menus and thus cannot take full advantage of the
customized menus.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Is this an XPCOM plugin?

Comment 2

[Peter Lubczynski]

Jazmin,

Can you attach your patch to this bug? Thanks!

Comment 3

[Jazmin Jonson]

Attachment: Code for enhancement to script fullpage plugins

Comment 4

[Jazmin Jonson]

Attachment: Tarred up files of diff attachment

Comment 5

[Jazmin Jonson]

No this is no an XPCOM plugin but rather code
that will allow you to script a FullPage XPCOM
and 4x plugin.

Comment 6

[Jazmin Jonson]

Oops! I think I lied a little about 4x plugins
This solution will lets 4x Fullpage Plugins work but
you still can't normally script them because there
are still issues with scripting for 4x Plugins
However you can script Fullpage XPCOM new style plugins through
the dom. You can access the plugin in javascript
like so:
window._content.document.plugin.PluginInterfaceName.PluginIDLItem

where PluginInterfaceName is your plugin's IDL interface
and PluginIDLItem is your plugins IDL attribute or method.

Comment 7

[Peter Lubczynski]

Tested patch on Win32 against the trunk. Here are my observations:

1) Simple Acrobat PDF loaded, but toolbar missing and window sizing did not work
2) Complex Acrobat PDF did not load at all!!!
3) Simple Quicktime movie file did not load at all!!!

I haven't even tried it on Mac yet, but I know it will also require changes to
MANIFEST and the project files for the new interface.

Also, you'll need to remove nsPluginViewer along with its interface.

Sorry it has taken me so long to look at but it was difficult to apply. Can you
attach future patches with a cvs -N -u to include new files, thanks!

Comment 8

[Arun Ranganathan]

peterl, I'm suggesting that this is an 'nsenterprise' bug, and I'm cc'ing Vidur.
 He may be able to review your patch(es), since Vidur and I talk to a third
party SVG group to whom this fix is important.

Comment 9

[Peter Lubczynski]

I no longer think that Jazim's patch is the right way to go (as not even a
simple testcase works) and I have some new suggestions. Basically, I think it's
completely incorrect to use an ObjectFrame for full-page plugins. Fooling an
EMBED tag to be a full-page plugin in a generated document isn't the way to go.
However, we may still need the synthetic document. Arun, IMO, it also seems kind
of risky for 0.9.2 branch (even 0.9.4 for that matter). What are the SVG's group
specific needs on this bug. Can we have all known engineering issues documented
in Bugzilla or Bugscape?

1. I propose that we split off nsPluginInstanceOwner (currently in
nsObjectFrame.cpp) into it's own file and have the same class able to handle
both full-page and embedded plugins. Since this is the "instance owner", it has
everything we already need in terms of scripting and it would eliminate the
duplicate code in nsPluginViewer.cpp

2. nsPluginViewer currently only inherits from nsIContentViewer which really
doesn't give it too many abilities. Jazim proposes that we wrap a document
around full-page plugins. I'm not quite sure why we need a whole document, but
if we MUST, I would rather have nsPluginViewer directly inherit
nsIDocumentViewer and work out the bugs there. There are a lot of issues to
hammer out and we may not even need to do this.

3. How did one script full-page documents in 4.x? Was there a "document" object
thrown in there or not:
window._content.[document?????.]plugin.PluginInterfaceName.PluginIDLItem
Perhaps with Vidur's help, we can just hook up the DOM for scripting to our new
nsPluginInstanceOwner with minimal impact to nsPluginViewer.

Andrei, what do you think?

Comment 10

[av (gone)]

Leaving alone the fact of having a wrong path trying to achieve the goal, I am 
really concerned by it simply not working. Jazmin, what test cases did you use? 
Can you post them here?

So, this is definitely not a branch candidate. I think we need more cycles/help 
to investigate the needs and solution.

I absolutely agree with factoring out nsPluginInstanceOwner code. Not only have 
we duplicate code in nsPluginViewer but also I suspect we may potentially have 
all sorts problems due to not solid instance-owner-peer relationship. Peer can 
e.g. have a stale pointer to the owner and there is no decent way to update it. 
This code cleaning up may help if we do it right.

Comment 11

[Peter Lubczynski]

Also, re-reading the initial bug description, it seems the orriginal purpose of
this bug was to be able to cutomize XUL menus. Why can't plugins simply use the
C++ route of |do_GetService()| like everything else?

For example, if you wanted to show a simple alert box, here is an example I
ripped off from nsCrypto.cpp:

void alertUser(char *message)
{
nsCOMPtr<nsIWindowWatcher>wwatch(do_GetService("@mozilla.org/embedcomp/window-watcher;1"));
nsCOMPtr<nsIPrompt> prompter;

if (wwatch)
  wwatch->GetNewPrompter(0, getter_AddRefs(prompter));
  if (prompter) {
    nsAutoString wmessage;
    wmessage.AssignWithConversion(message);
    prompter->Alert(0, wmessage.get());
  }
}

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

The original purpose of the bug was to let customized XUL menus -- we already
have great mechanisms for that -- interact with full-page plugins, which means
making them scriptable.  I don't see what any of that has to do with popping up
an alert, of course.

I wouldn't be recommending that plugin authors develop a binary dependency on
do_GetService, nor on the window watcher -- neither are binary APIs that Mozilla
has committed to supporting in the long run, or even milestone-to-milestone. 
You can use the component manager and service manager "manually" to work around
the do_GetService API issue, but I don't think you want to be touching the
window watcher.  I'm adding danm to the Cc: list in case he wants to contradict
me on this.

I just don't want to see people in the same mess they were in when they used
NS_IMPL_NSGETMODULE in their plugins, and ended up with a binary dependency on a
non-frozen -- and subsequently changed -- API.  Plugin authors need to be very
careful about the APIs they use.  In the absence of plugin-specific
documentation regarding Mozilla APIs, they should probably rely on the embedding
documents for their "exbedding" situation.

Comment 13

[Jazmin Jonson]

Reply to Peter's comment on 7/30/01

1. Toolbar missing: You have to put the change in for the nsObjectFrame.cpp.
The method nsPluginInstance:GetMode needs to change to the new code included.
This code has a little bit of a memory leak in it. I have a fix for it
but I got a bad build the day I had the fix so I could not test it. That was
the day before the redeployment. Also window resizing could be a problem.
This is probably because I have to set the width on the embed tag to get
the plugin to size correctly. There may be a way of fixing this by listening
for a resize event and then resizing/doing a set window in the plugin.

2. Complex pdf. Send me the URL to the complex pdf and see if I can
see it with my build.

3. Quicktime I think has java in it. There maybe a problem with java
working with this solution.

Reply to Andrei's comment 8/13/01
I tested my fix with the following:

PC tests
npsimple.cpp test plugin - worked
adobe acrobat - worked
flash - worked
real - video worked ( my pc doesn't have sound card and was in
                      the process of getting one so couldn't test sound)
Linux
npsimple.cpp test plugin - scripting worked

Andrei send me what you are testing it with and I will
see if it works for me. 

Comment 14

[chris hofmann]

where are we at with this one.  what should we do for N6.2?

Comment 15

[Arun Ranganathan]

Talked to peterl.  There's going to be substantial amount of engineering work,
including large scale code rewrites.  For the next commercial browser release,
therefore, peterl counsels holding off on this one.  It's primarily very
important for SVG purposes, but we'll hold on to this one for another release.

Comment 16

[chris hofmann]

nsbranch-

Comment 17

[Peter Lubczynski]

-->peterl for 0.9.6

Comment 18

[Peter Lubczynski]

--->reassign to Andrei

Comment 19

[av (gone)]

Time is running out, moving.

Comment 20

[Peter Lubczynski]

nominating nsbeta1

Comment 21

[Jaime Rodriguez, Jr.]

We'll still meet with the Adobe SVG group, but for the next branch I think we'll
put nsbeta1- .

Comment 22

[rubydoo123]

based on nsbeta1-, moving this out to 1.1

Comment 23

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Why move this out?  Does the patch (sitting there for 8 months) not deserve
attention?

Comment 24

[Peter Lubczynski]

I think the patch in this bug still needs more work.

There needs to be a way for the initial stream to be handed off to the full-page
plugin instead of creating a new one with the object frame.

Also, isn't re-writing full-page plugins like this so close to Mozilla 1.0 a
little risky?

Perhaps there is another way to hook-up scripting? nsPluginViewer already has an
nsIDocument.

Comment 25

[Johnny Stenback (:jst)]

*** Bug 148992 has been marked as a duplicate of this bug. ***

Comment 26

[anthonyd]

After meeting with jst and vidur to discuss  this bug and the proper solution, 
it was decided that wrapping full screen plugins in a synthetic document was 
the correct way to go.  This solution will also help to stabilize plugins and 
allow plugins access to the dom event listeners.  jst thinks that rpotts is the 
perfect person to fix this because it is almost the exact same solution that 
was required for full screen images with the old image library, which rpotts 
wrote.

We really do need rpotts expertise with this, because while we understand what 
needs to be done, there is the question of how to do it (specifically with 
streams, one of the remaining issues with the attached patch).  Both peterl and 
I would be happy and grateful to assist rpotts with this very important 
plugin/dom bug.

It would also be major cleanup win because we would then share the same code 
path as embeded plugins.  This would also us to remove the extraneous 
implementation for full screen plugins.

There are many, many numerous reasons and benefits for doing this.

Comment 27

[Peter Lubczynski]

---->back to me, the plugins group needs to do this -- for several reasons. 

Note: Even IE 6 (on WinXP) does this. Do a View Source on a full-page plugin and
notice they use an EMBED tag with a fullpage attribute in a synthetic document too.

Comment 28

[rubydoo123]

setting to PL2:P1, we need to get scripting of full page plug-ins to work

Comment 29

[Peter Lubczynski]

Because of stability issues, we will be fixing this bug on the plugins branch.

Comment 30

[Peter Lubczynski]

okay, new plan: instead of one big branch I've broken this down into smaller
pieces. I've got an up-to-date patch that's working that allows scripting of
full-page plugins and resues the object frame code. There is a run-time pref
that allows going back to the old code path. I've opened bug 195502 for removing
nsPluginViewer.cpp plus the pref and opened bug 195501 for moving
nsPluginInstanceOwner to the plugins module.

Comment 31

[Peter Lubczynski]

Attachment: patch v.2

Comment 32

[Peter Lubczynski]

Attachment: testcase

Here's a testcase for full-page plugin scriptability. As with images, security
blocks scripting the iframe from remote. Using a local Flash file doesn't have
the same restrictions.

Comment 33

[Johnny Stenback (:jst)]

Comment on attachment 115958 [details] [diff] [review]
patch v.2

- In nsIDocument.h:

+extern NS_EXPORT nsresult
+   NS_NewPluginDocument(nsIDocument** aInstancePtrResult);
+

Loose the "extern NS_EXPORT" part of that signature, it's not needed, and was
recently removed form all similar methods in content and layout.

- In nsPluginDocument.cpp

+class PluginProxyListener: public nsIStreamListener
+{
+public:
...
+  nsPluginDocument*		 mDocument;
+  nsCOMPtr<nsIStreamListener>	 mNextStream;

Should these members not be protected?

And you could make mDocument of type nsRefPtr<nsPluginDocument> and not worry
about manual refcounting of mDocument.

- In the declaration of the class nsPluginDocument

+protected:
+  NS_IMETHOD CreateSyntheticDocument(const char *aMimeType);

I don't see a need for this NS_IMETHOD return type here, this doesn't need to
be virtual does it? Nor is it part of an interface, is it? I say nsresult is
enough here.

+  PluginProxyListener*        mStreamListener;

Make this be of type nsRefPtr<PluginProxyListener> to not have to worry about
manual reference counting.

...

Seems like this could be done with far less code if nsPluginDocument was to
inherit from nsImageDocument, or better yet, make a separate class that both
nsImageDocument and nsPluginDocument both inherit from (nsFullPageDocument?).
The more I read of this code, the more I see code that is *identical* to the
code in nsImageDocument, and that's a maintenance nightmare waiting to happen,
not to mention the unnecessary code bloat.

Please consider this, I'm reluctant to sr'ing this as is.

- In nsIPluginDocument.idl:

+ /**
+  * Causes the plugin to print in full-page mode
+  */
+  void Print();

Is this something that's needed for compatibility with IE or something? If not,
wouldn't it be better to just make window.print() call into the plugin's print
method if window holds a full-page plugin?

Other than those issues, this approach seems good, but it's not there yet.

Comment 34

[John Keiser (jkeiser)]

Comment on attachment 115958 [details] [diff] [review]
patch v.2

I may as well make my comments anyway:

Is there any reason not to set last-modified in StartDocumentLoad() as well,
like HTMLDocument does?  Perhaps that's worth splitting from HTMLDocument into
a method like RetrieveRelevantHeaders(httpChannel) or something.

Also, plugin.diable_load_full_page_via_content should read "disable", not
"diable".

Wouldn't it make more sense to not tell the nsIPluginInstance to create the
stream until there actually is one?  You could actually do that in
PluginProxyListener::OnStartRequest(), for example.  This would make
InstantiateFullPagePlugin() not set up that stream anymore and eliminate the
need for nsIPluginDocument::setStreamListener().  This might cause strange
initialization problems though, if the frame is not reflowed (and therefore the
plugin initialized) before OnStartRequest() happens--is that possible now?

That's all the comments I have.

Comment 35

[Peter Lubczynski]

Attachment: patch v.3

Alrighty, here's a new patch:

* Removed changed nsDOMClassInfo
* Factored out common document and listener code into nsMediaDocument
* Factored out header checking code into
nsHTMLDocument::RetrieveRelevantHeaders
* Fixed the pref typo and used nsIPrefBranch
* Used nsRefPtr where I could. Blocked in one spot due to bug 107291.

I rather not change the way the stream listener are hooked up for this patch 
because I'd like to keep the code as close to the object frame and what it did
before as possible. Also, I think we'll always need a stream listener here as
we're loading a full-page document.

Comment 36

[Peter Lubczynski]

See bug 195502 and bug 195501 for actually factoring nsPluginInstanceOwner.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Any good reason to take the nsACString mime type, call .get on it, then convert
the resulting char* to UCS2 instead of just converting the nsACString?

nice refactoring, esp. with the header stuff!

Comment 38

[Johnny Stenback (:jst)]

Comment on attachment 117961 [details] [diff] [review]
patch v.3

- In nsHTMLDocument::RetrieveRelevantHeaders():

No need for the local httpChannel, use mHttpChannel.

-    rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("last-modified"),
+    nsresult rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("..."),
					 lastModHeader);

Please fix the indentation of the next line argument.

- In nsHTMLDocument::StartDocumentLoad():

   PRBool isPostPage = PR_FALSE;
   //check if current doc is from POST command
+  nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(aChannel);
   if (httpChannel) {

Use mHttpChannel here, it will already be set.


-class ImageListener: public nsIStreamListener
+class ImageListener: public nsMediaDocumentStreamListener
 {
 public:
   ImageListener(nsImageDocument* aDocument);
   virtual ~ImageListener();
...
-  nsImageDocument*		 mDocument;
-  nsCOMPtr<nsIStreamListener>	 mNextStream;
+  nsImageDocument*		 mImageDocument;

Make mImageDocument an nsRefPtr<nsImageDocument>, that way you don't need to do
the manual refcounting. Or do you even need this, couldn't you cast mDocument
to nsImageDocument in stead, you know it's the right type...?


 NS_IMETHODIMP
 ImageListener::OnDataAvailable(nsIRequest* request, nsISupports *ctxt,
				nsIInputStream *inStr, PRUint32 sourceOffset,
PRUint32 count)
 {
-  if (mNextStream) {
-    return mNextStream->OnDataAvailable(request, ctxt, inStr, sourceOffset,
count);
-  }
-
-  return NS_OK;
+  return nsMediaDocumentStreamListener::OnDataAvailable(request, ctxt, inStr,
sourceOffset, count);
 }

Looks like this method is not needed at all, you could (if you don't use the
macro NS_DECL_NSISTREAMLISTENER in ImageListener) simply not override this
method here nad the base calss would be called automatically.

+class nsMediaDocumentStreamListener: public nsIStreamListener
+{
+public:
+  nsMediaDocumentStreamListener(nsMediaDocument *aDocument);
+  virtual ~nsMediaDocumentStreamListener();
+  void SetStreamListener(nsIStreamListener *aListener);
+
+  NS_DECL_ISUPPORTS
+
+  NS_DECL_NSIREQUESTOBSERVER
+
+  NS_DECL_NSISTREAMLISTENER
+
+protected:
+  nsMediaDocument	       *mDocument;

Make mDocument an nsRefPtr<nsMediaDocument>.

- In nsPluginDocument.cpp:

\ No newline at end of file

Add a newline.

And as Boris mentioned, make CreateSyntheticDocument() take an const
nsACString&.

- In nsIPluginDocument.idl:

>+interface nsIPluginDocument : nsISupports
+{
...
+  void setStreamListener(in nsIStreamListener aStreamListener);
...
+  void Print();

Make 'P' in 'Print' lowercase. It'll be uppercased by the XPIDL compiler.

- In nsPluginHostImpl.cpp

+    if (sPrefService) {
+	  nsCOMPtr<nsIPrefBranch> prefBranch;
+	  sPrefService->GetBranch(nsnull, getter_AddRefs(prefBranch));
+	  if (prefBranch)
+	 prefBranch->GetBoolPref("plugin.disable_load_full_page_via_content",
&sLoadViaPlugin);
+    }

Eek, tabs!

sr=jst

Comment 39

[John Keiser (jkeiser)]

Comment on attachment 117961 [details] [diff] [review]
patch v.3

Heh, jst caught everything I found and more.  r=me.

Comment 40

[Peter Lubczynski]

>+protected:
>+  nsMediaDocument	       *mDocument;

>Make mDocument an nsRefPtr<nsMediaDocument>.

Bug 107291 is blocking me here. I think I have to use a raw pointer in this case. 

Comment 41

[Peter Lubczynski]

Attachment: patch v.4

never mind, just had to reorder things a bit, here's the update patch to
comments

Comment 42

[Boris Zbarsky [:bzbarsky]]

You can't use nsRefPtr on an object whose only constructor requires arguments
(this keeps coming up...) on some compilers.

So this patch broke OS/2 and a few other ports.... jsut going back to manual
refcounting should make things happy.

Comment 43

[Peter Lubczynski]

Patch in trunk, marking FIXED.

Thanks to mkaply and dbaron for checking in a default constructor for
nsMediaDocumentStreamListener to make the ports compilers happy.

Comment 44

[Boris Zbarsky [:bzbarsky]]

This caused nsImageDocument (and nsPluginDocument too) to leak any time it's
instantiated.  See bug 200058