Closed Bug 54778 Opened 24 years ago Closed 24 years ago

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

Categories

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

Product:
Core Graveyard
Component:
Plug-ins
Platform:
PowerPC
Mac System 8.5
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: ekrock, Assigned: sdagley)

References

Details

(Whiteboard: [rtm++] fix checked into trunk, simmering for a day before basting branch)

Attachments

(2 files)

Patch 1 of 2: mozilla browser.jst diff
1021 bytes, patch
Details | Diff | Splinter Review
Pactah 2 of 2: ns mac browser.jst diff
986 bytes, patch
Details | Diff | Splinter Review 
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
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.
Severity: normal → critical
Keywords: 4xp, quicktime, rtm
Priority: P3 → P1
Eric, if your suspicion that we overlooked some internal dependency is true than 
this problem would show up with other plugins too.
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
Peter, Other plugins are not showing this behaviour at all. They are working 
fine.
any news about the veracity of this bug ? Thanks!
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.

Yes, Am still seeing this problem. Renaming the folder to 'plugins' makes 
quicktime work fine.2000100910 branch build on mac.
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.

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.
Assignee: av → sgehani
Summary: Nav4 QuickTime plug-in only works in Moz/N6 on Mac if plug-ins directory is named "plugins" ?! → installer on Mac must create a plugins directory as well as the plug-ins directory
Affects both ns and moz browser.xpi install scripts.
Status: NEW → ASSIGNED
Target Milestone: --- → M19
Whiteboard: [rtm need info]
Samir, making this change won't be a problem, right? Please confirm. Thanks!
Eric,
No, it won't be a problem.

r=ssu
Whiteboard: [rtm need info] → [rtm need info] have reviewed patch; seeking super review
dveditz gets to give an a= on this -- he has my marker.

/be
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.

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!!!!!
"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.
OS: Windows NT → Mac System 8.5
Hardware: PC → Macintosh
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).
Assignee: sgehani → sdagley
Status: ASSIGNED → NEW
Summary: installer on Mac must create a plugins directory as well as the plug-ins directory → must fix plug-in finder code on Mac so it always looks in "plug-ins" directory
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 bnesse@netscape.com.  
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
Status: NEW → ASSIGNED
Much nicer!

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;

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

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

So what does this mean for XPInstall? Currently we now install plugins into the 
"Plug-ins" -- is that wrong?
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.

rtm++ on assumption second reviewer approves the fix.
Whiteboard: [rtm need info] have reviewed patch; seeking super review → [rtm++] have reviewed patch; seeking super review
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!
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

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"
Whiteboard: [rtm++] have reviewed patch; seeking super review → [rtm++] have reviewed patch; seeking module owner review
av has reviwed and approved as module owner
Whiteboard: [rtm++] have reviewed patch; seeking module owner review → [rtm++] fix checked into trunk, simmering for a day before basting branch
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!
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.
Just landed fix on branch (landed on trunk last week)
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Keywords: vtrunk
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.
*** Bug 57809 has been marked as a duplicate of this bug. ***
*** Bug 57809 has been marked as a duplicate of this bug. ***
*** Bug 57809 has been marked as a duplicate of this bug. ***
Verified on the 10/31 Mac trunk build.
Status: RESOLVED → VERIFIED
Keywords: vtrunk
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: