Bug 156458 - Remove UsePointersEvenForNonNullableObjectArguments from HTMLSelectElement
Summary: Remove UsePointersEvenForNonNullableObjectArguments from HTMLSelectElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-10 18:26 PDT by Darin Adler
Modified: 2016-04-11 21:15 PDT (History)
3 users (show)

See Also:


Attachments
Patch (39.22 KB, patch)
2016-04-10 18:36 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (39.24 KB, patch)
2016-04-10 18:50 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (35.76 KB, patch)
2016-04-10 18:52 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (39.92 KB, patch)
2016-04-11 20:07 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2016-04-10 18:26:55 PDT
Remove UsePointersEvenForNonNullableObjectArguments from HTMLSelectElement
Comment 1 Darin Adler 2016-04-10 18:36:40 PDT
Created attachment 276116 [details]
Patch
Comment 2 Darin Adler 2016-04-10 18:50:44 PDT
Created attachment 276117 [details]
Patch
Comment 3 Darin Adler 2016-04-10 18:52:03 PDT
Created attachment 276118 [details]
Patch
Comment 4 Chris Dumez 2016-04-10 21:03:16 PDT
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
Comment 5 Darin Adler 2016-04-11 11:50:58 PDT
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.
Comment 6 Darin Adler 2016-04-11 11:52:14 PDT
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!
Comment 7 Chris Dumez 2016-04-11 11:53:03 PDT
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.
Comment 8 Darin Adler 2016-04-11 11:53:26 PDT
(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.
Comment 9 Chris Dumez 2016-04-11 11:54:39 PDT
(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
Comment 10 Darin Adler 2016-04-11 20:07:19 PDT
Created attachment 276204 [details]
Patch
Comment 11 Darin Adler 2016-04-11 21:15:10 PDT
Committed r199334: <http://trac.webkit.org/changeset/199334>