Closed Bug 326944 Opened 16 years ago Closed 3 years ago

Remove nsIComboboxControlFrame

Categories

(Core :: Layout, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: marcldl+mozbugs, Assigned: enndeakin)

References

Details

Attachments

(1 file, 6 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060124 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060124 Firefox/1.5.0.1

This interface isn't necessary since only one class implements it.

Reproducible: Always

Steps to Reproduce:
Status: UNCONFIRMED → NEW
Ever confirmed: true
I think this is how it's meant to be. I had to add a lot of stuff to the makefiles. Hopefully that was part of the plan all along.

I didn't edit the methods on nsComboboxControlFrame as they'll change if the other patch on bug 326843 gets checked in. So I can wait until after that and edit the patch to devirtualize the the methods but I'm just making sure I did this bit right.
Attachment #211774 - Flags: review?(roc)
(In reply to comment #1)
> Created an attachment (id=211774) [edit]
> Removing all mention of nsIComboboxControlFrame
> 
> I think this is how it's meant to be. I had to add a lot of stuff to the
> makefiles. Hopefully that was part of the plan all along.

I think that's OK. It's worth it because we'll be able to remove several interfaces this way.

> I didn't edit the methods on nsComboboxControlFrame as they'll change if the
> other patch on bug 326843 gets checked in. So I can wait until after that and
> edit the patch to devirtualize the the methods but I'm just making sure I did
> this bit right.

Yeah, it looks pretty good.

+    *aInstancePtr = NS_STATIC_CAST(void*, NS_STATIC_CAST(nsComboboxControlFrame*, this));

Why was this necessary? 'this' is already an nsComboboxControlFrame*

+extern const nsIID kComboboxControlFrameCID;

Hmm, does accessible/ actually get linked into gklayout now? Have you tried a debug build?
Attachment #211774 - Attachment is obsolete: true
Attachment #211914 - Flags: review?(roc)
Attachment #211774 - Flags: review?(roc)
+extern const nsIID kComboboxControlFrameCID;

I think this should be replaced with "const nsIID& nsComboboxControlFrame::GetCID() { static nsIID kComboboxControlFrameCID = ...; return kComboboxControlFrameCID; }"
or something like that
Attached patch Now with added GetCID() (obsolete) — Splinter Review
Attachment #211914 - Attachment is obsolete: true
Attachment #212049 - Flags: review?(roc)
Attachment #211914 - Flags: review?(roc)
Attached patch Devirtualized methods (obsolete) — Splinter Review
I updated the patch to trunk since bug 326843 made a line or two not apply. I also went ahead and removed the virtual from the methods.
Attachment #212049 - Attachment is obsolete: true
Attachment #212289 - Flags: review?(roc)
Does accessibility link with the last patch?  I'm pretty sure linking will complain about GetDropDown, since accessibility doesn't link with layout.  You could make it inline or make it virtual again to fix that.
It doesn't complain about anything here. Could that be something to do with my mozconfig? I'm not explicitly enabling or disabling anything to do with accessibility afaik.

Also in the latest patch I left nsIComboboxFrame.h untouched. I wasn't sure whether it needs to be modified or not since no-one is using it anymore.
Hmm, now it seems to be trying to load accessibility but I'm getting an error about GetCID:

undefined symbol: _ZN22nsComboboxControlFrame6GetCIDEv

Did I not define/implement it correctly?
I have no idea how there can be no issue with GetDropdown and yet an issue with GetCID; as far as I know, they should both have the same issue.  The only relevant option I can think of would be --enable-static, but if that affected one, it ought to affect both.

Oh, and by the way, would you delete the irrelevant changes from the patch?  Extra stuff just makes it more confusing.
Attached file nsCombobboxControlFrame.* (obsolete) —
This is a patch for just nsComboboControlFrame.h and nsComboboControlFrame.cpp. I went back and readded virtual to the methods until I've fixed GetCID.

There's no complaints while I'm building with it but when I try to run the build I get the LoadModule error. It probably only complained about GetCID because it comes first in whatever ordering it uses.
This one leave GetDropDown virtual. I get a nice *** Registering nsAccessibilityModule components (all right -- a generic module!) message so it seems fine.
Attachment #212289 - Attachment is obsolete: true
Attachment #212930 - Flags: review?(roc)
Attachment #212289 - Flags: review?(roc)
Comment on attachment 212930 [details] [diff] [review]
Remove all nsIComboboxControlFrame uses

I think this looks good, but it might cause Tinderbox bustage because of unforseen compiler/config issues. So I'll check it in sometime when no-one's around.
Attachment #212930 - Flags: superreview+
Attachment #212930 - Flags: review?(roc)
Attachment #212930 - Flags: review+
should those files really be exported?
You mean nsCounterManager.h, nsGenConList.h, nsQuoteList.h, etc, etc?

No, they should not, imo.

If nsIComboboxControlFrame is actually used in modules outside layout (and hence needs to be exported), then it's not really a candidate for being eliminated, imo.  If you want to deCOMtaminate it by adding members to it or something, that's fine, of course.
Unfortunately I checked this in just before reading the comments. I backed it out.

I'm reluctant to believe that we're just stuck with the extra pointer in nsComboboxFrame (and the other places where I'd like to get rid of interfaces that map to just one implementation class).
Perhaps the right way to go here is to land most of this patch, make nsIComboboxControlFrame a tearoff and use it only from accessibility.
Note: checking in the patch made btek catch fire.

../src/html/libaccessibility_html_s.a(nsHTMLSelectAccessible.o): In function `nsHTMLSelectOptionAccessible::DoAction(unsigned char)':
nsHTMLSelectAccessible.o(.text+0x1ba7): undefined reference to `nsComboboxControlFrame::GetCID(void)'
../src/html/libaccessibility_html_s.a(nsHTMLSelectAccessible.o): In function `nsHTMLComboboxAccessible::GetState(unsigned int *)':
nsHTMLSelectAccessible.o(.text+0x22ad): undefined reference to `nsComboboxControlFrame::GetCID(void)'
../src/html/libaccessibility_html_s.a(nsHTMLSelectAccessible.o): In function `nsHTMLComboboxAccessible::GetFocusedOptionAccessible(void)':
nsHTMLSelectAccessible.o(.text+0x2460): undefined reference to `nsComboboxControlFrame::GetCID(void)'
../src/html/libaccessibility_html_s.a(nsHTMLSelectAccessible.o): In function `nsHTMLComboboxButtonAccessible::GetActionName(unsigned char, nsAString_internal &)':
nsHTMLSelectAccessible.o(.text+0x296f): undefined reference to `nsComboboxControlFrame::GetCID(void)'
../src/html/libaccessibility_html_s.a(nsHTMLSelectAccessible.o): In function `nsHTMLComboboxButtonAccessible::GetState(unsigned int *)':
nsHTMLSelectAccessible.o(.text+0x2c07): undefined reference to `nsComboboxControlFrame::GetCID(void)'
../src/html/libaccessibility_html_s.a(nsHTMLSelectAccessible.o)(.text+0x2d87): more undefined references to `nsComboboxControlFrame::GetCID(void)' follow
That might work.  Or we could add to nsISelectElement, use that in accessibility, and have the content code use nsComboboxControlFrame....
Blocks: 328680
Assignee: nobody → enndeakin
Attachment #212363 - Attachment is obsolete: true
Attachment #212930 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #9012162 - Flags: review?(mats)
Attachment #9012162 - Flags: review?(mats) → review+
Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/785a53ae44c0
remove nsIComboboxControlFrame interface, r=mats
https://hg.mozilla.org/mozilla-central/rev/785a53ae44c0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.