Closed Bug 236138 Opened 20 years ago Closed 20 years ago

Bulletproof startup against a silly lack of preferences

Categories

(Core Graveyard :: Cmd-line Features, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.7beta

People

(Reporter: benjamin, Assigned: benjamin)

Details

Attachments

(1 file, 1 obsolete file)

I got my build into a situation where the chrome://browser-locale/locale/ chrome
package wasn't registered properly, and instead of failing gracefully, the app
crashed because the browser.startup.homepage pref wasn't present. This is a
little patch to bulletproof against that situation.
Attachment #142700 - Flags: superreview?(darin)
Attachment #142700 - Flags: review?(darin)
Comment on attachment 142700 [details] [diff] [review]
bulletproof missing browser.startup.homepage pref

> NS_IMETHODIMP nsBrowserContentHandler::GetDefaultArgs(PRUnichar **aDefaultArgs)
...
>+  nsXPIDLString args;
...
>   if (NeedHomepageOverride(prefs)) {
>     nsXPIDLString url;
>     rv = prefs->GetLocalizedUnicharPref(PREF_HOMEPAGE_OVERRIDE_URL, getter_Copies(url));
>     if (NS_SUCCEEDED(rv) && (const PRUnichar *)url) {
>       args = url;
>     }

it looks like you could avoid this copy by just using args directly in the
getter_Copies above.  in fact, there doesn't even seem to be much point to
inspecting |rv| if you assume that GetLocalizedUnicharPref will return an
empty string whenever it returns NS_FAILED(rv).


>+  if (!args.get()) {

i think |args.IsEmpty()| is better.


>         case 1: {
>           // skip the code below
>+          rv = GetHomePageGroup(prefs, getter_Copies(args));
>+          break;

why assign a value to |rv|?  it is never referenced down below.  hmm... looks
like
the other cases have the same problem.	care to clean those up as well?


r+sr=darin with those changes.


can this situation happen to a normal user?  or were you doing something
unusual?
do we need to worry about older releases?  is this a candidate for a 1.6.1 if
that
ever happens?
Attachment #142700 - Flags: superreview?(darin)
Attachment #142700 - Flags: superreview+
Attachment #142700 - Flags: review?(darin)
Attachment #142700 - Flags: review+
Neil made some good suggestions in bug 236208 which intersect with this, so I
just integrated them... we can use early returns to drop the xpidlstring
altogether.
Attachment #142700 - Attachment is obsolete: true
Comment on attachment 142756 [details] [diff] [review]
do-over, with some extra code from Neil

darin, could you just look over this again to make sure I didn't do anything
stupid?
Attachment #142756 - Flags: superreview?(darin)
Comment on attachment 142756 [details] [diff] [review]
do-over, with some extra code from Neil

>Index: nsBrowserInstance.cpp

>+              *aDefaultArgs = ToNewUnicode(NS_ConvertUTF8toUTF16(curl));

this is what "UTF8ToNewUnicode" is for :-)

sr=darin with that change.
Attachment #142756 - Flags: superreview?(darin) → superreview+
fixed on trunk

Neil, I incorporated your changes, so all you need to do in bug 236208 is change
cargs from nsXPIDLCString to nsCString and remove the getter_Copies.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.7beta
Product: Core → Core Graveyard
QA Contact: bugzilla → cmd-line
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: