227032 - nsDownloadProxy::Init improvement

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

74     nsCOMPtr<nsIPrefService> prefs =
do_GetService("@mozilla.org/preferences-service;1", &rv);
 75     if (NS_FAILED(rv)) return rv;
 76     nsCOMPtr<nsIPrefBranch> branch = do_QueryInterface(prefs);

it would be better to use GetBranch

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

also did a few other changes

Comment 2

[[e-mail]]

Comment on attachment 136473 [details] [diff] [review]
patch

>+      if (NS_SUCCEEDED(rv)) {
>+        rv = branch->GetIntPref(DOWNLOAD_MANAGER_BEHAVIOR_PREF, &behavior);
>+      }
Nit: style appears to be no braces for a single statement.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 136473 [details] [diff] [review]
patch

changing request to darin because bz is on vacation

Comment 4

[Darin Fisher]

Comment on attachment 136473 [details] [diff] [review]
patch

>Index: nsDownloadProxy.h

> 
>     PRInt32 behavior = 0;
>     nsCOMPtr<nsIPrefService> prefs = do_GetService("@mozilla.org/preferences-service;1", &rv);
>+    if (NS_SUCCEEDED(rv)) {
>+      nsCOMPtr<nsIPrefBranch> branch;
>+      rv = prefs->GetBranch(nsnull, getter_AddRefs(branch));
>+      if (NS_SUCCEEDED(rv)) {
>+        rv = branch->GetIntPref(DOWNLOAD_MANAGER_BEHAVIOR_PREF, &behavior);
>+      }
>+    }

nit: this can be collapsed down to:

      PRInt32 behavior = 0;
      nsCOMPtr<nsIPrefBranch> branch =
do_GetService("@mozilla.org/preferences-service;1");
      if (branch)
	branch->GetIntPref(DOWNLOAD_MANAGER_BEHAVIOR_PREF, &behavior);

nsPrefService implements nsIPrefBranch for the top-level pref branch.  also,
no need to check |rv| since XPCOM methods only set the out-param if they
succeed.


>+    if (behavior == 0 || NS_FAILED(rv))
>       return dm->Open(nsnull, this);
>     if (behavior == 1) {
>       nsAutoString path;
>       rv = aTarget->GetPath(path);
>+      if (NS_FAILED(rv))
>+        return rv;
> 
>       NS_ConvertUCS2toUTF8 utf8Path(path);
>       return dm->OpenProgressDialogFor(utf8Path, nsnull, PR_TRUE);
>     }
>+    return NS_OK;
>   }

and to save on footprint, this last section should not have any early returns.
early return from an inner scope will cause nsAutoString virtual destructor
invocation code to be repeated.  likewise, you end up repeating calls to the
nsCOMPtr destructor.  try this instead to greatly reduce codesighs:

    if (behavior == 0 || NS_FAILED(rv))
      rv = dm->Open(nsnull, this);
    else if (behavior == 1) {
      nsAutoString path;
      rv = aTarget->GetPath(path);
      if (NS_SUCCEEDED(rv)) {
	NS_ConvertUCS2toUTF8 utf8Path(path);
	rv = dm->OpenProgressDialogFor(utf8Path, nsnull, PR_TRUE);
      }
    }
    return rv;
  }

on the subject of early returns, if all of your return statements look similar.
i.e., if all of your return statements look like |return rv|, then the
compilers
will do a good job of coalescing the necessary destructor calls to minimize 
code bloat.  if you mix statements like |return NS_OK;| with |return
NS_ERROR_FAILURE;| than you will increase codesize on GCC builds (assuming
of course that there are some class object with non-empty destructors on the
stack).  this will be fixed i'm told in GCC 3.5 :-(

however, in both MSVC++ and GCC 3.x if you add code for an early return from
an inner scope (like the body of an if (...) conditional) than the destructor
code will be repeated at the early return and at the end of scope.

sr=darin if you apply these changes.  might as well help cut down code/sighs/
while you're making improvements to this code, right? ;-)

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

>nsPrefService implements nsIPrefBranch for the top-level pref branch. 

it does... but is that reliable? it's nowhere documented...

> also,
>no need to check |rv| since XPCOM methods only set the out-param if they
>succeed.

really? many methods start by setting out pointers to nsnull, for example.
I don't think I can rely on that...

Comment 6

[Darin Fisher]

>>nsPrefService implements nsIPrefBranch for the top-level pref branch. 
>
>it does... but is that reliable? it's nowhere documented...

it should be documented.  it is a nice way to simplify working with prefs.


>> also,
>>no need to check |rv| since XPCOM methods only set the out-param if they
>>succeed.
>
>really? many methods start by setting out pointers to nsnull, for example.
>I don't think I can rely on that...

true... in an ideal world it doesn't make sense to set out-params unless a
function is going to succeed.  reality wins :-/

perhaps then you can just catch failure and set the variable to 0 in that case?

  PRInt32 behavior = 0;
  // ...
    if (NS_FAILED(branch->GetIntPref(..., &behavior)))
      behavior = 0;

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in nsDownloadProxy.h;
/cvsroot/mozilla/xpfe/components/download-manager/src/nsDownloadProxy.h,v  <-- 
nsDownloadProxy.h
new revision: 1.19; previous revision: 1.18
done