Don't register nsDownloadHistory if it's not needed

RESOLVED FIXED in mozilla1.9beta2

Status

Core Graveyard
File Handling
RESOLVED FIXED
11 years ago
2 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
mozilla1.9beta2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Comment 1

11 years ago
Created attachment 290045 [details] [diff] [review]
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.
Assignee: nobody → comrade693+bmo
Status: NEW → ASSIGNED
Attachment #290045 - Flags: superreview?(bzbarsky)
Attachment #290045 - Flags: review?(cbiesinger)
(Assignee)

Updated

11 years ago
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-
(Assignee)

Comment 3

11 years ago
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.
(Assignee)

Comment 5

11 years ago
Created attachment 290121 [details] [diff] [review]
v1.1

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+
(Assignee)

Comment 7

11 years ago
(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+
(Assignee)

Updated

11 years ago
Whiteboard: [has patch][needs r biesi][has sr] → [has patch][has r][has sr][needs approval]
(Assignee)

Updated

11 years ago
Attachment #290121 - Flags: approval1.9?

Updated

11 years ago
Attachment #290121 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

11 years ago
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
Last Resolved: 11 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.