420811 - Change broken-image.gif and loading-image.gif to PNGs

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Ryan VanderMeulen [:RyanVM]]

Presently, broken-image.gif and loading-image.gif are present within the res directory of the install, making them unavailable to themers. Also, it means that the files reside within layout/generic, which is hardly an intuitive location for them.

This bug will serve two purposes:
1.) Convert the GIF versions of these two images to PNG
2.) Get the two images within the skin and remove the old copies from layout/generic

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: broken-image.png

PNG version of broken-image

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Attachment: loading-image.png

PNG version of loading-image

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Attachment: Step 1 - s/gif/png

This changes all instances of broken-image.gif and loading-image.gif in the tree to .png. I don't even have a clue as to who should review this.

When this goes in, the two images attached to this bug should also go in and their .gif counterparts can be removed.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 308174 [details] [diff] [review]
Step 1 - s/gif/png

The Camino changes aren't strictly correct, since we override these images already (note the paths in the lines below) and our copies of the GIFs in mozilla/camino/resources/images/gecko/ will need to be converted to PNGs to prevent bustage.

>Index: camino/Camino.xcodeproj/project.pbxproj
>===================================================================
>RCS file: /cvsroot/mozilla/camino/Camino.xcodeproj/project.pbxproj,v
>retrieving revision 1.75
>diff -u -p -8 -r1.75 project.pbxproj
>--- camino/Camino.xcodeproj/project.pbxproj	27 Feb 2008 15:09:47 -0000	1.75
>+++ camino/Camino.xcodeproj/project.pbxproj	8 Mar 2008 17:33:02 -0000

> 		F55A9BC60228CCE101DAE4DB /* security-prefs.js */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.javascript; name = "security-prefs.js"; path = "../dist/Embed/greprefs/security-prefs.js"; sourceTree = SOURCE_ROOT; };
>-		F55B6BF402EF1F7E01026D5D /* broken-image.gif */ = {isa = PBXFileReference; lastKnownFileType = image.gif; name = "broken-image.gif"; path = "resources/images/gecko/broken-image.gif"; sourceTree = SOURCE_ROOT; };
>+		F55B6BF402EF1F7E01026D5D /* broken-image.png */ = {isa = PBXFileReference; lastKnownFileType = image.gif; name = "broken-image.png"; path = "resources/images/gecko/broken-image.png"; sourceTree = SOURCE_ROOT; };
> 		F55C4DD302D2864D0130B065 /* UserDefaults.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; name = UserDefaults.h; path = src/application/UserDefaults.h; sourceTree = "<group>"; };

> 		F59E9F3F0237E43401A967DF /* ContentClickListener.mm */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.objcpp; name = ContentClickListener.mm; path = src/browser/ContentClickListener.mm; sourceTree = "<group>"; };
>-		F5A112C902DF270F01026D5D /* loading-image.gif */ = {isa = PBXFileReference; lastKnownFileType = image.gif; name = "loading-image.gif"; path = "resources/images/gecko/loading-image.gif"; sourceTree = SOURCE_ROOT; };
>+		F5A112C902DF270F01026D5D /* loading-image.png */ = {isa = PBXFileReference; lastKnownFileType = image.gif; name = "loading-image.png"; path = "resources/images/gecko/loading-image.png"; sourceTree = SOURCE_ROOT; };
> 		F5A8CE4102DFEF4C013CA8EC /* PreferencePaneBase.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; path = PreferencePaneBase.h; sourceTree = "<group>"; };

When those images move into a theme (step 2), we'll need another couple of changes.

r=ardissone on the Camino portion if you remove our old GIFs and land our new PNGs in mozilla/camino/resources/images/gecko/ (I'll attach PNG versions in a second) when you land the rest of Step 1.


>Index: embedding/tests/cocoaEmbed/CocoaEmbed.pbproj/project.pbxproj
>===================================================================

The CocoaEmbed test app has been dead since forever and was missed in the CocoaEmbed cleanup last year; I filed bug 421857 on finally removing it.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: mozilla/camino/resources/images/gecko/broken-image.png

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: mozilla/camino/resources/images/gecko/loading-image.png

Comment 7

[Ryan VanderMeulen [:RyanVM]]

(In reply to comment #4)
> (From update of attachment 308174 [details] [diff] [review])
> The Camino changes aren't strictly correct, since we override these images
> already (note the paths in the lines below) and our copies of the GIFs in
> mozilla/camino/resources/images/gecko/ will need to be converted to PNGs to
> prevent bustage.
> 
> >Index: camino/Camino.xcodeproj/project.pbxproj
> >===================================================================

Am I right that in addition to including the replaced images, the "lastKnownFileType = image.gif" will need to change to .png as well? Other than that, it's OK?

> >Index: embedding/tests/cocoaEmbed/CocoaEmbed.pbproj/project.pbxproj
> >===================================================================
> 
> The CocoaEmbed test app has been dead since forever and was missed in the
> CocoaEmbed cleanup last year; I filed bug 421857 on finally removing it.
> 

OK, I won't include it in the next revision of the patch.

Faaborg et al, who else should be reviewing this?

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #7)
> > >Index: camino/Camino.xcodeproj/project.pbxproj
> > >===================================================================
> 
> Am I right that in addition to including the replaced images, the
> "lastKnownFileType = image.gif" will need to change to .png as well? Other than
> that, it's OK?

Ah, I missed that; yes, you're correct (though luckily that isn't crucial to anything; we also seem to have some TIFFs listed in project with "image.png" as their lastKnownFileType :p), and yes, other than that, everything's good.

Comment 9

[Alex Faaborg [:faaborg] (Firefox UX)]

Nominating for blocking to pick up wanted+

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

Will replacing these 14x16 gifs with 16x16 gifs cause any problems?

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #10)
> Will replacing these 14x16 gifs with 16x16 gifs cause any problems?

The core files are still 14x16, but as for Camino's files, we've been using 16x16 for years now without any noticeable issue.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Alex, can you post PNGs of the 1.9-era images being used now?

Comment 13

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Loading image and error loading image as PNG files

Here is the current artwork as .png files.  If we switch to using pngs we will want to later update this artwork to take advantage of having an alpha channel.

Comment 14

[Alex Faaborg [:faaborg] (Firefox UX)]

I would like to get the current png files landed so that later we can just do an image drop in to update them with higher quality versions (taking advantage of that newfangled alpha channel :)

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

Very likely too late for 3.1 now, hopefully we will get this for 3.2.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Attachment: Updated to hg, moz-central patch

Alex, can you review these or does someone else have to?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Attachment: Updated to hg, comm-central patch

Whoever checks these in will need to hg remove the old GIF files and add the PNG files from attachment 337574 [details] as well.

Comment 18

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 363448 [details] [diff] [review]
Updated to hg, moz-central patch

Reassigning the review to someone who just left to go to the platform team :p (Shawn, re-re-asign if there is someone else who should take this).

Comment 19

[[e-mail]]

Comment on attachment 363449 [details] [diff] [review]
Updated to hg, comm-central patch

Are the core changes are intended to land on 1.9.1? If not then these will need #ifdefs.

>+res/broken-image.gif
>+res/loading-image.gif
Please put these before and/or after res/cmessage.txt

Punting the review for non-suite bits...

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Based on comment #15, I'm assuming this is 1.9.2-only material.

Comment 21

[Phil Ringnalda (:philor)]

Comment on attachment 363449 [details] [diff] [review]
Updated to hg, comm-central patch

Looks like mail/ is already covered with Magnus, but you'll want a calendar/ reviewer.

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 363449 [details] [diff] [review]
Updated to hg, comm-central patch

r=mkmelin for the mail/ bits
(needs --fuzz=8  to apply)

Comment 23

[Mike Connor [:mconnor]]

Comment on attachment 363448 [details] [diff] [review]
Updated to hg, moz-central patch

r+sr=mconnor

Let's get this on trunk ASAP (make sure to land the images as well) and we'll go from there.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Attachment: comm-central patch v2

Updated to tip and addresses neil's review comment. Carrying over previous r/sr and requesting a calendar review.

The v1 moz-central patch still applies without any complaint.

Comment 25

[Phil Ringnalda (:philor)]

Comment on attachment 367931 [details] [diff] [review]
comm-central patch v2

Looks like you only caught one of Neil's two review comments: right now, comm-central  builds on top of both 1.9.1 and trunk with a single branch, so to not break when you land your core changes on trunk but not 1.9.1 you need MOZILLA_1_9_1_BRANCH ifdefs/ifndefs around everything.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 367931 [details] [diff] [review]
comm-central patch v2

Would have been nice for someone to have mentioned that after my obvious misunderstanding of that in comment #20. Oh well, new patch coming...

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Attachment: comm-central patch v3

Includes all the ifdef/ifndef goodness. I made sure to update jar.mn so that pageInfo.xul gets preprocessed as well. Asking for neil to sr again since I'm not familiar at all with comm-central (as is readily apparent) and I want to make sure this is right.

Carrying over Magnus' earlier r+ for the mail components. Fallen, can you r+ the calendar bits?

Comment 28

[[e-mail]]

Comment on attachment 363448 [details] [diff] [review]
Updated to hg, moz-central patch

>-  NS_NAMED_LITERAL_STRING(loadingSrc,"resource://gre/res/loading-image.gif"); 
>-  NS_NAMED_LITERAL_STRING(brokenSrc,"resource://gre/res/broken-image.gif");
>+  NS_NAMED_LITERAL_STRING(loadingSrc,"resource://gre/res/loading-image.png"); 
>+  NS_NAMED_LITERAL_STRING(brokenSrc,"resource://gre/res/broken-image.png");
Nit: could remove the trailing whitespace while you're there ;-)

Comment 29

[[e-mail]]

Comment on attachment 367948 [details] [diff] [review]
comm-central patch v3

> bin\res\dtd\*
> bin\res\fonts\*
> bin\res\html\*
>+#ifdef MOZILLA_1_9_1_BRANCH
>+bin/res/broken-image.gif
>+bin/res/loading-image.gif
>+#else
>+bin/res/broken-image.png
>+bin/res/loading-image.png
>+#endif
Looks like these need to have \s
sr=me with that fixed.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Stupid copy/paste errors. I'll update the two patches tonight when I get home from work.

Comment 31

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 367948 [details] [diff] [review]
comm-central patch v3

Calendar part looks fine, r=philipp

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Attachment: moz-central patch v2

Picking up neil's whitespace nit. This is applies cleanly and is ready for check-in. Carrying over previous r/sr.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Attachment: comm-central s/gif/png patch v4 (checked in)

Fixing the slashes in the wrong direction neil noticed. This is applies cleanly and is ready for check-in. Carrying over previous r/sr.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

The step 1 patches are ready to go in. Attachments 337574, 368600, and 368601 need to go in.

Comment 35

[Dão Gottwald [:dao]]

Can you please include the new files in the patch?

(In reply to comment #24)
> Carrying over previous r/sr and requesting a calendar review.
(In reply to comment #27)
> Carrying over Magnus' earlier r+ for the mail components.
(In reply to comment #32)
> Carrying over previous r/sr.
(In reply to comment #33)
> Carrying over previous r/sr.

This isn't useful, makes it harder to see who did the reviews.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Attachment: moz-central s/gif/png patch v3 (checked in)

moz-central v2 + the new PNGs included. Not carrying over reviews per dao's request.

Comment 37

[Dão Gottwald [:dao]]

Comment on attachment 368744 [details] [diff] [review]
moz-central s/gif/png patch v3 (checked in)

http://hg.mozilla.org/mozilla-central/rev/7f0eced90e2a

Comment 38

[Nick Thomas [:nthomas] (UTC+12)]

This sort of change requires a clobber on mac tinderboxes (or at least culling a bunch of stuff out of dist/bin), because the packaging story is not strong there. You get this sort of error:

make -C obj-firefox/ppc/browser/installer \
          UNIVERSAL_BINARY= SIGN_NSS= PKG_SKIP_STRIP=1 stage-package
/tools/python/bin/python /builds/moz2_slave/mozilla-central-macosx/build/config/Preprocessor.py [snip] /mozilla-central-macosx/build/browser/installer/removed-files.in > removed-files
Creating package directory...
building file list ... file has vanished: "/builds/moz2_slave/mozilla-central-macosx/build/obj-firefox/ppc/dist/Minefield.app/Contents/MacOS/res/broken-image.gif"
file has vanished: "/builds/moz2_slave/mozilla-central-macosx/build/obj-firefox/ppc/dist/Minefield.app/Contents/MacOS/res/loading-image.gif"
[snip]
total size is 53487575  speedup is 1.00
rsync warning: some files vanished before they could be transferred (code 24) at /SourceCache/rsync/rsync-30/rsync/main.c(717)
make[2]: *** [stage-package] Error 24
make[1]: *** [postflight_all] Error 2
make: *** [build] Error 2

Please find the buildduty person on IRC (or ask in #build) to get a mac clobber before this lands on mozilla-1.9.1.

Comment 39

[Dão Gottwald [:dao]]

This won't land on 1.9.1.

Comment 40

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #39)
Ok, no worries. 

Did you see the bustage on the mobile tree ?
http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1237712400.1237716576.19392.gz
There's a CAB manifest at mobile-browser/installer/wince/fennec.inf to update too.

Comment 41

[Dão Gottwald [:dao]]

Landed this to fix the wince tinderbox:
http://hg.mozilla.org/mobile-browser/rev/15ed2ad8ceb7

Comment 42

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/comm-central/rev/c1d6aa6e6314

Comment 43

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.

Technically this is in the primary interface, but it is only encountered when on a very slow connection or a sever is having trouble serving images.  These icons could look a lot better and considerably more platform native than they currently do.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

This should be pretty easy now that these are packaged in omnijar. Should just be a matter of changing where they're packaged and changing the corresponding resource URLs. Patch coming.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Attachment: Move broken-image.png and loading-image.png to theme (mozilla-central)

This passes on Try and the Windows build seems to work as expected. Alex, does this need review from a layout peer too? If you're happy with this, I'll spin up a comm-central patch next.

Builds are available here if anyone wants to try them out.
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ab86760afeb5/

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Alex left MoCo and isn't really working on Mozilla these days. In any case, he's not really the right person to review that... ask a toolkit peer?

Comment 47

[Ryan VanderMeulen [:RyanVM]]

According to https://wiki.mozilla.org/Modules/Toolkit, you're a toolkit peer. Want to review? :)

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 598600 [details] [diff] [review]
Move broken-image.png and loading-image.png to theme (mozilla-central)

Dao, can you take a look?

Comment 49

[Dão Gottwald [:dao]]

Why should we do this? Who needs theme-dependent broken-image placeholders?

Hardcoded skin/ image paths are usually considered a bug -- the common entry points for themes are CSS files.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

(In reply to Dão Gottwald [:dao] from comment #49)
> Why should we do this? Who needs theme-dependent broken-image placeholders?

Intuitively, it makes more sense at least to me to have all chrome images included together instead of scattered around the tree. Also, I don't see a downside in giving theme creators the flexibility to make changes if they want.

> Hardcoded skin/ image paths are usually considered a bug -- the common entry
> points for themes are CSS files.

The images are already hardcoded. This patch just changes the location of them. If there's a way to un-hardcode them and you're open to doing so, I'm willing to work on it.

Comment 51

[Dão Gottwald [:dao]]

> Intuitively, it makes more sense at least to me to have all chrome images included
> together instead of scattered around the tree.

These images aren't exactly chrome images in my book. They also are close to the code that uses them, which doesn't seem particularly bad.

> Also, I don't see a downside in giving theme creators the flexibility to make changes
> if they want.

The downside is that they'd be required to provide the images.

> The images are already hardcoded.

resource:// locations are fixed as much as those hardcoded references are fixed (i.e. at build time). chrome://*/skin/ locations aren't fixed.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 598600 [details] [diff] [review]
Move broken-image.png and loading-image.png to theme (mozilla-central)

In that case, it sounds like all the useful work of this bug is already done. Thanks for explaining.