563668 - Be able to know if a form control is an input element or a button element from nsIFormControl

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch showing the idea

At the moment, if we have a |nsIFormControl| object, we have no simple way to know if it is an input element or a button element. The best way is to QI but as we know the type, we should be able to prevent that.

The idea of this bug is to prevent:
nsCOMPtr<nsIFormControl> control = [...]
nsCOMPtr<nsIDOMHTMLInputElement> input = do_QueryInterface(control);
if (input) {
 // it's an input element
}

Instead, we could do:
nsCOMPtr<nsIFormControl> control = [...]
if (control->GetType() & NS_FORM_INPUT_ELEMENT) {
 // it's an input element
}

We could even imagine an helper function like:
/*static*/ nsIFormControl::IsInputELement() { return GetType() & NS_FORM_INPUT_ELEMENT; }

Comment 1

[Boris Zbarsky [:bzbarsky]]

Can't you do this now, by just doing two <= compares using the return value of GetType()?

Note that the attached patch looks wrong to me, since it makes the types no longer fit in a PRInt8 (which is what <button> and <input> store the type in).

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to comment #1)
> Can't you do this now, by just doing two <= compares using the return value of
> GetType()?

I don't think it's safe to do that. There is no explicit rules about how to add #define lines in nsIFormControl.h so a new input type could be added in the middle of others or at the end of the list.

> Note that the attached patch looks wrong to me, since it makes the types no
> longer fit in a PRInt8 (which is what <button> and <input> store the type in).

As nsIFormControl, nsHTMLInputElement and nsHTMLButtonElement are returning PRInt32, I thought the type was stored as PRInt32.
Would you accept the same patch with #define instead of enum's ?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> There is no explicit rules about how to add #define lines in nsIFormControl.h

We'd add such rules, of course, if needed.

> As nsIFormControl, nsHTMLInputElement and nsHTMLButtonElement are returning
> PRInt32, I thought the type was stored as PRInt32.

This is where testing comes in.

> Would you accept the same patch with #define instead of enum's ?

How would that help?

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to comment #3)
> This is where testing comes in.
> 
> > Would you accept the same patch with #define instead of enum's ?
> 
> How would that help?

The most important part of the patch is the mask used for input and button elements. We can do the same think will keeping #define's.

Comment 5

[Mounir Lamouri (:mounir)]

hmmm, with this mask, we will not have to test if an element is an input element with <= and the value will still be a PRInt8.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, so the point is that GetType() wouldn't return mType but rather mType | something?  Or is the point something else?

I don't see how you plan to do the mask while fitting in a PRInt8.

Comment 7

[Mounir Lamouri (:mounir)]

Instead of:
#define NS_FORM_INPUT_TEXT 42
it would be:
#define NS_FORM_INPUT_ELEMENT 0x10000000
#define NS_FORM_INPUT_TEXT (NS_FORM_INPUT_ELEMENT | 1)

So, |GetType() & NS_FORM_INPUT_ELEMENT| will let us know if the element is an input element.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, but why is that different from the enum approach?  What would mType on the nsHTMLInputElement store?

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to comment #8)
> OK, but why is that different from the enum approach?  What would mType on the
> nsHTMLInputElement store?

Oups, sorry, I didn't mean 0x but 0b.

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK, that works.  But then why can't you do that for your enum too?

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to comment #10)
> OK, that works.  But then why can't you do that for your enum too?

Actually, I was thinking of 0b in my patch too. Sorry for this stupid mistake.
But, enum's are int32 values so we may have overflows. However, we can control that much more with enum values than with #define's, right ? Let me know what you think would be the best.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> But, enum's are int32 values so we may have overflows.

I don't follow.  As long as the enum values are < (1<<7) (which can be PR_STATIC_ASSERTed), how would we have overflows?

Comment 13

[Mounir Lamouri (:mounir)]

(In reply to comment #12)
> > But, enum's are int32 values so we may have overflows.
> 
> I don't follow.  As long as the enum values are < (1<<7) (which can be
> PR_STATIC_ASSERTed), how would we have overflows?

That's what I meant by "However, we can control that". Then I guess you think enum's are better ;)

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

As we were discussing about this patch, I think you may want to review it, Boris.

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Mhmm, gcc is warning me because '0b' is a gcc extension. Better not to use it. I've also add some (int) to prevent other warnings.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 443604 [details] [diff] [review]
Patch v1.1

With this patch, |NS_FORM_BUTTON_BUTTON&NS_FORM_INPUT_ELEMENT| tests nonzero.  Is the latter meant to be 0x80 instead?

Also "for 'oninput'", not "from 'oninput'".

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Indeed, 0b10000000 isn't 0x90 but 0x80. Don't know why I put 0x90 :-/

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 443634 [details] [diff] [review]
Patch v2

r+ is you replace |1<<8| with |1<<7| (the member is PRInt8, not PRUint8).

Comment 19

[Mounir Lamouri (:mounir)]

(In reply to comment #18)
> (From update of attachment 443634 [details] [diff] [review])
> r+ is you replace |1<<8| with |1<<7| (the member is PRInt8, not PRUint8).

I don't agree even if I begin to understand by experience I am going to be wrong ;)
Indeed, |1<<7| equals to 0b10000000 so |PR_STATIC_ASSERT((int)eInputElementTypesMax  < 1<<7);| would assert because NS_FORM_INPUT_ELEMENT begin at 0b10000000. I don't see any valid reason to stop enum values to 0b01111111 instead of 0b11111111, we are missing half of them...
Having a negative |mType| could be problematic for any reason ?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> NS_FORM_INPUT_ELEMENT begin at 0b10000000.

Well, you need to move that one bit down.  Or make mType PRUint8.  Or both.

> Having a negative |mType| could be problematic for any reason ?

Yes, due to how GetType works.  Say I set mType to NS_FORM_INPUT_BUTTON.  NS_FORM_INPUT_BUTTON is 0x81, or 257.  But mType is PRInt8, so the 0x81 is assigned as-is bitwise and means -127.  Now when GetType is called it returns a PRInt32 as you noted in comment 2.  And -127 is a perfectly reasonable PRInt32 value, so GetType will just return -127 as a PRInt32 (or 0xffffff81 if you look at the bits).

Comment 21

[Boris Zbarsky [:bzbarsky]]

Quite honestly, I think making mType PRUint8 and making GetType return PRUint32 (esp. if you plan to do bit-masking on it) would be the way to go.

Comment 22

[Mounir Lamouri (:mounir)]

(In reply to comment #20)
> > Having a negative |mType| could be problematic for any reason ?
> 
> Yes, due to how GetType works.  Say I set mType to NS_FORM_INPUT_BUTTON. 
> NS_FORM_INPUT_BUTTON is 0x81, or 257.  But mType is PRInt8, so the 0x81 is
> assigned as-is bitwise and means -127.  Now when GetType is called it returns a
> PRInt32 as you noted in comment 2.  And -127 is a perfectly reasonable PRInt32
> value, so GetType will just return -127 as a PRInt32 (or 0xffffff81 if you look
> at the bits).

Ok, I didn't think about the PRInt8 -> PRInt32 conversion... As usual, you were right ;)

(In reply to comment #21)
> Quite honestly, I think making mType PRUint8 and making GetType return PRUint32
> (esp. if you plan to do bit-masking on it) would be the way to go.

I agree. With the bit-masking we better not waste values. Then I will update the patch to have GetType functions only returning unsigned int's.

And thank you for your explanations :)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Not a problem at all.  Thanks for working on this stuff!

Comment 24

[Mounir Lamouri (:mounir)]

Attachment: Patch v3

Asking another review as the change may be sensitive.
Let me know if you prefer me to ask a sr to someone else instead.

By the way, |EnumTable.value| is PRInt16. Maybe it could be changed to PRUint16 but it's not an issue as we are saving PRUint8 in it (in nsHTMLInputElement and nsHTMLButtonElement). I've chose not to change it to prevent side effects.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 444406 [details] [diff] [review]
Patch v3

r=me.  This shouldn't need sr.

Comment 26

[Boris Zbarsky [:bzbarsky]]

I tried applying this locally to push it, but it doesn't apply cleanly on tip (my own fault for taking so long to review).

Comment 27

[Mounir Lamouri (:mounir)]

Attachment: Patch v3.1

Indeed, with input type tel and input type search, the patch wasn't up to date anymore. This one should be better.

Comment 28

[Mounir Lamouri (:mounir)]

Attachment: Patch v3.2

r=bzbarsky

New patch against the current tip.

Comment 29

[Justin Wood (:Callek)]

Pushed as: http://hg.mozilla.org/mozilla-central/rev/c46b625c886e

Comment 30

[Justin Wood (:Callek)]

Followup typo fix: http://hg.mozilla.org/mozilla-central/rev/9e21705b2075

Comment 31

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

You forgot to change one PRInt8:
  PRInt8 oldType = NS_CONTROL_TYPE(aVisitor.mItemFlags);
<http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#1860>, which caused the following warnings:

content/html/content/src/nsHTMLInputElement.cpp: In member function ‘virtual nsresult nsHTMLInputElement::PostHandleEvent(nsEventChainPostVisitor&)’:
content/html/content/src/nsHTMLInputElement.cpp:1889: warning: overflow in implicit constant conversion
content/html/content/src/nsHTMLInputElement.cpp:1890: warning: overflow in implicit constant conversion
content/html/content/src/nsHTMLInputElement.cpp:1921: warning: comparison is always false due to limited range of data type
content/html/content/src/nsHTMLInputElement.cpp:2142: warning: comparison is always false due to limited range of data type
content/html/content/src/nsHTMLInputElement.cpp:2143: warning: comparison is always false due to limited range of data type
content/html/content/src/nsHTMLInputElement.cpp:2180: warning: comparison is always false due to limited range of data type
content/html/content/src/nsHTMLInputElement.cpp:2181: warning: comparison is always false due to limited range of data type

Comment 32

[Boris Zbarsky [:bzbarsky]]

Er... those "always false" comparisons should at least be leading to test failures.  If they're not, we need tests for that code!

Mounir, can you file a followup bug and fix?

Comment 33

[Mounir Lamouri (:mounir)]

(In reply to comment #32)
> Er... those "always false" comparisons should at least be leading to test
> failures.  If they're not, we need tests for that code!
> 
> Mounir, can you file a followup bug and fix?

I will do that.