Content filter replacement data uses the encoding from the blocked page's response headers

[Andy Estes]

Content filter replacement data uses the encoding from the blocked page's response headers

[Andy Estes]

<rdar://problem/14374097>

[Andy Estes]

Created attachment 207700 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 207700 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        Forget about encodings determined from these sources. The replacement
> +        data will specify an encoding in a <meta charset>, so let that be used
> +        instead.

This doesn’t give me a good feeling. Why is charset special? What about all other HTTP header fields? Just as one example, will we still download if disposition was an attachment?

> Source/WebCore/loader/DocumentLoader.h:407
>          RefPtr<ContentFilter> m_contentFilter;
> +        bool m_loadWasBlockedByContentFiltering;

Can we get this information from m_contentFilter instead of tracking it separately?

[Darin Adler]

Comment on attachment 207700 [details]
Patch

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

r=me but please consider Alexey’s question

> Source/WebCore/loader/DocumentLoader.cpp:807
> +        // The content filter's replacement data has a known encoding. Ignore
> +        // both the override encoding and the blocked response's encoding.
> +        if (m_loadWasBlockedByContentFiltering) {
> +            userChosen = false;
> +            encoding = String();
> +        }

It’s a little awkward to compute and then ignore userChosen and encoding. Would be nice to structure the code so they were not even computed.

[Andy Estes]

(In reply to comment #3)
> (From update of attachment 207700 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207700&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        Forget about encodings determined from these sources. The replacement
> > +        data will specify an encoding in a <meta charset>, so let that be used
> > +        instead.
> 
> This doesn’t give me a good feeling. Why is charset special? What about all other HTTP header fields? Just as one example, will we still download if disposition was an attachment?

I don't think charset is special. As well as the problem you mention, mitzpettel also pointed out that we parse the replacement data using the content-type of the original response (luckily we control this data). So I'm also uneasy about how we're using headers from the original response.

This bug isn't trying to address these issues across the board, though. The problem of using the wrong encoding for the replacement data was encountered often enough that a targeted fix made sense to me, the other issues notwithstanding.

> 
> > Source/WebCore/loader/DocumentLoader.h:407
> >          RefPtr<ContentFilter> m_contentFilter;
> > +        bool m_loadWasBlockedByContentFiltering;
> 
> Can we get this information from m_contentFilter instead of tracking it separately?

Yes, good idea. I'll make the change under the auspices of Darin's review, since he asked me to consider your feedback.

[Lucas Forschler]

<rdar://problem/14374097>

[Andy Estes]

Committed r153502: <http://trac.webkit.org/changeset/153502>