Bug 47746 – Respect charset in handleDataURL

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Kwang Yul Seo]

We must use TextEncoding(charset) to encode data URL again. Otherwise TextEncoding::encode() returns an empty string because encoding name is not specified.

Also use "US-ASCII" as fallback charset as extractCharsetFromMediaType can be empty.

[Kwang Yul Seo]

Created attachment 70909 [details]
Patch

[Darin Adler]

Comment on attachment 70909 [details]
Patch

Is there a regression test that covers this? If so, where is the patch to the expected results to show the progression?

[Kwang Yul Seo]

(In reply to comment #2)
> (From update of attachment 70909 [details])
> Is there a regression test that covers this? If so, where is the patch to the expected results to show the progression?

We need this patch to pass Acid 3 test 97 (data URL).

[Kwang Yul Seo]

(In reply to comment #2)
> (From update of attachment 70909 [details])
> Is there a regression test that covers this? If so, where is the patch to the expected results to show the progression?

r69183 passed all LayoutTests. I think we need to add a new test case for this.

[Kwang Yul Seo]

Adding Patrick to CC.

[Patrick R. Gansterer]

Created attachment 74968 [details]
Patch

I added an additonal change to the patch (we don't need to declare the default charset twice).

(In reply to comment #2)
> (From update of attachment 70909 [details])
> Is there a regression test that covers this? If so, where is the patch to the expected results to show the progression?
The reason for this is, that none of the current ports with a buildbot uses this functionality.
I've run the GTK LayoutTests on the patch which introduced DataURL.cpp. I thought GTK uses the curl backend, but this was wrong and so I didn't see the regression :-(.

I did a small test and changed a port with LayoutTests to use DataURL.cpp: This patch fixed over 130 layout tests.
So I don't think that we need additional tests. We only need to run the existing ones.

[Patrick R. Gansterer]

@darin: ping

[Eric Seidel (no email)]

Comment on attachment 74968 [details]
Patch

The ChangeLog needs to mention that this fixes 130 layout tests, and for which port.  Otherwise the change looks fine.

(Every change requires a test, or explanation of why testing is impossible.)  In this case, you have many tests, you just didn't explain that you're fixing them (which is importnat both for reviewers and folks looking back through teh svn history).

[Patrick R. Gansterer]

Created attachment 77507 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 77507 [details]
Patch

What do other ports do when extractCharsetFromMediaType returns empty?  Do they already assume ASCII?  If so, how?

Seems like it should be possible to write a test for this which would fail on other ports as well, no?

[Adam Barth]

Comment on attachment 77507 [details]
Patch

130!  Wow!

[Adam Barth]

> Seems like it should be possible to write a test for this which would fail on other ports as well, no?

Not very many folks use this file.

[WebKit Commit Bot]

Comment on attachment 77507 [details]
Patch

Clearing flags on attachment: 77507

Committed r84976: <http://trac.webkit.org/changeset/84976>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.