Closed Bug 227032 Opened 21 years ago Closed 21 years ago

nsDownloadProxy::Init improvement

Categories

(SeaMonkey :: Download & File Handling, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Biesinger, Assigned: Biesinger)

Details

Attachments

(1 file, 1 obsolete file)

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
Attached patch patch (obsolete) — Splinter Review
also did a few other changes
Attachment #136473 - Flags: review?(neil.parkwaycc.co.uk)
Status: NEW → ASSIGNED
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.
Attachment #136473 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #136473 - Flags: superreview?(bz-vacation)
Comment on attachment 136473 [details] [diff] [review]
patch

changing request to darin because bz is on vacation
Attachment #136473 - Flags: superreview?(bz-vacation) → superreview?(darin)
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? ;-)
Attachment #136473 - Flags: superreview?(darin) → superreview-
>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...
>>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;
Attached patch patch v2Splinter Review
Attachment #136473 - Attachment is obsolete: true
Attachment #136861 - Flags: superreview?(darin)
OS: Windows 2000 → All
Hardware: PC → All
Attachment #136861 - Flags: superreview?(darin) → superreview+
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
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: