deCOMtaminate nsIFormControlFrame.h

RESOLVED FIXED

Status

()

Core
Layout: Misc Code
--
minor
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: vodangkhoa, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a3) Gecko/20040807
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a3) Gecko/20040807

deCOMtaminate nsIFormControlFrame.h interface

Reproducible: Always
Steps to Reproduce:
1.
2.
3.
(Reporter)

Comment 1

13 years ago
Created attachment 155466 [details]
deCOMtaminate_nsIFormControlFrame.h

so I change NS_IMETHOD to Virtual and 
this: take in a pointer instead of a reference
//  NS_IMETHOD GetProperty(nsIAtom* aName, nsAString& aValue) = 0; 
virtual nsresult GetFormProperty(nsIAtom* aName, nsAString* aValue) = 0;
Nothing to do with accessibility....
Assignee: aaronleventhal → nobody
Component: Accessibility APIs → Layout: Misc Code
QA Contact: core.accessibility-apis → core.layout.misc-code
Instead of attaching a whole file like this, it's usually better to attach a CVS
diff. "cvs diff -utp12" is a good set of options to use. To make this work
you'll need a CVS checkout area, there are instructions at mozilla.org to tell
you how to set one up.

The changes you've made are only some of the changes that we need to make during
deCOMtamination. For example:

  virtual nsresult GetName(nsAString* aName) = 0;

This method should return a string directly instead of taking it as a parameter.
Check the callers and implementors to see if they need a failure code; they
almost certainly don't. You should check the other methods that return nsresult.

  virtual void ScrollIntoView(nsPresContext* aPresContext) = 0;  

This does not need to take the prescontext parameter. All frames can get their
prescontext by calling GetPresContext().

  virtual nsresult SetSuggestedSize(nscoord aWidth, nscoord aHeight) = 0;  

Should take an nsSize parameter instead of two coordinates.

Please make these changes on all the applicable methods and then reattach a CVS
diff...
(Reporter)

Comment 4

13 years ago
Created attachment 160034 [details] [diff] [review]
a little updated
Attachment #155466 - Attachment is obsolete: true
Looking better!

+   virtual nsresult  GetFormContent(nsIContent*& aContent) const = 0;

This should just return nsIContent* directly.

For all the methods that return nsresult, can you determine whether or not they
ever return anything but NS_OK? If they always return NS_OK then we should make
them return void instead.
(Reporter)

Comment 6

13 years ago
Created attachment 165246 [details]
update
Attachment #160034 - Attachment is obsolete: true
(Reporter)

Comment 7

13 years ago
Comment on attachment 165246 [details]
update

64c64,66
<   virtual PRInt32 GetFormControlType() const =  0;
---
>   NS_IMETHOD (PRInt32) GetFormControlType() const =  0;
>   
> NS_IMETHOD GetName(nsAString* aName) = 0;
66,67d67
<   virtual nsresult GetName(nsAString* aName) = 0;
< 
70c70
<   virtual void ScrollIntoView() = 0;	
---
> virtual void ScrollIntoView(nsPresContext* aPresContext) = 0;  
72c72
<   virtual void MouseClicked() = 0;
---
> virtual void MouseClicked(nsPresContext* aPresContext) = 0;
83c83,84
<   virtual void SetSuggestedSize(nsSize asSize) = 0;  
---
> NS_IMETHOD SetSuggestedSize(nscoord aWidth, nscoord aHeight) = 0;
>  
91,92c92,94
<    
<    virtual nsIContent*  GetFormContent(nsIContent*& aContent) const = 0;
---
>   // virtual nsresult  GetFormContent(nsIContent*& aContent) const = 0;
>   NS_IMETHOD GetFormContent(nsIContent*& aContent) const = 0;
> 
100,101c102,103
<    
<   virtual void SetFormProperty(nsIAtom* aName, const nsAString& aValue) = 0;
---
> 
>   NS_IMETHOD SetProperty(nsPresContext* aPresContext, nsIAtom* aName, const nsAString& aValue) = 0;
110c112,113
< virtual void GetFormProperty(nsIAtom* aName, nsAString* aValue) = 0; 
---
> 
>   NS_IMETHOD GetProperty(nsIAtom* aName, nsAString& aValue) = 0; 
115,116c118,119
<  virtual void OnContentReset() = 0;
< };
---
>   NS_IMETHOD OnContentReset() = 0;
>   };
This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/
Seems to me that the bug is still relevant? If I submit a new patch (based on the old one) minus the bitrot, would it be worth the time?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Yes.  If nothing else, should save some codesize, hopefully.  And give more readable code.

Comment 11

12 years ago
Ryan were you planning on working on this? If not I started a patch a while ago for this, I could finish it up. 
I've looked into it, but when I try to basically re-do the original patch, I get compiler errors that I have no experience in dealing with. If you've got something you've been working on, don't hold it back on my account :)

Comment 13

12 years ago
Created attachment 205192 [details] [diff] [review]
patch_v1

Thanks Ryan. Here's the patch. A couple comments:

There is a whole bunch of stuff that could be cleaned up in this area, I tried to limit the size of the patch though.

I couldn't figure out how to return the name in GetName, as this function eventually calls nsIContent->GetAttr which needs somewhere to stuff the return value. Should it return an already addreffed nsAString*?

I won't ask for review quite yet until that is figured out.
Strings are not reference counted; in general using an nsAString& out param is the right thing to do for strings (though in rare cases you can return a const nsAString& or const nsAFlatString& directly).  In this case, the out param sounds right.

Comment 15

12 years ago
Thanks, that makes sense, and is what the patch does. bz can you suggest a reviewer for this?
roc or dbaron (or me if we get to January and are still looking for a reviewer)...  I'd start with roc.

Updated

12 years ago
Attachment #205192 - Flags: review?(roc)
doesn't GetFormContent always just return this->GetContent()? If so, it should just be eliminated and callers should call GetContent() directly.

Comment 18

12 years ago
I thought of that as well, but assumed there was a mysterious reason for it. I'll do that and attach a new patch.

Comment 19

12 years ago
Created attachment 205267 [details] [diff] [review]
patch_v2

I eliminated GetFormContent, and determined GetName was unused so I eliminated that as well. This compiles and I couldn't find any bugs in my testing.
Attachment #205192 - Attachment is obsolete: true
Attachment #205267 - Flags: review?(roc)
Attachment #205192 - Flags: review?(roc)
There's a few more things we can do...

All nsIFormControlFrame implementations just call nsFormControlHelper::ScrollIntoView. So we should remove the method from nsIFormControlFrame and its implementations and have all callers call nsFormControlHelper::ScrollIntoView directly.

But actually nsFormControlHelper::ScrollIntoView should be in nsLayoutUtils. Also, it doesn't need to take a prescontext parameter, it can get that from the frame. And it will never be null so don't null-check it.

nsIFormControlFrame::SetProperty and GetProperty should be renamed to SetFormProperty and GetFormProperty. Currently they shadow nsIFrame methods with the same names but different signatures which causes warnings with gcc and could possibly confuse people.

Could GetFormControlType be removed/replaced? Could all callers just call nsFormControlHelper::GetType directly? Take a look.

+  virtual void SetSuggestedSize(const nsSize& aSize) {};

the trailing semicolon is unnecessary.

Looks like OnContentReset could be moved to nsIComboboxControlFrame nsIListControlFrame. As a followup patch we should probably merge nsIComboboxControlFrame into nsComboboxControlFrame itself so it's not a separate interface and likewise for listControlFrame.

Comment 21

12 years ago
Thanks for the quick response again. Replies follow...

> All nsIFormControlFrame implementations just call
> nsFormControlHelper::ScrollIntoView.
Good point. I think this will require QIing from the nsIFormControlFrame to nsIFrame, right?(Best I can tell the callers don't have an nsIFrame around)

> nsIFormControlFrame::SetProperty and GetProperty should be renamed to
> SetFormProperty and GetFormProperty.
Sure, that would make things clearer.

> Could GetFormControlType be removed/replaced? Could all callers just call
> nsFormControlHelper::GetType directly? Take a look.
I think you are right. There are 3 callers of GetFormControlType. Two check their own type, nsGFXButtonControlFrame and nsTextControlFrame. Both of these implement this function to be nsFormUtils::GetType. nsComboBoxControlFrame calls this function on another frame, but only checks if the type is NS_FORM_INPUT_BUTTON. None of the implementations which define this function to be something other than GetType return this. Here are my notes:

Uses:
nsComboBoxControlFrame::setInitialChildList
-Checks if other frame type == NS_FORM_INPUT_BUTTON

nsGfxButtonControlFrame::GetDefaultLabel(nsXPIDLString& aString)
-Checks if own type is NS_FORM_INPUT_RESET or NS_FORM_INPUT_SUBMIT or NS_FORM_INPUT_BUTTON

nsTextControlFrame::IsPasswordTextControl() const
Checks if own type is NS_FORM_INPUT_PASSWORD

Implementations:
nsComboBoxControlFrame returns NS_FORM_SELECT
nsFileControlFrame returns NS_FORM_INPUT_FILE
nsFormControlFrame return nsFormControlHelper::GetType()
nsHTMLButtonControlFrame return nsFormControlHelper::GetType()
nsImageControlFrame returns NS_FORM_INPUT_IMAGE
nsListControlFrame returns NS_FORM_SELECT
nsTextControlFrame returns nsFormControlHelper::GetType()

nsFormControlHelper QIs content to nsIFormControl, returns NS_FORM_INPUT_TEXT if it fails. 

> the trailing semicolon is unnecessary.
fixed

> Looks like OnContentReset could be moved to nsIComboboxControlFrame
> nsIListControlFrame.
Would this mean that callers have to take their nsIFormControlFrame and QI to nsIListControlFrame and nsIComboBoxControlFrame? QIing to both seems awkward, but a common interface seems like overkill.
(In reply to comment #21)
> Good point. I think this will require QIing from the nsIFormControlFrame to
> nsIFrame, right?(Best I can tell the callers don't have an nsIFrame around)

Ok. If they don't, do the QI (use CallQueryInterface).

> > Looks like OnContentReset could be moved to nsIComboboxControlFrame
> > nsIListControlFrame.
> Would this mean that callers have to take their nsIFormControlFrame and QI to
> nsIListControlFrame and nsIComboBoxControlFrame? QIing to both seems awkward,
> but a common interface seems like overkill.

There are only five interesting callers:
http://lxr.mozilla.org/mozilla/source/layout/forms/nsComboboxControlFrame.cpp#2468
-> nsIListControlFrame
http://lxr.mozilla.org/mozilla/source/content/html/content/src/nsHTMLTextAreaElement.cpp#769
-> remove, this will never have a list or combobox control frame
http://lxr.mozilla.org/mozilla/source/content/html/content/src/nsHTMLSelectElement.cpp#1849
http://lxr.mozilla.org/mozilla/source/content/html/content/src/nsHTMLSelectElement.cpp#1948
-> call IsCombobox to decide which one to QI for
http://lxr.mozilla.org/mozilla/source/content/html/content/src/nsHTMLInputElement.cpp#2099
-> remove, this will never have a list or combobox control frame

Comment 23

12 years ago
Created attachment 205448 [details] [diff] [review]
patch_v3

Addresses latest round of comments
Attachment #205267 - Attachment is obsolete: true
Attachment #205267 - Flags: review?(roc)

Updated

12 years ago
Attachment #205448 - Flags: review?(roc)
Comment on attachment 205448 [details] [diff] [review]
patch_v3

+  return false;

return PR_FALSE;

One more thing. SetSuggestedSize is only called by nsComboboxControlFrame::SetChildFrameSize and the call sites of that function know which frame type the target frame is. In one case it's the dropdown, but that's an nsListControlFrame which ignores SetSuggestedSize, so that can just be removed. The other case is the dropdown button, which is a nsGfxButtonControlFrame. So, I suggest you remove SetSuggestedSize from nsIFormControlFrame and all frames other than nsGfxButtonControlFrame.  Make it a non-virtual method in nsGfxButtonControlFrame. Remove the call to SetChildFrameSize on the combobox dropdown and then have SetChildFrameSize check that the frame type is a gfxButtonControlFrame and if it is, cast directly to nsGfxButtonControlFrame.h.

Make sense?

Comment 25

12 years ago
Sure, I'm having a little trouble figuring out which call to SetChildFrameSize is being called on the dropdown though, and should be removed. Looks to me like all three calls to it are passing aDropDownBtn as the frame, which always turns out to be the button display frame. I added a not reached warning if SetChildFrameSize was called with any other frame type than nsGFXButtonControlFrame, but I couldn't get it to fire.
You're right! Sorry. In that case they can all change to the same thing. I guess SetChildFrameSize should just lose its frame parameter and be renamed SetButtonFrameSize(nsSize aSize).

Comment 27

12 years ago
Created attachment 205628 [details] [diff] [review]
patch_v4

Updated to comments
Attachment #205448 - Attachment is obsolete: true
Attachment #205628 - Flags: review?(roc)
Attachment #205448 - Flags: review?(roc)

Comment 28

12 years ago
I don't think this is Mac only.
OS: MacOS X → All
Hardware: Macintosh → All
Comment on attachment 205628 [details] [diff] [review]
patch_v4

okay, I'll check it in. thanks!!!!!
Attachment #205628 - Flags: superreview+
Attachment #205628 - Flags: review?(roc)
Attachment #205628 - Flags: review+

Comment 30

12 years ago
Roc, thanks for all your help and speedy responses to my numerous questions. I've tested the patch fairly well, but CC me on any regressions and I'll do my best to help. Watch out for any MSVC compiler errors on the tinderboxes as well since I compiled on GCC, and as I'm sure you know MSVC and GCC don't always agree, especially on constness. I was thinking about cleaning up some other interfaces in forms that I noticed while I was working on this, if you have any thoughts on these shoot me an email.
I'm trying to patch this so I can test it with MSVC2003 for you, but I'm getting a few patch failures with nsComboboxControlFrame.cpp.

patching file layout/forms/nsComboboxControlFrame.cpp
Hunk #2 FAILED at 428.
Hunk #12 FAILED at 1846.
Hunk #13 FAILED at 2062.
3 out of 15 hunks FAILED -- saving rejects to file layout/forms/nsComboboxControlFrame.cpp.rej

Comment 32

12 years ago
Thanks Ryan, I think there are a couple conflicts with bz removing mGoodToGo. I think they're just in context, but I'll merge them and post a new patch in an hour or two when I get the chance.
Checked in.

Thanks for this. It's a big improvement.

Whatever you're interested in doing next, you should take a swing at. One thing I've noticed is that XUL frames contain a lot of virtual functions that don't need to be virtual.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
But you may be more interested in doing something else, e.g., just pick a bug to fix and follow where it leads.
OK, my MSVC2003 build failed with this patch in the tree (as I'm sure you'll see on Tinderbox soon). Here's the error:
d:/mozilla\mozilla\layout\forms\nsFileControlFrame.cpp(580) : error C2373: 'nsFileControlFrame::SetFormProperty' : redefinition; different type modifiers
       d:\mozilla\mozilla\layout\forms\nsFileControlFrame.h(79) : see declaration of 'nsFileControlFrame::SetFormProperty'

Comment 36

12 years ago
Yeah my mistake, I forgot to change NS_IMETHODIMP to nsresult.

So NS_IMETHODIMP nsFileControlFrame::SetFormProperty(nsIAtom* aName,
                                              const nsAString& aValue)
should be nsresult nsFileControlFrame::SetFormProperty(nsIAtom* aName,
                                              const nsAString& aValue)
Builds fine now
reduced codesize by 3.5K. Great.

Other things that need doing:
-- remove mPresContext members from any frames that still have one
-- in many cases we have a singleton interface like nsIComboboxControlFrame that is only implemented by one class, is not defined in IDL (and is therefore not scriptable), and is not visible outside Gecko. In these cases we can save code and data size by eliminating the interface, moving its methods to the concrete class and devirtualizing them, and casting to the class directly, like we just did with nsGfxButtonControlFrame. We can even assign the class an IID so you can get to it with QueryInterface. For example you can QI to nsBlockFrame this way.
Blocks: 292527
You need to log in before you can comment on or make changes to this bug.