396992 - On Linux, beautified FTP listings should use the stock icons

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Michael Ventnor]

Attachment: Patch

We have stock icon infrastructure on Linux, let's use it. This patch will make the up and directory icons use native icons from GTK.

Comment 1

[Dão Gottwald [:dao]]

Ah, another dirListing.css to sync. I love that :)

Is size=menu semantically correct? If it is, you might want to patch nsIndexedToHTML.cpp to use the same size in the Linux case. Otherwise stick with size=16.

Comment 2

[Michael Ventnor]

(In reply to comment #1)
> Is size=menu semantically correct? If it is, you might want to patch
> nsIndexedToHTML.cpp to use the same size in the Linux case. Otherwise stick
> with size=16.

No, this is correct because stock icons use a completely different path. When calling GTK's icon renderer, you can only specify relative sizes (menuitem, button, dialog) and whoever implemented the moz-icon infrastructure decided to use strings in the size attribute to represent that.

Comment 3

[Michael Ventnor]

And you can't specify numbers at all. If you do, it will fall back to the menuitem size (believe me, it took ages to read code to figure out why numbers didn't work when I first used this)

Comment 4

[Michael Ventnor]

Mano, just as a note before your review, the CSS file is a carbon copy of the Winstripe one with the exception of the last two rules, which specify the stock icons.

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #2)
> No, this is correct because stock icons use a completely different path. When
> calling GTK's icon renderer, you can only specify relative sizes (menuitem,
> button, dialog) and whoever implemented the moz-icon infrastructure decided to
> use strings in the size attribute to represent that.

And the file icons don't support size=menu?

Also, could you utilize the directory stock icon as the favicon for local listings?

Comment 6

[Michael Ventnor]

(In reply to comment #5)
> And the file icons don't support size=menu?

No, different codepath.

> Also, could you utilize the directory stock icon as the favicon for local
> listings?

That would mean some ugly ifdef's but I guess :(

Comment 7

[Michael Ventnor]

(In reply to comment #6)
> > Also, could you utilize the directory stock icon as the favicon for local
> > listings?
> 
> That would mean some ugly ifdef's but I guess :(

Ugh, it seems that needs bug 388553 because I can't use them as favicons for now

Comment 8

[Michael Ventnor]

(In reply to comment #7)
> (In reply to comment #6)
> > > Also, could you utilize the directory stock icon as the favicon for local
> > > listings?
> > 
> > That would mean some ugly ifdef's but I guess :(
> 
> Ugh, it seems that needs bug 388553 because I can't use them as favicons for
> now
> 

It looks like it might not be that bug after all, but for some odd reason I can't use those as favicons at all.

Comment 9

[Michael Ventnor]

Attachment: Patch 2

Smaller patch that uses ifdefs instead.

Comment 10

[Asaf Romano (gone)]

Comment on attachment 281894 [details] [diff] [review]
Patch 2

r=mano.

Comment 11

[Michael Ventnor]

Comment on attachment 281894 [details] [diff] [review]
Patch 2

This patch is all CSS so it contains next to no risk. The benefit is use of native icons on the FTP listings.

Comment 12

[Dão Gottwald [:dao]]

Hrm, I just realized that the fix should probably be in the base stylesheet (and %ifndef MOZ_WIDGET_GTK2 for the winstripe .up icon) so that it works for embedders.

Michael, do you agree?

Boris, do you agree?

Comment 13

[Jeff Walden [:Waldo]]

Why not add a special class that can be targeted here, avoiding the ifdefs?  Theme authors otherwise can't do OS-agnostic themes, because they'll need to split the theme based on these ifdefs.

Comment 14

[Michael Ventnor]

(In reply to comment #12)
> Hrm, I just realized that the fix should probably be in the base stylesheet
> (and %ifndef MOZ_WIDGET_GTK2 for the winstripe .up icon) so that it works for
> embedders.
> 
> Michael, do you agree?

Embedders can just replace the res:// folder.png with their own style, can't they?

(In reply to comment #13)
> Why not add a special class that can be targeted here, avoiding the ifdefs? 
> Theme authors otherwise can't do OS-agnostic themes, because they'll need to
> split the theme based on these ifdefs.

Mano suggested using the ifdefs, I just followed his suggestion. I don't think theme authors would need a split anyway, the point of the default theme is to match the OS, every other theme provides their own unique style across all platforms.

Comment 15

[Michael Ventnor]

(In reply to comment #14)
> (In reply to comment #12)
> > Hrm, I just realized that the fix should probably be in the base stylesheet
> > (and %ifndef MOZ_WIDGET_GTK2 for the winstripe .up icon) so that it works for
> > embedders.
> > 
> > Michael, do you agree?
> 
> Embedders can just replace the res:// folder.png with their own style, can't
> they?

Er, forget that idea.
That might be better but I didn't think about it at the time. Although I think the current patch is simpler...

Comment 16

[Jeff Walden [:Waldo]]

(In reply to comment #14)
> I don't think theme authors would need a split anyway, the point of the
> default theme is to match the OS, every other theme provides their own
> unique style across all platforms.

I strongly disagree with the assumption that themes don't try to integrate with the host operating system (or that only the default theme would try to match the OS), and I think this is the wrong patch for the bug.  We should add the platform ifdefs to the C++ that generates file listings, not to the CSS that styles them.

Comment 17

[Jeff Walden [:Waldo]]

(In reply to comment #16)
> We should add the platform ifdefs to the C++ that generates file listings, not
> to the CSS that styles them.

I should note that this exactly parallels the logic for adding ifdefs to XUL files to differentiate platform-specific strings, providing multiple entities for different platforms so that localization packs don't have to have OS-specific packs.

Comment 18

[Asaf Romano (gone)]

Er, you can provide platform-specific style sheets in themes.

Comment 19

[Reed Loden [:reed]]

Checking in toolkit/themes/winstripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/winstripe/global/jar.mn,v  <--  jar.mn
new revision: 1.36; previous revision: 1.35
done
Checking in toolkit/themes/winstripe/global/dirListing/dirListing.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/dirListing/dirListing.css,v  <--  dirListing.css
new revision: 1.5; previous revision: 1.4
done

Comment 20

[Boris Zbarsky [:bzbarsky]]

> (and %ifndef MOZ_WIDGET_GTK2 for the winstripe .up icon)

I think the right fix here is to use the same stock-icon URI for all platforms and patch the non-linux icon impls to use the chrome:// or res:// or whatever icon as the stock icon on that OS.  That would incidentally make these easily available at other places in the code in a uniform way, as needed.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Then again, that would screw over platforms without a -moz-icon: implementation.  But I guess the new file listing has already screwed them over...  It might really be a good idea to have some sort of "xp" fallback -moz-icon impl that would at least do something sane (e.g. unknown file icon) or something instead of simply showing nothing.