Closed Bug 420811 Opened 13 years ago Closed 9 years ago

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

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: RyanVM, Assigned: RyanVM)

References

Details

(Keywords: polish, Whiteboard: [polish-hard][polish-visual][icon-namoroka][polish-p2])

Attachments

(2 files, 12 obsolete files)

9.21 KB, patch
RyanVM
: review+
RyanVM
: superreview+
Details | Diff | Splinter Review
10.96 KB, patch
Details | Diff | Splinter Review
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
Attached image broken-image.png (obsolete) —
PNG version of broken-image
Attached image loading-image.png (obsolete) —
PNG version of loading-image
Attached patch Step 1 - s/gif/png (obsolete) — Splinter Review
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.
Attachment #308174 - Flags: review?
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.
Attachment #308174 - Flags: review+
(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?
(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.
Nominating for blocking to pick up wanted+
Flags: blocking1.9?
Flags: wanted-next+
Flags: blocking1.9?
Flags: blocking1.9-
Keywords: polish, ue
Will replacing these 14x16 gifs with 16x16 gifs cause any problems?
(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.
Alex, can you post PNGs of the 1.9-era images being used now?
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.
Whiteboard: [polish-hard][polish-visual]
Whiteboard: [polish-hard][polish-visual] → [polish-hard][polish-visual][icon-3.1]
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 :)
Very likely too late for 3.1 now, hopefully we will get this for 3.2.
Whiteboard: [polish-hard][polish-visual][icon-3.1] → [polish-hard][polish-visual][icon-3.2]
Attachment #307159 - Attachment is obsolete: true
Attachment #307160 - Attachment is obsolete: true
Attached patch Updated to hg, moz-central patch (obsolete) — Splinter Review
Alex, can you review these or does someone else have to?
Attachment #308174 - Attachment is obsolete: true
Attachment #308374 - Attachment is obsolete: true
Attachment #308375 - Attachment is obsolete: true
Attachment #363448 - Flags: review?
Attachment #308174 - Flags: review?
Whoever checks these in will need to hg remove the old GIF files and add the PNG files from attachment 337574 [details] as well.
Attachment #363449 - Flags: review?(faaborg)
Attachment #363448 - Flags: review? → review?(faaborg)
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).
Attachment #363448 - Flags: review?(faaborg) → review?(sdwilsh)
Attachment #363449 - Flags: review?(faaborg) → review?(sdwilsh)
Attachment #363448 - Flags: superreview?(mconnor)
Attachment #363448 - Flags: review?(sdwilsh)
Attachment #363448 - Flags: review?(mconnor)
Attachment #363449 - Flags: superreview?(neil)
Attachment #363449 - Flags: review?(sdwilsh)
Attachment #363449 - Flags: review?(neil)
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...
Attachment #363449 - Flags: superreview?(neil)
Attachment #363449 - Flags: superreview+
Attachment #363449 - Flags: review?(philringnalda)
Attachment #363449 - Flags: review?(neil)
Attachment #363449 - Flags: review?(mkmelin+mozilla)
Based on comment #15, I'm assuming this is 1.9.2-only material.
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.
Attachment #363449 - Flags: review?(philringnalda)
Comment on attachment 363449 [details] [diff] [review]
Updated to hg, comm-central patch

r=mkmelin for the mail/ bits
(needs --fuzz=8  to apply)
Attachment #363449 - Flags: review?(mkmelin+mozilla) → review+
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.
Attachment #363448 - Flags: superreview?(mconnor)
Attachment #363448 - Flags: superreview+
Attachment #363448 - Flags: review?(mconnor)
Attachment #363448 - Flags: review+
Attached patch comm-central patch v2 (obsolete) — Splinter Review
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.
Attachment #363449 - Attachment is obsolete: true
Attachment #367931 - Flags: superreview+
Attachment #367931 - Flags: review+
Attachment #367931 - Flags: review?(philipp)
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.
Attachment #367931 - Flags: review+ → review-
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...
Attachment #367931 - Attachment is obsolete: true
Attachment #367931 - Flags: review?(philipp)
Attached patch comm-central patch v3 (obsolete) — Splinter Review
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?
Attachment #367948 - Flags: superreview?(neil)
Attachment #367948 - Flags: review?(philipp)
Attachment #367948 - Flags: review+
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 ;-)
Attachment #367948 - Flags: superreview?(neil) → superreview+
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.
Stupid copy/paste errors. I'll update the two patches tonight when I get home from work.
Attachment #367948 - Flags: review?(philipp) → review+
Comment on attachment 367948 [details] [diff] [review]
comm-central patch v3

Calendar part looks fine, r=philipp
Attached patch moz-central patch v2 (obsolete) — Splinter Review
Picking up neil's whitespace nit. This is applies cleanly and is ready for check-in. Carrying over previous r/sr.
Attachment #363448 - Attachment is obsolete: true
Attachment #368600 - Flags: superreview+
Attachment #368600 - Flags: review+
Fixing the slashes in the wrong direction neil noticed. This is applies cleanly and is ready for check-in. Carrying over previous r/sr.
Attachment #367948 - Attachment is obsolete: true
Attachment #368601 - Flags: superreview+
Attachment #368601 - Flags: review+
The step 1 patches are ready to go in. Attachments 337574, 368600, and 368601 need to go in.
Keywords: checkin-needed
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.
moz-central v2 + the new PNGs included. Not carrying over reviews per dao's request.
Attachment #337574 - Attachment is obsolete: true
Attachment #368600 - Attachment is obsolete: true
Comment on attachment 368744 [details] [diff] [review]
moz-central s/gif/png patch v3 (checked in)

http://hg.mozilla.org/mozilla-central/rev/7f0eced90e2a
Attachment #368744 - Attachment description: moz-central patch v3 → moz-central patch v3 (checked in)
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.
This won't land on 1.9.1.
Target Milestone: --- → mozilla1.9.2a1
(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.
Landed this to fix the wince tinderbox:
http://hg.mozilla.org/mobile-browser/rev/15ed2ad8ceb7
Keywords: ue
Depends on: 484681
Attachment #368601 - Attachment description: comm-central patch v4 → comm-central patch v4 (checked in)
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.
Whiteboard: [polish-hard][polish-visual][icon-3.2] → [polish-hard][polish-visual][icon-namoroka][polish-p2]
No longer blocks: 420810
Severity: normal → enhancement
Component: Layout → Theme
Flags: wanted-next+
Flags: blocking1.9-
Product: Core → Firefox
QA Contact: layout → theme
Target Milestone: mozilla1.9.2a1 → ---
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.
Attachment #368601 - Attachment description: comm-central patch v4 (checked in) → comm-central s/gif/png patch v4 (checked in)
Attachment #368744 - Attachment description: moz-central patch v3 (checked in) → moz-central s/gif/png patch v3 (checked in)
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/ryanvm@gmail.com-ab86760afeb5/
Attachment #598600 - Flags: review?(faaborg)
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?
According to https://wiki.mozilla.org/Modules/Toolkit, you're a toolkit peer. Want to review? :)
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?
Attachment #598600 - Flags: review?(faaborg) → review?(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.
(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.
> 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 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.
Attachment #598600 - Attachment is obsolete: true
Attachment #598600 - Flags: review?(dao)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Summary: broken-image.gif and loading-image.gif should be skinnable → Change broken-image.gif and loading-image.gif to PNGs
Component: Theme → Layout
Product: Firefox → Core
QA Contact: theme → layout
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.