Summary: | Share font-family CSS values through CSSValuePool. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||
Component: | CSS | Assignee: | Andreas Kling <kling> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | dglazkov, koivisto, macpherson, menard, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 78070 | ||||||||
Attachments: |
|
Description
Andreas Kling
2012-02-14 07:08:36 PST
Created attachment 126975 [details]
Patch
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 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 Created attachment 127123 [details]
Patch v2
Okay, it wasn't that simple, let's try again.
Comment on attachment 127123 [details]
Patch v2
Looks good to me.
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. Committed r107919: <http://trac.webkit.org/changeset/107919> (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. |