Last Comment Bug 666901 - docshell should use mozilla::Preferences
: docshell should use mozilla::Preferences
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on: 656826
Blocks: 309654
  Show dependency treegraph
 
Reported: 2011-06-24 04:13 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2013-12-06 06:59 PST (History)
4 users (show)
jruderman: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (32.96 KB, patch)
2011-06-24 04:13 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bzbarsky: review+
Details | Diff | Review
followup: remove now-unused variable |nsresult rv;| (907 bytes, patch)
2011-08-24 12:04 PDT, Daniel Holbert [:dholbert]
masayuki: review+
Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-24 04:13:03 PDT
Created attachment 541655 [details] [diff] [review]
Patch v1.0
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-24 12:23:23 PDT
Comment on attachment 541655 [details] [diff] [review]
Patch v1.0

>+    if (prefPrefix)  // XXX why don't you test by IsEmpty()?

Because having an empty prefix is a totally reasonable thing to want.  Please remove that comment.

>+    if (prefSuffix) // XXX why don't you test by IsEmpty()?

Likewise.

>+++ b/docshell/base/nsDocShell.cpp
> PingsEnabled(PRInt32 *maxPerLink, PRBool *requireSameHost)
>     if (allow) {
>+    Preferences::GetInt(PREF_PINGS_MAX_PER_LINK, maxPerLink);
>+    Preferences::GetBool(PREF_PINGS_REQUIRE_SAME_HOST, requireSameHost);
>   }

Fix the indent, please.

> nsDocShell::SetUseErrorPages(PRBool aUseErrorPages)
>     if (mObserveErrorPages) {
>+        Preferences::RemoveObserver(this, "browser.xul.error_pages.enabled");
>             mObserveErrorPages = PR_FALSE;
>         }

Fix indent here too.

>@@ -4555,22 +4529,19 @@ nsDocShell::Destroy()
>     if (mObserveErrorPages) {
>+        Preferences::RemoveObserver(this, "browser.xul.error_pages.enabled");
>             mObserveErrorPages = PR_FALSE;
>         }

And here.

>@@ -9207,33 +9170,27 @@ nsresult nsDocShell::DoChannelLoad(nsICh
>+        switch (Preferences::GetInt("browser.cache.check_doc_frequency", -1)) {

And around here.

>+++ b/docshell/shistory/src/nsSHistory.cpp
> nsSHistoryObserver::Observe(nsISupports *aSubject, const char *aTopic,
....
>   if (!strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) {
>+    nsSHistory::UpdatePrefs();
>       nsSHistory::EvictGlobalContentViewer();
>   } else if (!strcmp(aTopic, NS_CACHESERVICE_EMPTYCACHE_TOPIC_ID) ||

And here.

>+nsSHistory::UpdatePrefs()
>+  Preferences::GetInt(PREF_SHISTORY_MAX_TOTAL_VIEWERS,
>                           &sHistoryMaxTotalViewers);

And here.

> nsSHistory::Startup()

And all over the place here.

>+    if (!gObserver) {

Can't happen, so don't check for it.

r=me with the nits fixed.
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-24 18:59:13 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/9c2e57bef564

# I'm sorry, I attached -w patch that's the cause of the wrong indentation.
Comment 3 Marco Bonardo [::mak] 2011-06-25 03:33:56 PDT
http://hg.mozilla.org/mozilla-central/rev/9c2e57bef564
Comment 4 Daniel Holbert [:dholbert] 2011-08-24 12:04:49 PDT
Created attachment 555482 [details] [diff] [review]
followup: remove now-unused variable |nsresult rv;|

>@@ -484,27 +466,27 @@ PRBool nsDefaultURIFixup::MakeAlternateU
> 
> 
>     nsresult rv;
> 
>     // Get the prefix and suffix to stick onto the new hostname. By default these
>     // are www. & .com but they could be any other value, e.g. www. & .org
> 
>     nsCAutoString prefix("www.");
>-    nsXPIDLCString prefPrefix;
>-    rv = mPrefBranch->GetCharPref("browser.fixup.alternate.prefix", getter_Copies(prefPrefix));
>-    if (NS_SUCCEEDED(rv))
[...]
>     nsCAutoString suffix(".com");
>-    nsXPIDLCString prefSuffix;
>-    rv = mPrefBranch->GetCharPref("browser.fixup.alternate.suffix", getter_Copies(prefSuffix));
>-    if (NS_SUCCEEDED(rv))

These removed lines ^ were the only uses of the variable |nsresult rv| declared at the top of the context quoted there.  So we now get this build warning:
> docshell/base/nsDefaultURIFixup.cpp: In member function ‘PRBool nsDefaultURIFixup::MakeAlternateURI(nsIURI*)’
> docshell/base/nsDefaultURIFixup.cpp:468:14: warning: unused variable ‘rv’ [-Wunused-variable]

The |nsresult rv;| can just go away.  Attached followup-patch fixes this.
Comment 5 Daniel Holbert [:dholbert] 2011-08-24 19:42:51 PDT
Landed followup: http://hg.mozilla.org/integration/mozilla-inbound/rev/31c4c7ad46d8
Comment 6 Marco Bonardo [::mak] 2011-08-25 04:38:46 PDT
http://hg.mozilla.org/mozilla-central/rev/31c4c7ad46d8

Note You need to log in before you can comment on or make changes to this bug.