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

RESOLVED FIXED

Status

()

Core
Build Config
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 obsolete attachments)

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.
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
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

13 years ago
Is classic.jar supposed to have a preview.png and icon.png?
(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.
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.
Created attachment 186188 [details] [diff] [review]
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.
Assignee: nobody → rob_strong
Status: NEW → ASSIGNED
Attachment #186188 - Flags: review?(benjamin)

Comment 7

13 years ago
Please note the messed up brackets:

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

Comment 8

13 years ago
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).
Attachment #186188 - Attachment is obsolete: true
Attachment #186188 - Flags: review?(benjamin)
Created attachment 186189 [details] [diff] [review]
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.
Attachment #186189 - Flags: review?(benjamin)

Comment 10

13 years ago
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?)
(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.
Attachment #186189 - Attachment is obsolete: true
Attachment #186189 - Flags: review?(benjamin)
Created attachment 186191 [details] [diff] [review]
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.
Attachment #186191 - Flags: review?(benjamin)
Attachment #186191 - Attachment is obsolete: true
Attachment #186191 - Flags: review?(benjamin)
Created attachment 186193 [details] [diff] [review]
patch
Attachment #186193 - Flags: review?(benjamin)

Updated

13 years ago
Attachment #186193 - Flags: review?(benjamin) → review+
Attachment #186193 - Flags: approval-aviary1.1a2?
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.
Attachment #186193 - Flags: approval-aviary1.1a2?
Created attachment 186221 [details] [diff] [review]
patch

Addresses Mike's comments.
Attachment #186193 - Attachment is obsolete: true
Attachment #186221 - Flags: review?(shaver)
Comment on attachment 186221 [details] [diff] [review]
patch

r+a=shaver, thanks.
Attachment #186221 - Flags: review?(shaver)
Attachment #186221 - Flags: review+
Attachment #186221 - Flags: approval-aviary1.1a2+
Created attachment 186291 [details] [diff] [review]
patch for mail

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

Patch for Sunbird - this should not be landed until after attachment 186221 [details] [diff] [review] has
landed
Attachment #186291 - Attachment is obsolete: true
Created attachment 186293 [details] [diff] [review]
patch for mail

Patch for Thunderbird - this should not be landed until after attachment 186221 [details] [diff] [review]
has landed.
Whiteboard: needs checkin
Whiteboard: needs checkin → [checkin needed]

Comment 20

13 years ago
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
Attachment #186221 - Attachment is obsolete: true

Updated

13 years ago
Whiteboard: [checkin needed]
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.
Attachment #186292 - Flags: review?(shaver)
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.
Attachment #186293 - Flags: review?(mscott)

Updated

13 years ago
Attachment #186293 - Flags: review?(mscott) → review+
Attachment #186293 - Flags: approval-aviary1.1a2?
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.
Attachment #186292 - Flags: approval-aviary1.1a2?

Updated

13 years ago
Attachment #186293 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+

Updated

13 years ago
Attachment #186292 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
checkin needed on aattachment 186293
Whiteboard: [checkin needed][a+]
arghh...  attachment 186293 [details] [diff] [review]
Comment on attachment 186292 [details] [diff] [review]
patch for calendar

r=shaver, thanks
Attachment #186292 - Flags: review?(shaver) → review+
checkin needed for the last two patches which are attachment 186292 [details] [diff] [review] and
attachment 186293 [details] [diff] [review]
fixed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed][a+]
Attachment #186292 - Attachment is obsolete: true
Attachment #186293 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.