Last Comment Bug 504645 - Stop shipping document.png on Linux
: Stop shipping document.png on Linux
Status: VERIFIED FIXED
[icon-namoroka][good first bug]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: Firefox 7
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
:
Mentors:
Depends on:
Blocks: 705974
  Show dependency treegraph
 
Reported: 2009-07-16 12:43 PDT by Justin Dolske [:Dolske]
Modified: 2011-11-28 19:53 PST (History)
10 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for bug 504645 (10.05 KB, patch)
2011-06-20 15:08 PDT, Jared Wein [:jaws] (please needinfo? me)
dolske: review+
Details | Diff | Splinter Review
Patch for bug 504645 - version 2 (10.46 KB, patch)
2011-06-23 17:09 PDT, Jared Wein [:jaws] (please needinfo? me)
dolske: review+
mark.finkle: review+
Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2009-07-16 12:43:48 PDT
Spun off from bug 499226, where we were removing unneeded branding images from Linux. Turns out document.png was still being used.

See: browser/components/shell/src/nsGNOMEShellService.cpp#288, where it copies
document.png to ~/.icons/firefox-document.png when you trigger the "set default
browser" code. This seems kinda crappy, since it means icon updates should
repeat the copy or the user has the old version floating around. I'm not even sure this is the right thing to be doing.
Comment 1 Michael Ventnor 2009-07-17 04:05:27 PDT
This doesn't even do anything. MIME icons for common types like HTML are provided by the GTK theme, not each application. It should be safe to just remove that code.
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2011-06-16 14:16:06 PDT
I ran a search on MXR and could only find references to a "document.png" within Makefiles.

There is a reference to a "firefox-document.png" in browser/components/shell/src/nsGNOMEShellService.cpp#98 but it seems that the variable, |kDocumentIconPath|, is defined but never referenced.

Could this have been fixed already?
Comment 3 Justin Dolske [:Dolske] 2011-06-19 18:26:54 PDT
Ah, indeed, bug 294375 removed this code.

*** This bug has been marked as a duplicate of bug 294375 ***
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-20 08:26:27 PDT
Should we stop shipping the actual image then? People might depend on us shipping it somehow, I guess, but I'm not sure we should let that stop us.
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2011-06-20 08:41:23 PDT
In reply to comment #4: If it's not being used, then I think we should stop shipping it.

This gives us an opportunity to decrease the filesize of our initial download (albeit by a tiny bit) but every little step counts.
Comment 6 Justin Dolske [:Dolske] 2011-06-20 13:40:05 PDT
Hmm, yeah, I wasn't thinking. We should nuke the icon from Hg and the bits that ship it.
Comment 7 Jared Wein [:jaws] (please needinfo? me) 2011-06-20 15:08:08 PDT
Created attachment 540603 [details] [diff] [review]
Patch for bug 504645

Removed document.png from source control and updated the Makefiles to not copy the file anymore.

Also updated XULRunner to remove document.png, but I'm not sure if we wanted that.
Comment 8 Jared Wein [:jaws] (please needinfo? me) 2011-06-20 15:11:05 PDT
Try submission: http://tbpl.mozilla.org/?tree=Try&rev=8d30e2b3c7c3
Comment 9 Justin Dolske [:Dolske] 2011-06-22 17:20:04 PDT
Comment on attachment 540603 [details] [diff] [review]
Patch for bug 504645

Review of attachment 540603 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the above fixed.

::: xulrunner/app/Makefile.in
@@ +189,1 @@
>    $(NULL)

Since ICON_FILES is empty now, you should go ahead and remove the immediately-following libs:: and install:: targets that now have nothing to do.

Might be a good idea to do a quick xulrunner build just to make sure nothing blows up.
Comment 10 Justin Dolske [:Dolske] 2011-06-22 17:20:44 PDT
Err, "r+ with the following fixed". I thought Splinter put that at the bottom.
Comment 11 Jared Wein [:jaws] (please needinfo? me) 2011-06-23 17:09:51 PDT
Created attachment 541554 [details] [diff] [review]
Patch for bug 504645 - version 2

I have made the changes requested. I also built xulrunner but had issues running |make package| from within the objdir.

This was the error that I encountered:
sed: can't read components/components.manifest: No such file or directory

There doesn't appear to be a "components" directory.
Comment 12 Justin Dolske [:Dolske] 2011-06-24 15:25:48 PDT
Comment on attachment 541554 [details] [diff] [review]
Patch for bug 504645 - version 2

That's ok, I don't think that build thing matters.

Bouncing over to mfinkle for a rubberstamp on the xulrunner change, but it looks fine to me.
Comment 13 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-24 15:46:25 PDT
Comment on attachment 541554 [details] [diff] [review]
Patch for bug 504645 - version 2

no downsides come to mind
Comment 14 Jared Wein [:jaws] (please needinfo? me) 2011-06-24 17:39:50 PDT
Dolske, can you land this for me? Thanks!
Comment 16 Mounir Lamouri (:mounir) 2011-06-27 02:15:48 PDT
Merged:
http://hg.mozilla.org/mozilla-central/rev/898a771d5825
Comment 17 George Carstoiu 2011-06-28 07:20:57 PDT
Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110627 Firefox/7.0a1

document.png no longer present in branding directories. Setting status to Verified Fixed.
Comment 18 Nick Thomas [:nthomas] 2011-11-28 19:53:25 PST
For the future, please add any deprecated files to browser/installer/removed-files.in, so that users updating have them removed too. Fixing that for this bug in bug 705974.

Note You need to log in before you can comment on or make changes to this bug.