294479 - Page Setup does not show icons for landscape and portrait mode

Status: VERIFIED FIXED

(Toolkit :: Printing, defect)

Description

[Simon Paquet [:sipaq]]

http://lxr.mozilla.org/mozilla/source/toolkit/themes/winstripe/global/printPageSetup.css
references the non-existent files Portrait.png and Landscape.png. Those files
were originally in located in mozilla/toolkit/skin/win/icons before
mozilla/toolkit/thems was created on the aviary branch.

See:
http://bonsai.mozilla.org/rview.cgi?cvsroot=/cvsroot&dir=mozilla/toolkit/skin/win/icons/Attic&module=default

As far as I can see this regressed on the trunk when the Winstripe theme landed
and has never worked on the aviary branch, because someone forgot to add the
relevant files.

In addition to adding the two files, the files Landscape-small.png and
Portrait-small.png should be added. This would allow us to get rid of the XPFE
leftovers pg-landscape-small.gif, pg-landscape.gif, pg-portrait-small.gif and
pg-portrait.gif

Comment 1

[Simon Paquet [:sipaq]]

Taking, asking for blocking-aviary1.1? and CC'ing mconnor, since this is a
longstanding regression (11 months), which he should know about and should be
pretty easy to fix.

Comment 2

[Mike Connor [:mconnor]]

This is a polish bug, but if there's a fix forthcoming we want to take this.

Comment 3

[Asa Dotzler [:asa]]

would like to have. Kevin, can you help out here with icon/css?

Comment 4

[Kevin Gerich]

Attachment: add Portrait.png and Landscape.png

I require approval to land this right?

Comment 5

[Kevin Gerich]

Attachment: screenshot of new icons

Comment 6

[Aaron Slunt]

Kevin, please request review from [e-mail].

Comment 7

[Aaron Slunt]

I take that back. Yes, you just need approval, sorry.

Comment 8

[Kevin Gerich]

Attachment: actual patch to add icons

Comment 9

[Simon Paquet [:sipaq]]

Comment on attachment 186433 [details] [diff] [review]
actual patch to add icons

Kevin, 
while you're at it could you please replace the old pg-landscape.gif and
pg-portrait.gif files (which come from the suite classic theme) with the new
icons and remove the old gif files from the jar.mn file?

We will also need small versions of your icons to replace the small versions of
the above-mentioned files:

- pg-landscape-small.gif
- pg-portrait-small.gif

The only occurence of both the big and the small versions of these old gif
files is
http://lxr.mozilla.org/seamonkey/source/themes/modern/global/printing.css#71
and below.

Comment 10

[Kevin Gerich]

Attachment: also don't package legacy images

Simon, we already have small versions of the icons in Print-preview.png. I'll
simply remove the old gifs from the jar.mn.

Comment 11

[Simon Paquet [:sipaq]]

Comment on attachment 186464 [details] [diff] [review]
also don't package legacy images

Kevin, you're right, I forgot that we already have these images.

But your patch will introduce a new regression since you did not change 
toolkit/themes/winstripe/global/printing.css

This file still uses the old pg-whatever.gif files. See this link:
http://lxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/printin
g.css#71 and the lines below.

I meant to include this link in my last post, but screwed that up and linked to
a file in the suite modern theme. Sorry for that.

Comment 12

[Kevin Gerich]

Is printing.css legacy? It is packaged, but I don't see it linked in toolkit
apps from my 3-second glance at lxr. Should I remove it as well?

Comment 13

[Simon Paquet [:sipaq]]

(In reply to comment #12)
> Is printing.css legacy? It is packaged, but I don't see it linked in toolkit
> apps from my 3-second glance at lxr. Should I remove it as well?

You're right it does not seem to be used. IMO it should be removed, but that
should be done in another bug. Want me to open one?

With that out of the way, your patch looks fine and you should ask for review
and approval. 
BTW sorry for wasting everyone's time!

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 186464 [details] [diff] [review]
also don't package legacy images

a=asa

Comment 15

[Kevin Gerich]

Checked in on trunk but I didn't remove the old gif files or the printing.css
file. Simon, can you open another for the removal of the old printing files?

Comment 16

[Simon Paquet [:sipaq]]

(In reply to comment #15)
> Checked in on trunk but I didn't remove the old gif files or the printing.css
> file. Simon, can you open another for the removal of the old printing files?

Thanks Kevin. I opened bug 297992 for that.