Add image/x-icon to Blacklisted types from encoding detection

[zhoresh, 2010-02-25 23:39:39]

This is the simple fix of just adding type to blacklist.

Is it time to revisit this and run the encoding detector only on text/html or
text/* instead of using blacklist?

[johnfargo, 2010-02-26 01:09:58]

This change looks fine as-is, but I agree that a whitelist approach is
preferable. I'd start with "text/*" as you mentioned. We might also want to
pass Mime type to the encoding detector in such case as we want more
fine-grained control over certain detection passes (eg. HTML-based).

On Thu, Feb 25, 2010 at 3:39 PM, <zhoresh@gmail.com> wrote:

> This is the simple fix of just adding type to blacklist.
>
> Is it time to revisit this and run the encoding detector only on
> text/html or text/* instead of using blacklist?
>
>
>
>
> http://codereview.appspot.com/224064/show
>

[johnfargo, 2010-02-26 01:16:41]

Committed this patch: it's a clear win over the current code. Thoughts from any
others re: restructuring as Ziv and I have proposed?

[chirag, 2010-02-26 02:19:50]

Why was image/x-icon added to the blacklist, but not: image/vnd.microsoft.icon,
image/ico, image/icon, text/ico, application/ico

On 2010/02/25 23:39:39, zhoresh wrote:
> This is the simple fix of just adding type to blacklist.
> 
> Is it time to revisit this and run the encoding detector only on text/html or
> text/* instead of using blacklist?

[johnfargo, 2010-02-26 07:23:54]

Because that's the particular use case we saw in our logs resulting in
uncaught exceptions :)

IMO your examples call for changing the blacklist to whitelist as discussed.
See any noteworthy issues w/ that option?

Thanks,
John

On Thu, Feb 25, 2010 at 6:19 PM, <chiragshah1@gmail.com> wrote:

> Why was image/x-icon added to the blacklist, but not:
> image/vnd.microsoft.icon, image/ico, image/icon, text/ico,
> application/ico
>
>
> On 2010/02/25 23:39:39, zhoresh wrote:
>
>> This is the simple fix of just adding type to blacklist.
>>
>
>  Is it time to revisit this and run the encoding detector only on
>>
> text/html or
>
>> text/* instead of using blacklist?
>>
>
>
>
> http://codereview.appspot.com/224064/show
>

[chirag, 2010-02-26 18:35:55]

Ah, got it.  +1 to a whitelist

On 2010/02/26 07:23:54, johnfargo wrote:
> Because that's the particular use case we saw in our logs resulting in
> uncaught exceptions :)
> 
> IMO your examples call for changing the blacklist to whitelist as discussed.
> See any noteworthy issues w/ that option?
> 
> Thanks,
> John
> 
> On Thu, Feb 25, 2010 at 6:19 PM, <mailto:chiragshah1@gmail.com> wrote:
> 
> > Why was image/x-icon added to the blacklist, but not:
> > image/vnd.microsoft.icon, image/ico, image/icon, text/ico,
> > application/ico
> >
> >
> > On 2010/02/25 23:39:39, zhoresh wrote:
> >
> >> This is the simple fix of just adding type to blacklist.
> >>
> >
> >  Is it time to revisit this and run the encoding detector only on
> >>
> > text/html or
> >
> >> text/* instead of using blacklist?
> >>
> >
> >
> >
> > http://codereview.appspot.com/224064/show
> >
>