Closed Bug 405241 Opened 14 years ago Closed 14 years ago

Don't register nsDownloadHistory if it's not needed

Categories

(Core Graveyard :: File Handling, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch v1.0 (obsolete) — Splinter Review
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.
Assignee: nobody → comrade693+bmo
Status: NEW → ASSIGNED
Attachment #290045 - Flags: superreview?(bzbarsky)
Attachment #290045 - Flags: review?(cbiesinger)
Whiteboard: [has patch][needs r biesi][needs sr bz]
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.
Attachment #290045 - Flags: superreview?(bzbarsky) → superreview-
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?
> 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.
Attached patch v1.1Splinter Review
This should do the trick then.
Attachment #290045 - Attachment is obsolete: true
Attachment #290121 - Flags: superreview?(bzbarsky)
Attachment #290121 - Flags: review?(cbiesinger)
Attachment #290045 - Flags: review?(cbiesinger)
Comment on attachment 290121 [details] [diff] [review]
v1.1

s/ourself/ourselves/, and sr=bzbarsky
Attachment #290121 - Flags: superreview?(bzbarsky) → superreview+
(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.
Whiteboard: [has patch][needs r biesi][needs sr bz] → [has patch][needs r biesi][has sr]
Attachment #290121 - Flags: review?(cbiesinger) → review+
Whiteboard: [has patch][needs r biesi][has sr] → [has patch][has r][has sr][needs approval]
Attachment #290121 - Flags: approval1.9?
Attachment #290121 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Whiteboard: [has patch][has r][has sr][needs approval] → [has patch][has r][has sr][needs approval][can land]
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has r][has sr][needs approval][can land]
(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
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.