Improve ContentTypeParser, so that it could be used to validate mime type according to RFC

• Component: XML
• Status: RESOLVED FIXED

[Alexander Shalamov]

At the moment ContentTypeParser contains validation and tokenization logic.
HTTPValidation::isValidHTTPHeader provides basic validation, but that is not enough to validate Content-Type value for XHR request.

It would be beneficial to make ContentTypeParser more flexible and add e.g. ContentTypeValidator that would only validate mime types.

[Alexander Shalamov]

Created attachment 171796 [details]
Patch 1

- Added ContentTypeValidator. Content type string could be validated using ContentTypeValidator<>(contentTypeString).isValid()
- Removed StringBuilder usage that was inefficient.
- Removed "Content-Type" prefix that is not required to validate and parse mime type value.

[Alexey Proskuryakov]

Comment on attachment 171796 [details]
Patch 1

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

This interface seems overly complicated. The task is to tell if a string is valid, so the client should just call a function. Figuring out how to combine the templates into something usable is an unnecessary complication in this case.

If templates are desired for implementation, that could be an implementation detail, no need to expose that at call sites.

> Source/WebCore/platform/network/ContentTypeParser.cpp:186
> +    const unsigned contentTypeLength = contentType.length();

As a matter of coding style, we don't use const with local variables.

> Source/WebCore/platform/network/ContentTypeParser.h:82
> -    KeyValuePairs m_parameters;
> -    String m_mimeType;
> +    String m_contentType;
> +    mutable KeyValuePairs m_parameters;
> +    mutable String m_mimeType;

If "ContentTypeParser" encapsulates parsed results, then it's not a parser, but a "ParsedContentType".

[Alexander Shalamov]

Created attachment 172034 [details]
Patch 2

- Renamed ContentTypeParser to ParsedContentType.
- Moved implementation details to cpp.
- const unsigned contentTypeLength ... => unsigned contentTypeLength ...
- Added isValidContentType function that could be used to validate content type strings.

[Alexander Shalamov]

Created attachment 172037 [details]
Patch 3

- rebased to master

[WebKit Review Bot]

Comment on attachment 172037 [details]
Patch 3

Attachment 172037 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14687918

New failing tests:
mhtml/shared_buffer_bug.mht
mhtml/simple_page_unmht.mht
mhtml/page_with_image_ie.mht
mhtml/multi_frames_ie.mht
mhtml/multi_frames_binary.mht
mhtml/check_domain.mht
mhtml/page_with_image_unmht.mht
mhtml/multi_frames_unmht.mht
mhtml/page_with_css_and_js_ie.mht
mhtml/page_with_css_and_js_unmht.mht

[Alexander Shalamov]

Created attachment 172289 [details]
Patch 4

- Check for \r \n characters inside isValidContentType(const String&) function.

[WebKit Review Bot]

Comment on attachment 172289 [details]
Patch 4

Attachment 172289 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14722695

New failing tests:
mhtml/shared_buffer_bug.mht
mhtml/simple_page_unmht.mht
mhtml/page_with_image_ie.mht
mhtml/multi_frames_ie.mht
mhtml/multi_frames_binary.mht
mhtml/check_domain.mht
mhtml/page_with_image_unmht.mht
mhtml/multi_frames_unmht.mht
mhtml/page_with_css_and_js_ie.mht
mhtml/page_with_css_and_js_unmht.mht

[Alexander Shalamov]

Created attachment 172318 [details]
Patch 5

- fixed typo lenght => length
- fixed token length calculation

[WebKit Review Bot]

Comment on attachment 172318 [details]
Patch 5

Attachment 172318 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14748040

New failing tests:
mhtml/shared_buffer_bug.mht
mhtml/simple_page_unmht.mht
mhtml/page_with_image_ie.mht
mhtml/multi_frames_ie.mht
mhtml/multi_frames_binary.mht
mhtml/check_domain.mht
mhtml/page_with_image_unmht.mht
mhtml/multi_frames_unmht.mht
mhtml/page_with_css_and_js_ie.mht
mhtml/page_with_css_and_js_unmht.mht

[Alexander Shalamov]

Created attachment 172365 [details]
Patch 6

- exclude quotes from parsed value

[Alexey Proskuryakov]

Comment on attachment 172365 [details]
Patch 6

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

Looks good to me. A few nits below.

> Source/WebCore/platform/network/ParsedContentType.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Is it intentional that you did not add your copyright, or update the date if you work for Google?

> Source/WebCore/platform/network/ParsedContentType.cpp:39
> +class EmptyParsedContentType {

We normally use "dummy" for unused variables. Perhaps it would be more understandable here, too.

> Source/WebCore/platform/network/ParsedContentType.cpp:59
> +    const unsigned inputLength = input.length();
> +    const unsigned index = startIndex;

As a matter of coding style, we don't use const with local variables.

> Source/WebCore/platform/network/ParsedContentType.cpp:77
> +    const unsigned inputLength = input.length();
> +    const unsigned index = startIndex + 1;

Ditto.

"index" is not a good name for a variable other than a loop one.

> Source/WebCore/platform/network/ParsedContentType.cpp:153
> +bool parseContentType(const String& contentType, T& parserType)

parserTyoe does not seem to be accurate. I think that this should be "ReceiverType& receiver".

> Source/WebCore/platform/network/ParsedContentType.h:36
> +#include <wtf/text/WTFString.h>

This include should not be needed.

[Alexander Shalamov]

Created attachment 174924 [details]
Patch 7

- fixes according to review comments
- rebased to master

[Alexander Shalamov]

Created attachment 174950 [details]
Patch 8

- fixed typo

[WebKit Review Bot]

Comment on attachment 174950 [details]
Patch 8

Clearing flags on attachment: 174950

Committed r135176: <http://trac.webkit.org/changeset/135176>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.