54291 - Need Alternative Location to Load Java Plugins

Status: VERIFIED FIXED

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

Description

[Joe Chou]

In order to provide the flexibility of from where the Java plugins is loaded,
and for what version of the browser, an alternative Path to the new location of
the Java plugins is needed. The new path, rather than the current system default
("../mozilla/dist/bin/plugins"), will be set in an environment variable. When t
looking for the location to load the Java plugins, the environment variable is
checked first. If it is set, then the plugins will be loaded from the
alternative location. Otherwise, the system default will be used.

Comment 1

[Joe Chou]

Sun requires that mozilla on Unix provide some way of specifying at
  runtime an additional location to be used for searching for plugins.
  This is basically the "NPX_PLUGIN_PATH feature", if you remember it
  from 4.x on Unix.  This feature defined the name of an environment
  variable that the user can set.  Netscape would interpret the value of 
  this environment variable as a filesystem path and would search this
  path for plugins.

  What Sun would like is: On Unix only, Mozilla looks for the variable
  NETSCAPE60_PLUGIN_PATH in the user's environment.  If set, Mozilla
  will search this directory first before searching the standard plugins 
  directory.  If not set, the plugin search algorithm is unchanged.

  It is VERY IMPORTANT that there be some version information in name of 
  the environment variable, as above.

Comment 2

[Joe Chou]

Attachment: Changed source file

Comment 3

[Joe Chou]

Attachment: Code changes (output of cvs diff -u)

Comment 4

[Joe Chou]

Attachment: Code changes (output of "cvs diff")

Comment 5

[Joe Chou]

Attachment: binary in tar.gz

Comment 6

[Joe Chou]

Attachment: 2nd submit, with corrections. (output of "cvs -n update")

Comment 7

[Joe Chou]

Attachment: Code changes (output of "cvs diff -u")

Comment 8

[Joe Chou]

Attachment: (output of "cvs diff")

Comment 9

[Joe Chou]

Attachment: Tar file of changed code

Comment 10

[edburns]

Hi Joe,

Here are my comments for your changes:

RCS file: /cvsroot/mozilla/configure,v

  This doesn't look like something related this bug.  Please don't check
  it in' and don't include un-related things in your bug diffs.

RCS file: /cvsroot/mozilla/modules/plugin/nglsrc/nsPluginsDirUNIX.cpp,v

  Use PR_GetEnv() instead of libc's getenv().

  You don't need to use sprintf, you can still use the overloaded
  operator += in nsSpecialSystemDirectory:

  Instead of 

+	sprintf(buf, "%s%s", sysdir.GetCString(), PLUGIN_DIR);
+        pluginsDir = (const char *)buf;

  you could do

   sysdir += "plugins"; 
   pluginsDir = sysdir.GetCString(); // native path

A good rule of thumb is: NEVER use standard c functions.  There are
always mozilla equivalents that exist for a reason (usually to fix some
cross-platform compatability problem).

Please try my suggetions and resubmit your diff.  This time you can just
do cvs diff -u.  Don't worry about the other formats.

Ed

Comment 11

[Phil Peterson]

PDT marking [rtm need info]. Please get a code review from Ed, and then one from
the mozilla.org reviewers list (http://www.mozilla.org/hacking/reviewers.html)
and then change the whiteboard to [rtm+] in order to get approval to check in to
the Netscape 6 RTM branch (given by [rtm++])

Comment 12

[Joe Chou]

Attachment: Latest changed source code file

Comment 13

[edburns]

I have reviewed Joe's patch.  r=edburns.

Comment 14

[edburns]

Let brendan put in rtm+

Comment 15

[Brendan Eich [:brendan]]

I'm looking at nsPluginsDirUNIX.cpp (btw, Unix is spelled that way, not as if it
were an acronym -- please fix this name).  I see tabs and two-space-equivalent
indentation.  The Emacs modeline says

/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-

which means 4-space equivalent tabs (tab key gesture adds up to four spaces, to
go to the next 1 mod 4 column), no tab chars in the file, and 4-space-equivalent
indentation per statement level.  If you want two-space indentation, change the
first and third params to 2.  Don't change the second param, but do expand all
tabs to the correct modulus.

Hmm, now I see four-space indentation, probably waterson code.  And I see
varying brace style, not a bug to fuss about in itself, but a sign of no
ownership.  I'd like to see someone make this file look better before giving a=.

These changes need r= too, before a= and [rtm+].  I'm cc'ing waterson and scc
for more code review.

/be

Comment 16

[Joe Chou]

Attachment: Indentation and spacing changes in nsPluginsDirUNIX.cpp

Comment 17

[Joe Chou]

Attachment: tar.gz of changed source file and Makefile

Comment 18

[Joe Chou]

The source file has been changed to nsPluginsDirUnix.cpp, as requested, and the
make file has also been updated. 

Ed,
Please review the modified files (nsPluginsDirUnix.cpp and Makefile). Thanks.

Comment 19

[edburns]

Joe, you need to modify Makefile.in, not Makefile.  Makefile.in is used to 
generate the Makefile during your run of ./configure.

Comment 20

[edburns]

I've looked at your modified file.  Looks Good.  r=edburns

Make sure you do cvs remove of nsPluginsDirUNIX.cpp before doing the commit.  
I'll step you through the process when it comes time.

Comment 21

[Joe Chou]

Attachment: tar.gz for Makefile.in and nsPluginsDirUnix.cpp

Comment 22

[Joe Chou]

Attachment: Re-attach again.

Comment 23

[Joe Chou]

Attachment: Attach with the right file type (Binary).

Comment 24

[edburns]

Attachment: cvs diff -u of nsPluginsDirUNIX.cpp before reformatting.

Comment 25

[edburns]

From Brendan's email:

 It
doesn't look leaky to me, given how PR_GetEnv and GetCString work.  Probably
scc should bless it too, including the wierd selection of the right
assignment operator (*(nsFileSpec*)this = pluginsDir;).

So a=[e-mail], contingent on scc agreeing.

/be

Comment 26

[Scott Collins]

This is not the right way to pick the assignment operator you care about.  This
code is not obvious to anybody who isn't a C++ expert.  The right way to say this is

  nsFileSpec::operator=(pluginsDir);
    // use |nsFileSpec|s assignment operator to set the path, would be better
    //  if there was an, e.g., |nsFileSpec::SetPath( const char* )|.

or if you feel better about it

  this->nsFileSpec::operator=(pluginsDir);

I know we've talked about this before, Ed :-)  I'll break you of this habit yet!

I guess my main question is: why doesn't this use |nsIFile| and
|NS_GetSpecialDirectory| instead of the deprectated |nsFileSpec| scheme?

Comment 27

[edburns]

Hi Scott, 

Thanks for your review.

You said:

I guess my main question is: why doesn't this use |nsIFile| and
|NS_GetSpecialDirectory| instead of the deprectated |nsFileSpec| scheme?

The answer, the engineer that wrote it is new to mozilla.  If he fixed the 
assignment operator thing, would you bless it as is, without making the nsIFile 
change?

Comment 28

[edburns]

Scott, I changed the assignment as you suggested.  About using nsIFile instead, 
that would be waaaaaay to destabilizing at this point.  I agree we should do it 
in the future, but the way this is architected, we can't do it now.

You see, we'd have to change it in nsPluginsDirWin.cpp as well.  Since we're 
trying hard to get this into RTM, we want to minimize risk.

Please let me know if I have r= so I can put this up for [rtm+].

Thanks,

Ed

Comment 29

[edburns]

Hi Brendan,

I've done as scc suggested regarding the nsPluginsDir assignment operator, but 
I think the answer to his question about "why not use nsIFile?" is: too de-
stabalizing.  If you agree, can I mark this [rtm+]?
Ed

Comment 30

[edburns]

Attachment: scc requested this file. There's nothing to diff it with, since this is a new file.

Comment 31

[Scott Collins]

If you file another bug to update |nsFileSpec| to |nsIFile| and use the
directory service as mentioned in my earlier review ... then I can accept this
patch as an immediate fix: sr=scc.

Comment 32

[edburns]

New bug is bug 56154.

Comment 33

[edburns]

Since we have a=brendan and r=edburns and sr=scc, I'm marking this [rtm+]

Risk: 0, this is a small enhancement, if the env var NS600_PLUGIN_PATH is NOT 
set, the behavior is the same before this bug is fixed.  If it is set, the 
behavior varies in that the plugin search location is changed.

Gain: this allows users that do not have root access to use Java by installing 
the java plugin into a non-root location and telling mozilla to point to that 
location.

Comment 34

[Phil Peterson]

Since no crash or data loss or must-fix bug is involved, this doesn't meet the
higher bar we have for branch checkins this week. Of course, you're welcome to
go in on the trunk, or on a Solaris-specific branch off the Netscape branch. [rtm-]

Comment 35

[Ben Bucksch (:BenB)]

Please change the name of the env var to MOZSOMETHING instead of NSSOMETHING.

Comment 36

[edburns]

Fix checked in to the trunk, won't be in RTM.

Comment 37

[Margaret Chan]

I have checked in the fix to the OEM branch.

Comment 38

[shrirang khanzode]

verified(rubberstamp)