Bug 71337 - Enable 8 Bit Strings in JavaScriptCore
Summary: Enable 8 Bit Strings in JavaScriptCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on: 71761 71862 72288 72289 72290 72317 72323 72326
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-01 16:34 PDT by Michael Saboff
Modified: 2011-11-17 23:46 PST (History)
4 users (show)

See Also:


Attachments
Patch for review (30.53 KB, patch)
2011-11-16 10:09 PST, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2011-11-01 16:34:50 PDT
This bug is to track other changes made or yet to be made in order to enable 8 bit strings in JavaScriptCore. There are several small changes that need to be made to "turn on" the 8 bit path in code that has been or will be checked in.
Comment 1 Michael Saboff 2011-11-01 17:26:35 PDT
There are several "FIXME" occurrences in Source/JavaScriptCore/parser/Lexer.cpp that need to be changed as part of turning on 8 bit strings.

These are in the methods append8(), parseIdentifier() and parseString().
Comment 2 Michael Saboff 2011-11-07 19:39:44 PST
In Lexer.cpp, parseIdentifier() was refactored and now only the #if 0 needs to be removed.
Comment 3 Michael Saboff 2011-11-16 10:09:02 PST
Created attachment 115402 [details]
Patch for review

This patch may have export issues building on windows.
Comment 4 Geoffrey Garen 2011-11-16 12:35:42 PST
Comment on attachment 115402 [details]
Patch for review

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

r=me

> Source/JavaScriptCore/ChangeLog:13
> +        type.  This change riplled into WebCore code as well.

Typo: riplled.

> Source/JavaScriptCore/runtime/Identifier.cpp:172
> +    if (string.is8Bit()) {

Would be nice to have a template helper for this, to achieve only one copy of the code vs two.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:540
> +        const LChar* from = characters8();

This is another candidate for a template helper function.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:-1351
> -            if (!bc)

This is another candidate for a template helper function -- would have only needed to fix this bug in one place instead of two.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:1579
> +PassRefPtr<StringImpl> StringImpl::adopt(StringBuffer<LChar>& buffer)

This is another candidate for templatization.
Comment 5 Michael Saboff 2011-11-16 14:42:45 PST
(In reply to comment #4)
> (From update of attachment 115402 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115402&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:13
> > +        type.  This change riplled into WebCore code as well.
> 
> Typo: riplled.

Fixed.
 
> > Source/JavaScriptCore/runtime/Identifier.cpp:172
> > +    if (string.is8Bit()) {
> 
> Would be nice to have a template helper for this, to achieve only one copy of the code vs two.

Done.

> > Source/JavaScriptCore/wtf/text/StringImpl.cpp:540
> > +        const LChar* from = characters8();
> 
> This is another candidate for a template helper function.

Done.

> > Source/JavaScriptCore/wtf/text/StringImpl.cpp:-1351
> > -            if (!bc)
> 
> This is another candidate for a template helper function -- would have only needed to fix this bug in one place instead of two.

Done.

> > Source/JavaScriptCore/wtf/text/StringImpl.cpp:1579
> > +PassRefPtr<StringImpl> StringImpl::adopt(StringBuffer<LChar>& buffer)
> 
> This is another candidate for templatization.

Didn't template this case due to making StringImpl.h dependent on StringBuffer.h.  Tried putting only declaration in StringImpl.h and the implementation in StringImpl.cpp and then explicitly instatiate the two flavors, but that creates symbols with Weak references that doesn't compile.  Given these various issues, felt that the implementation in the original patch was preferred.
Comment 6 Michael Saboff 2011-11-16 14:46:48 PST
Committed r100510: <http://trac.webkit.org/changeset/100510>
Comment 7 Csaba Osztrogonác 2011-11-16 23:16:35 PST
(In reply to comment #6)
> Committed r100510: <http://trac.webkit.org/changeset/100510>

I don't know how, but it broke 4 tests on the Qt bot:

--- /ramdisk/qt-linux-64-release/build/layout-test-results/editing/selection/find-yensign-and-backslash-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/editing/selection/find-yensign-and-backslash-actual.txt 
@@ -2,12 +2,12 @@
    
 Results
 
-We can find a backslash in EUC-JP page by finding a yen sign: PASS
-We can find a backslash in EUC-JP text control by finding a yen sign: PASS
-We can find a backslash in Shift_JIS page by finding a yen sign: PASS
-We can find a backslash in Shift_JIS text control by finding a yen sign: PASS
-We can find a backslash in ISO-2022-JP page by finding a yen sign: PASS
-We can find a backslash in ISO-2022-JP text control by finding a yen sign: PASS
+We can find a backslash in EUC-JP page by finding a yen sign: FAIL
+We can find a backslash in EUC-JP text control by finding a yen sign: FAIL
+We can find a backslash in Shift_JIS page by finding a yen sign: FAIL
+We can find a backslash in Shift_JIS text control by finding a yen sign: FAIL
+We can find a backslash in ISO-2022-JP page by finding a yen sign: FAIL
+We can find a backslash in ISO-2022-JP text control by finding a yen sign: FAIL
 We can NOT find a backslash in UTF8 page by finding a yen sign: PASS
 We can NOT find a backslash in UTF8 text control by finding a yen sign: PASS
 

--- /ramdisk/qt-linux-64-release/build/layout-test-results/editing/selection/find-yensign-and-backslash-with-japanese-fonts-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/editing/selection/find-yensign-and-backslash-with-japanese-fonts-actual.txt 
@@ -1,24 +1,24 @@
 No specified font + yen sign: PASS
 No specified font + backslash: PASS
-MS PGothic + yen sign: PASS
+MS PGothic + yen sign: FAIL
 MS PGothic + backslash: PASS
-MS Gothic + yen sign: PASS
+MS Gothic + yen sign: FAIL
 MS Gothic + backslash: PASS
-MS PMincho + yen sign: PASS
+MS PMincho + yen sign: FAIL
 MS PMincho + backslash: PASS
-MS Mincho + yen sign: PASS
+MS Mincho + yen sign: FAIL
 MS Mincho + backslash: PASS
-Meiryo + yen sign: PASS
+Meiryo + yen sign: FAIL
 Meiryo + backslash: PASS
- ���� + yen sign: PASS
+ ���� + yen sign: FAIL
  ���� + backslash: PASS
- ���� + yen sign: PASS
+ ���� + yen sign: FAIL
  ���� + backslash: PASS
- �� + yen sign: PASS
+ �� + yen sign: FAIL
  �� + backslash: PASS
- �� + yen sign: PASS
+ �� + yen sign: FAIL
  �� + backslash: PASS
-���� + yen sign: PASS
+���� + yen sign: FAIL
 ���� + backslash: PASS
 Times + yen sign: PASS
 Times + backslash: PASS


--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/text/find-kana-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/text/find-kana-actual.txt 
@@ -27,7 +27,7 @@
 PASS canFind(katakanaLetterGa, katakanaLetterGa) is true
 PASS canFind(katakanaLetterKa, katakanaLetterKa) is true
 PASS canFind(katakanaLetterSmallA, katakanaLetterSmallA) is true
-PASS canFind(latinCapitalLetterAWithGrave, latinCapitalLetterAWithGrave) is true
+FAIL canFind(latinCapitalLetterAWithGrave, latinCapitalLetterAWithGrave) should be true. Was false.
 
 Hiragana, katakana, and half width katakana: Must be treated as equal
 


--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/text/find-spaces-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/text/find-spaces-actual.txt 
@@ -1,1 +1,1 @@
-SUCCESS: Found all the spaces as expected.
+FAILURE: Cannot find space. Cannot find two spaces in a <pre> element. Cannot find an &nbsp; when searching for a space. Cannot find a space followed by &nbsp; when searching for two spaces. Cannot find an &nbsp; followed by a space when searching for two spaces.
Comment 8 Gabor Loki 2011-11-17 23:46:12 PST
> > Committed r100510: <http://trac.webkit.org/changeset/100510>
> 
> I don't know how, but it broke 4 tests on the Qt bot:

The problem was fixed in bug 72602.