Bug 54333 - Expose checked field of HTMLInputElement to Chromium API
Summary: Expose checked field of HTMLInputElement to Chromium API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-11 19:49 PST by Jay Soffian
Modified: 2011-02-15 10:24 PST (History)
3 users (show)

See Also:


Attachments
Patch (1.96 KB, patch)
2011-02-11 19:51 PST, Jay Soffian
no flags Details | Formatted Diff | Diff
Patch (1.97 KB, patch)
2011-02-14 10:24 PST, Jay Soffian
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jay Soffian 2011-02-11 19:49:41 PST
Expose checked field of HTMLInputElement to Chromium API
Comment 1 Jay Soffian 2011-02-11 19:51:37 PST
Created attachment 82226 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 2011-02-13 21:02:34 PST
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.
Comment 3 Jay Soffian 2011-02-14 07:25:51 PST
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
Comment 4 Darin Fisher (:fishd, Google) 2011-02-14 10:03:03 PST
(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.
Comment 5 Jay Soffian 2011-02-14 10:16:19 PST
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.
Comment 6 Jay Soffian 2011-02-14 10:24:37 PST
Created attachment 82330 [details]
Patch
Comment 7 Darin Fisher (:fishd, Google) 2011-02-14 11:16:08 PST
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.
Comment 8 WebKit Commit Bot 2011-02-14 11:43:24 PST
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: dumi@chromium.org)
The commit-queue is continuing to process your patch.
Comment 9 WebKit Commit Bot 2011-02-14 11:46:03 PST
Comment on attachment 82330 [details]
Patch

Clearing flags on attachment: 82330

Committed r78494: <http://trac.webkit.org/changeset/78494>
Comment 10 WebKit Commit Bot 2011-02-14 11:46:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Darin Adler 2011-02-14 12:13:18 PST
(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.
Comment 12 Darin Fisher (:fishd, Google) 2011-02-15 09:52:29 PST
(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?
Comment 13 Darin Adler 2011-02-15 09:57:10 PST
(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.
Comment 14 Darin Fisher (:fishd, Google) 2011-02-15 10:24:23 PST
OK, I created a bug about the naming issue:  see bug 54476.