549746 - Back/Forward button not showing up in FF. DOM error?

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jeremy Huddleston]

This was reported in bug #527612 but is not related to that issue:

application does not have all the chrome properly installed, as shown in the
attached image.

DOM inspector for the OBJDIR version, shows the following for the forward
button:

list-style-image:
url("moz-icon://stock/gtk-go-forward-ltr?size=toolbar&state=disabled");

while that computed style entry does not exist when running from INSTALLDIR
(although the forward-button (and other missing icons) do show up as nodes in
DOM Inspector).  I think this is a different bug, but I don't have any time
points that differentiate between this and comment #0.  I will open a new bug
if this is a different issue.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

What do you mean by "not showing up"? Do you mean the icon on the button, or the button itself? It's not clear to me why you're bringing up the objdir vs. installdir difference, either.

Comment 2

[Hanspeter Niederstrasser]

Gavin,

I was the commenter that brought up that issue in the other bug.  I brought up objdir vs installdir because firefox had all chrome available when running from objdir/dist/bin, but not when running from installdir/bin.  It's been a while since I looked at this specific issue, but if I remember correctly, it was the icons on the buttons that were not showing up, but the buttons could still be seen on hover/click, but they were very narrow (about 10-20% the normal width).

I tracked down the issue to package-manifest looking for libimgicon.so, but it was being built as libimgicon.dylib (Darwin/X11) ...so it wasn't getting installed and so most chrome icons weren't being loaded when firefox was run from installdir/bin.  I patched package-manifest.in to look for libimgicon.dylib and that made chrome look correct when running firefox from installdir.

Comment 3

[Jeremy Huddleston]

Attachment: untested possible fix

This is likely the fix then.  I'll test it before requesting a review.

Comment 4

[Jeremy Huddleston]

Comment on attachment 429857 [details] [diff] [review]
untested possible fix

Tested fix

Comment 5

[Hanspeter Niederstrasser]

Attachment: patch against mozilla-central [Checkin: Comment 6]

Jeremy, that's a much cleaner fix than my directly changing .so to .dylib in package-manifest.  I updated your patch to 1) reflect slight differences between mozilla-central and 1.9.2, and 2) removed the '.' between FILENAME and @DLL_SUFFIX@ since that macro already adds a period.

Comment 6

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/2b8a0778faf0

Comment 7

[(not currently active) Ted Mielczarek]

I actually just wrote a patch and filed bug 553635 today, which will get rid of this library (link it into libxul).

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-CC) Copy it to SeaMonkey

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a-CC) Copy it to SeaMonkey, Unify other cases

Bv1-CC, with additional nits.

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 434249 [details] [diff] [review]
(Bv1a-CC) Copy it to SeaMonkey, Unify other cases

technically good to go; but over to KaiRo as I'm not sure if the preprocessing on the OS-Specific files is needed/warranted in most of these cases.

Comment 11

[Robert Kaiser]

Comment on attachment 434249 [details] [diff] [review]
(Bv1a-CC) Copy it to SeaMonkey, Unify other cases

>-@BINPATH@/components/libalerts_s.dylib
>+@BINPATH@/components@DLL_PREFIX@alerts_s@DLL_SUFFIX@

1) This hunk is wrong, it misses a / character.

2) I see no reason for making single-platform lines harder to read.

3) I don't see any real change in there, nothing you did actually "copy".

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2-CC) Copy it to SeaMonkey, Unify other cases [Moved to bug 717702]

Bv1a-CC, with comment 11 suggestion(s).

(In reply to comment #11)
> 1) This hunk is wrong, it misses a / character.

Good catch: fixed.

> 2) I see no reason for making single-platform lines harder to read.

See bug 554017 comment 2...

> 3) I don't see any real change in there, nothing you did actually "copy".

I copied this bug and bug 554017.

Comment 13

[Robert Kaiser]

Comment on attachment 435392 [details] [diff] [review]
(Bv2-CC) Copy it to SeaMonkey, Unify other cases
[Moved to bug 717702]

(In reply to comment #12)
> > 2) I see no reason for making single-platform lines harder to read.
> 
> See bug 554017 comment 2...

Things like that deserve their own bug with a fitting description that describes that you're changing how we do our stuff in those files.

> > 3) I don't see any real change in there, nothing you did actually "copy".
> 
> I copied this bug and bug 554017.

I still don't see anything in this patch that actually deals with the bug it's attached to.

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 435392 [details] [diff] [review]
(Bv2-CC) Copy it to SeaMonkey, Unify other cases
[Moved to bug 717702]

(In reply to Robert Kaiser (:[e-mail]) from comment #13)

> Things like that deserve their own bug with a fitting description that
> describes that you're changing how we do our stuff in those files.

Bug 717702, at last.

> I still don't see anything in this patch that actually deals with the bug
> it's attached to.

'nullplugin' and 'imgicon' lines were copied++ from this bug.
There are gone in the meantime anyway.