Bug 52926 - Add segmentation test for range.expand
Summary: Add segmentation test for range.expand
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-21 14:45 PST by Xiaomei Ji
Modified: 2011-09-30 13:33 PDT (History)
6 users (show)

See Also:


Attachments
layout test (5.86 KB, patch)
2011-01-21 14:49 PST, Xiaomei Ji
rniwa: review-
Details | Formatted Diff | Diff
layout test (7.36 KB, patch)
2011-01-24 13:31 PST, Xiaomei Ji
no flags Details | Formatted Diff | Diff
layout test (7.53 KB, patch)
2011-01-24 14:00 PST, Xiaomei Ji
no flags Details | Formatted Diff | Diff
layout test (7.70 KB, patch)
2011-01-24 14:17 PST, Xiaomei Ji
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaomei Ji 2011-01-21 14:45:26 PST
Add segmentation test for range.expand.
Comment 1 Xiaomei Ji 2011-01-21 14:49:24 PST
Created attachment 79798 [details]
layout test
Comment 2 Ryosuke Niwa 2011-01-21 14:58:39 PST
Comment on attachment 79798 [details]
layout test

This is going to break other platforms. We should either be adding chromium specific test or should be adding a new test when Xiaomei lands a patch to upstream fixes.
Comment 3 Jungshik Shin 2011-01-21 15:22:47 PST
Just some clarification: 

ICU bug to upstream Chrome's CJK segmentation patch is http://bugs.icu-project.org/trac/ticket/2229

Even if that's added to the ICU trunk today, it'll be a year or more before Apple picks it up in next version of Mac OS X. 

Moreover, there are webkit ports that do not use ICU (e.g. Qt, GTK). So fixing the above ICU bug wouldn't help them.
Comment 4 Jungshik Shin 2011-01-21 15:31:42 PST
BTW, shouldn't the test be more 'interesting' (dealing with non-trivial but fairly clear-cut cases)? 

国务院公布《国有土地)  

国务院 
公布 
(
国有 
土地
)
Comment 5 Xiaomei Ji 2011-01-24 13:31:28 PST
Created attachment 79966 [details]
layout test
Comment 6 Xiaomei Ji 2011-01-24 14:00:11 PST
Created attachment 79973 [details]
layout test
Comment 7 Ryosuke Niwa 2011-01-24 14:03:38 PST
Comment on attachment 79973 [details]
layout test

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

> LayoutTests/platform/chromium/fast/text/international/cjk-segmentation-expected.txt:3
> +Test Chinese Segmentation.
> +
> +

We should print out PASS at the end or otherwise, or otherwise it's hard to tell if the test really passed or not.
Comment 8 Ryosuke Niwa 2011-01-24 14:04:54 PST
Comment on attachment 79973 [details]
layout test

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

> LayoutTests/fast/text/international/cjk-segmentation.html:2
> +<html>
> +<head>

Missing DOCTYPE here: <!DOCTYPE html>

> LayoutTests/fast/text/international/cjk-segmentation.html:3
> +<META HTTP-EQUIV="CONTENT-TYPE" CONTENT="text/html; charset=UTF-8">

We probably want to use lowercases for tag and attribute names for the consistency.

> LayoutTests/fast/text/international/cjk-segmentation.html:21
> +    }

Maybe print PASS here?
Comment 9 Xiaomei Ji 2011-01-24 14:17:14 PST
Created attachment 79976 [details]
layout test
Comment 10 Ryosuke Niwa 2011-01-24 14:20:41 PST
Comment on attachment 79976 [details]
layout test

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

> LayoutTests/fast/text/international/cjk-segmentation.html:5
> +<title>Test for CJK segmentation.</title>

Nit: Don't put period in the title.

> LayoutTests/fast/text/international/cjk-segmentation.html:50
> +    if (pass == true) {
> +        log("ALL PASS");
> +    }

Nit: no { } around a single line statement.
Comment 11 Xiaomei Ji 2011-01-24 14:27:00 PST
Committed r76548: <http://trac.webkit.org/changeset/76548>
Comment 12 WebKit Review Bot 2011-01-24 14:55:49 PST
http://trac.webkit.org/changeset/76548 might have broken Qt Linux Release
The following tests are not passing:
fast/text/international/cjk-segmentation.html
Comment 13 Ryosuke Niwa 2011-01-24 16:03:14 PST
This test is failing on Chromium mac port.
Comment 14 David Kilzer (:ddkilzer) 2011-09-30 13:33:24 PDT
(In reply to comment #3)
> Just some clarification: 
> 
> ICU bug to upstream Chrome's CJK segmentation patch is http://bugs.icu-project.org/trac/ticket/2229
> 
> Even if that's added to the ICU trunk today, it'll be a year or more before Apple picks it up in next version of Mac OS X. 

No patch has ever been posted to that ICU bug.  Could you at least post a WIP patch so someone else can work on it if you don't have time?