Bug 240532 – check-webkit-style: allow all upper case value in ResourceRequestBase::Requester

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Yury Semikhatsky]

The enum already contains XHR value which fails style checks when the code is touched.

[Yury Semikhatsky]

Pull request: https://github.com/WebKit/WebKit/pull/685

[EWS]

Committed r294353 (250660@main): <https://commits.webkit.org/250660@main>

Reviewed commits have been landed. Closing PR #685 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/93456202>

[Darin Adler]

Seems like a special case for the value "XHR" would be better than a special case for an enumeration of a particular name.

[Yury Semikhatsky]

(In reply to Darin Adler from comment #4)
> Seems like a special case for the value "XHR" would be better than a special
> case for an enumeration of a particular name.

Yeah, I started with that but then decided to keep it in line with other enums, e.g. NSType has both all upper case and camel case values. Can change that to add exception for a bunch of common abbreviations instead (URL, XHR etc)if you have strong preference.

[Darin Adler]

(In reply to Yury Semikhatsky from comment #5)
> (In reply to Darin Adler from comment #4)
> > Seems like a special case for the value "XHR" would be better than a special
> > case for an enumeration of a particular name.
> 
> Yeah, I started with that but then decided to keep it in line with other
> enums, e.g. NSType has both all upper case and camel case values. Can change
> that to add exception for a bunch of common abbreviations instead (URL, XHR
> etc)if you have strong preference.

I don’t think the way we are handling the other enumerations based on the names of the enumerations is good. I think a list of actual all capital values would be much better. Those would self-evidently be correct. Whereas names of enumerations could be reused in different namespaces, and it’s not at all clear why these enumerations are special.

So I would have started doing exceptions by value for this one, "XHR", and also considered migrating the other exceptions too.

Or maybe there’s an even better scheme. I don’t think the list of enumeration type names is a good scheme.

[Yury Semikhatsky]

(In reply to Darin Adler from comment #6)
> (In reply to Yury Semikhatsky from comment #5)
> > (In reply to Darin Adler from comment #4)
> > > Seems like a special case for the value "XHR" would be better than a special
> > > case for an enumeration of a particular name.
> > 
> > Yeah, I started with that but then decided to keep it in line with other
> > enums, e.g. NSType has both all upper case and camel case values. Can change
> > that to add exception for a bunch of common abbreviations instead (URL, XHR
> > etc)if you have strong preference.
> 
> I don’t think the way we are handling the other enumerations based on the
> names of the enumerations is good. I think a list of actual all capital
> values would be much better. Those would self-evidently be correct. Whereas
> names of enumerations could be reused in different namespaces, and it’s not
> at all clear why these enumerations are special.
> 
> So I would have started doing exceptions by value for this one, "XHR", and
> also considered migrating the other exceptions too.
> 

I can easily add exceptions for the values of Meridiem, NSType and Requester from the current list, but it will not work for JSTokenType which has too many values and they look like plain violations of the style rule: https://github.com/WebKit/WebKit/blob/b1494de3e5ef6bda876a9d26e5b2f618ba620330/Source/JavaScriptCore/parser/ParserTokens.h#L63

In that case the most practical approach would be to keep enum name exception for JSTokenType and add exceptions for the values of the other enums and remove them from _ALLOW_ALL_UPPERCASE_ENUM. WDYT?

[Darin Adler]

Yes, that’s a great next step.

I think for JSTokenType we might just just consider actually changing them enumeration values at some point. I’m not sure these need to be all uppercase.

Whereas "XHR" has to be!