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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

enhancement
RESOLVED FIXED
12 years ago
7 years ago

People

(Reporter: RyanVM, Assigned: RyanVM)

Tracking

({polish})

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph

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

12 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

12 years ago
Posted image broken-image.png (obsolete) —
PNG version of broken-image
Assignee

Comment 2

12 years ago
Posted image loading-image.png (obsolete) —
PNG version of loading-image
Assignee

Comment 3

12 years ago
Posted 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+
Assignee

Comment 7

11 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

11 years ago
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]
Assignee

Updated

11 years ago
Attachment #307159 - Attachment is obsolete: true
Assignee

Updated

11 years ago
Attachment #307160 - Attachment is obsolete: true
Assignee

Comment 16

11 years ago
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

11 years ago
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

11 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)
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)
Assignee

Comment 20

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

10 years ago
Posted 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+
Assignee

Updated

10 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

10 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

10 years ago
Posted 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)
Assignee

Updated

10 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

10 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

10 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

10 years ago
Posted 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+
Assignee

Comment 33

10 years ago
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

10 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

10 years ago
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
Assignee

Updated

10 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

9 years ago
No longer blocks: 420810
Assignee

Updated

7 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

7 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

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

Updated

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

Comment 45

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

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

Comment 48

7 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

7 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

7 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

7 years ago
Status: NEW → RESOLVED
Closed: 7 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.