814723 - Warning about undefined preference 'browser.newtab.url'

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> I/Gecko   ( 1470): [Child 1470] WARNING: NS_ENSURE_SUCCESS(rv, true) failed with result 0x8000FFFF: file /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-801658/docshell/base/nsDocShell.cpp, line 10323

When entering the home screen (e.g., when waking up from suspend), this warning appears because the preference 'browser.newtab.url' is missing.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Set preference browser.newtab.url to about:blank

Comment 2

[Justin Lebar (not reading bugmail)]

I don't think this is the right fix.

This pref starts with "browser." because it's a pref for the "browser", by which we mean the desktop browser, Firefox.  It's only read from files inside browser/ and in that one place in nsDocShell.  So I don't think it makes sense to define it for B2G.

I'd r+ a patch which changes the pref check from an NS_ENSURE_TRUE to an if (NS_FAILED(rv)), which would have the same effect.

Comment 3

[Justin Lebar (not reading bugmail)]

Note that fixing this by modifying nsDocShell will also get rid of the warning on Fennec and other products.

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Return default value if key is missing

This patch silences the warning by simply returning 'true' if the key is not present.

Comment 5

[Justin Lebar (not reading bugmail)]

Comment on attachment 685234 [details] [diff] [review]
Return default value if key is missing

>+    rv = Preferences::GetDefaultCString("browser.newtab.url", &pref);
>+
>+    if (NS_FAILED(rv) && rv == NS_ERROR_UNEXPECTED) {
>+        // We assume 'true' if 'browser.newtab.url' is not
>+        // explicitly set, but don't warn about it.
>+        return true;
>+    }

It's sufficient to check |if (NS_FAILED(rv))| -- we avoid checking for specific
error codes in Gecko where we can help it.

Also, I'm not wild about this comment; instead of saying what we're doing, we
should explain /why/ we're doing it.  Maybe something like "Apps other than the
browser don't have the 'browser.newtab.url' pref set." would be sufficient.  Or
I'd be OK if you left off the comment altogether.

>+    NS_ENSURE_SUCCESS(rv, true);

If we get rid of the rv == NS_ERROR_UNEXPECTED check above, we don't need this check.

r=me with these nits picked.

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Return default value if key is missing [v2]

An update according to the review.

Comment 7

[Justin Lebar (not reading bugmail)]

(In reply to Thomas Zimmermann from comment #6)
> Created attachment 685551 [details] [diff] [review]
> Return default value if key is missing [v2]
> 
> An update according to the review.

Since you got r+ on the last patch, you don't need to ask for another review; you can go straight to checkin-needed.  Unless of course you want my input on the new patch, in which case you're welcome to ask for review.

Comment 8

[Justin Lebar (not reading bugmail)]

The only thing you need here before going to checkin-needed is "r=jlebar" in the first line of the checkin message.

Although since you asked for review: I might remove the newline between the Preferences:: call and the if statement.  But that's up to you.  :)

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Return default value if key is missing [v3]

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks a lot!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d5420af2c27

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d5420af2c27

Comment 13

[Chris Kreinbring [:CKreinbring]]

Verified in Unagi version 20130102070202.