[webkit-reviews] review denied: [Bug 54476] HTMLInputElement has two similarly named methods: "checked" and "isChecked" : [Attachment 100435] Patch v3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied SravanKumar <sravan.ken at gmail.com>'s
request for review:
Bug 54476: HTMLInputElement has two similarly named methods: "checked" and
"isChecked"
https://bugs.webkit.org/show_bug.cgi?id=54476

Attachment 100435: Patch v3
https://bugs.webkit.org/attachment.cgi?id=100435&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=100435&action=review


Since the entire purpose of this patch is renaming, and I am suggesting a
different name, review-.

> Source/WebCore/html/HTMLInputElement.h:129
> +    // renderAsChecked is used by the rendering tree/CSS while checked() is
used by JS to determine checked state
> +    virtual bool renderAsChecked() const;

I think it’s great to choose a name here that is distinct from the DOM function
checked, but I am not a big fan of the term “render” here. I think there are
clearer less-jargony terms we can use. The name I like best is
shouldAppearChecked. Other possible names could be isVisiblyChecked or
appearsChecked.

The only reason this function or isIndeterminate was a virtual function was the
WML support. This should not be virtual any more. If it was really a virtual
function then we’d have to rename the other implementations, but there are
none. No reason to spend the extra runtime overhead of a virtual function
dispatch.

> Source/WebCore/html/HTMLInputElement.h:130
>      virtual bool isIndeterminate() const { return indeterminate(); }

Not your responsibility in this patch, but this function needs to be removed.
It only existed because of WML, and it’s now just a better-named, slow synonym
for the DOM indeterminate function.