Bug 20797 - add Settings entries for per-script/lang generic family
Summary: add Settings entries for per-script/lang generic family
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jungshik Shin
URL:
Keywords:
Depends on: 63316
Blocks: 18085 24574 10874
  Show dependency treegraph
 
Reported: 2008-09-12 10:44 PDT by Jungshik Shin
Modified: 2011-07-01 12:23 PDT (History)
15 users (show)

See Also:


Attachments
tentative patch for ports with ICU (9.60 KB, patch)
2009-03-09 23:24 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff
patch that should build for ports not using ICU (14.62 KB, patch)
2009-03-13 14:32 PDT, Jungshik Shin
mjs: review-
Details | Formatted Diff | Diff
patch updated (macros expanded) (21.20 KB, patch)
2009-07-21 11:47 PDT, Jungshik Shin
eric: review-
Details | Formatted Diff | Diff
patch addressing Eric's concerns (19.64 KB, patch)
2009-08-25 13:32 PDT, Jungshik Shin
abarth: review-
eric: commit-queue-
Details | Formatted Diff | Diff
patch with WebCore.base.exp (missing in the previous patch) (32.58 KB, patch)
2010-03-03 15:40 PST, Jungshik Shin
eric: review-
Details | Formatted Diff | Diff
patch with style issues addressed (32.56 KB, patch)
2010-03-30 23:13 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff
patch with style issues addressed (and duplicate part removed) (22.49 KB, patch)
2010-03-31 10:36 PDT, Jungshik Shin
darin: review-
Details | Formatted Diff | Diff
a new patch with UScriptCode moved to JavascriptCore/wtf/unicode (28.46 KB, patch)
2010-04-02 12:42 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff
Revise Jungshik's patch. (30.28 KB, patch)
2011-05-16 23:29 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
patch with CMakefile build file added (30.76 KB, patch)
2011-05-18 15:50 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff
update to the trunk (30.76 KB, patch)
2011-05-18 18:31 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff
got rid of a tab in ChangeLog (style error) (30.76 KB, patch)
2011-05-18 18:37 PDT, Jungshik Shin
eric: review-
Details | Formatted Diff | Diff
patch updated per review comments (65.53 KB, patch)
2011-06-22 15:41 PDT, Jungshik Shin
ap: review-
Details | Formatted Diff | Diff
patch updated per ap's comments (53.12 KB, patch)
2011-06-22 17:23 PDT, Jungshik Shin
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff
final patch (53.08 KB, patch)
2011-06-23 00:42 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff
Patch for landing (53.09 KB, patch)
2011-06-23 10:26 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff
updated patch to fix Qt layouttest failure (53.19 KB, patch)
2011-06-30 15:35 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jungshik Shin 2008-09-12 10:44:25 PDT
Spun off of bug 13139 where David wrote:

> Using the parent is just a heuristic and a poor one at that.  We choose "serif"
> on the assumption that a majority of the fonts specified on the Web are
> "serif", and so that maximizes the probability that we will be right.

Would you consider adding a preference to control this (and the corresponding UI to Safari)?  Firefox has a preference and UI as to which to use, serif or sans-serif. In Firefox, it's per langGroup/scripts/languages (preferences are font.default.{langGroup} whose value can be either serif or sans-serif. For CJK, it's 'sans-serif' by default while for all others, it's serif by default). 

 In Safari (which just has 'across-the-languages' font selection UI as of now), it'd be global as well. Anyway, CJK users usually prefer 'sans-serif' to 'serif' (on screen) and some user control on this would be nice to have.
Comment 1 Jungshik Shin 2009-03-09 11:16:17 PDT
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. 
Comment 2 Jungshik Shin 2009-03-09 23:24:33 PDT
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.
Comment 3 Jungshik Shin 2009-03-10 14:08:16 PDT
This also (indirectly) blocks bug 18085. 
Comment 4 Jungshik Shin 2009-03-13 14:32:15 PDT
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 5 Eric Seidel (no email) 2009-03-13 14:55:38 PDT
Comment on attachment 28601 [details]
patch that should build for ports not using ICU

I am not the right reviewer for this.
Comment 6 Jungshik Shin 2009-03-13 15:50:16 PDT
Comment on attachment 28601 [details]
patch that should build for ports not using ICU

Giving this to mitz per hyatt on IRC.
Comment 7 Jungshik Shin 2009-03-26 14:49:23 PDT
Comment on attachment 28601 [details]
patch that should build for ports not using ICU

mitz, can you take a look?
Comment 8 Maciej Stachowiak 2009-05-21 23:59:58 PDT
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)
>  {
Comment 9 Jungshik Shin 2009-07-21 11:47:02 PDT
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 10 Jungshik Shin 2009-07-21 11:49:57 PDT
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 11 Jungshik Shin 2009-07-31 11:39:31 PDT
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 12 Eric Seidel (no email) 2009-08-07 09:15:15 PDT
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!
Comment 13 Jungshik Shin 2009-08-25 13:32:18 PDT
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).
Comment 14 Eric Seidel (no email) 2009-08-27 16:28:09 PDT
I should not be marked as the reviewer.  In general that just slows down reviews since others are more likely to ignore it.
Comment 15 Eric Seidel (no email) 2009-08-27 16:29:56 PDT
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.
Comment 16 Jungshik Shin 2009-09-02 12:01:21 PDT
(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 17 Eric Seidel (no email) 2009-09-03 00:27:15 PDT
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". :)
Comment 18 Jungshik Shin 2009-09-04 16:58:49 PDT
(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 19 Eric Seidel (no email) 2009-10-06 08:48:03 PDT
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.
Comment 20 Adam Barth 2009-10-19 21:36:41 PDT
Will land.

In the future, please upload revised patches instead of fixing review comments locally where they can't be checked.
Comment 21 Adam Barth 2009-10-19 21:37:34 PDT
Committed r49837: <http://trac.webkit.org/changeset/49837>
Comment 22 Adam Barth 2009-10-19 21:46:24 PDT
Committed r49839: <http://trac.webkit.org/changeset/49839>
Comment 23 Adam Barth 2009-10-19 21:47:44 PDT
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 24 mitz 2009-10-19 22:04:52 PDT
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?
Comment 25 Jungshik Shin 2010-03-03 15:40:36 PST
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).
Comment 26 WebKit Review Bot 2010-03-03 15:44:42 PST
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.
Comment 27 Eric Seidel (no email) 2010-03-04 13:15:54 PST
There are still style problems with this patch. :(
Comment 28 Eric Seidel (no email) 2010-03-15 12:59:49 PDT
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.
Comment 29 Jungshik Shin 2010-03-30 23:13:11 PDT
Created attachment 52130 [details]
patch with style issues addressed
Comment 30 Jungshik Shin 2010-03-31 10:36:26 PDT
Created attachment 52186 [details]
patch with style issues addressed (and duplicate part removed)
Comment 31 Jungshik Shin 2010-03-31 11:53:32 PDT
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.
Comment 32 Eric Seidel (no email) 2010-03-31 11:54:38 PDT
The bots only test patches marked r=?, that is correct.
Comment 33 Darin Adler 2010-03-31 17:14:28 PDT
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.
Comment 34 Jungshik Shin 2010-04-02 12:42:44 PDT
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'
Comment 35 Matt Falkenhagen 2011-05-16 23:29:09 PDT
Created attachment 93741 [details]
Revise Jungshik's patch.
Comment 36 Jungshik Shin 2011-05-18 15:50:59 PDT
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.
Comment 37 Jungshik Shin 2011-05-18 18:31:54 PDT
Created attachment 94023 [details]
update to the trunk 

xcode project file got updated leading to a patch failure.
Comment 38 WebKit Review Bot 2011-05-18 18:34:03 PDT
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.
Comment 39 Jungshik Shin 2011-05-18 18:37:18 PDT
Created attachment 94025 [details]
got rid of a tab in ChangeLog (style error)
Comment 40 Eric Seidel (no email) 2011-05-26 14:22:20 PDT
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.
Comment 41 Jungshik Shin 2011-05-31 02:13:41 PDT
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.
Comment 42 Eric Seidel (no email) 2011-05-31 08:14:41 PDT
(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?
Comment 43 Alexey Proskuryakov 2011-05-31 09:23:47 PDT
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.
Comment 44 Jungshik Shin 2011-06-15 14:28:51 PDT
(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.
Comment 45 Jungshik Shin 2011-06-22 15:41:25 PDT
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.
Comment 46 WebKit Review Bot 2011-06-22 15:45:08 PDT
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 47 Alexey Proskuryakov 2011-06-22 16:29:03 PDT
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.
Comment 48 Jungshik Shin 2011-06-22 16:41:50 PDT
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.
Comment 49 Jungshik Shin 2011-06-22 16:46:49 PDT
(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.
Comment 50 Alexey Proskuryakov 2011-06-22 16:58:43 PDT
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.
Comment 51 Jungshik Shin 2011-06-22 17:23:58 PDT
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.
Comment 52 WebKit Review Bot 2011-06-22 17:28:11 PDT
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.
Comment 53 Alexey Proskuryakov 2011-06-22 17:30:27 PDT
> (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>
Comment 54 Alexey Proskuryakov 2011-06-22 17:34:02 PDT
Looks like the plist was wrong indeed.
Comment 55 Alexey Proskuryakov 2011-06-22 17:38:06 PDT
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.
Comment 56 Jungshik Shin 2011-06-23 00:42:56 PDT
Created attachment 98318 [details]
final patch 

Thank you for review. 

Fixed two issues in the review comment. Carrying along r+.
Comment 57 WebKit Review Bot 2011-06-23 00:43:14 PDT
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.
Comment 58 WebKit Review Bot 2011-06-23 00:44:59 PDT
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 59 Jungshik Shin 2011-06-23 01:05:34 PDT
Comment on attachment 98318 [details]
final patch 

I'll land by hand (style errors come from uscript.h - ICU header).
Comment 60 Alexey Proskuryakov 2011-06-23 01:12:02 PDT
> 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)
Comment 61 Eric Seidel (no email) 2011-06-23 01:13:24 PDT
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).
Comment 62 Jungshik Shin 2011-06-23 10:26:45 PDT
Created attachment 98366 [details]
Patch for landing
Comment 63 WebKit Review Bot 2011-06-23 11:23:46 PDT
Comment on attachment 98366 [details]
Patch for landing

Clearing flags on attachment: 98366

Committed r89594: <http://trac.webkit.org/changeset/89594>
Comment 64 WebKit Review Bot 2011-06-23 11:23:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 65 Ryosuke Niwa 2011-06-23 16:28:05 PDT
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?
Comment 66 Jungshik Shin 2011-06-23 18:07:55 PDT
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.
Comment 67 Csaba Osztrogonác 2011-06-24 00:50:20 PDT
(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? :)
Comment 68 Csaba Osztrogonác 2011-06-24 06:02:51 PDT
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
Comment 69 Csaba Osztrogonác 2011-06-24 06:04:25 PDT
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
Comment 70 Zoltan Herczeg 2011-06-27 02:16:41 PDT
(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.
Comment 71 Zoltan Herczeg 2011-06-27 02:27:07 PDT
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!
Comment 72 Zoltan Herczeg 2011-06-27 02:52:00 PDT
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? ]:>
Comment 73 Matt Falkenhagen 2011-06-27 03:56:38 PDT
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 :-)
Comment 74 Jungshik Shin 2011-06-30 11:28:13 PDT
(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.
Comment 75 Jungshik Shin 2011-06-30 15:35:51 PDT
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();
Comment 76 Jungshik Shin 2011-06-30 15:45:58 PDT
(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();
Comment 77 WebKit Review Bot 2011-06-30 15:49:36 PDT
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.
Comment 78 Zoltan Herczeg 2011-06-30 16:02:02 PDT
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?
Comment 79 Jungshik Shin 2011-06-30 17:22:36 PDT
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 80 WebKit Review Bot 2011-07-01 12:23:35 PDT
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>
Comment 81 WebKit Review Bot 2011-07-01 12:23:46 PDT
All reviewed patches have been landed.  Closing bug.