Bug 71337 – Enable 8 Bit Strings in JavaScriptCore

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

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.

[Michael Saboff]

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().

[Michael Saboff]

In Lexer.cpp, parseIdentifier() was refactored and now only the #if 0 needs to be removed.

[Michael Saboff]

Created attachment 115402 [details]
Patch for review

This patch may have export issues building on windows.

[Geoffrey Garen]

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.

[Michael Saboff]

(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.

[Michael Saboff]

Committed r100510: <http://trac.webkit.org/changeset/100510>

[Csaba Osztrogonác]

(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.

[Gabor Loki]

> > 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.