54778 - must fix plug-in finder code on Mac so it always looks in "plug-ins" directory

Status: VERIFIED FIXED

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

Description

[ekrock's old account (dead)]

We have a bug report here which is very difficult to believe. It is reported 
that changing the Moz/N6 plug-ins directory name from "plugins" back to 
"plug-ins" (for consistency with the naming scheme used on Nav4 Mac) has caused 
The Nav4 Mac QuickTime plug-in binary to stop working, and the changing the name 
back to "plugins" fixes this problem.

See related bug 36089 "All 4.x plug-in installers will fail on Mac because 
folder is named 'plugins' instead of 'plug-ins'" and bug 53976 "on Mac, change 
name of 'plugins' folder to 'plug-ins' for backward compatibility." (both FIXED)

Supposedly, the legacy Nav4 QuickTime plug-in binary only works in Mozilla/N6 if 
the directory for plug-ins is named "plugins" (consistent with naming in Win32 
and Linux) rather than "plug-ins" as it always was in Nav4 Mac.

This is difficult to understand: since the legacy Nav4 QuickTime Mac plug-in 
binary was written for Nav4 Mac, on which the plug-in install directory was 
called "plug-ins", why would it work when the Moz/N6 Mac directory was 
(previously) named "plugins" but now fail when we restore consistency with the 
previous naming scheme and change the directory name to "plug-ins"? How can a 
legacy plug-in binary be dependent on a new, incompatibile naming system that 
didn't exist when it was written?

My suspicion: somewhere in the Mozilla Mac codebase, we have a dependency of our 
own that we've overlooked on naming the directory "plugins" rather than 
"plug-ins", and this broken dependency is for some reason only fouling up 
QuickTime.

First step: we need to verify that this bug report is valid. 

Then, we need to figure out whether there's a simple fix that will get QuickTime 
working again while leaving the plug-ins directory named "plug-ins."

If not, the simplest solution may be to live with having both a "plugins" and 
"plug-ins" directory on Moz/N6 Mac for the first release, and advise everyone to 
use the "plug-ins" directory for all plug-ins except QuickTime, and put 
QuickTime in "plugins" until we figure out what's going on here.

More info from email thread:

-----
Peter, 
  I found out the problem. After I renamed the plugin folder from "Plug-ins" to 
"plugins", quicktime started working again in today's branch build. It seems 
that this is a side-effect of bug 36089 getting fixed. If  I rename the plugins 
folder to what it was earlier (plugins), quicktime works just as fine on 
windows. 

  Cc'ing Eric and Andrei on this. 

  Thanks, 
  Shrirang 
    

  shrirang 
-----
Try the Quicktime on the branch build. It WFM on the build but not optimized. I 
wonder if some other code someplace else was changed. All my stuff should effect 
all plugins equally. 

    -peterl 
-----
 Peter: 
 It(quicktime) used to work in M17 as mentioned initially in bug 36046. Nobody 
was working on bug 36046 for a long time.( see comments). I read your comment in 
that bug about quicktime working and tried to get it working today, which it did 
not. I have updated bug 36046 with the latest status. 
 
 As far as other plugins are concerned, shockwave and flash are working for me. 
I have already verified a few bugs on the branch build for these plugins. 
Realplayer looks good too (on an initial test) on branch mac build. I see a new 
problem of the plug-ins's context menu not popping up when pressing CONTROL 
+mouse button. But that is a small issue. Am logging a bug on it. 
 
 Shrirang 
-----
Shrir, 

I just pulled today's Commercial Netscape from ftp://sweetlou to try Plugins and 
there is a problem. I haven't tried the others yet, but Quicktime doesn't work. 
I dragged the 2 files from my build's plugin folder to the PR3 plugin folder but 
I can't get Quicktime to display. What's going on? Did it work for you? 

-peterl

Comment 1

[ekrock's old account (dead)]

Marking priority P1/severity critical as this is a high profile partner (Apple) 
and high profile backward compatibility (#5 plug-in on the web). Nominating for 
rtm.

Comment 2

[av (gone)]

Eric, if your suspicion that we overlooked some internal dependency is true than 
this problem would show up with other plugins too.

Comment 3

[Peter Lubczynski]

cc:ing [e-mail]

Comment 4

[ekrock's old account (dead)]

Peter Lubczynski wrote:

  bug  53976 is: on Mac, change name of 'plugins' folder to 'plug-ins' for
  backward compatibility

  Samir,
  I guess you checked in a patch to change the name of the "plugins"
  folder to "plug-ins" on the Mac. Shrir and I just tried this and it
  looks like Quicktime doesn't like being put into the "plug-ins" folder.
  Moz isn't finding the plugin. You may have missed a few spots as the bug
  report indicates that could have possibly happened. Renaming the folder
  seems to get Quicktime and all the others to work, but may cause other
  problems inadvertantly.

  Shrir,
   From your testing, do any other plugins have problems with being in the
  "plug-ins" folder? Should this bug be re-oppened?

  Eric,
  I did some work over the past week to get all plugins at least somewhat
  usable on the Mac for PR3. However, the plugins that this bug affects
  makes them unusable unless the folder is renamed. Using aliases don't
  seem to do it either. Is there a simple solution here such as creating
  two folders that can be done for PR3?

  -peterl

Comment 5

[shrirang khanzode]

Peter, Other plugins are not showing this behaviour at all. They are working 
fine.

Comment 6

[shrirang khanzode]

any news about the veracity of this bug ? Thanks!

Comment 7

[ekrock's old account (dead)]

shrir, are we still seeing this bug?

ssu, didn't we already fix this by a recent Installer check-in by you that
created both a plug-ins and a plugins directory on the Mac? If so, please close.

Note that there's a separate bug 36046, QuickTime movies fail to display, and we
want to make sure we know which bug is at fault if QT isn't running.

Comment 8

[shrirang khanzode]

Yes, Am still seeing this problem. Renaming the folder to 'plugins' makes 
quicktime work fine.2000100910 branch build on mac.

Comment 9

[Samir Gehani]

Eric,
A discussion began about the "Plugins" -> "Plug-ins" folder rename you folks had
requested.  I'm not sure if this discussion completed.  If it did complete and
resulted in us requiring a folder named "Plugins" to also be installed I was not
aware of this decision.  Let me and jj know what the resolution is if mac
installer changes are needed.

Comment 10

[av (gone)]

Changing summary from "Nav4 QuickTime plug-in only works in Moz/N6 on Mac if 
plug-ins directory is named "plugins" ?!" to "installer on Mac must create a 
plugins directory as well as the plug-ins directory". This is the solution to 
the problem we discovered in which most legacy plug-ins on the Mac need to be 
installed into a directory called "plug-ins", but for unknown reasons the 
QuickTime binary only works if it's installed into a directory called "plugins". 
--> sgehani. All this involves is a one-line change to an installer script. 
Extremely low risk and if we don't fix this then N6 RTM won't support QuickTime 
on the Mac. PDT please approve; if you're even thinking of minusing, please call  
ekrock directly.

Comment 11

[Samir Gehani]

Affects both ns and moz browser.xpi install scripts.

Comment 12

[ekrock's old account (dead)]

Samir, making this change won't be a problem, right? Please confirm. Thanks!

Comment 13

[Samir Gehani]

Eric,
No, it won't be a problem.

Comment 14

[Samir Gehani]

Attachment: Patch 1 of 2: mozilla browser.jst diff

Comment 15

[Samir Gehani]

Attachment: Pactah 2 of 2: ns mac browser.jst diff

Comment 16

[Sean Su]

r=ssu

Comment 17

[Brendan Eich [:brendan]]

dveditz gets to give an a= on this -- he has my marker.

/be

Comment 18

[Steve Dagley]

Pardon my french but this is the WRONG @#$%^ WAY!!!!!! TO FIX THE PROBLEM.

I spent about half an hour looking at the problem tonight and I _think_ the 
correct fix is to change nsPluginsDir::nsPluginsDir() in nsPluginsDirMac.cpp to 
properly return the correct leaf name for the plugins folder.  Currently it 
relies on a parameter to the constructor to determine if it should use "Plug-ins" 
or "Plugins" but the proper use should always be "Plug-ins".  I'll look more in 
the AM so please don't do anything silly in the meantime.

Comment 19

[Henrik Gemal]

I'm not a Mac user, BUT:
Why is the Plugin directory called "plugins" on Windows and "Plug-ins" on Mac.
Please, please make it the same!!!!!

Comment 20

[Steve Dagley]

"Plug-ins" is the legacy name of the plugins folder on the Mac and can't be 
changed without breaking all existing plugin installers.  Blindly changing it to 
"Plugins" to match the PC name without considering the aforementioned 
consequences is what got us into this mess in the first place.

Comment 21

[ekrock's old account (dead)]

Henrik -- Steve is correct. Read bug 36089 if you want to understand the 
background. Bottom line: the plug-ins directory on N6/Moz Mac must be named 
"plug-ins" for backward compatibility. Steve now has an improved patch that 
fixes the underlying problem and eliminates the need to have two directories 
("plugins" and "plug-ins").

Changing summary from "installer on Mac must create a plugins directory as well 
as the plug-ins directory" to "must fix plug-in finder code on Mac so it always 
looks in 'plug-ins' directory".

sdagley has a simple, bulletproof, zero-risk fix which simply eliminates an if 
check that we shouldn't have been doing and always returns "plug-ins" as the 
directory on the Mac instead of "plugins" sometimes and "plug-ins" other times 
(which is what was causing the problem).

Comment 22

[Steve Dagley]

See previous comments for reasons but it is never correct to return "Plugins" as 
the name of the plugins folder on the Mac.

Patch below corrects this problem and has been reviewed by [e-mail].  
Seeking super review from scc.


Index: nsPluginsDirMac.cpp
===================================================================
RCS file: /cvsroot/mozilla/modules/plugin/nglsrc/nsPluginsDirMac.cpp,v
retrieving revision 1.13
diff -c -2 -r1.13 nsPluginsDirMac.cpp
*** nsPluginsDirMac.cpp	2000/09/22 00:58:46	1.13
--- nsPluginsDirMac.cpp	2000/10/12 18:36:57
***************
*** 71,78 ****
  	if (NS_SUCCEEDED(mError)) 
    {
!     if(location == PLUGINS_DIR_LOCATION_MAC_OLD)
  		  SetLeafName("Plug-ins");
!     else
! 		  SetLeafName("Plugins");
  	}
  #endif
--- 71,78 ----
  	if (NS_SUCCEEDED(mError)) 
    {
! //    if(location == PLUGINS_DIR_LOCATION_MAC_OLD)
  		  SetLeafName("Plug-ins");
! //    else
! //		  SetLeafName("Plugins");
  	}
  #endif

Comment 23

[Samir Gehani]

Much nicer!

Comment 24

[Steve Dagley]

After discussion with sfraser we decided that we'd leave the door open for future 
plugins being able to live in a "Plugins" directory but fix the current legacy 
plugin code to work from the "Plug-ins" folder.  Below is a new patch, ignore the 
one in my comment from 11:45.  Code has been reviewed by bnesse, waiting on super 
review from either scc or sfraser.

Index: ns4xPlugin.cpp
===================================================================
RCS file: /cvsroot/mozilla/modules/plugin/nglsrc/ns4xPlugin.cpp,v
retrieving revision 1.37
diff -c -2 -r1.37 ns4xPlugin.cpp
*** ns4xPlugin.cpp	2000/09/20 05:44:17	1.37
--- ns4xPlugin.cpp	2000/10/12 21:31:43
***************
*** 322,326 ****
      callbacks.size = sizeof(callbacks);	
  
! 	nsPluginsDir pluginsDir;
  	if(!pluginsDir.Valid())
  		return NS_ERROR_FAILURE;
--- 322,326 ----
      callbacks.size = sizeof(callbacks);	
  
! 	nsPluginsDir pluginsDir(PLUGINS_DIR_LOCATION_MAC_OLD);
  	if(!pluginsDir.Valid())
  		return NS_ERROR_FAILURE;

Comment 25

[Simon Fraser [no longer active]]

sr=sfraser. Please do some heavy testing of this before checking in.

Comment 26

[Steve Dagley]

I just noticed that av wasn't on the cc: list and as module owner for Plugins his 
a= is needed to checkin

Comment 27

[Daniel Veditz [:dveditz]]

So what does this mean for XPInstall? Currently we now install plugins into the 
"Plug-ins" -- is that wrong?

Comment 28

[Steve Dagley]

Installing to "Plug-ins" is the correct for now.  If in the future we decide to 
also allow use of the "Plugins" folder on Mac (see previous comments above on why 
we don't just use that name now) the latest patch allows that possiblity but will 
require additional code to look in both folders for legacy plugins to work.

Comment 29

[selmer (gone)]

rtm++ on assumption second reviewer approves the fix.

Comment 30

[ekrock's old account (dead)]

Am I correct in understanding that the patch will preserve the possibility of 
using a "plugins" folder on the Mac at some time in the future, but that we will 
not create a "plugins" folder within the RTM product? If that's the case, I'm 
fine with this. For now, we will have only a "plug-ins" folder on the Mac and 
will advise plug-in developers to write with that in mind, yes? Thanks!

Comment 31

[Steve Dagley]

ekrock, your assumption is correct.  The latest fix solves the non-functionality 
in the "Plug-ins" case and leaves the possibility that the "Plugins" case could 
be made to work in the future

Comment 32

[Scott Collins]

It looks like you've got your sr= from sfraser and you're waiting for an r= from
av?  The patch looks good to me, and I would give it an sr= _except_ I don't
want to accidentally eliminate the module owner from the review cycle as I
consider that the most important review.  Super-reviewers can look for local
problems, but module owners have the domain knowledge complete enough to find
global wrongs.  If av can't get around to this or thinks the patch is safe
enough to defer judgement to a non-owner, then you can use my review instead.
Adjusting the whiteboard to read "seeking module owner review"

Comment 33

[Steve Dagley]

av has reviwed and approved as module owner

Comment 34

[Kazu Yamamoto]

There is another folder name problems in bug 48872.

> Mozilla/Mac have a folder named "Search Plugins" different from Win/Linux 
platforms.
> On Win/Linux platform, this folder is named "searchplugins". (It does not 
have a black)
> I think Mozilla should  have a same folder name on any platform.
> This problem influence a making XPI file including search plugins.
> We must prepare two XPI files? (for Mac platform and for any platforms)

Please make it the same folder name!

Comment 35

[Daniel Veditz [:dveditz]]

A simple folder difference should not be the cause of creating two XPInstall 
packages for otherwise cross-platform content -- this is a red herring and has 
nothing to do with this bug. First, someone should file an enhancement request 
for a search plugins target for the getFolder() method that will return the 
correct location on all platforms. Secondly, even without that new feature the 
XPInstall script could--as an ugly workaround--simply install those files 
within an if statement that picks two different folder names based on which 
platform you're on.

The Mac folk absolutely insist on "pretty" names in the visible top level 
directory. Windows folks probably don't care, but unix folk tend not to like 
spaces in file or directory names. Impasse.

Comment 36

[Steve Dagley]

Just landed fix on branch (landed on trunk last week)

Comment 37

[shrirang khanzode]

Verified on mac branch build that quicktime plugin is again recognized from 
within the 'Plug-ins' folder. No need to put it in 'plugins'  folder as I had to 
do earlier. Build used 20001018. I hope this is enough to verify this fix. 
Adding keyword: VTRUNK so that this gets verified on the trunk build.

Comment 38

[Jonathan S Abrams]

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

Comment 39

[Jonathan S Abrams]

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

Comment 40

[Jonathan S Abrams]

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

Comment 41

[John Unruh]

Verified on the 10/31 Mac trunk build.