Bug 101860 - RenderStyle: Pack Color members tighter in substructures.
Summary: RenderStyle: Pack Color members tighter in substructures.
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on: 101937
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-11 05:05 PST by Andreas Kling
Modified: 2012-11-12 07:56 PST (History)
3 users (show)

See Also:


Attachments
EWS experiment (36.15 KB, patch)
2012-11-11 05:06 PST, Andreas Kling
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
EWS experiment 2 (36.29 KB, patch)
2012-11-11 10:54 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (40.70 KB, patch)
2012-11-11 12:30 PST, Andreas Kling
koivisto: 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-11-11 05:05:55 PST
WebCore::Color is currently quite wasteful with its { RGBA32 rgba; bool isValid; } layout.
For colors that can never be invalid, we should just store an RGBA32. For others, we can pack the validity flags with other bitfield members.
Comment 1 Andreas Kling 2012-11-11 05:06:24 PST
Created attachment 173497 [details]
EWS experiment
Comment 2 WebKit Review Bot 2012-11-11 05:08:03 PST
Attachment 173497 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/rendering/style/RenderStyle..." exit_code: 1
Source/WebCore/rendering/style/RenderStyle.cpp:977:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Early Warning System Bot 2012-11-11 05:12:01 PST
Comment on attachment 173497 [details]
EWS experiment

Attachment 173497 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14792714
Comment 4 Early Warning System Bot 2012-11-11 05:13:27 PST
Comment on attachment 173497 [details]
EWS experiment

Attachment 173497 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14813025
Comment 5 EFL EWS Bot 2012-11-11 05:18:37 PST
Comment on attachment 173497 [details]
EWS experiment

Attachment 173497 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14809114
Comment 6 WebKit Review Bot 2012-11-11 05:32:03 PST
Comment on attachment 173497 [details]
EWS experiment

Attachment 173497 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14794644
Comment 7 Peter Beverloo (cr-android ews) 2012-11-11 05:41:20 PST
Comment on attachment 173497 [details]
EWS experiment

Attachment 173497 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14794646
Comment 8 Andreas Kling 2012-11-11 10:54:10 PST
Created attachment 173510 [details]
EWS experiment 2
Comment 9 Andreas Kling 2012-11-11 12:30:05 PST
Created attachment 173515 [details]
Patch
Comment 10 Antti Koivisto 2012-11-12 02:12:20 PST
Comment on attachment 173515 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyle.cpp:978
>  Color RenderStyle::color() const { return inherited->color; }
>  Color RenderStyle::visitedLinkColor() const { return inherited->visitedLinkColor; }
> -void RenderStyle::setColor(const Color& v) { SET_VAR(inherited, color, v) };
> -void RenderStyle::setVisitedLinkColor(const Color& v) { SET_VAR(inherited, visitedLinkColor, v) }
> +void RenderStyle::setColor(const Color& v) { SET_VAR(inherited, color, v.rgb()); }
> +void RenderStyle::setVisitedLinkColor(const Color& v) { SET_VAR(inherited, visitedLinkColor, v.rgb()); }

Should these just take/return RGB32s? It would be good if the interface would make it clear what is being saved. 

At least there should be ASSERT(c.isValid()).
Comment 11 Andreas Kling 2012-11-12 06:45:27 PST
Committed r134224: <http://trac.webkit.org/changeset/134224>
Comment 12 WebKit Review Bot 2012-11-12 07:56:37 PST
Re-opened since this is blocked by bug 101937