Bug 37881 – Numerous style violations in HTMLInputElement.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Ray]

HTMLInputElement.cpp contains numerous style violations --- most are either misformated switch/case blocks or multi-line if statements.

I will provide a patch shortly.

[Ray]

Created attachment 53864 [details]
Corrections for all style test failures.

This patch contains style changes only --- mostly white space and some small changes across multiple lines (for malformed if conditionals and the like).

[Darin Adler]

Comment on attachment 53864 [details]
Corrections for all style test failures.

Since this is all about style I was picky about style.

> +         RegularExpression patternRegExp(pattern, TextCaseSensitive);
> +        int matchLength = 0;

Looks like the patternRegExp line is incorrectly indented.

> -            if (focusedInput->inputType() == RADIO && focusedInput->form() == form() &&
> -                focusedInput->name() == name())
> +            if (focusedInput->inputType() == RADIO && focusedInput->form() == form() 
> +                && focusedInput->name() == name())
>                  return false;

I think this would be better if it was just all on one long line.

> -    } else if (attr->name() == usemapAttr ||
> -               attr->name() == accesskeyAttr) {
> +    } else if (attr->name() == usemapAttr 
> +            || attr->name() == accesskeyAttr) {

I think this would be better if it was just all on one long line.

> +    } else if (attr->name() == onsearchAttr) {
> +    // Search field and slider attributes all just cause updateFromElement to be called through style recalcing.
>          setAttributeEventListener(eventNames().searchEvent, createAttributeEventListener(this, attr));

I think the comment should be indented at the same level as the code below it.

> -void HTMLInputElement::setIndeterminate(bool _indeterminate)
> +void HTMLInputElement::setIndeterminate(bool indet)

We normally use words, not abbreviations. I think "indet" is unclear. Maybe "newValue" would be better.

Otherwise looks fine. review- because you should fix at least some, preferably all, of what I mentioned above.

[Ray]

Created attachment 53894 [details]
Style changes as discussed with Darin Adler

Only style / format changes are in this patch.

[Ray]

(In reply to comment #2)
> (From update of attachment 53864 [details])
> Since this is all about style I was picky about style.
> [ suggestions removed for brevity]
> 
> Otherwise looks fine. review- because you should fix at least some, preferably
> all, of what I mentioned above.

Thanks, Darin! I agree with all of your comments, and have incorporated corrections for each of them.

Thanks!

[WebKit Commit Bot]

Comment on attachment 53894 [details]
Style changes as discussed with Darin Adler

Rejecting patch 53894 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact [e-mail] to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 53894 [details]
Style changes as discussed with Darin Adler

Rejecting patch 53894 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', '53894', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=53894&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=37881&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 53894 from bug 37881.
ERROR: /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Ray]

Created attachment 53960 [details]
ChangeLog update to permit build bot to check in.

ChangeLog now contains reviewer name to permit the BuildBot to take the changes. 
No changes to the code submission.

[Darin Adler]

Comment on attachment 53960 [details]
ChangeLog update to permit build bot to check in.

It's not actually correct to put the reviewer's name in the change log. Instead you are supposed to just leave the Reviewed by NOBODY line in.

[Eric Seidel (no email)]

Comment on attachment 53894 [details]
Style changes as discussed with Darin Adler

Cleared Darin Adler's review+ from obsolete attachment 53894 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 53960 [details]
ChangeLog update to permit build bot to check in.

Clearing flags on attachment: 53960

Committed r58057: <http://trac.webkit.org/changeset/58057>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.