256029 - OK doesn't work after changing home page and switching to another category

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[sairuh (rarely reading bugmail)]

couldn't find an exising bug which covers this. found using 2004081708-0.9+
firefox bits on linux (fedora 2); Tracy also sees this on today's Windows
aviary1.0 bits.

OK doesn't work after changing home page and switching to another category.

1. open Preferences (Options) and go to the General category.
2. manually (using keyboard) change the Home Page.
3. click another category (Advanced, Privacy, etc.).
4. click OK (or hit Enter) to save and dismiss Preferences.

expected: Prefs dlg should go away.
actual results: nothing happens, Prefs dlg persists. no js console output. :-\

workaround: go back to the General category, and OK/Enter work again.

Comment 1

[sairuh (rarely reading bugmail)]

if the fix is trivial, could this make it for PR? if not, then [also] nominating
for 1.0.

Comment 2

[Asa Dotzler [:asa]]

We can't ship with this. If not PR, then definitely final.

Comment 3

[Blake Ross]

I think we need to fix this for PR.

Comment 4

[Mike Connor [:mconnor]]

we're breaking here:

http://lxr.mozilla.org/aviarybranch/source/browser/components/prefwindow/content/pref-navigator.js#102

blame is bryner's, cheap hack is to just take the bits from utilityOverlay.js
and use those directly in the onOK function.

Comment 5

[Pierre Chanial]

Attachment: patch v1.0 diff -uw

This bug happens because |onOK()| is called as a callback function in a js
context in which |getShellService()| is not defined.
Unless lxr is drunk, there is no use of shouldBeDefaultBrowser in the code
base, but anyways the 'default browser' action is set on the fact, so we can
safely get rid of it in |onOK|.

There was two parallel switches to determine if the shell service is
implemented or not: one at run-time, and the other at compile-time. I am not
keen on making use of the xul/js preprocessor other than for platforms
discrimination and I think it's more confusing than helpful, I then removed the
preprocessor switch.

Comment 6

[Pierre Chanial]

Attachment: patch v1.0 diff -u

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 156901 [details] [diff] [review]
patch v1.0 diff -uw

What's with the all-caps variable naming? We don't use that anywhere else in
the frontend code that I can find.  Change it to shellSvc or shellService and
r=bryner.

Comment 8

[Pierre Chanial]

(In reply to comment #7)
> (From update of attachment 156901 [details] [diff] [review])
> What's with the all-caps variable naming? We don't use that anywhere else in
> the frontend code that I can find.  Change it to shellSvc or shellService and
> r=bryner.
> 

there are a lot of such instances: search.xml, printUtils.js, bookmarks.js.
The function initServices() in bookmarks.js was even planned (long long time
ago) to be moved to browser.js and make use of it.
I am not the investigator of such a convention (BMSVC, RDF, RDFCU predate my
involvement) but I sticked with it and therefore spread it.
I think that the all-caps convention for services is a good one that makes clear
that we are dealing with an interface.

Comment 9

[Pierre Chanial]

Attachment: patch v1.1 diff -uw

the pref component doesn't use the all-caps convention for services, let's use
shellSvc.
(the only difference between the -u and -uw diff is an indentation correction
in the default browser groupbox)

Comment 10

[Pierre Chanial]

Comment on attachment 157474 [details] [diff] [review]
patch v1.1 diff -uw

marking bryner's r+ and requesting aviary approval.

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 157474 [details] [diff] [review]
patch v1.1 diff -uw

a=asa for aviary checkin

Comment 12

[Pierre Chanial]

checked in trunk+aviary branch

Comment 13

[Asaf Romano (gone)]

regression on mac:
http://bugzilla.mozilla.org/show_bug.cgi?id=258110

Comment 14

[Tracy Walker [:tracy]]

verfied with Windows Firefox branch build 2004-09-09-08-0.9

Comment 15

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change