Bug 54333 – Expose checked field of HTMLInputElement to Chromium API

• Component: New Bugs
• Status: RESOLVED FIXED

[Jay Soffian]

Expose checked field of HTMLInputElement to Chromium API

[Jay Soffian]

Created attachment 82226 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 82226 [details]
Patch

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

> w/Source/WebKit/chromium/public/WebInputElement.h:81
> +        WEBKIT_API bool checked() const;

How about renaming this to isChecked to be more consistent with how other, similar API methods are named?
"checked" is really trying to ask a question, so it helps to word it like a question.

[Jay Soffian]

Every other WebInputElement method is named identically to the HTMLInputElement method it calls though to. This would be the first WebInputElement method that uses a different name. Isn't consistency more important? Also, there's this comment in HTMLInputElement.h:

// isChecked is used by the rendering tree/CSS while checked() is used by JS to determine checked state

[Darin Fisher (:fishd, Google)]

(In reply to comment #3)
> Every other WebInputElement method is named identically to the HTMLInputElement method it calls though to. This would be the first WebInputElement method that uses a different name. Isn't consistency more important?

You're talking about an implementation detail.  I'm arguing for making the publicly facing API consistent from the perspective of the guy who uses the API.

[Jay Soffian]

I will defer to your judgement, but in my experience abstractions leak and when that happens, it's less confusing for the same concepts at different layers to have the same name. Sending up new patch shortly.

[Jay Soffian]

Created attachment 82330 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 82330 [details]
Patch

R=me

It sounds like the internal naming is unfortunate.  "isChecked" and "checked" sound way too similar.  I'd recommend finding a better name for one of those.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 82330 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 82330 [details]
Patch

Clearing flags on attachment: 82330

Committed r78494: <http://trac.webkit.org/changeset/78494>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #7)
> It sounds like the internal naming is unfortunate.  "isChecked" and "checked" sound way too similar.  I'd recommend finding a better name for one of those.

I think we ended up with these names because isChecked is a good name and checked is the name used in the HTML DOM.

[Darin Fisher (:fishd, Google)]

(In reply to comment #11)
> (In reply to comment #7)
> > It sounds like the internal naming is unfortunate.  "isChecked" and "checked" sound way too similar.  I'd recommend finding a better name for one of those.
> 
> I think we ended up with these names because isChecked is a good name and checked is the name used in the HTML DOM.

You don't find the names confusingly similar?

[Darin Adler]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #7)
> > > It sounds like the internal naming is unfortunate.  "isChecked" and "checked" sound way too similar.  I'd recommend finding a better name for one of those.
> > 
> > I think we ended up with these names because isChecked is a good name and checked is the name used in the HTML DOM.
> 
> You don't find the names confusingly similar?

I do. My intention was to explain how we got here, not defend where we are ;-)

It’s bad to have two similar names with different behavior. They should definitely be named in a way that makes the difference clear.

[Darin Fisher (:fishd, Google)]

OK, I created a bug about the naming issue:  see bug 54476.