326944 - Remove nsIComboboxControlFrame

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Marc Liddell]

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:

Comment 1

[Marc Liddell]

Attachment: 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 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.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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?

Comment 3

[Marc Liddell]

Attachment: Fixes some build problems and some nits

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+extern const nsIID kComboboxControlFrameCID;

I think this should be replaced with "const nsIID& nsComboboxControlFrame::GetCID() { static nsIID kComboboxControlFrameCID = ...; return kComboboxControlFrameCID; }"
or something like that

Comment 5

[Marc Liddell]

Attachment: Now with added GetCID()

Comment 6

[Marc Liddell]

Attachment: Devirtualized methods

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.

Comment 7

[Eli Friedman]

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.

Comment 8

[Marc Liddell]

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.

Comment 9

[Marc Liddell]

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?

Comment 10

[Eli Friedman]

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.

Comment 11

[Marc Liddell]

Attachment: nsCombobboxControlFrame.*

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.

Comment 12

[Marc Liddell]

Attachment: Remove all nsIComboboxControlFrame uses

This one leave GetDropDown virtual. I get a nice *** Registering nsAccessibilityModule components (all right -- a generic module!) message so it seems fine.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

should those files really be exported?

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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).

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Perhaps the right way to go here is to land most of this patch, make nsIComboboxControlFrame a tearoff and use it only from accessibility.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

That might work.  Or we could add to nsISelectElement, use that in accessibility, and have the content code use nsComboboxControlFrame....

Comment 20

[Neil Deakin]

Attachment: Remove nsIComboboxControlFrame interface

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/785a53ae44c0
remove nsIComboboxControlFrame interface, r=mats

Comment 22

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/785a53ae44c0