532596 - Set Default Browser button in Preferences disappears if Home Page selection is blank

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Andy Boze]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.7pre) Gecko/20091202 SeaMonkey/2.0.1pre compatible Firefox/3.5.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.7pre) Gecko/20091202 SeaMonkey/2.0.1pre

In the Browser Preferences, if the Home Page selection box is left blank, the Set Default Browser button and its fieldset (I assume) container disappear.

Reproducible: Always

Steps to Reproduce:
1. Open Preferences | Browser
2. In the Home Page selection, clear the box so it is blank.
3. Close Preferences Window.
4. Open Preferences | Browser
Actual Results:  
The Default Browser fieldset is gone.

Expected Results:  
The Default Browser fieldset should still appear.

Don't know whether this is a Windows-only problem. The Default Browser fieldset never appears in Linux. Should it?

Comment 1

[rsx11m]

That new default-client code was introduced by bug 441050. I can reproduce it [Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.7pre) Gecko/20091202 SeaMonkey/2.0.1pre] as described, no duplicate found, thus confirming.

And yes, that part of the preference dialog doesn't seem to exist on Linux.

Comment 2

[Philip Chee]

The problem also existed in 1.1.x, only the symptoms were different.

Error: GetHomePagePref(0) is null
Source file: chrome://communicator/content/pref/pref-navigator.js
Line: 285

>  gDefaultHomePage = CanonifyURLList(GetHomePagePref(0).defaultValue);

This line failed because there is no home page pref at index 0 so all subsequent lines do not get executed.

>  SetHomePageValue(homePageGroup);
>  UpdateHomePageButtons();

>  // platform integration
>  InitPlatformIntegration();

This last line turns on the group box with the default browser button.

Comment 3

[rsx11m]

Attachment: Simple fix

Thanks Phil. For some strange reason, I didn't get the idea to look into the Error Console myself.

This patch catches now an empty home page. I made it a conditional operator
for simplicity, and it shouldn't call CanonifyURLList() unless count>0. I can expand this to a full if statement in case that's preferred.

Comment 4

[Philip Chee]

> +  gDefaultHomePage = (count > 0 ? CanonifyURLList(GetHomePagePref(0).defaultValue) : "");

I actually tried that (the most obvious line to fix) but for some reason the next time I started up SeaMonkey it would try to open a home page that was simply "a" (not "http://a" or anything, just |a|)

And FYI, you don't need the (brackets) here. The ternary operator has a higher precedence than the assignment operator.

Comment 5

[rsx11m]

Well, that seems to be a problem unrelated to the patch. Apparently removing a home page from the list does not reset the related browser.startup.homepage.* preference, even though browser.startup.homepage.count is reduced to the new value and HomePagePrefCleanup() should take care of resetting those prefs. That's done in UpdateHomePagePref() with a reference to bug 410562 workaround.

In addition, getHomePage() in navigator.js always sets the first URI to the browser.startup.homepage value regardless of the count. Setting that pref to an empty string gives me an about:blank home page as desired. However, adding a count > 0 condition into that function yet opens the homepage, so I'm still missing something. Apparently your 'a' was a residual of that preference.

Comment 6

[rsx11m]

Attachment: Proposed patch (v2)

This patch takes care of comment #4 of the user entering an empty home page list by defaulting it to "about:blank" in this case (which may be also more intuitive for the user if there is always something in the list).

That's a workaround to the problem, but there are various places where it is assumed that the home page list is not empty, thus that would need a more thorough look into those. If it's ok with the reviewers, I'd spin that aspect off to a new bug to look into a proper case handling and cleanup if the list is reduced (which doesn't seem to properly occur based on comment #5), so that the bug here can concentrate on solving the browser-default button issue.

Comment 7

[[e-mail]]

Comment on attachment 416077 [details] [diff] [review]
Proposed patch (v2)

>+  gDefaultHomePage = count > 0 ? CanonifyURLList(GetHomePagePref(0).defaultValue) : "about:blank";
This code is definitely wrong. The default home page is just the default value of the default home page preference, and is never blank.

We're always going to need the default home page preference on startup so that we can calculate the default home page. This will at the very least need some rewriting of the startup code. It may be worthwhile to add the default home page preference element to the XUL.

After that you can then decide whether, and if so, how, you want to enforce the minimum home page count of 1. This could be done in a number of ways, and I'm not too keen on the one you chose, but here's the list for completeness:
1. If the home page group array turns out to be empty, set the default home page to about:blank anyway
2. If the home page group turns out to be empty, set it to about:blank before turning it into an array
3. If the home page group array turns out to be empty, unshift about:blank before setting the prefs
4. Write the default home page separately, defaulting to about:blank; since the pref always exists so you don't need to call AddHomePagePref(0)

Comment 8

[rsx11m]

Attachment: Proposed patch (v3)

Ok, now I understand better what that code is actually doing. The problem is not that the case has to be caught where the default preference doesn't exist, but the issue is that the <preference> node for browser.startup.homepage is missing in the first place.

> (In reply to comment #7) It may be worthwhile to add the default home
> page preference element to the XUL.

Done, this patch explicitly contains a <preference> for the default home page. Furthermore, Startup() and HomePagePrefCleanup() do no longer create or remove it based on an index of zero. In fact, browser.startup.homepage.count can no longer get less than 1, thus a value below that is an error case.

Considering the previous sentence, I'm distinguishing between the case where the user empties the textbox manually (then, "about:blank" will be used with a count of 1) in contrast to the case where count<1 on startup (this is assumed to be an error in the preferences, therefore the home page is reset to default as if the "Restore Default" button had been clicked on). This distinction seems to make sense, and the latter case should only occur on preference corruption or if you set the count reference manually to zero.

Comment 9

[[e-mail]]

Comment on attachment 416313 [details] [diff] [review]
Proposed patch (v3)

>+  else
>+  {
>+    // set to about:blank so that we don't have an empty list
>+    newCount = 1;
>+    GetHomePagePref(0).value = "about:blank";
So you didn't appreciate any of my other suggestions? At the very least you could move the newCount = 1 into the initialisation.

>   var count = GetHomePagePrefCount();
>   for (var j = count; j < gHomePagePrefPeak; ++j)
>   {
>-    // clear <preference>
>+    // clear <preference>, don't remove the default home page pref
>     var pref = GetHomePagePref(j);
>     pref.reset();
>-    pref.parentNode.removeChild(pref);
>+    if (j > 0 )
>+      pref.parentNode.removeChild(pref);
I'm not sure that count can be zero here, since this is called from UpdateHomePagePrefs which now always ensures the count is at least 1.

>   for (var i = 0; i < count; ++i)
>   {
>-    var pref = AddHomePagePref(i);
>+    var pref = i > 0 ? AddHomePagePref(i) : GetHomePagePref(0);
This looks a little ugly to me, perhaps you can do the first pref before the loop? That mirrors what the code in navigator.js etc. does.

Comment 10

[rsx11m]

(In reply to comment #9)
> So you didn't appreciate any of my other suggestions? At the very least you
> could move the newCount = 1 into the initialisation.

I figured that was the equivalent of your suggestion #1 in comment #7,
"If the home page group array turns out to be empty, set the default home
page to about:blank anyway", certainly I missed the initialization to 1.

Option #2 would require to look if the homePageGroup contains anything other than white space, which the initialization of that variable is doing already with the split() operator. I'm not sure what you meant with "unshift" in your option #3. The last option #4 would put the GetHomePagePref(0).value assignment before the loop rather than being part of the if/else statement, the latter may be easier for understanding and avoids a back-to-back assignment if index 0. Unless I misunderstood any of your suggestions, I'd still stick with the current version (but with the newCount=1 moved to the initialization). 

> I'm not sure that count can be zero here, since this is called from
> UpdateHomePagePrefs which now always ensures the count is at least 1.

There is an explicit call in UpdateHomePagePrefs(), that's correct, but there is also an EventListener defined later, so that may be deferred. I figured it cannot hurt to double-check that we don't remove the default preference, even though it may be redundant.

> This looks a little ugly to me, perhaps you can do the first pref before the
> loop? That mirrors what the code in navigator.js etc. does.

I can check the count>0 upfront and have this inside the if() part with the index-0 assignment moved before the loop, that should work and avoid the conditional operator.

Comment 11

[[e-mail]]

(In reply to comment #10)
> I figured that was the equivalent of your suggestion #1 in comment #7,
Yes, but they were in ascending order of preference ;-)

> Option #2 would require to look if the homePageGroup contains anything other
> than white space,
CanonifyURLList does that for you, by collapsing it into an empty string.

> I'm not sure what you meant with "unshift" in your option #3.
It's an array method ;-)

> There is an explicit call in UpdateHomePagePrefs(), that's correct, but there
> is also an EventListener defined later, so that may be deferred.
Yes, but UpdateHomePagePrefs controls the event listener too.

> > This looks a little ugly to me, perhaps you can do the first pref before the
> > loop? That mirrors what the code in navigator.js etc. does.
> I can check the count>0 upfront and have this inside the if() part with the
> index-0 assignment moved before the loop, that should work and avoid the
> conditional operator.
You don't need to check upfront since the default home page always exists.

Comment 12

[rsx11m]

Attachment: Proposed patch (v4)

(In reply to comment #11)
> (In reply to comment #10)
> > I figured that was the equivalent of your suggestion #1 in comment #7,
> Yes, but they were in ascending order of preference ;-)

Ok, you didn't quite say that. :-) Thus, I'm initializing now index 0 with about:blank and it will be overwritten if the homePageGroup isn't empty.
That should be close enough to #4 and requires the least amount of statements.

> > There is an explicit call in UpdateHomePagePrefs(), that's correct, but there
> > is also an EventListener defined later, so that may be deferred.
> Yes, but UpdateHomePagePrefs controls the event listener too.

I removed that part.

> You don't need to check upfront since the default home page always exists.

Good point. Thus, index 0 is now written before the loop and 1+ inside, where I also simplified the assignment by getting rid of the redundant pref variable.

Comment 13

[[e-mail]]

Comment on attachment 416370 [details] [diff] [review]
Proposed patch (v4)

>-  SetHomePageValue(homePageGroup);
>+
>+  if (count > 0)
>+    SetHomePageValue(homePageGroup);
>+  else
>+    SetHomePageToDefaultPage();
I don't think you need this change any more, do you? You're picking up the default page automatically now.

Comment 14

[rsx11m]

One behavior that bothered me during testing was that a count=0 resulted in the previous home page still being used as home page, even though this is more of an "undefined" in its meaning (comment #8). The motivation for setting it back to the default was to (a) give the user a hint "hey, something is wrong" and (b) to get the setup back to a "defined" state, which I'd assumed should be best the default as no other valid/trustworthy setting exists.

Comment 15

[rsx11m]

Neil, was your comment #13 a question or a request to post a new patch with
that portion removed? I still think it makes sense thought to keep it.

Comment 16

[[e-mail]]

(In reply to comment #14)
> One behavior that bothered me during testing was that a count=0 resulted in the
> previous home page still being used as home page, even though this is more of
> an "undefined" in its meaning (comment #8). The motivation for setting it back
> to the default was to (a) give the user a hint "hey, something is wrong" and
> (b) to get the setup back to a "defined" state, which I'd assumed should be
> best the default as no other valid/trustworthy setting exists.
And the motivation for not setting it back to the default is that this way it shows the page you're currently getting as your home page, rather than changing your home page (or worse, suggesting it has changed back when it hasn't).

(In reply to comment #15)
> Neil, was your comment #13 a question or a request to post a new patch with
> that portion removed? I still think it makes sense thought to keep it.
Both, really; nothing happened because the first time I read comment #14 I couldn't figure out what you meant, but now I know you're wrong ;-)

Comment 17

[rsx11m]

Attachment: Proposed patch (v5) [Checkin: Comment 19]

All right, here now the previous (v4) patch without resetting the home page to
default for an invalid count per comment #13. Instead, just the current value
of the 0-index home page preference is filled into the textarea.

Guess I was overthinking this a bit and wanted to add too many safeguards.  :-)

Comment 18

[rsx11m]

Thanks for the reviews and the branch approval.
Push for comm-central and comm-1.9.1, please.

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 417206 [details] [diff] [review]
Proposed patch (v5)
[Checkin: Comment 19]


http://hg.mozilla.org/comm-central/rev/8d1e7a4e4cd5
http://hg.mozilla.org/releases/comm-1.9.1/rev/60602e374984

Comment 20

[rsx11m]

Fixed in today's 2.0.2pre nightly build.

I've filed bug 535128 as a follow-up to comment #5, removing a home page from the list does not reset the related browser.startup.homepage.* preference.