Bug 47746

Summary: Respect charset in handleDataURL
Product: WebKit Reporter: Kwang Yul Seo <skyul>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, eric, paroga
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Kwang Yul Seo 2010-10-15 15:50:43 PDT 
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.
Comment 1 Kwang Yul Seo 2010-10-15 15:52:48 PDT 
Created attachment 70909 [details]
Patch
Comment 2 Darin Adler 2010-10-15 15:54:59 PDT 
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?
Comment 3 Kwang Yul Seo 2010-10-15 15:56:03 PDT 
(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).
Comment 4 Kwang Yul Seo 2010-10-15 15:59:06 PDT 
(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.
Comment 5 Kwang Yul Seo 2010-10-15 16:00:05 PDT 
Adding Patrick to CC.
Comment 6 Patrick R. Gansterer 2010-11-27 17:29:19 PST 
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.
Comment 7 Patrick R. Gansterer 2010-11-30 01:52:57 PST 
@darin: ping
Comment 8 Eric Seidel (no email) 2010-12-13 00:37:19 PST 
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).
Comment 9 Patrick R. Gansterer 2010-12-27 07:46:15 PST 
Created attachment 77507 [details]
Patch
Comment 10 Eric Seidel (no email) 2011-01-06 12:39:16 PST 
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?
Comment 11 Adam Barth 2011-04-26 15:30:05 PDT 
Comment on attachment 77507 [details]
Patch

130!  Wow!
Comment 12 Adam Barth 2011-04-26 15:30:28 PDT 
> 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.
Comment 13 WebKit Commit Bot 2011-04-26 16:26:18 PDT 
Comment on attachment 77507 [details]
Patch

Clearing flags on attachment: 77507

Committed r84976: <http://trac.webkit.org/changeset/84976>
Comment 14 WebKit Commit Bot 2011-04-26 16:26:23 PDT 
All reviewed patches have been landed.  Closing bug.