Closed Bug 75293 Opened 23 years ago Closed 23 years ago

Plugin continues to play even after leaving page

Categories

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

PowerPC
Mac System 9.x

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9

People

(Reporter: shrir, Assigned: peterlubczynski-bugs)

References

()

Details

Attachments

(1 file)

Seen on mac trunk 0405 , using Beatnik plugin 2.2
Do not have quicktime installed.

Steps:

Get the latest beatnik player plugin from beatnik.com (2.2)
Go to above url
url plays in beatnik player
After a while, leave the page and surf to another page
Notice that the music continues in the background..plugin does not stop
Sean says this is a case of Stop() not being called for full-page plugins. 
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.9.1
I'm updating my mac build.  Will let you know when I know.
Hardware: PC → Macintosh
I've confirmed that nsIPluginInstance::Stop() is not getting called.
I think the problem is that PluginViewerImpl::Destroy() in nsPluginViewer.cpp 
is not implemented.  It should probably do something similar to 
nsObjectFrame::Destroy() in nsObjectFrame.cpp.
Here's Beatnik specific url testcase (Beatnik is the only plugin that handles 
rmf content):

http://www.beatnik.com/software/documentation/music_object34/music/trance.rmf

This will allow you to test with the Beatnik plugin even if Quicktime is 
installed.
Upping priority. I'll see if I can fix this sooner.

Thanks Sean.
Priority: P3 → P2
Target Milestone: mozilla0.9.1 → mozilla0.9
Keywords: patch
The patch fixes this particular problem.  I'm getting some pretty wierd mouse 
behavior though - when I get some time I'll file bugs.
Andrei,

Can I get a review?
OK, I guess you coppied this from ObjectFrame, but You can probably change this:

+        if (doCallSetWindowAfterDestroy) {
+          inst->Stop();
+          inst->Destroy();
+          inst->SetWindow(nsnull);
+        }
+        else {
+          inst->SetWindow(nsnull);
+          inst->Stop();
+          inst->Destroy();
+        }

to this:

          !doCallSetWindowAfterDestroy ? inst->SetWindow(nsnull) : 0;
          inst->Stop();
          inst->Destroy();
          doCallSetWindowAfterDestroy ? inst->SetWindow(nsnull) : 0;

not very important though.

As we spoke about earlier, having this code duplicate in two places is not good
- can it be factored? Should you open a bug on that. Lastly, is the wierd mouse
behavior a result of the patch or does it happen with or without the patch?
Assuming the latter, and assuming that you get appropriate review, sr=attinasi
Opened bug 75582 for the mouse problem I commented on.
Bug 75582 is not related to this and I think I know how to fix it.

Bug 74457 was opened on refactoring code from nsObjectFrame.cpp and 
nsPluginViewer.cpp.
I like this language thing! Also 'traditional' if statement may look easier to 
read to some.

I agree with Marc on factorizing the code but this is definitely not important 
now. May be we could somehow to write a common base class for both full-page and 
embedded plugins to inherit from. Task for the future, if any.

Another thing. Once you are touching the file. May be I am missing something, 
but is there any sense to have class declaration in the .cpp file?

With or without all this -- ra=av
Patch checked in, marking FIXED.

/cvsroot/mozilla/modules/plugin/nglsrc/nsPluginViewer.cpp,v  <--  
nsPluginViewer.cpp
new revision: 1.52; previous revision: 1.51
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
I've always liked the "quickie" conditional statement, especially in this case 
as, I think, it reads well with the variable name. I've always found braces more 
confusing unless your IDE "helps" you as CodeWarrior.
verified on mac.(0416)
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: