18940 - browser.startup.homepage not read from all.js

Status: VERIFIED FIXED

(SeaMonkey :: General, defect, P3)

Description

[Daniel (Leaf) Nunes]

Changing the pref in mozilla/modules/libpref/src/init/all.js does not change the
default start page. Other prefs get read fine from all.js, however (like
general.startup.mail).

Comment 1

[Daniel (Leaf) Nunes]

http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/locale/en-US/navigator.dtd#18
and
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator.xul#298

make me afraid. someone tell me that locale files aren't overriding prefs.
please. i'm begging you.

Comment 2

[Ben Bucksch (:BenB)]

It looks like the following to me: We have 2 functions reading the homepage:
<URL:http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator.js#544>
and
<URL:http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#1376>,
which uses
<URL:http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.h#50>.
Both 1. don't get the default pref (CopyDefaultCharPref) and 2. aren't used at
all.

The homepage is set at
<URL:http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator.js#447>
("var defaultURL = homebutton.getAttribute("defaultURL" );"), which uses
<URL:http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator.xul#298>,
which uses
<URL:http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/locale/en-US/navigator.dtd#18>.

Trying to fix.

Comment 3

[Ben Bucksch (:BenB)]

Attachment: Fix for GlobalWindow and Navigator.js

Comment 4

[Ben Bucksch (:BenB)]

It's a quick and dirty hack (30 lines from another function in the file copied),
but WORKSFORME - not more.

Please test.

Comment 5

[(not reading, please use [e-mail] instead)]

yes, someone added js that sets the pref from the dtd value!

that shit ain't cool.

I've got the fix, benb ([e-mail]) is reviewing now.

Comment 6

[(not reading, please use [e-mail] instead)]

Attachment: here's a patch for this bug. I've also changed it to cache the prefs service for performance.

Comment 7

[(not reading, please use [e-mail] instead)]

Attachment: the fix again, after comments from benb

Comment 8

[(not reading, please use [e-mail] instead)]

we've got the fix, benb is reviewing now.

re-assign to me, add benb to the cc list, and change component from libPref to
browser-general.

also add davidm to the cc list.

Comment 9

[(not reading, please use [e-mail] instead)]

fixed.  thanks to benb for the help.

the bug was caused by this:

we were setting the default home page in the dtd and making it an attribute of
the home button.  (why?)  this value is a pref and should be set in all.js

then, we'd get that attribute of the home page and set the default value of the
home page pref!

yikes.

Comment 10

[davidm]

What are you trying to do? First off I do not see what the code you are adding
is going to do. GetDefaultPref should not return a value that is more valid than
GetPref. None of this code has any performance impact so doing things like
caching had better be for other reasons. And finally there was a big long
meeting where it was decided that the default URL's for these prefs were to be
stored in DTD files. I assume you have permission to change this and that all
the reasons for storing it in the DTD file have either been addressed or are no
longer considered valid? If I have to deal with this again I will be distrubed.
If you have issues with storing url's in DTD files, selmers the man. I am just
following orders;)

Comment 11

[(not reading, please use [e-mail] instead)]

the home page has been been stored in prefs since god knows when.

we store url for the throbber in the dtd, yes.

but home page is a pref.

the changes were two fold:

one, cache the pref service.
two, fix it so we don't set the homepage pref, instead we get if from the prefs.

Comment 12

[(not reading, please use [e-mail] instead)]

I guess the big debate is should this url be put in the dtd or in the prefs?

I say prefs, since there is ui to change it, and you can't hook the pref ui to
change a dtd value.

Comment 13

[davidm]

Just to be absolutely clear. The question is where does the default value for
the pref comes from( all.js or some DTD file). As far as clients needing to
access the value they should be getting the pref  value.

Comment 14

[Ben Bucksch (:BenB)]

Yes, the user pref was used and is used. But the default pref came from the DTD.
I have to agree with Seth here. It is a default for a user pref and default
prefs are the usual way to set them. Why should the homepage be an exception?

I might see some arguments for doing so, like I18n or "Client Customation" by
ISPs, but the simple fact, that leaf search half a week for the setting should
give enough reason to stay with default prefs.

Comment 15

[Ben Bucksch (:BenB)]

(BTW: We still have this code trying to figure out a homepage:
<URL:http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#1376>
and
<URL:http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.h#50>.)

Comment 16

[davidm]

CCK is the reason. I do not understand what your complaint about nsGlobalWindow
is? In the case where there is no preferences, what would you prefer the code to
do? return and error or load about:blank spring to mind but I don't consider
either of them better than just loading mozilla.org

Comment 17

[(not reading, please use [e-mail] instead)]

the last bit of code that ben points out uses the same pref, and if we fail to
get that pref, we use a http://www.mozilla.org, cause its better than nothing.

keep in mind nsGlobalWindow::Home() may not be used anymore.  I need to look
into it..

Comment 18

[Ben Bucksch (:BenB)]

Davidm,
> I do not understand what your complaint about nsGlobalWindow

I have a problem with doubled code. There should be only one function that
performs a certain task or maintainance will be hell.

Comment 19

[davidm]

What doubled code?  The stuff in nsGlobalWindow implements the JS window API.
Now if the browser code looks up the pref rather than doing
window.content.home() that should be fixed. Remember also that nsGlobalWindow is
part of Gecko and not the browser.

Comment 20

[(not reading, please use [e-mail] instead)]

whoops, I removed the call to window.content.home().

I'll fix it now.

Comment 21

[Ben Bucksch (:BenB)]

Seems like it is used by
<URL:http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsJSWindow.cpp#1401>
(nsGlobalWIndow is a nsIDOMWindow).
I look at the function closer and I don't see good solution to solve the
redundancy (nsGlobalWIndow::Home(), navigator.js/StartUp() and
navigator.js/BrowserHome() all try to figure out a Homepage).

Forget it :).

Comment 22

[(not reading, please use [e-mail] instead)]

ok, now we are using window.content.home() again in navigator.js

no need to duplicate that code.

Comment 23

[Paul MacQuiddy]

verified

Comment 24

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/fec6e0634a84
[fenix] For https://github.com/mozilla-mobile/fenix/issues/18940 - Remove reserved icon spacing for "Nimbus Experiments" setting preference (https://github.com/mozilla-mobile/fenix/pull/18949)