93140 - Make content pack installer script platform independent

Status: VERIFIED FIXED

(Core :: Internationalization: Localization, defect, P2)

Description

[Yuying Long]

Build: 07-26 branch Mac build / Mac9.0-Ja 

Steps to reproduce:
1. Launch the US browser, View | Set Language/Region | Download More.2. Choose
Japanese download pack and install it.
3. Exit browser and restart it, pick up JP Region and re-launch again.
4. Go Edit | Preferences | Navigator | Internet Search.

Result:
The Search engine doesn't including Netscape-Japan search.  
If you check the file/folders under that build, you will find a "searchplugins"
folder has been added, Netscape-Japan files are there.  
And "Search Plugins" doesn't change.

Comment 1

[Ying-Lin Xia]

This is for US build, Mac only, when you download the JP region pack, the 
NetscapeSearchJapan will be copied into a new folder "searchplugins", but 
actually the search engine should goto folder "Search Plugins" on Mac.

Think it's i18n issue. Should reassign to ftang.

Comment 2

[Andreas Becker]

mass change, switching qa contact from jonrubin to ruixu.

Comment 3

[rchen]

MAC -> ying. 

Comment 4

[tao]

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

Comment 5

[Ying-Lin Xia]

Reassign to Frank for i18n fix.

Comment 6

[tao]

I'll take this, then. 

Comment 7

[tao]

reassign to new owner ->dbragg

Comment 8

[Ying-Lin Xia]

The following patch makes the folder name same as Win32, "searchplugins" 
instead of "Search Plugins". It should fix this problem.

Comment 9

[Ying-Lin Xia]

Attachment: Patch to make the folder name same as Win32

Comment 10

[dbragg]

So, if I understand the problem correctly, the Mozilla build scripts were
putting the english search plugins files in the Search Plugins folder but the
build scripts for the JA language pack is putting them in the searchplugins
folder. If all the other language pack build scripts are also using the
searchplugins folder then this patch looks good to me.  

Can you verify that the other languages build scripts are doing the same thing
as JA?

Comment 11

[Ying-Lin Xia]

Actually the US region pack from Netscape ftp is Win32 file, which means the 
search plugin folder name is "searchplugins", all other languages should be 
same.

Comment 12

[Ying-Lin Xia]

Also, the commercial tree should be changed in the same way.

Comment 13

[tao]

CC Simon & JJ who might care about this change. 

Don - please get review from sfraser

Comment 14

[Simon Fraser [no longer active]]

I care about this change. Unlike users on other platforms, Mac users actually see 
the contents of application directories, and use what they see as an idicator of 
the quality, and "Mac-savvyness" of an application. For this reason, we try to 
stick with use-friendly directory names, and thus diverge from the other 
platforms in some cases. I think changing "Search Plugins" to "searchplugins" is 
a backward step. I'd like to see a more platform-sympathetic solution.

Comment 15

[tao]

CC dveditz

Ying - would you consult with Dan on the installer script? I suspect you can
designate the target directory name in the script.

Comment 16

[Ying-Lin Xia]

Attachment: Just an example, not a patch.

Then, this bug could be fixed by modifying the Win32 install.js like this. With
this kind of change, we don't need to change either Mac nor Win32 regus.xpi
packaging, only the Win32 install.js.

Comment 17

[dbragg]

Since this is now cross-platform we don't need a Mac-specific regus.xpi right? 
So this should no longer be referred to as the "win32" regus.xpi. regus.xpi
should be the same whether the user is downloading the full install or just the
xpi file.

Comment 18

[tao]

ying - does this sample script address the "bin" v.s. "viewer" discrepency between
Mac and other two platforms? Also, you might want to get sr from dveditz to
ensure your changes in the installer script cover all platform-specific
functions/code segments.

Comment 19

[Ying-Lin Xia]

Attachment: A cross-platform install.js.

To make regus.xpi really cross-platform, we need to make the install.js as
above for all platforms. Plus, the packaging for regus.xpi should be same on
all platforms, ie, the file structure is same for all platforms:
   bin
    +-- chrome
    +-- defaults
    +-- searchplugins
With this new install.js, on Mac, the search plugin files will be still
installed into the old folder name "Search Plugins".

Comment 20

[Ying-Lin Xia]

This change will also benefit our localization process. One regxx.xpi file will 
be good for all, we do not need to rebuild it for different platforms.

If this change is good, we should consider to make same changes for langenus.xpi 
and deflenus.xpi.

Comment 21

[Ying-Lin Xia]

FYI. I use this new install.js re-created regus.xpi using Win32 files, and 
tested on Windows/MacOS/Liunx, everything works fine.

Comment 22

[Jimmy Uen]

Ying, Don - Since there is resolution for this bug can we have someone check-in
the fix so that we can close this bug?

Comment 23

[Ying-Lin Xia]

Attachment: Patch: a simple version just make the win32 xpi installable on mac

Instead of making the js cross-platform, I just made a simple patch here, for
Windows install.js only, which can fix the original reported problem:
"On Mac, when user install new region packs from menu 'View/Set region/Download
more', the search engines will go to a new folder 'searchplugins', not 'Search
Plugins', thus, user can not use the new search engine on the sidebar. This is
because the online-installable region packs on the web are win32 files, does
not support Mac different 'Search Plugins' folder."

Comment 24

[tao]

Hi, Ying:


We one for ns/ tree as well. Also, I believe this patch would work for Linux as
well. 

Don - would you help Ying to provide the patch for ns/ and linux? thx!

Comment 25

[dbragg]

r=dbragg for the 2/06/02 patch.

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 68234 [details] [diff] [review]
Patch: a simple version just make the win32 xpi installable on mac

>+var platformNode;
 ....
>+platformNode = getPlatform();

Why not combine these two into "var platformNode = getPlatform();" ?


>+var fSearchPlugins = "searchplugins";

elsewhere in the script the 'f' prefix appears to mean a folder
object; this is just a string.


>+logComment("initInstall: platformNode=" + platformNode);
>+if (platformNode == 'mac')
>+{
>+  fSearchPlugins = "Search Plugins";
>+}
>+logComment("initInstall: fSearchPlugins=" + fSearchPlugins);

"initInstall" as part of these comments doesn't make a lot of sense.

Does this development oriented debugging info belong in the
final script, where they end up in the user's [un]install log?
Most of the rest of them are pseudo-justified on the basis of
tech support, but surely once you've debugged your script the
user will know what platform they're on.

>+  fComponent = getFolder("Program", "chrome");

This would be much better as getFolder("Chrome");

Here's an idea, instead of having to add a new addDirectory() call
for each bin subdirectory, and having to add more if ever more stuff
is added, why not leave most everything and instead change the
package lists (mozilla/xpinstall/packager/packages-*) to put
the searchplugins stuff in a different (non-bin) directory.

so packages-win would be
- bin\searchplugins\*
+ bin\searchplugins\*,searchplugins

and similar for the other package lists (be sure to do the
commercial tree as well).

Now you leave the "bin" addDirectory alone and add a single
  err = addDirectory("",
		     "searchplugins",  // where you've renamed it
		     getFolder("Program"), 
		     fSearchPlugins); // subdirectory to create

I guess if you're only updating the windows version of the script
you should only update the windows package lists, although I'm not
fond of these scripts starting to diverge.

Comment 27

[dbragg]

Thanks for the review comments Dan.

I didn't know you could do that with the packager files.  I looked around on
mozilla.org.  Is this documented somewhere?  

I thought changing the location of the searchplugin files would require more
changes to the build system.  For this reason I suggested to Ying that he make
the changes exclusively in the install.js file to minimize impact.  That's why
there are individual calls to addDirectory.

As for the other things like the f prefix and the logComment, I didn't see it as
a big issue but I'll change them with the new patch.

Comment 28

[dbragg]

Attachment: New patch implementing Dan's comments

Changed to match Dan's comments.  searchplugins should only be installed if
there is access to the program dir.  They don't work in the Profile tree.  This
was added as a logComment per Tao's suggestion.

Comment 29

[dbragg]

Attachment: NS version of above patch

Comment 30

[tao]

For what it worths, a few MLP contributors had been distributing repackaged 
regXX.xpi (w/ modified install.js) like what Dan suggested here at least a year 
back. Dan's endorsement is certainly a vote of confidence.

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 69494 [details] [diff] [review]
New patch implementing Dan's comments

>Index: packages-win

You need to do packages-static-win as well, otherwise the static build
installer will be broken.

>Index: windows/regus.jst

Looks great!

You need an r=, sounds like Tao has done it so he just needs to
note that in the bug.

Fix the static build thing and sr=dveditz for this patch.

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 69496 [details] [diff] [review]
NS version of above patch

> -bin\searchplugins\bugzilla.gif
> -bin\searchplugins\bugzilla.src
> -bin\searchplugins\dmoz.gif
> -bin\searchplugins\dmoz.src
> -bin\searchplugins\google.gif
> -bin\searchplugins\google.src
> -bin\searchplugins\lxrmozilla.gif
> -bin\searchplugins\lxrmozilla.src
> -bin\searchplugins\mozilla.gif
> -bin\searchplugins\mozilla.src

Here's a wrinkle: the lines starting with - are deleting individual
Mozilla search plugins we don't want to ship in commercial, but because
of this patch those files aren't at that location anymore.

Change the above lines from -bin\searchplugins\foo to -searchplugins\foo

Make the above changes, get an r= and I don't need to see an updated patch.

sr=dveditz

Comment 33

[tao]

Comment on attachment 69494 [details] [diff] [review]
New patch implementing Dan's comments

r=tao

Comment 34

[tao]

Comment on attachment 69496 [details] [diff] [review]
NS version of above patch

r=tao. Don - please log bugs
to sync up linux and mac's
as well -thx!

Comment 35

[dbragg]

Fix checked in.

Comment 36

[Daniel Veditz [:dveditz]]

Didn't actually work because of a broken pkgcp.pl (bug 125962). This one blocks
bug 125820 (which has an alternate fix). Since their descriptions are so
different they should probably both be open until it's fixed.

Comment 37

[dbragg]

Fixed when patch for bug 125820 was checked in.

Comment 38

[Jimmy Uen]

Verified with build 2002040403 trunk on all platforms, Win32, Linux and Mac