move do_QueryObject templates into their own header (or at least out of nsAutoPtr.h)

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: froydnj, Assigned: denis.volk, Mentored, NeedInfo)

Tracking

unspecified
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

41.59 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
nsAutoPtr.h has these odd do_QueryObject definitions:

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsAutoPtr.h#810

They don't really belong in nsAutoPtr.h, and they're the only bits in there that require nsAutoPtr.h to #include nsCOMPtr.h.  Let's move them out of there.

Steps:

1) Make a new header in xpcom/base/, call it nsQueryObject.h (suggested name).
2) Add nsQueryObject.h to EXPORTS in xpcom/base/moz.build.
3) Move the relevant definitions from nsAutoPtr.h to nsQueryObject.h.
4) Make all the source files that use do_QueryObject #include nsQueryObject.h.

We can remove the nsCOMPtr.h include in a different bug, as that's liable to require some fix-and-rebuild cycles.
Assignee

Comment 1

5 years ago
Seems nice for a first contrib. I'd like to do this.
Reporter

Comment 2

5 years ago
Great!  Let me know if you have questions, either here or on IRC (my IRC nick is froydnj).
Assignee: nobody → denis.volk
Assignee

Comment 3

5 years ago
Posted patch Proposed patch (obsolete) — Splinter Review
Reporter

Comment 4

5 years ago
Comment on attachment 8525819 [details] [diff] [review]
Proposed patch

Review of attachment 8525819 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks good, one minor change necessary.

::: xpcom/base/nsQueryObject.h
@@ +10,5 @@
> +#include "nsCOMPtr.h"
> +#include "nsRefPtr.h"
> +
> +#include "nsCycleCollectionNoteChild.h"
> +#include "mozilla/MemoryReporting.h"

These two includes are unnecessary, please remove them.
Attachment #8525819 - Flags: feedback+
Assignee

Comment 5

5 years ago
Thanks. I removed the unnecessary includes.
Assignee

Updated

5 years ago
Attachment #8525819 - Attachment is obsolete: true
Reporter

Comment 6

5 years ago
Thanks for the quick adjustments!  Apologies for the delay in looking at this patch; vacation and US Thanksgiving hit at precisely the wrong times to provide quick feedback for this.

In the future, please request review on the patch by following instructions here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed

The patch looks good, so I've pushed it to try to make sure it compiles everywhere:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eaee2d3b3048
Reporter

Updated

5 years ago
Attachment #8525966 - Flags: review+
Reporter

Comment 7

5 years ago
Try's green, marking checkin-needed.  Thanks for the patch!
Keywords: checkin-needed
I had to do some minor fixups to get the patch to apply, but apparently I either did it wrong, or something's changed enough since this patch was written so that it's breaking the builds now:

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4442901&repo=mozilla-inbound

I had to back it out in https://hg.mozilla.org/integration/mozilla-inbound/rev/da41a9308c2d

Can we get an updated patch that applies cleanly to the current tip of the repo?
Flags: needinfo?(nfroyd)
Flags: needinfo?(denis.volk)
Reporter

Comment 10

5 years ago
I spoke to Denis on IRC and I believe he's updating the patch.  I'll try to be around next time it lands to fixup any problems that crop up.
Flags: needinfo?(nfroyd)
https://hg.mozilla.org/mozilla-central/rev/9395a2b1d994
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

Updated

4 years ago
Depends on: 1156777
You need to log in before you can comment on or make changes to this bug.