Bug 22882 - Make fast\encoding\char-encoding.html not depend on the ability to have reference fragments on data URLs
Summary: Make fast\encoding\char-encoding.html not depend on the ability to have refer...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Jungshik Shin
URL:
Keywords:
: 22883 (view as bug list)
Depends on:
Blocks: 91791
  Show dependency tree / graph
 
Reported: 2008-12-16 11:19 PST by xlyuan
Modified: 2012-07-19 15:16 PDT (History)
2 users (show)

See Also:

Attachments
Patch file (3.67 KB, patch)
2008-12-16 11:22 PST, xlyuan 		no flags Details | Formatted Diff | Diff
patch built on top of xlyuan's change (10.01 KB, patch)
2008-12-29 12:55 PST, Jungshik Shin 		no flags Details | Formatted Diff | Diff
patch with ChangeLog (12.10 KB, patch)
2008-12-30 14:18 PST, Jungshik Shin 		no flags Details | Formatted Diff | Diff
patch with changelog diff fixed (11.32 KB, patch)
2008-12-30 14:23 PST, Jungshik Shin 		ap: review+
Details | Formatted Diff | Diff
updated patch with ap's review comments addressed (11.13 KB, patch)
2009-01-05 13:07 PST, Jungshik Shin 		jshin: review+
Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.