Removing charset information from meta http-equiv content type

Adding a rewriter that removes charset information from meta http-equiv content type tag in html. An html document can specify its charset encoding in the html itself which is used by the browsers in case of ambiguity, like when Content-Type http response header does not specify encoding.

This rewriter removes the charset parameter so the browser cannot get confused. This would be useful as we make sure that we return the page with correct encoding (almost always the page will be parsed and serialized in UTF-8 encoding).

Links with more information about
Content type header: http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.17
Media types: http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.7

[vikaas.arora, 2010-07-22 05:22:30]

http://codereview.appspot.com/1841045/diff/1/4
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverVisitor.java
(right):

http://codereview.appspot.com/1841045/diff/1/4#newcode44
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverVisitor.java:44:
"content-type".equalsIgnoreCase(httpEquip)) {
Make this "content-type" also a constant (final String)

http://codereview.appspot.com/1841045/diff/1/3
File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java
(right):

http://codereview.appspot.com/1841045/diff/1/3#newcode216
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:216: *
without the charset.
Rephrase the method description.
Here the 'charset' attribute is stripped from the content header.

http://codereview.appspot.com/1841045/diff/1/3#newcode220
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:220:
public static String getContentTypeWithoutCharset(String content) {
Add a unit testcase for this new public method.

[gagan.goku, 2010-07-23 02:21:53]

http://codereview.appspot.com/1841045/diff/1/4
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverVisitor.java
(right):

http://codereview.appspot.com/1841045/diff/1/4#newcode44
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverVisitor.java:44:
"content-type".equalsIgnoreCase(httpEquip)) {
On 2010/07/22 05:22:30, vikaas.arora wrote:
> Make this "content-type" also a constant (final String)

Done.

http://codereview.appspot.com/1841045/diff/1/3
File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java
(right):

http://codereview.appspot.com/1841045/diff/1/3#newcode216
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:216: *
without the charset.
On 2010/07/22 05:22:30, vikaas.arora wrote:
> Rephrase the method description.
> Here the 'charset' attribute is stripped from the content header.

Done.

http://codereview.appspot.com/1841045/diff/1/3#newcode220
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:220:
public static String getContentTypeWithoutCharset(String content) {
On 2010/07/22 05:22:30, vikaas.arora wrote:
> Add a unit testcase for this new public method.

Done.

[vikaas.arora, 2010-07-23 03:40:45]

lgtm

Also just add in description, as to why are we removing media-type from
content-type. Also not sure if 'charset' is the only 'media-type' that can occur
in the content-type.

Ref: http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.17

[gagan.goku, 2010-07-23 10:55:09]

Thanks for the review Vikas.
Content type is the header and charset is the parameter. 'text/html;
charset=utg8' is the media type.
there can be more parameters and we are making sure that we retain them as
is.

On Fri, Jul 23, 2010 at 9:10 AM, <vikaas.arora@gmail.com> wrote:

> lgtm
>
> Also just add in description, as to why are we removing media-type from
> content-type. Also not sure if 'charset' is the only 'media-type' that
> can occur in the content-type.
>
> Ref: http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.17
>
>
> http://codereview.appspot.com/1841045/show
>

[zhoresh, 2010-08-25 17:52:16]

lgtm, please just add few more tests, thanks

http://codereview.appspot.com/1841045/diff/26001/27001
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java
(right):

http://codereview.appspot.com/1841045/diff/26001/27001#newcode45
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java:45:
public void testContentTypeCharsetRemoved() throws Exception {
Maybe add few more tests: no meta node, other meta node, bad charset in meta
node

[gagan.goku, 2010-08-26 01:00:06]

http://codereview.appspot.com/1841045/diff/26001/27001
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java
(right):

http://codereview.appspot.com/1841045/diff/26001/27001#newcode45
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java:45:
public void testContentTypeCharsetRemoved() throws Exception {
On 2010/08/25 17:52:16, zhoresh wrote:
> Maybe add few more tests: no meta node, other meta node, bad charset in meta
> node
> 

Done.

[zhoresh, 2010-08-26 22:29:45]

Thanks, but didn't you just caught an issue?

http://codereview.appspot.com/1841045/diff/34001/24002
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java
(right):

http://codereview.appspot.com/1841045/diff/34001/24002#newcode114
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java:114:
+ "<META Content=\"text/html ; charsett=\'hello\'; hello=world\" "
I don't think this should be recognized as charset, it should not be removed.

[gagan.goku, 2010-08-27 02:27:59]

http://codereview.appspot.com/1841045/diff/34001/24002
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java
(right):

http://codereview.appspot.com/1841045/diff/34001/24002#newcode114
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java:114:
+ "<META Content=\"text/html ; charsett=\'hello\'; hello=world\" "
On 2010/08/26 22:29:45, zhoresh wrote:
> I don't think this should be recognized as charset, it should not be removed.

I thought we should retain such attributes because most likely the user meant
charset.
Removing for now. Lets take care of it when we find such an issue.

[zhoresh, 2010-08-31 16:06:38]

On Thu, Aug 26, 2010 at 7:27 PM, <gagan.goku@gmail.com> wrote:

>
> http://codereview.appspot.com/1841045/diff/34001/24002
> File
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java
> (right):
>
> http://codereview.appspot.com/1841045/diff/34001/24002#newcode114
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java:114:
> + "<META Content=\"text/html ; charsett=\'hello\'; hello=world\" "
> On 2010/08/26 22:29:45, zhoresh wrote:
>
>> I don't think this should be recognized as charset, it should not be
>>
> removed.
>
> I thought we should retain such attributes because most likely the user
> meant charset.
> Removing for now. Lets take care of it when we find such an issue.


In the case of accel you probably want to treat bad input the same as a
browser does.
Maybe you can verify what browser does for such case and correct shindig if
needed.


>
>
> http://codereview.appspot.com/1841045/
>

[Jasvir Nagra, 2010-08-31 17:09:18]

On Tue, Aug 31, 2010 at 9:06 AM, Ziv Horesh <zhoresh@gmail.com> wrote:

> On Thu, Aug 26, 2010 at 7:27 PM, <gagan.goku@gmail.com> wrote:
>
> >
> > http://codereview.appspot.com/1841045/diff/34001/24002
> > File
> >
> >
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java
> > (right):
> >
> > http://codereview.appspot.com/1841045/diff/34001/24002#newcode114
> >
> >
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java:114:
> > + "<META Content=\"text/html ; charsett=\'hello\'; hello=world\" "
> > On 2010/08/26 22:29:45, zhoresh wrote:
> >
> >> I don't think this should be recognized as charset, it should not be
> >>
> > removed.
> >
> > I thought we should retain such attributes because most likely the user
> > meant charset.
> > Removing for now. Lets take care of it when we find such an issue.
>
>
> In the case of accel you probably want to treat bad input the same as a
> browser does.
> Maybe you can verify what browser does for such case and correct shindig if
> needed.
>

There isn't just one browser behavior.  If you go down this path, please aim
for the behavior that is the intersection of the Grade
A<http://developer.yahoo.com/yui/articles/gbs/#gbschart>browsers - ie.
feel free to accept bad input, but output something that is
consistently interpreted on all popular browsers.


> >
> >
> > http://codereview.appspot.com/1841045/
> >
>

[gagan.goku, 2010-09-01 01:34:43]

Tested by putting the following content-type response header in a gbk
encoded website and removing the charset information from Content-Type meta
http-equiv tag in firefox:

Response header:
Content-Type: text/html; charsett=gb2312

Firefox does not render the page correctly (but renders it correct if i
spell it as charset=gb2312) , which means that it ignores the misspelt
'charsettt' attribute. So i think ignoring it for now as we are doing is the
right thing to do.
Please commit if you approve of the change.

Thanks
Gagan


2010/8/31 ๏̯͡๏ Jasvir Nagra <jasvir@google.com>

>
>
> On Tue, Aug 31, 2010 at 9:06 AM, Ziv Horesh <zhoresh@gmail.com> wrote:
>
>> On Thu, Aug 26, 2010 at 7:27 PM, <gagan.goku@gmail.com> wrote:
>>
>> >
>> > http://codereview.appspot.com/1841045/diff/34001/24002
>> > File
>> >
>> >
>>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java
>> > (right):
>> >
>> > http://codereview.appspot.com/1841045/diff/34001/24002#newcode114
>> >
>> >
>>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java:114:
>> > + "<META Content=\"text/html ; charsett=\'hello\'; hello=world\" "
>> > On 2010/08/26 22:29:45, zhoresh wrote:
>> >
>> >> I don't think this should be recognized as charset, it should not be
>> >>
>> > removed.
>> >
>> > I thought we should retain such attributes because most likely the user
>> > meant charset.
>> > Removing for now. Lets take care of it when we find such an issue.
>>
>>
>> In the case of accel you probably want to treat bad input the same as a
>> browser does.
>> Maybe you can verify what browser does for such case and correct shindig
>> if
>> needed.
>>
>
> There isn't just one browser behavior.  If you go down this path, please
> aim for the behavior that is the intersection of the Grade
A<http://developer.yahoo.com/yui/articles/gbs/#gbschart>browsers - ie. feel free
to accept bad input, but output something that is
> consistently interpreted on all popular browsers.
>
>
>> >
>> >
>> > http://codereview.appspot.com/1841045/
>> >
>>
>
>

[Jasvir Nagra, 2010-09-01 01:56:40]

LGTM

On Tue, Aug 31, 2010 at 6:34 PM, Gagandeep singh <gagan.goku@gmail.com>wrote:

> Tested by putting the following content-type response header in a gbk
> encoded website and removing the charset information from Content-Type meta
> http-equiv tag in firefox:
>
> Response header:
> Content-Type: text/html; charsett=gb2312
>
> Firefox does not render the page correctly (but renders it correct if i
> spell it as charset=gb2312) , which means that it ignores the misspelt
> 'charsettt' attribute. So i think ignoring it for now as we are doing is the
> right thing to do.
> Please commit if you approve of the change.
>
> Thanks
> Gagan
>
>
> 2010/8/31 ๏̯͡๏ Jasvir Nagra <jasvir@google.com>
>
>
>>
>> On Tue, Aug 31, 2010 at 9:06 AM, Ziv Horesh <zhoresh@gmail.com> wrote:
>>
>>> On Thu, Aug 26, 2010 at 7:27 PM, <gagan.goku@gmail.com> wrote:
>>>
>>> >
>>> > http://codereview.appspot.com/1841045/diff/34001/24002
>>> > File
>>> >
>>> >
>>>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java
>>> > (right):
>>> >
>>> > http://codereview.appspot.com/1841045/diff/34001/24002#newcode114
>>> >
>>> >
>>>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java:114:
>>> > + "<META Content=\"text/html ; charsett=\'hello\'; hello=world\" "
>>> > On 2010/08/26 22:29:45, zhoresh wrote:
>>> >
>>> >> I don't think this should be recognized as charset, it should not be
>>> >>
>>> > removed.
>>> >
>>> > I thought we should retain such attributes because most likely the user
>>> > meant charset.
>>> > Removing for now. Lets take care of it when we find such an issue.
>>>
>>>
>>> In the case of accel you probably want to treat bad input the same as a
>>> browser does.
>>> Maybe you can verify what browser does for such case and correct shindig
>>> if
>>> needed.
>>>
>>
>> There isn't just one browser behavior.  If you go down this path, please
>> aim for the behavior that is the intersection of the Grade
A<http://developer.yahoo.com/yui/articles/gbs/#gbschart>browsers - ie. feel free
to accept bad input, but output something that is
>> consistently interpreted on all popular browsers.
>>
>>
>>> >
>>> >
>>> > http://codereview.appspot.com/1841045/
>>> >
>>>
>>
>>
>

[Jasvir Nagra, 2010-09-01 02:02:36]

LGTM

On Tue, Aug 31, 2010 at 6:34 PM, Gagandeep singh <gagan.goku@gmail.com>wrote:

> Tested by putting the following content-type response header in a gbk
> encoded website and removing the charset information from Content-Type meta
> http-equiv tag in firefox:
>
> Response header:
> Content-Type: text/html; charsett=gb2312
>
> Firefox does not render the page correctly (but renders it correct if i
> spell it as charset=gb2312) , which means that it ignores the misspelt
> 'charsettt' attribute. So i think ignoring it for now as we are doing is the
> right thing to do.
> Please commit if you approve of the change.
>
> Thanks
> Gagan
>
>
> 2010/8/31 ๏̯͡๏ Jasvir Nagra <jasvir@google.com>
>
>
>>
>> On Tue, Aug 31, 2010 at 9:06 AM, Ziv Horesh <zhoresh@gmail.com> wrote:
>>
>>> On Thu, Aug 26, 2010 at 7:27 PM, <gagan.goku@gmail.com> wrote:
>>>
>>> >
>>> > http://codereview.appspot.com/1841045/diff/34001/24002
>>> > File
>>> >
>>> >
>>>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java
>>> > (right):
>>> >
>>> > http://codereview.appspot.com/1841045/diff/34001/24002#newcode114
>>> >
>>> >
>>>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentTypeCharsetRemoverRewriterTest.java:114:
>>> > + "<META Content=\"text/html ; charsett=\'hello\'; hello=world\" "
>>> > On 2010/08/26 22:29:45, zhoresh wrote:
>>> >
>>> >> I don't think this should be recognized as charset, it should not be
>>> >>
>>> > removed.
>>> >
>>> > I thought we should retain such attributes because most likely the user
>>> > meant charset.
>>> > Removing for now. Lets take care of it when we find such an issue.
>>>
>>>
>>> In the case of accel you probably want to treat bad input the same as a
>>> browser does.
>>> Maybe you can verify what browser does for such case and correct shindig
>>> if
>>> needed.
>>>
>>
>> There isn't just one browser behavior.  If you go down this path, please
>> aim for the behavior that is the intersection of the Grade
A<http://developer.yahoo.com/yui/articles/gbs/#gbschart>browsers - ie. feel free
to accept bad input, but output something that is
>> consistently interpreted on all popular browsers.
>>
>>
>>> >
>>> >
>>> > http://codereview.appspot.com/1841045/
>>> >
>>>
>>
>>
>

[gagan.goku, 2010-09-01 02:12:09]

Synced to head so committers don't get any merge failures. I'm so considerate,
aren't i ?

[Paul Lindner, 2010-09-01 06:59:01]

thanks!

submited
On 2010/09/01 02:12:09, gagan.goku wrote:
> Synced to head so committers don't get any merge failures. I'm so considerate,
> aren't i ?

[gagan.goku, 2010-09-01 07:51:04]

Thanks lots Paul :)

On Wed, Sep 1, 2010 at 12:29 PM, <lindner@inuus.com> wrote:

> thanks!
>
> submited
>
> On 2010/09/01 02:12:09, gagan.goku wrote:
>
>> Synced to head so committers don't get any merge failures. I'm so
>>
> considerate,
>
>> aren't i ?
>>
>
>
>
> http://codereview.appspot.com/1841045/
>