Bug 20797

I realized that the UI of a webkit embedder is not supposed to be dealt with here. This bug is actually about making Settings class in WebCore deal with per-script/lang generic families. There might be a way to fix bug 10874 without this. Anyway, tentatively, I'm marking this bug as blocking bug 10874.

Created attachment 28428 [details] tentative patch for ports with ICU This does not work for ports that do not use ICU, but it should be easy to make it work with them. It uses scripts rather than languages as keys. Embedders that do not want to have preferences (that would be translated to Settings) for per-script generic font family do not have to do anything because a new argument (script code) added to setters/getters for generic font families have the default value. The only cost for them is using a single entry hash map instead of a direct access to fontFamily.

This also (indirectly) blocks bug 18085.

Created attachment 28601 [details] patch that should build for ports not using ICU I copied ICU's UScriptCode enum (from icu/unicode/uscript.h) to platform/text/UScriptCode.h (expecting it to be used in files other than Settings.{h,cpp} being changed in the current patch). So, this patch can be used on ports that do not use ICU as well. I'm not sure what to do about the copyright statement/license at the beginning. I put ICU's copyright and license statements at the beginning of UScriptCode.h because it's almost a verbatim copy of the first part of uscript.h.

Comment on attachment 28601 [details] patch that should build for ports not using ICU I am not the right reviewer for this.

Comment on attachment 28601 [details] patch that should build for ports not using ICU Giving this to mitz per hyatt on IRC.

Comment on attachment 28601 [details] patch that should build for ports not using ICU mitz, can you take a look?

Comment on attachment 28601 [details] patch that should build for ports not using ICU I would advise against using macros here. It's true that the code was a bit boilerplate-ish, but using macros this way makes debugging more painful, and in my opinion it's not worth the amount of duplication saved here. I don't see any other problems, but r- for this style issue. Perhaps Mitz has additional comments. > Index: WebCore/page/Settings.cpp > =================================================================== > --- WebCore/page/Settings.cpp (revision 41628) > +++ WebCore/page/Settings.cpp (working copy) > +#define GENERIC_FONT_GETTER_SETTER(generic, Generic) \ > +const AtomicString& Settings::generic##FontFamily(UScriptCode script) const\ > +{\ > + if (m_##generic##FontFamilyMap.contains(static_cast<int>(script)))\ > + return m_##generic##FontFamilyMap.find(static_cast<int>(script))->second;\ > + return emptyAtom;\ > +}\ > +\ > +void Settings::set##Generic##FontFamily(const AtomicString& family, UScriptCode script)\ > +{\ > + if (m_##generic##FontFamilyMap.set(static_cast<int>(script), family).second)\ > + setNeedsReapplyStylesInAllFrames(m_page);\ > +} > + > +GENERIC_FONT_GETTER_SETTER(standard, Standard) > +GENERIC_FONT_GETTER_SETTER(fixed, Fixed) > +GENERIC_FONT_GETTER_SETTER(serif, Serif) > +GENERIC_FONT_GETTER_SETTER(sansSerif, SansSerif) > +GENERIC_FONT_GETTER_SETTER(cursive, Cursive) > +GENERIC_FONT_GETTER_SETTER(fantasy, Fantasy) > > void Settings::setMinimumFontSize(int minimumFontSize) > {

Created attachment 33195 [details] patch updated (macros expanded) Thank you for taking a look and sorry for this terrible delay. I've always had this issue in mind, but somehow I missed the review comment. This patch now has font setters and getters that are expanded out of macros. UScriptCode.h for ports (not using ICU) has the latest script code available from ICU 4.2.1 (Unicode 5.1).

Comment on attachment 33195 [details] patch updated (macros expanded) Maciej, if you could take another look, that'd be great. Thanks for taking a look earlier and in advance and sorry again for the delay.

Comment on attachment 33195 [details] patch updated (macros expanded) Because Maciej didn't have any issue other than macros for the previous patch and this patch removed them, I'm asking mitz to take a look.

Comment on attachment 33195 [details] patch updated (macros expanded) Why do two hash lookups here? 120 if (m_standardFontFamilyMap.contains(static_cast<int>(script))) 121 return m_standardFontFamilyMap.find(static_cast<int>(script))->second; Seems you should just store the result of find in a local and check !null We could do this w/o templates using simple static functions, no? setFamilyInMap(m_fixedFontFamilyMap, script, m_page); static inline setFamilyInMap(ScriptFontFamilyMap& familyMap, UScriptCode code, Page* page) { if (familyMap.set(static_cast<int>(script), family).second) setNeedsReapplyStylesInAllFrames(page); } Same could be done for the getters. Why do we need USCRIPT_CODE_H? Should it be autogenerated? Please re-post the patch using the static functions and avoiding the double-lookup in the getters. (Unless I"m missing some obvious reason why we can't use static funtions here?) Please also explain in the ChangeLog why we're adding UScriptCode, where it came from, and why it shouldn't be autogenerated (from scriptnames.txt... whatever that is.) I'm happy to r+ it with the above changes! Thanks for the patch!

Created attachment 38565 [details] patch addressing Eric's concerns Thanks a lot for the review !!! And, sorry for the late reply. I addressed all your concerns. Unlike the comment in UScriptCodes.h (which I copied from ICU), the Unicode character database doesn't have ScriptNames.txt. Instead, it has Scripts.txt listing scripts for all the characters, from which it's possible to generate the list of script codes. However, the order will not be compatible with that in ICU's common/unicode/uscript.h. The CLDR (Common Locale Data Repository) also has a file from which we can derive the list. Again, the order will be different. Apparently, ICU (in each successive version) adds new scripts to the end to keep old values remain compatible. I added comments to this effect to ChangeLog and UScriptCodes.h. BTW, I'm a bit worried about exposing ScriptFontMap (typedef) in WebCore namespace, which is not a big deal but I want to avoid. We can avoid that by making two static helper functions class static. However, the setter helper cannot be inline because it refers to a static helper in cpp file. Either we can leave them as in this patch or we can make them class static (but not inline).

I should not be marked as the reviewer. In general that just slows down reviews since others are more likely to ignore it.

Comment on attachment 38565 [details] patch addressing Eric's concerns style: 171 const AtomicString& Settings::cursiveFontFamily(UScriptCode script) const { I only looked briefly, will need to look again.

(In reply to comment #15) > (From update of attachment 38565 [details]) > style: > 171 const AtomicString& Settings::cursiveFontFamily(UScriptCode script) const > { Thanks for catching it. I've fixed it locally so that it'll be fixed when it's landed. > I only looked briefly, will need to look again. Any other issues? BTW, thank you for the advice on reviewer specification. I'll not specify a reviewer unless a patch warrants specifying one.

Comment on attachment 38565 [details] patch addressing Eric's concerns Style nit: const AtomicString& Settings::cursiveFontFamily(UScriptCode script) const { 172 return getGenericFontFamilyForScript(m_cursiveFontFamilyMap, script); 173 } It should be possible to test this kind of change with LayoutTests using calls like: layoutTestController.overridePreference("WebKitJavaScriptEnabled", false); Obviously since these are not yet exposed to any WebKit implementations it would not be possible to test, but I expect you've already written a Chromium implementation. You still probably don't want me set as the reviewer. Bugzilla gives no notification when you do that, so it's not at all like Google reviewee's. It functions more as a "don't anyone else review this until eric happens to notice that I've marked him" instead of a "review system, please email eric and tell him that i've asked him to review this". :)

(In reply to comment #17) > (From update of attachment 38565 [details]) > Style nit: > const AtomicString& Settings::cursiveFontFamily(UScriptCode script) const { > 172 return getGenericFontFamilyForScript(m_cursiveFontFamilyMap, script); > 173 } You already pointed this out earlier and I wrote that I had fixed it locally :-). If you want, I'll upload a new patch with this fixed. > > It should be possible to test this kind of change with LayoutTests using calls > like: layoutTestController.overridePreference("WebKitJavaScriptEnabled", > false); Ahah, that's finally in. Thank you for the information. > Obviously since these are not yet exposed to any WebKit implementations it > would not be possible to test, but I expect you've already written a Chromium > implementation. Actually, I haven't yet. With this in, I'm planning to start with revising my patch uploaded for bug 18085 to use now more expressive Settings class followed by the Chrome-side of changes (backend first and UI later). > You still probably don't want me set as the reviewer. Bugzilla gives no > notification when you do that, so it's not at all like Google reviewee's. It > functions more as a "don't anyone else review this until eric happens to notice > that I've marked him" instead of a "review system, please email eric and tell > him that i've asked him to review this". :) Thank you for the advice/info. Different projects have different conventions. In mozilla bugzilla, leaving the reviewer field blank gets you nowhere (nobody will take a look). I've been here long enough to know that the opposite is the case in webkit buzilla :-), but in this particular case, I put your name because I thought I'd get r+ quickly judging from the last sentence in your comment 12 :-)

Comment on attachment 38565 [details] patch addressing Eric's concerns As far as I can tell this is fine. This can't be landed by the bot due to the style problems.

Will land. In the future, please upload revised patches instead of fixing review comments locally where they can't be checked.

Committed r49837: <http://trac.webkit.org/changeset/49837>

Committed r49839: <http://trac.webkit.org/changeset/49839>

Comment on attachment 38565 [details] patch addressing Eric's concerns This broke the build: Ld /Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore normal i386 cd /Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebCore /Developer/usr/bin/g++-4.2 -arch i386 -dynamiclib -L/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebKitBuild/Debug -F/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebKitBuild/Debug -F/System/Library/Frameworks/Carbon.framework/Frameworks -F/System/Library/Frameworks/ApplicationServices.framework/Frameworks -filelist /Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/WebCore.LinkFileList -Wl,--no-demangle -exported_symbols_list /Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebKitBuild/Debug/DerivedSources/WebCore/WebCore.exp -install_name /Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore -mmacosx-version-min=10.5 -lWebCoreSQLite3 -lobjc -sub_library libobjc -umbrella WebKit -framework ApplicationServices -framework Carbon -framework Cocoa -framework JavaScriptCore -licucore -lobjc -lxml2 -framework OpenGL -framework QuartzCore -framework SystemConfiguration -Wl,-single_module -compatibility_version 1 -current_version 532.3 -o /Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore Undefined symbols: "__ZN7WebCore8Settings20setCursiveFontFamilyERKNS_12AtomicStringE", referenced from: -exported_symbols_list command line option "__ZN7WebCore8Settings18setFixedFontFamilyERKNS_12AtomicStringE", referenced from: -exported_symbols_list command line option "__ZN7WebCore8Settings22setSansSerifFontFamilyERKNS_12AtomicStringE", referenced from: -exported_symbols_list command line option "__ZN7WebCore8Settings18setSerifFontFamilyERKNS_12AtomicStringE", referenced from: -exported_symbols_list command line option "__ZN7WebCore8Settings20setFantasyFontFamilyERKNS_12AtomicStringE", referenced from: -exported_symbols_list command line option "__ZN7WebCore8Settings21setStandardFontFamilyERKNS_12AtomicStringE", referenced from: -exported_symbols_list command line option ld: symbol(s) not found collect2: ld returned 1 exit status ** BUILD FAILED **

Comment on attachment 38565 [details] patch addressing Eric's concerns > static inline void setGenericFontFamilyMap(ScriptFontFamilyMap& fontMap, const AtomicString& family, UScriptCode script, Page* page) > { > if (fontMap.set(static_cast<int>(script), family).second) > setNeedsReapplyStylesInAllFrames(page); I see that script is 0 (USCRIPT_COMMON) by default. Is it okay to use 0 as the key in a HashMap<int, AtomicString>? I thought 0 and -1 were special values (the empty value and the deleted value, respectively) for the default int key traits. How does this work?

Created attachment 49957 [details] patch with WebCore.base.exp (missing in the previous patch) Sorry that I haven't come back sooner. (it's rea....lly late). I thought I had uploaded a patch with WebCore.base.exp, but I didn't, which led to a broken tree. In addition, I addressed mitz's comment about -1 and 0 being special in int hashkey traits (by adding 2 to avoid -1 and 0).

Attachment 49957 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/text/UScriptCode.h:30: #ifndef header guard has wrong style, please use: UScriptCode_h [build/header_guard] [5] WebCore/page/Settings.h:80: Missing spaces around = [whitespace/operators] [4] WebCore/page/Settings.h:81: Missing spaces around = [whitespace/operators] [4] WebCore/page/Settings.h:83: Missing spaces around = [whitespace/operators] [4] WebCore/page/Settings.h:84: Missing spaces around = [whitespace/operators] [4] WebCore/page/Settings.h:86: Missing spaces around = [whitespace/operators] [4] WebCore/page/Settings.h:87: Missing spaces around = [whitespace/operators] [4] WebCore/page/Settings.h:89: Missing spaces around = [whitespace/operators] [4] WebCore/page/Settings.h:90: Missing spaces around = [whitespace/operators] [4] WebCore/page/Settings.h:92: Missing spaces around = [whitespace/operators] [4] WebCore/page/Settings.h:93: Missing spaces around = [whitespace/operators] [4] WebCore/page/Settings.h:95: Missing spaces around = [whitespace/operators] [4] WebCore/page/Settings.h:96: Missing spaces around = [whitespace/operators] [4] WebCore/page/Settings.cpp:180: Place brace on its own line for function definitions. [whitespace/braces] [4] WebCore/page/Settings.cpp:189: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 15 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

There are still style problems with this patch. :(

Comment on attachment 49957 [details] patch with WebCore.base.exp (missing in the previous patch) With the style bot these days there seems to be little reason to have patches up for review with style problems.

Created attachment 52130 [details] patch with style issues addressed

Created attachment 52186 [details] patch with style issues addressed (and duplicate part removed)

Comment on attachment 52186 [details] patch with style issues addressed (and duplicate part removed) asking for review. wondering if i have to ask for review to trigger bots (style, chromium, mac, etc) or bots just have too many patches to test. anyway, I ran locally the style check.

The bots only test patches marked r=?, that is correct.

Comment on attachment 52186 [details] patch with style issues addressed (and duplicate part removed) > +} UScriptCode; Our Unicode library goes into wtf/unicode. Things for non-ICU ports should go there. You should not stick this in platform/text.

Created attachment 52437 [details] a new patch with UScriptCode moved to JavascriptCore/wtf/unicode I couldn't find a public header (in Qt) for script names. I just moved UScriptCode.h to JavascriptCore/wtf/unicode, but this may not work because enum values are ICU-specific (the previous patch has the same problem). Now I begin to wonder if it's better to use string keys (of the standard 4-letter script codes like 'Latn', 'Hant', 'Hans', 'Deva', 'Japn', 'Arab' or lang+script like 'und_Latn', 'und_Deva', 'und_Arab', 'fa_Arab'

Created attachment 93741 [details] Revise Jungshik's patch.

Created attachment 93995 [details] patch with CMakefile build file added Thank you, Matt, for reviving this patch. I updated yours once more with several more scripts from the latest ISO 15024 spec. I also updated CMakefile.

Created attachment 94023 [details] update to the trunk xcode project file got updated leading to a patch failure.

Attachment 94023 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/WebCore/ChangeLog:23: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 94025 [details] got rid of a tab in ChangeLog (style error)

Comment on attachment 94025 [details] got rid of a tab in ChangeLog (style error) View in context: https://bugs.webkit.org/attachment.cgi?id=94025&action=review I think this is great. But I think we should use a special HashTraits instead of the +2 hack. Where are we going to add callers to these new methods? > Source/JavaScriptCore/wtf/unicode/UScriptCode.h:2 > + * Copyright (c) 1995-2011 International Business Machines Corporation and others If we're modifying this, we need to add our copyright, no? > Source/JavaScriptCore/wtf/unicode/UScriptCode.h:33 > +namespace WTF { Do we normally modify these headers? I didn't think we did. > Source/JavaScriptCore/wtf/unicode/UScriptCode.h:35 > +enum UScriptCode { I assume this is the right version of the enum to match the other headers? > Source/WebCore/page/Settings.cpp:59 > + // USCRIPT_INVALID_CODE is -1 and USRITP_COMMON is 0, which are special (deleted and empty) in HashMap with int keys. > + // To avoid using -1 and 0 as a key, we add 2. > + fontMap.set(static_cast<int>(script) + 2, family); This feels fragile. Why can't we use -2 and -3 as deleted and empty? We have full control over our Hash values with HashTraits. See HashTraits.h and StringTraits.h (I think that's what it's called) for examples, or search for HashTraits in the code base. > Source/WebCore/page/Settings.h:57 > + typedef HashMap<int, AtomicString> ScriptFontFamilyMap; Yeah, I think we just want a special HashTraits for WTF::UScriptCode.

Thank you for the review, Eric. I'll upload a new patch addressing your concern. Before that, I have a question about the following comment: >> Source/JavaScriptCore/wtf/unicode/UScriptCode.h:33 >> +namespace WTF { >Do we normally modify these headers? I didn't think we did. I'm not sure what you meant by the above. I was told that UScriptCode.h has to go under wtf/unicode instead of platform/text (comment #33). Could you clarify what you meant? Thank you again.

(In reply to comment #41) > Thank you for the review, Eric. I'll upload a new patch addressing your concern. Before that, I have a question about the following comment: > > >> Source/JavaScriptCore/wtf/unicode/UScriptCode.h:33 > >> +namespace WTF { > > >Do we normally modify these headers? I didn't think we did. > > I'm not sure what you meant by the above. I was told that UScriptCode.h has to go under wtf/unicode instead of platform/text (comment #33). Could you clarify what you meant? Thank you again. We have a copy of many of the ICU headers in WebCore/icu/unicode. I had confused that with wtf/unicode I assumed this is an ICU header which we're modifying to our needs?

Content of this file is part of uscript.h from ICU, which we already have in WebKit repository <http://trac.webkit.org/browser/trunk/Source/WebCore/icu/unicode/uscript.h>. This is not quite the right way to add the file to WTF. Any public symbols must be put into global namespace (so you need "using WTF::UScriptCode" at the end of the file) - but then the names would conflict with ICU. I think that you want to do this instead: 1. Copy uscript.h from WebCore/icu to JavaScriptCore/icu. 2. For Unicode platforms, just include it from wtf/unicode/Unicode.h. 3. Create wtf/unicode/ScriptCodesFromICU.h, and put the definitions there. 4. Include the new file from Unicode.h for non-ICU platforms.

(In reply to comment #43) > Content of this file is part of uscript.h from ICU, which we already have in WebKit repository <http://trac.webkit.org/browser/trunk/Source/WebCore/icu/unicode/uscript.h>. > > This is not quite the right way to add the file to WTF. Any public symbols must be put into global namespace (so you need "using WTF::UScriptCode" at the end of the file) - but then the names would conflict with ICU. > > I think that you want to do this instead: > 1. Copy uscript.h from WebCore/icu to JavaScriptCore/icu. > 2. For Unicode platforms, just include it from wtf/unicode/Unicode.h. > 3. Create wtf/unicode/ScriptCodesFromICU.h, and put the definitions there. > 4. Include the new file from Unicode.h for non-ICU platforms. Thanks a lot for detailed instruction and sorry for the late reply. I'm starting to do what you suggested.

Created attachment 98254 [details] patch updated per review comments The patch uses HashTraits for UScriptCode (-3 : deleted, -2: empty value) per Eric's comment. For uscript.h header, does what ap suggested.

Attachment 98254 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptGlue/icu/unicode/uscript.h:16: #ifndef header guard has wrong style, please use: WTF_uscript_h [build/header_guard] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:244: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:244: Missing space after , [whitespace/comma] [3] Source/JavaScriptGlue/icu/unicode/uscript.h:255: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:266: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:277: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/page/Settings.h:61: Should have a space between // and comment [whitespace/comments] [4] Source/JavaScriptCore/wtf/unicode/ScriptCodesFromICU.h:6: #ifndef header guard has wrong style, please use: WTF_ScriptCodesFromICU_h [build/header_guard] [5] Source/JavaScriptCore/icu/unicode/uscript.h:16: #ifndef header guard has wrong style, please use: WTF_uscript_h [build/header_guard] [5] Source/JavaScriptCore/icu/unicode/uscript.h:244: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:244: Missing space after , [whitespace/comma] [3] Source/JavaScriptCore/icu/unicode/uscript.h:255: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:266: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:277: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/wtf/unicode/Unicode.h:33: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/wtf/unicode/Unicode.h:36: "wtf/unicode/ScriptCodesFromICU.h" already included at Source/JavaScriptCore/wtf/unicode/Unicode.h:30 [build/include] [4] Source/JavaScriptCore/wtf/unicode/Unicode.h:39: "wtf/unicode/ScriptCodesFromICU.h" already included at Source/JavaScriptCore/wtf/unicode/Unicode.h:30 [build/include] [4] Source/JavaScriptCore/wtf/unicode/Unicode.h:42: "wtf/unicode/ScriptCodesFromICU.h" already included at Source/JavaScriptCore/wtf/unicode/Unicode.h:30 [build/include] [4] Total errors found: 18 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 98254 [details] patch updated per review comments View in context: https://bugs.webkit.org/attachment.cgi?id=98254&action=review Thank you, this looks mostly fine. > Source/JavaScriptCore/GNUmakefile.list.am:633 > + Source/JavaScriptCore/wtf/unicode/ScriptcodesFromICU.h \ Typo (capitalization of "codes"). >> Source/JavaScriptCore/wtf/unicode/ScriptCodesFromICU.h:6 >> +#ifndef ScriptCodesFromICU_H > > #ifndef header guard has wrong style, please use: WTF_ScriptCodesFromICU_h [build/header_guard] [5] Unlike most style bot warnings for this patch, this one would be good to address. > Source/JavaScriptCore/wtf/unicode/Unicode.h:30 > #include "qt4/UnicodeQt4.h" > +#include <wtf/unicode/ScriptCodesFromICU.h> I would suggest following the example of UnicodeMacrosFromICU.h and including the new file from platform specific headers (like UnicodeQt4.h). Not sure if it's actually better, but it's consistent. > Source/WebCore/ChangeLog:16 > + uscript.h has been updated to that of ICU 4.6 with two function > + declarations new to ICU 4.6 removed. I don't understand this. Can we just match the version of ICU in Mac OS X 10.5 (which is probably the oldest that we care about now)? Using a version of the file from 4.6 that doesn't quite match 4.6 will be confusing. Is there anything in the new version that isn't in the old, and that we actually need in WebKit? >> Source/WebCore/page/Settings.h:61 >> + //static const bool needsDestruction = false; > > Should have a space between // and comment [whitespace/comments] [4] Please don't land commented out code.

Thank you for the review. Do you happen to know what version of ICU is included in Mac OS 10.5? It's released on Oct 2007. So, it should be something like ICU 3.4, but I'm not sure. What happens if I used a copy from ICU newer than what's in Mac OS 10.5? The reason for using ICU 4.6's copy was that I wanted to use some newly added values for UScriptJapanese (added in ICU 3.8) and UScriptKorean (added in ICU 4.0) in a subsequent CL, but I think I can use Katakana/Hiragana and Hangul in their place.

(In reply to comment #48) > What happens if I used a copy from ICU newer than what's in Mac OS 10.5? The above question is assuming that there's no new function is declared in the header.

Mostly developer confusion, I think - if there is something that's not available on supported OS versions, people could use that later, and get burnt. I do not know if there will be any bigger problem. <http://opensource.apple.com/release/mac-os-x-105/> says that it's ICU 3.4 indeed. If you can use that without much trouble, it's probably easiest to just do that.

Created attachment 98269 [details] patch updated per ap's comments Can you take another look? uscript.h from ICU 3.6 (it turned out that Mac OS 10.5 has ICU 3.6) is included instead of one from ICU 4.6. And, style issues are fixed and ScriptCodesFromICU.h is included from individual platform files.

Attachment 98269 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptGlue/icu/unicode/uscript.h:15: #ifndef header guard has wrong style, please use: WTF_uscript_h [build/header_guard] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:187: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:187: Missing space after , [whitespace/comma] [3] Source/JavaScriptGlue/icu/unicode/uscript.h:198: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:209: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:220: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:15: #ifndef header guard has wrong style, please use: WTF_uscript_h [build/header_guard] [5] Source/JavaScriptCore/icu/unicode/uscript.h:187: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:187: Missing space after , [whitespace/comma] [3] Source/JavaScriptCore/icu/unicode/uscript.h:198: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:209: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:220: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 12 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

> (it turned out that Mac OS 10.5 has ICU 3.6) Are you saying that the plist from 10.5 is wrong? <http://opensource.apple.com/source/ICU/ICU-8.11/ICU.plist>

Looks like the plist was wrong indeed.

Comment on attachment 98269 [details] patch updated per ap's comments View in context: https://bugs.webkit.org/attachment.cgi?id=98269&action=review > Source/JavaScriptCore/GNUmakefile.list.am:633 > + Source/JavaScriptCore/wtf/unicode/ScriptcodesFromICU.h \ This still has the typo. > Source/JavaScriptCore/wtf/unicode/Unicode.h:31 > +#include <unicode/uscript.h> This should be in UnicodeIcu.h, similar to others.

Created attachment 98318 [details] final patch Thank you for review. Fixed two issues in the review comment. Carrying along r+.

Comment on attachment 98318 [details] final patch Rejecting attachment 98318 [details] from review queue. jshin@chromium.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.

Attachment 98318 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptGlue/icu/unicode/uscript.h:15: #ifndef header guard has wrong style, please use: WTF_uscript_h [build/header_guard] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:187: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:187: Missing space after , [whitespace/comma] [3] Source/JavaScriptGlue/icu/unicode/uscript.h:198: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:209: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:220: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:15: #ifndef header guard has wrong style, please use: WTF_uscript_h [build/header_guard] [5] Source/JavaScriptCore/icu/unicode/uscript.h:187: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:187: Missing space after , [whitespace/comma] [3] Source/JavaScriptCore/icu/unicode/uscript.h:198: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:209: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:220: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 12 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 98318 [details] final patch I'll land by hand (style errors come from uscript.h - ICU header).

> jshin@chromium.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. To land via commit queue, you can manually put reviewer name into all ChangeLogs, and only set cq+ flag (no review flag at all)

webkit-patch land-safely will do that automatically for you (if there is a reviewed patch on the bug). In this case, since the reviewed patch is already obsolete, land-safely would probably get confused, so I would just add the reviewer manually (but land-safely can upload it for you and mark it commit-queue+ if you like).

Created attachment 98366 [details] Patch for landing

Comment on attachment 98366 [details] Patch for landing Clearing flags on attachment: 98366 Committed r89594: <http://trac.webkit.org/changeset/89594>

All reviewed patches have been landed. Closing bug.

After this patch was landed, 4 XSL tests started to fail on Qt: http://build.webkit.org/results/Qt%20Linux%20Release/r89629%20(34562)/results.html Maybe just some test flakiness?

I cannot possibly think of any way this patch breaks XSL tests in Qt because this just lays down the foundation for follow-up patches and didn't actually change anything in terms of layout / rendering.

(In reply to comment #65) > After this patch was landed, 4 XSL tests started to fail on Qt: http://build.webkit.org/results/Qt%20Linux%20Release/r89629%20(34562)/results.html > > Maybe just some test flakiness? It isn't flakiness, because after this patch these tests fail continously. I tried to reproduce these fails, but I can't when I run only one test. It must be a sideeffect bug caused or revealed by this patch. I tried to make tests green with adding tests to the Skipped list, but it didn't help, because other tests started to fail. :-/ I haven't better idea than rolling out until we find what caused this bug, sorry. ( http://trac.webkit.org/changeset/89655 ) I cc-ed Zoltan, our expert in crazy bug debugging. Zoltan, any idea? :)

I managed to reproduce a fail: $ Tools/Scripts/run-webkit-tests fast/xsl/xslt-bad-import-uri.html fast/xsl/xslt-doc-noenc.xml fast/xsl/xslt-doc-noenc.xml -> failed

one more test: $ Tools/Scripts/run-webkit-tests fast/xsl/xslt-mismatched-tags-in-xslt.xml fast/xsl/xslt-recursion.xml fast/xsl/xslt-recursion.xml -> failed

(In reply to comment #69) > one more test: > > $ Tools/Scripts/run-webkit-tests fast/xsl/xslt-mismatched-tags-in-xslt.xml fast/xsl/xslt-recursion.xml > > fast/xsl/xslt-recursion.xml -> failed This is strange. xslt-mismatched-tags-in-xslt.xml executes the following xml: <?xml version="1.0" encoding="UTF-8"?> <xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" version="1.0"> <xsl:output method="html" encoding="UTF-8"/> <xsl:template match="TEST"> <html xmlns="http://www.w3.org/1999/xhtml"> <body> <p>You should see error text above this</p> <p> </body> </html> </xsl:template> </xsl:stylesheet> This obviously contains a parse error, and the report is correct. However, when the next test runs (fast/xsl/xslt-recursion.xml) this xml is kept somewhere, and passed to the Qt specific XSLTProcessor again, which starts reporting extra parse errors.

This is the connection between the patch above, and setting this invalid xml: processLine calls resetToConsistentStateBeforeTesting, which calls setGenericFontFamilyMap (from this patch), and that applies an XSL transform. Perhaps to the previous test case ... ? #0 WebCore::XSLTProcessor::setXSLStyleSheet (this=0x6edaf0, styleSheet=...) at ../../../Source/WebCore/xml/XSLTProcessor.h:49 #1 0x00007ffff5ab08d6 in WebCore::Document::applyXSLTransform (this=0x6fd340, pi=0x707d40) at ../../../Source/WebCore/dom/Document.cpp:3987 #2 0x00007ffff5aab21f in WebCore::Document::recalcStyleSelector (this=0x6fd340) at ../../../Source/WebCore/dom/Document.cpp:2931 #3 0x00007ffff5aaab83 in WebCore::Document::styleSelectorChanged (this=0x6fd340, updateFlag=WebCore::DeferRecalcStyle) at ../../../Source/WebCore/dom/Document.cpp:2819 #4 0x00007ffff5eaa419 in WebCore::Page::setNeedsRecalcStyleInAllFrames (this=0x6862f0) at ../../../Source/WebCore/page/Page.cpp:400 #5 0x00007ffff5eceb57 in setGenericFontFamilyMap (fontMap=..., family=..., script=USCRIPT_COMMON, page=0x6862f0) at ../../../Source/WebCore/page/Settings.cpp:58 #6 0x00007ffff5ecfe77 in WebCore::Settings::setStandardFontFamily (this=0x688ae0, family=..., script=USCRIPT_COMMON) at ../../../Source/WebCore/page/Settings.cpp:219 #7 0x00007ffff589e7af in QWebSettingsPrivate::apply (this=0x6af1f0) at ../../../../Source/WebKit/qt/Api/qwebsettings.cpp:126 #8 0x00007ffff58a1c63 in QWebSettings::setAttribute (this=0x6a4910, attr=QWebSettings::ZoomTextOnly, on=true) at ../../../../Source/WebKit/qt/Api/qwebsettings.cpp:884 #9 0x00007ffff58691a3 in QWebFrame::setTextSizeMultiplier (this=0x6b9bd0, factor=1) at ../../../../Source/WebKit/qt/Api/qwebframe.cpp:1318 #10 0x0000000000418da7 in WebCore::DumpRenderTree::resetToConsistentStateBeforeTesting ( this=0x7fffffffe230, url=...) at ../../../../Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:530 #11 0x00000000004194d9 in WebCore::DumpRenderTree::open (this=0x7fffffffe230, url=...) at ../../../../Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:595 #12 0x000000000041a812 in WebCore::DumpRenderTree::processLine (this=0x7fffffffe230, input=...) Any idea is welcome!

Let's see how to hide bugs in WebKit way: Original code: void Settings::setStandardFontFamily(const AtomicString& standardFontFamily) { // Notice this clever, tricky "if" here. It does nothing if // the font does not changed! So absolutely no recalc business: if (standardFontFamily == m_standardFontFamily) return; m_standardFontFamily = standardFontFamily; m_page->setNeedsRecalcStyleInAllFrames(); } New code: static inline void setGenericFontFamilyMap(ScriptFontFamilyMap& fontMap, const AtomicString& family, UScriptCode script, Page* page) { fontMap.set(static_cast<int>(script), family); page->setNeedsRecalcStyleInAllFrames(); } Aha! No "if" here! Let's add it again to the code: static inline void setGenericFontFamilyMap(ScriptFontFamilyMap& fontMap, const AtomicString& family, UScriptCo { + ScriptFontFamilyMap::iterator it = fontMap.find(static_cast<int>(script)); + if (it != fontMap.end() && it->second == family) + return; fontMap.set(static_cast<int>(script), family); page->setNeedsRecalcStyleInAllFrames(); } Wow! No regressions! No bugs, isn't it? ]:>

D'oh, that's my fault. Jungshik's original patch had a check: + if (fontMap.set(static_cast<int>(script) + 2, family).second) + setNeedsReapplyStylesInAllFrames(page); I must have removed it when getting it to work locally and forgot to put it back in. Thanks for debugging it :-)

(In reply to comment #73) > D'oh, that's my fault. Jungshik's original patch had a check: > > + if (fontMap.set(static_cast<int>(script) + 2, family).second) > + setNeedsReapplyStylesInAllFrames(page); > > I must have removed it when getting it to work locally and forgot to put it back in. > > Thanks for debugging it :-) Well, it's my fault, too because I should have caught it. I'm uploading a new patch.

Created attachment 99379 [details] updated patch to fix Qt layouttest failure This one uses what's suggested instead of what I originally had 2 yrs ago. This is longer than the original, but it's not clear what 'HashMap::set' returns when a key exists and a value added is identical to the current value for that key. If it returns false in that case (and also does the checking done suggested by comment 27), I'd rather use the original as below: + if (fontMap.set(static_cast<int>(script), family).second) + page->setNeedsRecalcStyleInAllFrames();

(In reply to comment #75) original, but it's not clear what 'HashMap::set' returns when a key exists and a value added is identical to the current value for that key. If it returns false in that case (and also does the checking done suggested by comment 27), I'd rather use the original as below: > > > + if (fontMap.set(static_cast<int>(script), family).second) > + page->setNeedsRecalcStyleInAllFrames(); Oops. I meant comment 72 .The current patch has that: ScriptFontFamilyMap::iterator it = fontMap.find(static_cast<int>(script)); if (it != fontMap.end() && it->second == family) return; fontMap.set(static_cast<int>(script), family); page->setNeedsRecalcStyleInAllFrames();

Attachment 99379 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptGlue/icu/unicode/uscript.h:15: #ifndef header guard has wrong style, please use: WTF_uscript_h [build/header_guard] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:187: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:187: Missing space after , [whitespace/comma] [3] Source/JavaScriptGlue/icu/unicode/uscript.h:198: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:209: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptGlue/icu/unicode/uscript.h:220: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:15: #ifndef header guard has wrong style, please use: WTF_uscript_h [build/header_guard] [5] Source/JavaScriptCore/icu/unicode/uscript.h:187: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:187: Missing space after , [whitespace/comma] [3] Source/JavaScriptCore/icu/unicode/uscript.h:198: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:209: The parameter name "scriptCode" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/icu/unicode/uscript.h:220: The parameter name "err" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 12 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

I think we should eventually fix the XML parsing behaviour on Qt (I saw some discussion about it in general on webkit-dev). Since this change helps avoiding layout errors on the bots (and probably a good speed optimization as well), I think it is ok for now, just we should not forget about it. Simon (or other Qt experts), shall I open a new bug entry for this issue?

No doubt that we should call setNeedsRecalcStyleInAllFrames only when necessary regardless of Qt. My question was which is better way to do that (if one is better than the other). If two snippets in comment #76 are equivalent, I'd rather use a more succinct one.

Comment on attachment 99379 [details] updated patch to fix Qt layouttest failure Clearing flags on attachment: 99379 Committed r90259: <http://trac.webkit.org/changeset/90259>

All reviewed patches have been landed. Closing bug.