Bug 22882

Summary: Make fast\encoding\char-encoding.html not depend on the ability to have reference fragments on data URLs
Product: WebKit Reporter: xlyuan
Component: Tools / TestsAssignee: Jungshik Shin <jshin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, julian.reschke
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 91791    
Attachments:
Description Flags
Patch file
none
patch built on top of xlyuan's change
none
patch with ChangeLog
none
patch with changelog diff fixed
ap: review+
updated patch with ap's review comments addressed jshin: review+

Description xlyuan 2008-12-16 11:19:58 PST
1. Currently, fast\encoding\char-encoding.html depend on the ability to have reference fragments on data URLs, it makes the layout test on Chrome fails, modified char-encoding.html and added resources/dummy.html to make this test case can be passed on Chrome
2. Move Mac* encodings to char-encoding-mac.html
Comment 1 xlyuan 2008-12-16 11:22:17 PST
Created attachment 26058 [details]
Patch file
Comment 2 Mark Rowe (bdash) 2008-12-17 17:40:54 PST
*** Bug 22883 has been marked as a duplicate of this bug. ***
Comment 3 Alexey Proskuryakov 2008-12-19 03:35:43 PST
The attachment is not a patch, please see <http://webkit.org/coding/contributing.html> for instructions.
Comment 4 Jungshik Shin 2008-12-29 12:55:03 PST
Created attachment 26295 [details]
patch built on top of xlyuan's change

As in attachment 26058 [details] by Xiaolu, I used a local dummy file access instead of data: . While splitting char-encoding to two tests (char-encoding and char-encoding-mac), I moved the common functions to resources/char-encoding-utils.js
Comment 5 Alexey Proskuryakov 2008-12-29 13:16:32 PST
Could you please add a ChangeLog?
Comment 6 Jungshik Shin 2008-12-30 14:18:51 PST
Created attachment 26324 [details]
patch with ChangeLog

Oops. Now this one has the changelog.
Comment 7 Jungshik Shin 2008-12-30 14:23:05 PST
Created attachment 26325 [details]
patch with changelog diff fixed
Comment 8 Alexey Proskuryakov 2008-12-31 00:00:09 PST
Comment on attachment 26325 [details]
patch with changelog diff fixed

> +        possible to to skip it.

Typo: "to to".

> +        - Enable UTF-8 test for U+221A

How did you ensure that this is safe now? Did you test on Tiger?

This should really be mentioned in ChangeLog.

> +        * fast/encoding/resources/char-encoding-utils.js: Added.
> +        (encode):
> +        (testsDone):
> +        (processResult):
> +        (subframeLoaded):
> +        (loadTimedOut):
> +        (runTest):
> +        (testEncode):

We normally try to describe changes for each file, not just as a global overview. This is not very important in this case, but I think that you could omit function names for brevity, as you don't have descriptions of individual changes anyway.

r=me
Comment 9 Jungshik Shin 2009-01-05 13:07:38 PST
Created attachment 26437 [details]
updated patch with ap's review comments addressed

Moving r+ flag from the previous patch. I addressed ap's review comments:

I got rid of function names in ChangeLog. 
I also left alone the UTF-8 test case (i.e. it's still commented out) because I don't have an access to Tiger. I mistakenly thought that the issue was across platforms and not using 'data' url would resolve it.
Comment 10 Alexey Proskuryakov 2009-01-11 01:41:27 PST
Committed revision 39787.

I had to remove UTF-8 test results from expected results file to make the test pass.