405241 - Don't register nsDownloadHistory if it's not needed

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Shawn Wilsher :sdwilsh]

Per Bug 401174 Comment 19

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This should do it.  There are some macros that could reduce the code in nsGlobalHistoryAdapter, but I'm not too concerned about dealing with them.

I changed the test back to the contract id since it's really testing a behavior that is defined in the idl, so it should always work regardless of who/what implements it.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 290045 [details] [diff] [review]
v1.0

> so it should always work regardless of who/what implements it.

The point is that we need two separate tests, imo:

1)  A unit test for the code in the docshell/ directory
2)  A unit test for the Firefox code

With your changed test, the non-Firefox code will never get tested, so if someone breaks it we will never know.

Comment 3

[Shawn Wilsher :sdwilsh]

That doesn't seem true - it wouldn't register the docshell version if the firefox version existed, right?  At least, that's how nsIGlobalHistory seems to work.

Regardless, we'd have one version that simply wouldn't be tested.  Can I register the class with just the CID?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> it wouldn't register the docshell version if the firefox version existed,
> right? 

That's correct, as this patch is written.

> Regardless, we'd have one version that simply wouldn't be tested.  Can I
> register the class with just the CID?

I believe you can pass nsnull for the contract id string, yes.  See http://lxr.mozilla.org/seamonkey/source/xpcom/components/nsComponentManager.cpp#2666 for where that gets handled.

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

This should do the trick then.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 290121 [details] [diff] [review]
v1.1

s/ourself/ourselves/, and sr=bzbarsky

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> (From update of attachment 290121 [details] [diff] [review])
> s/ourself/ourselves/, and sr=bzbarsky
Fixed locally.  I'll attach a patch with any review comments fixed before checking in with this fix.

Comment 8

[Reed Loden [:reed]]

Checking in docshell/base/nsDownloadHistory.cpp;
/cvsroot/mozilla/docshell/base/nsDownloadHistory.cpp,v  <--  nsDownloadHistory.cpp
new revision: 1.2; previous revision: 1.1
done
Checking in docshell/base/nsDownloadHistory.h;
/cvsroot/mozilla/docshell/base/nsDownloadHistory.h,v  <--  nsDownloadHistory.h
new revision: 1.2; previous revision: 1.1
done
Checking in docshell/build/nsDocShellModule.cpp;
/cvsroot/mozilla/docshell/build/nsDocShellModule.cpp,v  <--  nsDocShellModule.cpp
new revision: 1.35; previous revision: 1.34
done

Comment 9

[Reed Loden [:reed]]

(In reply to comment #6)
> s/ourself/ourselves/

Checking in docshell/base/nsDownloadHistory.cpp;
/cvsroot/mozilla/docshell/base/nsDownloadHistory.cpp,v  <--  nsDownloadHistory.cpp
new revision: 1.3; previous revision: 1.2
done