Bug 156458 – Remove UsePointersEvenForNonNullableObjectArguments from HTMLSelectElement

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Darin Adler]

Remove UsePointersEvenForNonNullableObjectArguments from HTMLSelectElement

[Darin Adler]

Created attachment 276116 [details]
Patch

[Darin Adler]

Created attachment 276117 [details]
Patch

[Darin Adler]

Created attachment 276118 [details]
Patch

[Chris Dumez]

Comment on attachment 276118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276118&action=review

r=me but please fix the Window build before landing as the compilation error seems related to your change.

> Source/WebCore/ChangeLog:42
> +        protects itself, and unneeded call to updateValidity, since updateChildren

updateChildren? Do you mean HTMLSelectElement::childrenChanged()?

> Source/WebCore/html/HTMLOptionsCollection.cpp:56
>  void HTMLOptionsCollection::remove(HTMLOptionElement* option)

This should be updated to take a reference. It is only called by JSHTMLOptionsCollection::remove() as far as I can tell, which is custom bindings and already does a null check.
The null check added below does not seem needed.

> Source/WebCore/html/HTMLSelectElement.cpp:225
> +    if (!(is<HTMLOptionElement>(element) || element.hasTagName(hrTag) || is<HTMLOptGroupElement>(element)))

element.hasTagName(hrTag) should probably be is<HTMLHRElement>(element), for consistency.

> Source/WebCore/html/HTMLSelectElement.h:62
> +public:

Why the public?

> Source/WebCore/html/HTMLSelectElement.idl:25
>      [Reflect] attribute boolean disabled;

the form attribute below should be nullable although not strictly related to your change.
May be nice to add a FIXME comment though.

> Source/WebCore/html/HTMLSelectElement.idl:43
>      [SetterRaisesException] attribute unsigned long length;

item() / namedItem() below should return a nullable HTMLOptionElement. Maybe we could add a FIXME comment.

> Source/WebCore/html/HTMLSelectElement.idl:66
>  

FYI, the DOMString? parameter to setCustomValidity() below is not right. It should not be nullable. We should either fix or add a FIXME comment.
https://html.spec.whatwg.org/multipage/forms.html#htmlselectelement

[Darin Adler]

Comment on attachment 276118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276118&action=review

>> Source/WebCore/html/HTMLOptionsCollection.cpp:56
>>  void HTMLOptionsCollection::remove(HTMLOptionElement* option)
> 
> This should be updated to take a reference. It is only called by JSHTMLOptionsCollection::remove() as far as I can tell, which is custom bindings and already does a null check.
> The null check added below does not seem needed.

My plan was to do this in my next patch, which is going to remove UsePointersEvenForNonNullableObjectArguments from HTMLOptionsCollection, but I suppose I can do it now given it’s a custom hand-written binding that’s already doing a null check.

>> Source/WebCore/html/HTMLSelectElement.cpp:225
>> +    if (!(is<HTMLOptionElement>(element) || element.hasTagName(hrTag) || is<HTMLOptGroupElement>(element)))
> 
> element.hasTagName(hrTag) should probably be is<HTMLHRElement>(element), for consistency.

I thought the same thing. But that requires adding the macros to HTMLHRElement.h, which doesn’t have them at this time. Do you think I should do it?

>> Source/WebCore/html/HTMLSelectElement.h:62
>> +public:
> 
> Why the public?

Oops. This is an editing mistake from when I made lots of things private and then slowly made them all public again. I will definitely remove it before landing.

>> Source/WebCore/html/HTMLSelectElement.idl:25
>>      [Reflect] attribute boolean disabled;
> 
> the form attribute below should be nullable although not strictly related to your change.
> May be nice to add a FIXME comment though.

I’d be happy to make that change in this patch; I believe marking it nullable will have no effect on the bindings code generated; it supports null even if we don’t tell it to.

>> Source/WebCore/html/HTMLSelectElement.idl:43
>>      [SetterRaisesException] attribute unsigned long length;
> 
> item() / namedItem() below should return a nullable HTMLOptionElement. Maybe we could add a FIXME comment.

I’d be happy to make that change in this patch; I believe marking it nullable will have no effect on the bindings code generated; it supports null even if we don’t tell it to.

>> Source/WebCore/html/HTMLSelectElement.idl:66
>>  
> 
> FYI, the DOMString? parameter to setCustomValidity() below is not right. It should not be nullable. We should either fix or add a FIXME comment.
> https://html.spec.whatwg.org/multipage/forms.html#htmlselectelement

I wouldn’t want to fix that without test coverage. I’m surprised there is no test for that already. Guess I can "wimp out" and just add the FIXME.

[Darin Adler]

Comment on attachment 276118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276118&action=review

>> Source/WebCore/ChangeLog:42
>> +        protects itself, and unneeded call to updateValidity, since updateChildren
> 
> updateChildren? Do you mean HTMLSelectElement::childrenChanged()?

Yes, that’s exactly what I meant. No idea why I typed nonsense instead!

[Chris Dumez]

Comment on attachment 276118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276118&action=review

>>> Source/WebCore/html/HTMLSelectElement.cpp:225
>>> +    if (!(is<HTMLOptionElement>(element) || element.hasTagName(hrTag) || is<HTMLOptGroupElement>(element)))
>> 
>> element.hasTagName(hrTag) should probably be is<HTMLHRElement>(element), for consistency.
> 
> I thought the same thing. But that requires adding the macros to HTMLHRElement.h, which doesn’t have them at this time. Do you think I should do it?

No, I don't think you need to add any macros, they should be auto-generated already. You may need to include HTMLHRElement.h though if not done already.

[Darin Adler]

(In reply to comment #4)
> please fix the Window build before landing as the compilation error
> seems related to your change.

Looks like I need to add an include of Event.h to DOMCoreClasses.cpp.

[Chris Dumez]

(In reply to comment #7)
> Comment on attachment 276118 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276118&action=review
> 
> >>> Source/WebCore/html/HTMLSelectElement.cpp:225
> >>> +    if (!(is<HTMLOptionElement>(element) || element.hasTagName(hrTag) || is<HTMLOptGroupElement>(element)))
> >> 
> >> element.hasTagName(hrTag) should probably be is<HTMLHRElement>(element), for consistency.
> > 
> > I thought the same thing. But that requires adding the macros to HTMLHRElement.h, which doesn’t have them at this time. Do you think I should do it?
> 
> No, I don't think you need to add any macros, they should be auto-generated
> already. You may need to include HTMLHRElement.h though if not done already.

See:
namespace WebCore {
class HTMLHRElement;
}
namespace WTF {
template <typename ArgType>
class TypeCastTraits<const WebCore::HTMLHRElement, ArgType, false /* isBaseType */> {
public:
    static bool isOfType(ArgType& node) { return checkTagName(node); }
private:
    static bool checkTagName(const WebCore::HTMLElement& element) { return element.hasTagName(WebCore::HTMLNames::hrTag); }
    static bool checkTagName(const WebCore::Node& node) { return node.hasTagName(WebCore::HTMLNames::hrTag); }
};
}

In WebKitBuild/Release/DerivedSources/WebCore/HTMLElementTypeHelpers.h

[Darin Adler]

Created attachment 276204 [details]
Patch

[Darin Adler]

Committed r199334: <http://trac.webkit.org/changeset/199334>