230219 - go button is set off even when set on in prefs in installer

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Jan Matousek]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; cs-CZ; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; cs-CZ; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6

when customizing mozilla installer, if preference browser.toolbars.showbutton.go
is set on in the preference file all.js, when install this modified inslaller,
the go-button is hidden. I know i can edit navigator.xul too and set
"go-button"'s attribute hidden to false to show it, but i think it will be
better to do this only through prefs and do not edit xul files at all.

Reproducible: Always

Steps to Reproduce:
1. customize installer - set the browser.toolbars.showbutton.go pref to true
2. install this installer

Actual Results:  
the go-button is off

Expected Results:  
the go-button should be on

Comment 1

[Jan Matousek]

of course, this is about mozilla installer, not firebird instaler as the
UserAgent header look like. sorry :)

Comment 2

[[e-mail]]

Attachment: (Av1) <navigator.js> Proposed patch [Checked in: Comment 13]

Comment 3

[[e-mail]]

Comment on attachment 138531 [details] [diff] [review]
(Av1) <navigator.js> Proposed patch
[Checked in: Comment 13]

Always assuming that this is what we should be doing

Comment 4

[:deanis74]

Comment on attachment 138531 [details] [diff] [review]
(Av1) <navigator.js> Proposed patch
[Checked in: Comment 13]

Why don't the other pref listeners need to be initialized like this?

Comment 5

[[e-mail]]

Well, an obvious example, the home button tooltip is already initialized...

Comment 6

[:deanis74]

Comment on attachment 138531 [details] [diff] [review]
(Av1) <navigator.js> Proposed patch
[Checked in: Comment 13]

Is this still relevant?  If so, r=me.

Comment 7

[Alec Flett]

Comment on attachment 138531 [details] [diff] [review]
(Av1) <navigator.js> Proposed patch
[Checked in: Comment 13]

are these listeners ever removed? Do we really need to set listeners on ever
pref in the domain? I thought the way pref listeners worked, you could just
listen to the toplevel domain?

i.e. this.observe(pref, "nsPref:changed", this.domain);

Comment 8

[[e-mail]]

I'm not adding or removing listeners, I'm just poking the existing listener so
that it sees the initial value.

Comment 9

[Alec Flett]

Comment on attachment 138531 [details] [diff] [review]
(Av1) <navigator.js> Proposed patch
[Checked in: Comment 13]

oh, duh. I see. I think I even wrote the original code :)

You're forcing the observe() to be called, when it would normally be called by
a pref change

can you put a comment to that affect in init()? something like "Force observe()
to be called so that we get the initial value of the pref on startup"

(I realize your comment "Ensure button visibility.." hints at this, but
still..)

sr=alecf with the comment

Comment 10

[R.K.Aa.]

bug 211829 dup of this?

Comment 11

[Alec Flett]

Comment on attachment 138531 [details] [diff] [review]
(Av1) <navigator.js> Proposed patch
[Checked in: Comment 13]

this is a pretty minor change and sure would be nice to get in for 1.7

Comment 12

[chris hofmann]

Comment on attachment 138531 [details] [diff] [review]
(Av1) <navigator.js> Proposed patch
[Checked in: Comment 13]

a=chofmann for 1.7

Comment 13

[[e-mail]]

Fix checked in.

Comment 14

[R.K.Aa.]

so to what extent did this fix bug 166455?

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <navigator.js> additional comment and cleanup

Adds:

1)
(In reply to comment #9)
> can you put a comment to that affect in init()? something like "Force
observe()
> to be called so that we get the initial value of the pref on startup"

2)
(In reply to bug 166455 comment 14)
> In fact lines 93-105 of navigator.js could probably be cleaned up too.

Comment 16

[Boris Zbarsky [:bzbarsky]]

hmm.. This checkin was responsible for about 0.5% regression in Ts on both comet
and luna.  Anything we can do to mitigate that?

Comment 17

[[e-mail]]

Hmm... it might be because the allLeftButtonsAreHidden() is getting called five
times; I'll have to try special-casing the startup code to fix that.

Comment 18

[[e-mail]]

Attachment: (Cv1) <navigator.js> Startup optimization

Comment 19

[[e-mail]]

Comment on attachment 145901 [details] [diff] [review]
(Cv1) <navigator.js> Startup optimization

As well as separating out the button and separator update I used .hidden
because the startup check makes persist unnecessary (see attachment 145839 [details] [diff] [review]).

Comment 20

[[e-mail]]

Attachment: (Cv1b) <navigator.js> Extra bulletproofing [Checked in: Comment 26]

Comment 21

[[e-mail]]

Comment on attachment 145902 [details] [diff] [review]
(Cv1b) <navigator.js> Extra bulletproofing
[Checked in: Comment 26]

...is needed for bug 230219

Comment 22

[R.K.Aa.]

Attachment 145902 [details] [diff] should also go into 1.7 - nominating as blocker
(Or there will be a lot of reports like bug 240288)

Comment 23

[James Rome]

The builds of Mozilla with this fix checked in have been unusable since 4/10.
Can they please be removed?

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 145902 [details] [diff] [review]
(Cv1b) <navigator.js> Extra bulletproofing
[Checked in: Comment 26]

sr=dveditz
this works

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 145902 [details] [diff] [review]
(Cv1b) <navigator.js> Extra bulletproofing
[Checked in: Comment 26]

a=asa (on behalf of drivers) for checkin to 1.7

Comment 26

[[e-mail]]

Fix checked in.

Comment 27

[[e-mail]]

*** Bug 211829 has been marked as a duplicate of this bug. ***