Bug 78604 – Share font-family CSS values through CSSValuePool.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Andreas Kling]

SSIA

[Andreas Kling]

Created attachment 126975 [details]
Patch

[WebKit Review Bot]

Attachment 126975 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 126975 [details]
Patch

Attachment 126975 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11514341

New failing tests:
fast/text/backslash-to-yen-sign-dynamic.html
editing/selection/find-yensign-and-backslash-with-japanese-fonts.html
fast/text/backslash-to-yen-sign-euc.html
tables/mozilla/bugs/bug126742.html
fast/text/backslash-to-yen-sign.html

[Andreas Kling]

Created attachment 127123 [details]
Patch v2

Okay, it wasn't that simple, let's try again.

[Alexis Menard (darktears)]

Comment on attachment 127123 [details]
Patch v2

Looks good to me.

[Darin Adler]

Comment on attachment 127123 [details]
Patch v2

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

> Source/WebCore/css/CSSParser.cpp:4284
> +    { }

Normal style is to put these on successive lines instead of on one line.

> Source/WebCore/css/CSSParser.cpp:4313
> +    FontFamilyValueBuilder familyBuilder(list.get(), cssValuePool());
> +    bool inFamily = false;

It seems that it would be easy to leave out one call to commit in the code that follows. I the logic could be slightly clearer if more of the work was done through builder object and it had a bit more state.

> Source/WebCore/css/CSSValuePool.h:77
> +    typedef HashMap<String, RefPtr<FontFamilyValue> > FontFamilyValueCache;

It might be better to use AtomicString for this. We could possibly save some allocation by going directly from a parser string or a string builder to an existing AtomicString for an existing family name rather than making a temporary String that often ends up being used only for map lookup. Alternatively, we could work on enhancements to HashMap<String, xxx> that could allow us to look up in the map without allocating a new string. My guess is that this isn’t so performance-critical that either of those matters, but it is something I noticed while reviewing.

[Andreas Kling]

Committed r107919: <http://trac.webkit.org/changeset/107919>

[Andreas Kling]

(In reply to comment #7)
> Committed r107919: <http://trac.webkit.org/changeset/107919>

Landed this as-is to move things on. Will return to parseFont() momentarily.