58797 - Preview Image for Skins in Prefs Doesn't Work!

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect, P3)

Description

[Joe Hewitt (gone)]

I recently discovered that user installed themes are copied into a chrome 
directory in the user's profile.  I previously thought they all got copied into 
resource:/chrome/.  With this in mind, it is not possible to know the complete 
url where the theme preview image can be found.  

Currently, in the manifest/confests.rdf file, we put a line like this 

chrome:image="jar:resource:///chrome/sky-pilot.jar!/global/skin/preview.png" 

to indicate where to find this image.  However, I think the installer is going 
to need to generate this url when the theme is installed.  Instead, we should 
say something like this in the rdf:

chrome:image="/global/skin/preview.png"

and the installer should expand that when it generates all-skins.rdf, to 
something like this:

chrome:image="jar:file:///C:/DOCUMENTS%20AND%20SETTINGS/ME/APPLICATION%
20DATA/Mozilla/Users50/Joe/chrome/skypilot.jar!/global/skin/preview.png"

Comment 1

[Daniel Veditz [:dveditz]]

arg, this sucks, storing file: URLs is way bad on a Mac.

Would there be some way to create magic resource: URLs that are not necessarily 
taken to be a subdirectory of bin? I don't know what syntax is valid in 
the resource: protocol, but something along the lines of 
resource:[profile]/chrome/etc. where I'm assuming [] isn't particularly valid 
at that point otherwise. Warren: could you chime in with something that would 
work here?

Comment 2

[Daniel Veditz [:dveditz]]

Reasssigning to me.

Not likely to be accepted for fixing in RTM. Might require a change in Necko 
and/or the chrome registry. On the other hand not fixing this makes the theme 
switching UI pretty poor.

Comment 3

[Daniel Veditz [:dveditz]]

Reasigning to hyatt to make chrome:image relative, will open a new bug about
needing a resource: URL pointer to the current profile. The latter could wait
for 6.5, the relative image path needs to happen for RTM if we want themes not
to suck

Comment 4

[David Hyatt]

Attachment: Patch to turn relative URL into absolute URL upon install

Comment 5

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

a=[e-mail]

Comment 6

[Daniel Veditz [:dveditz]]

r=dveditz

Looks like you made the code work for both the old and new styles. This means 
you didn't have to update our skin contents.rdf files in the build which makes 
the patch look smaller (while adding an extra "if" line), but this has two 
drawbacks. 1) our own skin does not model what we want everyone else to do, and 
2) our own skins still exhibit this bug (don't display the image) if you 
disable jar packaging since we still have a non-relative jar: URL in the 
contents.rdf

I think "safe skins" still have to be restructured a bit from our shipping 
skins anyway (my code still looks only for a top-level manifest.rdf :-() so we 
can just document that relative image paths should be used. We can fix our own 
skins on the trunk.

Comment 7

[David Hyatt]

Changing the summary to make it sound appropriately scary.

Comment 8

[Todd Pringle]

We really need users to be able to preview skins before they are installed.
There could very reasonably be 50 to 100 themes on the theme park between now
and 6.5 and it would really suck if people had to guess (or remember) which
theme is which before applying. Can we put this on the limbo list?

Comment 9

[David Hyatt]

If this is refused, we need to at the very least patch the XUL to remove the
useless big blank black spot that will be sitting in the middle of the themes
dialog. :)

Please take it, PDT.  I'll do your laundry.  I'll walk your dog.  I'll cook you
dinner.

Comment 10

[Phil Peterson]

Do you guys have testing on all platforms?

Comment 11

[David Hyatt]

I've tested Win and Linux.

Comment 12

[selmer (gone)]

rtm-, not a ship stopper.

Comment 13

[David Hyatt]

Ok, guess this is fixed then.

Comment 14

[David Hyatt]

German, Todd.  We should market the preview image URL as a "link to a preview
image on the skin author's Web site".  If we make it an absolute URL that points
off to a Web site, then this will still work in 6.0.

Including the preview image with the skin, though, will now be a no-no.
Presumably Theme Park can host the preview images, since they will serve them up
as well anyway.

Comment 15

[Grace Bush]

changing QA contact

Comment 16

[Jimmy Lee]

Reassinging QA Contact.  This bug is more about themes than installation.  I did 
take a look at this, but I cannot figure out how preview works.  I was told 
there are some bugs related to this.

Comment 17

[Patty Mac]

After speaking with David Hyatt, this bug is partially fixed. Someone needs
to link the preview image on the skin author's web site.

Comment 18

[Daniel Veditz [:dveditz]]

You should also be able to use a relative URL to an image in the file. Simply 
give a partial URL path without a protocol and the base URL of the archive will 
be prepended. We should edit modern and classic to do this as well as models, 
and Ian should document it.

Comment 19

[Todd Pringle]

I'm not sure I understand the solution here - does it require theme authors to
host a preview image on their site?  If so, I'm not sure if this is the best
solution.  

Comment 20

[Daniel Veditz [:dveditz]]

No, a site link is an option that has *always* worked; that was the WORKAROUND 
in 6.0. What this bugfix allows is for the skin to specify a path 
relative to the archive (detected by having no ":" in it) so the image can be 
taken from the skin itself. This needs to be documented and tested.

Comment 21

[Todd Pringle]

Gotcha.  So in other words, we need to get this info out to theme developers so
that they can build their themes in the way that takes advantage of the fix?  In
any event, Patty I don't think we should mark this fixed until we can test that
it works.  Paul, can your team take one of our existing themes and modify it so
that it should work, and then Patty can take it and test this out?