Closed Bug 659722 Opened 9 years ago Closed 6 years ago

Replace _IMPL_NS_LAYOUT with MOZILLA_INTERNAL_API

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: khuey, Assigned: tbsaunde)

References

()

Details

Attachments

(1 file, 1 obsolete file)

In the post libxul world the layout engine doesn't need to hide things from the rest of the platform, only from the outside world, so we can replace _IMPL_NS_LAYOUT with the generic thing.
Let's trying removing the _IMPL_NS_LAYOUT macrology altogether: if non-libxul code relies on that stuff we should probably just fix that and then make it internal-only.
A common way _IMPL_NS_LAYOUT is used is like so:

  class nsIFoo() {
    void methodInternal();
    virtual void methodExternal();
#ifdef _IMPL_NS_LAYOUT
    void method() { methodInternal(); }
#else
    void method() { methodExternal(); }
#endif    
  }

where we do want to expose method() to external callers but don't want to pay the virtual dispatch costs on internal access.   Though perhaps at this point all the people we want to expose things to are inside libxul or this should be handled by using non-hidden visibility on methodInternal?
Yeah, I'm hoping they are all inside libxul. If not, then we'd need to keep a MOZILLA_INTERNAL_API ifdef there.
(In reply to comment #1)
> Let's trying removing the _IMPL_NS_LAYOUT macrology altogether: if
> non-libxul code relies on that stuff we should probably just fix that and
> then make it internal-only.

So would you suggest the best way to proceed would be to assume _IMPL_NS_LAYOUT is defined, remove external methods/ifdefs/ifndefs as appropriate, send to try and see what breaks?

Or should I just stick to the s/_IMPL_NS_LAYOUT/MOZILLA_INTERNAL_API + remove all |DEFINES += -D_IMPL_NS_LAYOUT| ?
Assignee: nobody → bmo
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Try won't tell you about non-Mozilla code that relies on the methods in question.
Sorry, I thought you meant external to layout, not the codebase in general.

Kyle asked if I wanted to do a simple cleanup in the form of s/_IMPL_NS_LAYOUT/MOZILLA_INTERNAL_API, but I'm now not really sure what I should be doing.
Just doing s/_IMPL_NS_LAYOUT/MOZILLA_INTERNAL_API/ should be safe no matter what.

If we want to eliminate the ifdefs altogether, then what needs to be done here is to evaluate the ifdef locations to see which of them are APIs we want to expose outside Gecko proper.  That probably needs me or dbaron or roc to decide on them one by one.  Those need to keep the external visibility we need to keep ifdefs for; the rest we can remove them.
Let's do what the title says and avoid scope creeping this bug.
I'd really like to scope-creep the bug, rather than making mechanical changes which are unnecessary. _IMPL_NS_LAYOUT isn't hurting anybody right now.

bz, considering that all of these ifdefs are in mostly-internal APIs, I think we should do what comment #4 says, and wait for the external consumers to complain...
Or how about we do what comment 7 says instead of gratuitously breaking people?

I just went through all the IMPL_NS_LAYOUT ifdefs.  The ones that _may_ still be useful (as MOZILLA_INTERNAL_API) are:

1)  Hiding styleset/frameconstructor/framemanager inline getters in nsIPreShell.
2)  Making GetRootFrame() work for non-gecko consumers on nsIPresShell.
3)  Making Add/RemoveWeakFrame work for non-gecko consumers on nsIPresShell.
4)  Making Add/RemoveRefreshObserver work for non-gecko consumers on
    nsIPresShell.
5)  As #1 (also for animation/transtion manager and refresh driver) but on
    nsPresContext.
6)  GetContainer on nsPresContext.
7)  BidiEnabled on nsPresContext.
8)  Preventing layout/style/Declaration.h from being included in external code.
9)  Hiding rulenode addref/release in nsStyleSet.h.
10) Allowing GetStyle* to work on nsIFrame for non-gecko consumers.
11) Hiding inline GetVisitedDependentColor on nsIFrame.
12) Making nsWeakFrame::Init work for non-Gecko consumers.
13) Making popup control state machinery on nsPIDOMWindow.h work for non-gecko
    consumers.
14) Making nsIDOMClassInfo.h work for non-internal code and corresponding bits
    in nsDOMClassInfoID.h
15) Whatever TabChild.h is doing with PBrowserChild.h
16) Exposing IsEditable() on nsINode to non-Gecko code.

For those:

#1 can maybe be nixed if we're ok with non-Gecko code grabbing those objects...
   and if non-Gecko includers of this file will still compile with these inlines
   visible.
#2 is needed, I think.
#3 is only needed if we want non-Gecko code to be able to use nsWeakFrame.
#4 is pretty questionable.
#5 see #1.
#6 not sure.
#7 I suspect we don't really need.
#8 not sure; can we just move it out of EXPORTS and change LOCAL_INCLUDES as
   needed?
#9 why do we need this?
#10 is probably needed.
#11 why is this needed?
#12 See #3.
#13 not sure whether we need to support that.  If we do, maybe doing it via
    window utils is better.
#14 is needed.
#15 no idea
#16 I suspect this can go.

dbaron, roc, thoughts on the items above?
I'm unable to do anything with this until comment 10 decisions are made. Throwing back into the pool.
Assignee: bmo → nobody
Status: ASSIGNED → NEW
FYI, just tripped over this and wasted a good part of a day :-(  Until this bug gets fixed, I'm going to add a line in nsDOMClassInfoID.h to warn people that if DOMCI_DATA() fails, make sure the define is set.
(In reply to Boris Zbarsky (:bz) from comment #10)
> Or how about we do what comment 7 says instead of gratuitously breaking
> people?

Given that I wrote a patch to do that already before finding this debate, and it seems like an improvement I agree with just doing comment 7 but have no objection to more patches to kill off things that we don't want in the public API.

> #14 is needed.

I imagine that keeping support for non-libxul domci is much less important now?
Attached patch remove _IMPL_NS_LAYOUT (obsolete) — Splinter Review
Attachment #726458 - Flags: review?(khuey)
rebased, no hurry to review
Attachment #784163 - Flags: review?(bzbarsky)
Comment on attachment 784163 [details] [diff] [review]
bug 659722 - remove _IMPL_NS_LAYOUT

r=me
Attachment #784163 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/50b000502eda
Assignee: nobody → trev.saunders
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/50b000502eda
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Attachment #726458 - Attachment is obsolete: true
Attachment #726458 - Flags: review?(khuey)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.