The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Layout
--
enhancement
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: RyanVM, Assigned: RyanVM)

Tracking

({polish})

Trunk
mozilla1.9.2a1
polish
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 12 obsolete attachments)

9.21 KB, patch
RyanVM
: review+
RyanVM
: superreview+
Details | Diff | Splinter Review
10.96 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
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
(Assignee)

Comment 1

9 years ago
Created attachment 307159 [details]
broken-image.png

PNG version of broken-image
(Assignee)

Comment 2

9 years ago
Created attachment 307160 [details]
loading-image.png

PNG version of loading-image
(Assignee)

Comment 3

9 years ago
Created attachment 308174 [details] [diff] [review]
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.
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+
Created attachment 308374 [details]
mozilla/camino/resources/images/gecko/broken-image.png
Created attachment 308375 [details]
mozilla/camino/resources/images/gecko/loading-image.png
(Assignee)

Comment 7

9 years ago
(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.
(Assignee)

Comment 12

9 years ago
Alex, can you post PNGs of the 1.9-era images being used now?
Created attachment 337574 [details]
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.
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]
(Assignee)

Updated

8 years ago
Attachment #307159 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #307160 - Attachment is obsolete: true
(Assignee)

Comment 16

8 years ago
Created attachment 363448 [details] [diff] [review]
Updated to hg, moz-central patch

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?
(Assignee)

Comment 17

8 years ago
Created attachment 363449 [details] [diff] [review]
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.
Attachment #363449 - Flags: review?(faaborg)
(Assignee)

Updated

8 years ago
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)

Updated

8 years ago
Attachment #363448 - Flags: superreview?(mconnor)
Attachment #363448 - Flags: review?(sdwilsh)
Attachment #363448 - Flags: review?(mconnor)

Updated

8 years ago
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)
(Assignee)

Comment 20

8 years ago
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 22

8 years ago
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+
(Assignee)

Comment 24

8 years ago
Created attachment 367931 [details] [diff] [review]
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.
Attachment #363449 - Attachment is obsolete: true
Attachment #367931 - Flags: superreview+
Attachment #367931 - Flags: review+
(Assignee)

Updated

8 years ago
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-
(Assignee)

Comment 26

8 years ago
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)
(Assignee)

Comment 27

8 years ago
Created attachment 367948 [details] [diff] [review]
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?
Attachment #367948 - Flags: superreview?(neil)
Attachment #367948 - Flags: review?(philipp)
(Assignee)

Updated

8 years ago
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 ;-)

Updated

8 years ago
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.
(Assignee)

Comment 30

8 years ago
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
(Assignee)

Comment 32

8 years ago
Created attachment 368600 [details] [diff] [review]
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.
Attachment #363448 - Attachment is obsolete: true
Attachment #368600 - Flags: superreview+
Attachment #368600 - Flags: review+
(Assignee)

Comment 33

8 years ago
Created attachment 368601 [details] [diff] [review]
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.
Attachment #367948 - Attachment is obsolete: true
Attachment #368601 - Flags: superreview+
Attachment #368601 - Flags: review+
(Assignee)

Comment 34

8 years ago
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.
(Assignee)

Comment 36

8 years ago
Created attachment 368744 [details] [diff] [review]
moz-central s/gif/png patch v3 (checked in)

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

Updated

8 years ago
Keywords: ue
http://hg.mozilla.org/comm-central/rev/c1d6aa6e6314
Keywords: checkin-needed
Depends on: 484681
(Assignee)

Updated

8 years ago
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]
(Assignee)

Updated

7 years ago
No longer blocks: 420810
(Assignee)

Updated

5 years ago
Severity: normal → enhancement
Component: Layout → Theme
Flags: wanted-next+
Flags: blocking1.9-
Product: Core → Firefox
QA Contact: layout → theme
Target Milestone: mozilla1.9.2a1 → ---
(Assignee)

Comment 44

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #368601 - Attachment description: comm-central patch v4 (checked in) → comm-central s/gif/png patch v4 (checked in)
(Assignee)

Updated

5 years ago
Attachment #368744 - Attachment description: moz-central patch v3 (checked in) → moz-central s/gif/png patch v3 (checked in)
(Assignee)

Comment 45

5 years ago
Created attachment 598600 [details] [diff] [review]
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/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?
(Assignee)

Comment 47

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

Comment 48

5 years ago
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.
(Assignee)

Comment 50

5 years ago
(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.
(Assignee)

Comment 52

5 years ago
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)
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 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

Updated

5 years ago
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.