326843 - DeCOMtaminate some of nsComboboxControlFrame

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

Replace a couple of out params with return values

Reproducible: Always

Steps to Reproduce:

Comment 1

[Marc Liddell]

Attachment: GetDropDown and IsDroppedDown and all callers

This changes a couple of methods return values from nsresult to an actual result.

If this is too small a change to bother or if there's a better plan for fixing these little things then ignore this. Otherwise I'd like to go around fixing these and continuing with some of the other decom tasks.

Comment 2

[Eli Friedman]

I can't officially review this, but I happened to spot it and have a couple comments.

1. Patches like this are definitely welcome.  It's good to help clean up code.  Thanks for taking the time.

2. See http://wiki.mozilla.org/Gecko:DeCOMtamination and http://wiki.mozilla.org/Gecko:Interface_Style_Guide for some general advice.

Some specific tips:

3. Don't use the NS_IMETHOD* macros on frames: write "virtual PRBool", not NS_IMETHOD_(PRBool).  Frames aren't XPCOM objects, so those macros shouldn't be used.  (And I guess I should add a note to the style guide that methods that need to be accessed outside gklayout.dll have to be virtual or inline).

4. Sort of tangential, but it would be nice to get rid of that interface altogether at some point, because the users can just use the frame directly, and then the methods can be devirtualized.

Comment 3

[Marc Liddell]

Attachment: More methods on nsComboboxControlFrame

So I went ahead and did most of the other methods using the virtual blah rather than the macros. All of the methods I changed now return an actual result or void. The methods changed from nsresult to void had no callers which checked the value.

About removing the interface. If you can guide me through how to go about that exactly I'd be happy to do it and if there are other unnecessary interfaces I can continue to remove those.

Comment 4

[Eli Friedman]

A couple of things:

1. You might as well get rid of GetAbsoluteRect; it's unused.

2. The rule for layout is don't pass around nsPresContexts, because GetPresContext() can get it from anywhere.  So remove the aPresContext parameter for RollupFromList.

Removing the interface involves changing users to nsComboboxControlFrame and having some way to allow getting one.  A good example to follow is how getting an nsBlockFrame works.  Some places to look at for that:
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsBlockFrame.cpp#316
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#2483.  Then you'll have to change the Makefile:
http://lxr.mozilla.org/seamonkey/source/layout/forms/Makefile.in#78
so that accessibility can see the header.  It's probably better to do that separately, though, because it's not required by the other changes, and therefore can be split off into a separate patch.  It makes things harder to sort out if there are many different changes in one patch.  (The current size of your patch is fine, because it is a simple, directed set of changes.)

Comment 5

[Marc Liddell]

Attachment: Remove unused method and unnecessary param

I did those 2 things. Thanks for taking the time to help with this. Anything else it needs?

Once this patch is finished I can move onto the interface removal and then find some other files like this one.

Comment 6

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

Comment on attachment 211615 [details] [diff] [review]
Remove unused method and unnecessary param

This looks great. Thanks to Eli for helping make it great.

If you had done the interface removal first, this patch would have been slightly simpler, but that's OK.

Comment 7

[Eli Friedman]

I'll check this in once the tree reopens.

Comment 8

[Eli Friedman]

Checked in.