53218 - Theme park can't be used to install skins with the current chrome registry

Status: VERIFIED FIXED

(Core Graveyard :: RDF, defect, P2)

Description

[Pete Collins (MDG)]

here you go Dave, 

Ok, I did find a bug with this... if you describe only a skin or
locale, and that skin or locale describes multiple packages (e.g., the way the
manifests for modern.jar, classic.jar, etc. are described right now), then I
don't append the package name.  I'll need a bug on this stat.  :)
"contents.rdf doesn't work for skins/locales with multiple packages"

Comment 1

[Peter Trudelle]

nsbeta3+, p2, required for jar packaging to land.

Comment 2

[Phil Peterson]

Marking nsbeta3- because (a) jar packaging has landed and (b) when we all met
about jar packaging, we agreed that multiple packages in a jar file could wait
until after seamonkey. So this sounds like feature creep.

Comment 3

[David Hyatt]

RTM.  Skin installation is impossible if this isn't fixed.

Comment 4

[Peter Trudelle]

cc'ing marketing for input, how important is installing themes from Theme Park?

Comment 5

[german]

whoa. Themepark wont work then I guess. Everybody who is trying to use a newer
direct strucure (without extra 'skin' folder underneath each component like this
is hosed:

theme.jar
|
contents.rdf
-----------------------------------------
|         |         |         |          |
navigator   mail  communicator global    whatever
|         |         |         |          |
skin files...

In other words people cannot model a theme structure much like we have in our
own modern.jar and classic.jar (this is oversimplified but you get the idea).
Please rtm++ this. Or will see very few themes on the themepark...

Comment 6

[german]

To be more precise, a 'friendly' theme install through a ThemePark javasScript
which has a 'installChrome' will not install directly from a ThemePark page.
The ThemePark will want to use those 'friendly' installs for 2 reasons:
- to avoid the .xpi installer that may contain harmful XPConnect stuff (and will
throw up correponding dialogs on install)
- to benefit from the functionality that lets users switch to the new skin on
the fly right after the install

Comment 7

[Rafael Ebron (:rebron)]

Ian, Dan, can you guys comment on this bug?
It's important that chrome install works.

Comment 8

[Peter Trudelle]

marking rtm need info, based on marketing's reaction - "WOOW!  That is very very
bad."

Comment 9

[johng]

Explanation behind "WOOW"

*Safe* skins are essential to our success with themes, and we can't have safe
skins without this.  Safe skins ...

> Make it easier for 3rd parties to create and install a theme (using javascript
to add a theme), is easier for our theme park to develope a true directory of
skins, and is much easier for users to install.

> If you have to use xpi, then it is harder for developers, distributors,
Netscape, and for users.

> If you have to use xpi, themes don't have to be safe, so users who want to try
themes will sooner or later get screwed.  Press will hit about the danger
(stability, privacy, security, or whatever) of skins, and our whole skins/themes
push will self-destruct and reduce people's confidence in the safety of Netscape 6.

Therefore, I stick with my "WOOW, this is very, very bad" and have gotten
similar feedback from others supporting that pov.

Comment 10

[german]

what john says!

Comment 11

[pete collins]

XPI is for full package/software installation. To my knowledge there is no way
to do safe skin installs, nor has there been. We talked about this stuff way
back at the last moz dev meeting. Yes, a skin only *safe* install would be nice.

But then again we knew this "n" months ago right?
I will agree w/ johng and add my "WOOW" into the mix as well . . .

;-)

--pete

Comment 12

[Daniel Veditz [:dveditz]]

Pete: there *are* safe skin installs, have been since PR2. If you zip up a skin 
in the old directory structure at least, and include a manifest.rdf at the top 
as we used to, then you can put

InstallTrigger.installChrome(InstallTrigger.SKIN, "url", "modern/1.0");

to cause the skin to be downloaded, installed into your profile dir, and 
"neutered" of unsafe XBL content. "url" can be a full url to the .jar, or a 
path relative to the triggering page.

The final arg is used to select the skin, and to tell the user which skin is 
being downloaded (maybe we need a fourth arg for a "pretty name"). It is 
implicitly designed to handle only a single skin, not a skin "pack". As long as 
we document the structure that *does* work this doesn't necessarily have to be 
a stop-ship bug, although it would be a bummer that people can't directly copy 
our existing skins without some minor restructuring.

Comment 13

[John Dobbins]

If you are even thinking about not fixing this, you will be shooting yourself in 
the foot.

First Netscape holds a Theme Contest, based on PR2 without bothering to tell the 
 people developing them that code changes might occur.

Then just before the contest ends a post appears on the themes newsgroup telling 
them thier themes aren't going to work in PR3, and recomended they redo them in 
jar format. That got a lot of people very ****.

Now after many of them have updated thier themes to jar, on the advice of Joe 
Hewitt, they may have to change back to xpi.

If this isn't fixed, these people will get the impression that Netscape dosen't 
know whats it's doing, and dosen't give a damn about them.

Comment 14

[andreww]

If this doesnt get fixed, then only old-style skins will be "safely
downloadable" - the ones based on blue and old-style classic.  These skins look
like utter junk on pr3+ because of XUL changes.  We need to be able to allow
skin designers the ability to base skins on the new - style skins without
forcing them to radically change the directory structure.  With this fix, it's
much more feasable to make a "safe" skin from new modern or new classic (the
classic that has tracked along with the changes).

Plus as new skins are developed, they will be invariably from what folks find,
which is modern.jar and classic.jar which dont have that additional "skin"
subdirectory which old style skins do, and which the "safe" install.js process
looks for.

This bug is low risk and very high return - we will pay dearly for this in the
months to come if we dont fix this now...

Comment 15

[andreww]

Just another quick comment - even if we were to document the "safe" way to do a
skin - we still will be basically saying "do as I say and not as I do"...

Comment 16

[Peter Trudelle]

We very much want to fix this, but it is lower priority than Dave's other bugs,
which are serious functional defects, and despite the rumors, he is only human.
 Adding helpwanted keyword, if anyone can help Dave with anything (bugfixes,
reviews, testing, laundry, yardwork,..;-), it might make a big difference. cc jrgm

Comment 17

[Pete Collins (MDG)]

Dan, as far as i understand it, you still need an install.js file right?
So what is to stop someone from saying that the jar is a skin and doing
something like:

addFile("My Cool Skin", "virus.exe", getFolder("Chrome"), "");

*Safe* is only a matter of how install.js is used right?

Also how do i see the available API's for the InstallTrigger object?
I tried:

for(var list in InstallTrigger)
    dump(list+"\n");

But i get nothing . . .

Comment 18

[Rafael Ebron (:rebron)]

A safe skin install does not use an install.js.  You need a manifest.rdf file
and the javascript trigger i.e. nstallTrigger.installChrome(InstallTrigger.SKIN,
"url", "modern/1.0");

Comment 19

[Ben Bucksch (:BenB)]

Can somebody please explain the problem detailed on a code-level or is that unknown?

Comment 20

[David Hyatt]

It's well-known.  The chrome registry needs some love.

Comment 21

[David Hyatt]

Attachment: Patch to fix the contents.rdf install process.

Comment 22

[David Hyatt]

Please someone/anyone give me an r= and ben, give me the a=.  This is a no-
brainer patch.  The code just says "If I'm a skin or locale that applies to 
more than one package, then append the package name when setting up the base 
URL for each package handled by the skin.

Comment 23

[Ben Goodger (use ben at mozilla dot org for email)]

a=[e-mail]. 

Comment 24

[lchiang]

Let's try to get an r so we can get this checked in with the limbo bugs on Monday!

Comment 25

[Chris Waterson]

- Is it the case that (doAppendPacakge == true) => (appendProviderName == true)?
If not, mightn't we end up appending the package without the provider?

- Why do you have to look at the count in the Seq? Couldn't you just use one of
the locals you've already computed (e.g., "skinCount" or "localeCount")? (This
might make the code a bit more consistent...)

Comment 26

[David Hyatt]

No, appendProviderName is not always true in this case.  In fact, the desired
structure in this case is

modern.jar
  contents.rdf
  navigator
    <files>
  communicator
    <files>

I only need to append the provider name if the contents.rdf file talked about
multiple skins.  Even a single skin can skin multiple packages, and in this case
you want to append package names but not provider names.

The perProviderPackageCount is distinct from any previously computed values.
SkinCount is how many skins contents.rdf talks about (e.g., modern, classic,
kiw, etc.).  PerProviderPackageCount is how many packages a particular skin
talks about, e.g., how many packages does modern skin.

Comment 27

[Stuart Parmenter]

r=pavlov

Comment 28

[selmer (gone)]

This bug is in candidate limbo.  We will reconsider this fix once we have a 
candidate in hand, but we can't take this fix before then.  Please check into 
the trunk ASAP.

Comment 29

[german]

tested hyatts fix using a mac mozilla trunk build and a theme.jar in the newer,
simpler format and it worked great.

Comment 30

[german]

come on lets go all the way and put it into rtm! 

otherwise we have this new much simpler themes folder format only in the next 

netscape release after 6.0. 

Then- what incentive does a theme developer have to adapt to that for 6.5 and 

later only? Lets for once get out act together and present a consistent story to 

theme developers. Thanks.

Comment 31

[Phil Peterson]

German, by putting this bug into the limbo state, we indicate our desire to fix
it as soon as the current limbo fixes are all verified. So we're bought into the
severity of the bug. 

Comment 32

[Daniel Veditz [:dveditz]]

An alternative solution, or perhaps one we should pursue in addition, would be 
to smarten up the chrome installer to handle the new structure. Right now I 
simple-mindedly (but correct at the time it was written) assume there's a 
manifest.rdf at the top of the chrome .jar. The code could be changed to 
enumerate the archive and register any contents.rdf it finds instead.

This would not be a small change, but would be straight-forward and easily 
testable. Skins would not be any less safe.

Comment 33

[selmer (gone)]

rtm++, please checkin ASAP so we can build today.

Comment 34

[Joe Hewitt (gone)]

Fixed on branch.

Comment 35

[Joe Hewitt (gone)]

Apologies, I marked the wrong bug fixed.

Comment 36

[David Hyatt]

Fixed.

Comment 37

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Build 2000-11-03-09.

I tried to verify this by installing themes from http://gooey/client/themes/

On Linux the first theme seemed to install ok, but I could no longer change my
themes because prefs was horked. So I tried installing the second theme: the app
hung.

On NT I installed the second theme first, then the first theme. I then went to
prefs, and noticed that the Classic theme was missing from the list. I was able
to switch back to modern, though.

Are these knows problems or should I file bugs?

Comment 38

[lchiang]

qa contact to patty to verify for themes

Comment 39

[Patty Mac]

Work fine on all platforms, marking verified (2000-11-06-09-MN6).

Comment 40

[Patty Mac]

Marking verified on all platforms (2000-11-28-08-Mtrunk).