Bug 78604 - Share font-family CSS values through CSSValuePool.
Summary: Share font-family CSS values through CSSValuePool.
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
Depends on:
Blocks: 78070
  Show dependency treegraph
Reported: 2012-02-14 07:08 PST by Andreas Kling
Modified: 2012-02-16 03:22 PST (History)
5 users (show)

See Also:

Patch (4.67 KB, patch)
2012-02-14 07:13 PST, Andreas Kling
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v2 (8.75 KB, patch)
2012-02-15 00:33 PST, Andreas Kling
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2012-02-14 07:08:36 PST
Comment 1 Andreas Kling 2012-02-14 07:13:39 PST
Created attachment 126975 [details]
Comment 2 WebKit Review Bot 2012-02-14 07:16:59 PST
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.
Comment 3 WebKit Review Bot 2012-02-14 07:59:08 PST
Comment on attachment 126975 [details]

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

New failing tests:
Comment 4 Andreas Kling 2012-02-15 00:33:54 PST
Created attachment 127123 [details]
Patch v2

Okay, it wasn't that simple, let's try again.
Comment 5 Alexis Menard (darktears) 2012-02-15 04:13:54 PST
Comment on attachment 127123 [details]
Patch v2

Looks good to me.
Comment 6 Darin Adler 2012-02-15 09:46:59 PST
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.
Comment 7 Andreas Kling 2012-02-16 03:05:37 PST
Committed r107919: <http://trac.webkit.org/changeset/107919>
Comment 8 Andreas Kling 2012-02-16 03:22:22 PST
(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.