Bug 255872 – Merge isXMLSpace() into isJSONOrHTTPWhitespace()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ahmad Saleem]

Hi All,

As follow-up for the bug 255841, I got following comments from @Anne and I am not creating this bug to do follow-up to fix it.

https://github.com/WebKit/WebKit/pull/13080/files#diff-55730619c1a379be231cf1fe2fc9b2af29c69d76771c313bc97dd4825a31135a

Just wanted to raise so I can address it.

Thanks!

[Alexey Proskuryakov]

I don’t really like the proposed name. It tells the reader what the function does mechanically, but not conceptually. How would one know if it’s used appropriately when looking at the code?

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #1)
> I don’t really like the proposed name. It tells the reader what the function
> does mechanically, but not conceptually. How would one know if it’s used
> appropriately when looking at the code?

I agree with Alexey.

[Ahmad Saleem]

(In reply to Alexey Proskuryakov from comment #1)
> I don’t really like the proposed name. It tells the reader what the function
> does mechanically, but not conceptually. How would one know if it’s used
> appropriately when looking at the code?

Fair point! The name was suggested by Anne due to similar naming convention used in ASCIICType.h for other stuff. I have two possible solutions:

1) Add comment to explain where this is used when defining it via ASCIICType
2) Think of new name like isASCIIWhitespaceMinusFormFeedForXPath or something highlighting a use case as well.

I am open for suggestion as well for name or other possible solutions.

[Anne van Kesteren]

I think we should call it isHTTPWhitespace as that will be the dominant use of this (see isHTTPSpace which it'll replace). And then we alias/comment locally whenever it's not about HTTP, such as this case.

[Ahmad Saleem]

(In reply to Anne van Kesteren from comment #4)
> I think we should call it isHTTPWhitespace as that will be the dominant use
> of this (see isHTTPSpace which it'll replace). And then we alias/comment
> locally whenever it's not about HTTP, such as this case.

I looked into current 'isHTTPWhitespace' and it is similar to what we did in the bug 255841 except the simplification with Alexey suggested.

So just for clarity:

1) Move current isHTTPWhitespace to ASCIICType.h and then change header from HTTPParser to ASCIICType.h wherever we are using this function.
2) Use 'simplification', which we did (since no compiler optimization issue) in isXMLSpace.
3) Leave comment in XPath to explain the use case.

Make sense?

[Anne van Kesteren]

1. In most (all?) cases you won't have to add an explicit import for this WTF file in my experience.
2. Yeah, I suppose we could do that cleanup more generally. Might want to triple check though.
3. Yeah, or assign it to an appropriately named variable as some kind of alias if that's possible?

[Radar WebKit Bug Importer]

<rdar://problem/108738795>

[Anne van Kesteren]

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

[EWS]

Committed 266253@main (f70bfc73d58c): <https://commits.webkit.org/266253@main>

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