Closed Bug 919806 Opened 6 years ago Closed 6 years ago

Get rid of NS_IMETHOD in nsIFrame.h (and NS_IMETHODIMP in method-implementations)

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: dholbert, Assigned: Six)

References

()

Details

Attachments

(2 files)

NS_IMETHOD stuff in nsIFrame.h is historical cruft - it's to make us use the XPCOM calling convention, when non-gecko-internal code calls into Gecko, but we don't allow that anymore as of bug 616127.

So, we should be able to remove all of the NS_IMETHOD in nsIFrame.h.  (Generally, for every NS_IMETHOD in nsIFrame.h, we should replace it with "virtual nsresult", and for the one instances of NS_IMETHOD_(nsFrameState), we should replace that with "virtual nsFrameState".

And similar for any implementations of the methods that we change, except there it'll be NS_IMETHODIMP.

Arnaud, any chance you'd be up for knocking this out?  (If not, I'll make it a mentored-bug in search of an owner, or possibly just fix it myself at some point.)

Handy link:
http://mxr.mozilla.org/mozilla-central/search?string=NS_IMETHOD&find=nsIFrame.h
Summary: Get rid of NS_IMETHOD in nsIFrame.h (and NS_IMETHODIMPL in method-implementations) → Get rid of NS_IMETHOD in nsIFrame.h (and NS_IMETHODIMP in method-implementations)
(er "for the one instance")
(I suspect we need to do this for all the declarations of these methods at the same time... We might get odd behavior if a subclass declares an inherited method as NS_IMETHOD when the parent class (nsIFrame in this case) does not. (though maybe that won't compile, on the platforms (just windows?) where it actually matters...? I'd like to hope so, but I'm not sure.)
NS_IMETHOD could be removed all over the place in layout and elsewhere. Every frame class, for sure.
I chatted with Arnaud (:Six) about this on IRC today -- I think he's going to look into fixing this, which is excellent.

I'm posting one thing we discussed here, for the record -- so one potential pitfall here is that, if there are any frame classes (or helper-classes in *Frame.cpp files) that override a NS_IMETHOD-method that's inherited from an IDL-defined superclass, then we have to be careful not to convert *those* methods.

I don't know of any exact spots like that offhand, but I'll bet they might exist.

(I briefly thought I found an example in nsComboboxControlFrame.h:
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsComboboxControlFrame.h?rev=b94e05c2de77#155
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsComboboxControlFrame.h?rev=b94e05c2de77#192
...but it turns out those NS_IMETHODS are for .h-file-defined interfaces (as opposed to .idl-file-defined), so they're safe to remove as long as we clean up the interface as well.)

Anyway, one easy way to avoid this pitfall is to *only* convert methods that are declared in nsIFrame.h, and I think that's what Arnaud's going to try.
Assignee: nobody → six.dsn
Status: NEW → ASSIGNED
replaces all NS_IMETHOD from nsIFrame.h and child classes in layout/
also replaces some NS_METHOD that should have been marked as NS_IMETHODIMP

build on tpbl is green, some test fails but don't know if it matters
https://tbpl.mozilla.org/?tree=Try&rev=22c2c4c65035
BTW there is an unknown issue with the linuxx64 Opt Valgrind build
Attachment #8376825 - Flags: review?(dholbert)
Attached file patch analysis
Thanks, Arnaud!

FWIW, here's a text file with some quick sanity-check analysis that I did on the patch (looking at lines added/removed & functions-signatures added/removed). (commands included, which I ran with the patch applied using mercurial queues)

I want to look a bit more before granting r+, but it's looking good so far.
One note: I just remembered that dbaron is planning to rename a bunch of nsIFrame methods (some of which overlap with this patch), as he posted about here:
  https://groups.google.com/forum/#!topic/mozilla.dev.tech.layout/eRVMq2jMULs

I'm not sure if there's a bug already filed for that change, but it looks like he may have a patch ready, and (if so) this patch here would definitely bitrot it.

Depending on which patch (yours vs. dbaron's) requires more manual labor to regenerate, it might be better to hold off on this until after dbaron's patch lands.
 
dbaron: do you a patch already prepared for your XUL nsIFrame-method renaming (and if so, would you prefer for this to wait until after that lands?)
Flags: needinfo?(dbaron)
No patch yet, so this should go ahead.
Flags: needinfo?(dbaron)
Hi Daniel,

my patch is fully scripted and doesn't require any manual modifications, so we can wait until dbaron's patch is landed.
I also added sanity checks to my script to be sure there are no other overrided nsIFrame methods in the files i change that still have a NS_[I]METHOD* return type

Edit:
As dbaron just commented it, it's your call Daniel :)
Great! I posted about this here, to give folks a heads-up:
  https://groups.google.com/d/msg/mozilla.dev.tech.layout/GKwLTiVcr7Y/y08WDM3eSqcJ
and I'll finish off the review later tonight or tomorrow (heading out for the day right now), and then we can go ahead and land it.
Attachment #8376825 - Flags: review?(dholbert) → review+
>+++ b/layout/forms/nsColorControlFrame.h
>   // nsIFrame
>-  NS_IMETHOD AttributeChanged(int32_t  aNameSpaceID,
>+  virtual nsresult AttributeChanged(int32_t  aNameSpaceID,
>                               nsIAtom* aAttribute,
>                               int32_t  aModType) MOZ_OVERRIDE;

Due to the mechanical nature of this patch, it left a lot of code looking like that^ (with broken indentation on second/third/etc. lines of a method-decl), so I spent some quality time with my "tab" key fixing as much of it as I could find. (I think I caught most, if not all, of it.)

I verified that "hg qdiff -w" showed no changes & that my changes didn't introduce any end-of-line whitespace (from "tab" over-mashing) & that my changes didn't somehow break my local build. After verifying those things, I pushed my reindentation changes, as a whitespace-only DONTBUILD followup:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ed86c35097
(In reply to Daniel Holbert from comment #4)
> I'm posting one thing we discussed here, for the record -- so one potential
> pitfall here is that, if there are any frame classes (or helper-classes in
> *Frame.cpp files) that override a NS_IMETHOD-method that's inherited from an
> IDL-defined superclass, then we have to be careful not to convert *those*
> methods.

I think nsISupports was removed from all frames a while back, but if not, that's a bug, since frames aren't refcounted of course.
https://hg.mozilla.org/mozilla-central/rev/f6d37fdcc976
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(In reply to neil@parkwaycc.co.uk from comment #13)
> I think nsISupports was removed from all frames a while back, but if not,
> that's a bug, since frames aren't refcounted of course.

Right - I wasn't suggesting we had nsIFrames that inherited from nsISupports, but rather that we have other interfaces that declare NS_IMETHOD-methods, and so blindly removing NS_IMETHOD from *all* methods on nsIFrame subclasses could trigger compile errors for frame classes that inherit from these other interfaces.

(These other interfaces almost certainly want to be de-NS_IMETHOD-ified, but until they are, we can't drop NS_IMETHOD from their concrete implementations.) 

I found one example of this:
 http://mxr.mozilla.org/mozilla-central/source/layout/svg/nsISVGChildFrame.h
which declares e.g. NS_IMETHOD PaintSVG, which the various SVG frame classes implement.

Arnaud, if you're up for applying this bug's same treatment to that interface (nsISVGChildFrame) and its implementations, that would be excellent (and I imagine it should be doable with a slight modification to the script(s) you used to fix this bug).
You need to log in before you can comment on or make changes to this bug.