297433 - On Mac OS X preview.png and icon.png in Firefox's classic.jar are not available when using the jar protocol

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Robert Strong (they/them - no direct email)]

I believe this is due to the way the icon.png and preview.png are packaged via
the build script. My reason for believing this is that I am able to extract
classic.jar, zip it with the exact same structure using zip, replace the
original classic.jar with this one, and I am then able to access the icon.png
and preview.png using the jar protocol.

To reproduce try accessing jar:resource:///chrome/classic.jar!/icon.png with
Firefox on a Mac OS X system.

Comment 1

[Robert Strong (they/them - no direct email)]

I forgot to mention that other images within the original jar can be accessed
using jar:file:/// for example while the icon.png and preview.png can not be
accessed.

jar:file:///path to classic.jar/classic.jar!skin/classic/browser/Go.png

Comment 2

[Robert Strong (they/them - no direct email)]

jar:file:///path to classic.jar/classic.jar!skin/classic/browser/Go.png
should be
jar:file:///path to classic.jar/classic.jar!/skin/classic/browser/Go.png

Comment 3

[Benjamin Smedberg]

Is classic.jar supposed to have a preview.png and icon.png?

Comment 4

[Robert Strong (they/them - no direct email)]

(In reply to comment #3)
> Is classic.jar supposed to have a preview.png and icon.png?
Yes and it does. This is rather strange since all I had to do to fix this is
unzip and zip again to be able to access the icon.png and preview.png.

The code doesn't extract the icon.png and preview.png from classic.jar since the
main reason for doing so is to not have a zipreader open in order to be able to
uninstall a theme which doesn't apply to classic.jar.

Comment 5

[Robert Strong (they/them - no direct email)]

A little more investigation. Using a fresh install of Deer Park and enumerating
the entries in classic.jar showed the entry name for icon.png and preview.png to
be prepended with a ./ (e.g. ./icon.png and ./preview.png) while doing the same
thing on Win32 showed them correctly as just icon.png and preview.png. Also, the
jar.mn for both pinstripe and winstripe have the following at the bottom:
  ./preview.png
  ./icon.png

So, it appears that these paths are being used for the paths inside the jar by
the compression utility when building on Mac OS X.

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: patch for browser

This patch should fix this issue for Mac OS X though I am unable to verify
since I don't own a Mac. :/

If this is acceptable I will create patches for the other products affected by
this but I would prefer to land this one first in order to verify it fixes this
issue on Mac OS X. What is preferable... one patch for all products or separate
patches?

Side note: when I first noticed this a couple of months ago I thought there
wasn't a preview.png for the Mac. It came to light this was failing after the
patch for bug 296578 landed since it no longer uses the fallsback image in the
case of the default theme. If we want to support using a theme supplied
fallback image for the default theme we would have to read the zip and since
this gets called multiple times I'd prefer just making it a requirement that
the default theme supplies an icon.png, etc.

Comment 7

[mmc]

Please note the messed up brackets:

-        ./icon.png
-        ./preview.png
+        icon.png                                                (icon.png)
+        preview.png                                             {preview.png)

Comment 8

[James Ross]

Comment on attachment 186188 [details] [diff] [review]
patch for browser

>-        ./icon.png
>-        ./preview.png
>+        icon.png                                                (icon.png)
>+        preview.png                                             {preview.png)
------------------------------------------------------------------^

Typo. :)

Also, do you need to specify the source file if it is the same? I thought the
point was you didn't, but then I don't see why the ./ was needed (if
make-jars.pl was written well).

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: patch w/ correct bracket - thanks mc

James - I believe the script always expects to find at least one / which is
probably why the ./ was added.

Comment 10

[James Ross]

Hmm, wont it have an issue with the patch then? Or is it so badly coded that it
only requires a / if there is just the one part? (surely that can be fixed?)

Comment 11

[Robert Strong (they/them - no direct email)]

(In reply to comment #10)
> Hmm, wont it have an issue with the patch then? Or is it so badly coded that it
> only requires a / if there is just the one part? (surely that can be fixed?)
It handles it differently based on whether a destination is specified so the
patch works... having said that I decided to take a look at the script and I
have another patch coming up.

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: patch

This hasn't been tested thoroughly but so far it does the right thing. My only
concern is I suspect that the ./ was prepended instead of handling files in the
root properly for some not so obvious reason so testing is needed.

Comment 13

[Robert Strong (they/them - no direct email)]

Attachment: patch

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 186193 [details] [diff] [review]
patch

I don't like the copy-paste or the lack of documentation for the somewhat
over-wrought regexps.  This code is already hard to read, and this makes it
worse.

If we're not going to do something simple like:

if (/\//) {
   s|.*/||;  # strip all but the last component
}
$cvsfile = File::Spec::Unix->canonpath($_);

then I'd like to see some  comments there explaining what we're doing, and why.

Comment 15

[Robert Strong (they/them - no direct email)]

Attachment: patch

Addresses Mike's comments.

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 186221 [details] [diff] [review]
patch

r+a=shaver, thanks.

Comment 17

[Robert Strong (they/them - no direct email)]

Attachment: patch for mail

Patch for Thunderbird - this should not be landed until after attachment 186221 [details] [diff] [review]
has landed

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: patch for calendar

Patch for Sunbird - this should not be landed until after attachment 186221 [details] [diff] [review] has
landed

Comment 19

[Robert Strong (they/them - no direct email)]

Attachment: patch for mail

Patch for Thunderbird - this should not be landed until after attachment 186221 [details] [diff] [review]
has landed.

Comment 20

[timeless]

Comment on attachment 186221 [details] [diff] [review]
patch

mozilla/config/make-chromelist.pl	3.10
mozilla/browser/themes/winstripe/browser/jar.mn 	1.11
mozilla/browser/themes/pinstripe/browser/jar.mn 	1.10

Comment 21

[Robert Strong (they/them - no direct email)]

Comment on attachment 186292 [details] [diff] [review]
patch for calendar

Mike, I verified that this does indeed fix this for Firefox on Mac OS X and
this is the same theme patch for Sunbird.

Comment 22

[Robert Strong (they/them - no direct email)]

Comment on attachment 186293 [details] [diff] [review]
patch for mail

Scott, I verified that this does indeed fix this for Firefox on Mac OS X and
this is the same theme patch for Thunderbird.

Comment 23

[Robert Strong (they/them - no direct email)]

Comment on attachment 186292 [details] [diff] [review]
patch for calendar

Mike, can I get an r & a from you for this and the patch for mail? Thanks.

Comment 24

[Robert Strong (they/them - no direct email)]

checkin needed on aattachment 186293

Comment 25

[Robert Strong (they/them - no direct email)]

arghh...  attachment 186293 [details] [diff] [review]

Comment 26

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 186292 [details] [diff] [review]
patch for calendar

r=shaver, thanks

Comment 27

[Robert Strong (they/them - no direct email)]

checkin needed for the last two patches which are attachment 186292 [details] [diff] [review] and
attachment 186293 [details] [diff] [review]

Comment 28

[Robert Strong (they/them - no direct email)]

fixed on trunk