Bug 58157

Summary: Need more efficient ways to set inline CSS styles
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: CSSAssignee: Dimitri Glazkov (Google) <dglazkov>
Status: RESOLVED FIXED    
Severity: Normal CC: arun.patole, eric.carlson, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 53020    
Bug Blocks:    
Attachments:
Description Flags
patch for discussion
none
updated patch
none
updated patch none

Dimitri Glazkov (Google)
Reported 2011-04-08 13:17:18 PDT
After media controls were refactored to use new shadow DOM, they use inline styles to show, hide, and animate. The inline styles are set using strings, which is not super-efficient, since we have to go through CSS parser to do this. Event though it's as good as JavaScript, we can go faster. And we should.
Attachments
patch for discussion (8.50 KB, patch)
2011-10-04 06:51 PDT, Arun Patole 		no flags
DetailsFormatted DiffDiff
updated patch (10.33 KB, patch)
2011-10-10 22:05 PDT, Arun Patole 		no flags
DetailsFormatted DiffDiff
updated patch (10.33 KB, patch)
2011-10-10 22:09 PDT, Arun Patole 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Dimitri Glazkov (Google)
Comment 1 2011-04-08 15:29:04 PDT
Also, "show()" actually means removal of display:none style, which makes it confusingly named.
Arun Patole
Comment 2 2011-10-04 06:51:02 PDT
Hi, I have tried to optimized media controls code related to setting inline CSS styles, attaching raw patch for discussion. Below are the changes: -Used CSSPropertyNames, CSSValues and CSSPrimitiveValues instead of Strings. -Removed displayString(), webkitTransitionString(), opacityString() functions as they will no longer be needed after this change. -Moved makeOpaque and makeTransparent functionality to MediaControlPanelElement from MediaControlRootElement. This is making it look better and also avoiding styles related code duplication in MediaControlRootElementChromium. If the patch looks good in general, I will work on making all media layout tests work with this change and submit the updated patch for review/commit.
Arun Patole
Comment 3 2011-10-04 06:51:42 PDT
Created attachment 109616 [details] patch for discussion
Arun Patole
Comment 4 2011-10-10 21:45:15 PDT
Adding Eric in cc...
Arun Patole
Comment 5 2011-10-10 22:05:10 PDT
Created attachment 110471 [details] updated patch
WebKit Review Bot
Comment 6 2011-10-10 22:06:34 PDT
Attachment 110471 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Arun Patole
Comment 7 2011-10-10 22:09:32 PDT
Created attachment 110473 [details] updated patch
Eric Carlson
Comment 8 2011-10-11 07:54:06 PDT
I didn't mark this r+ so dglazkov has a chance to check, since the original is his code, but this looks fine to me. Nice optimizations Arun!
Dimitri Glazkov (Google)
Comment 9 2011-10-11 08:43:46 PDT
Comment on attachment 110473 [details] updated patch awesome.
Arun Patole
Comment 10 2011-10-11 08:48:45 PDT
Thanks Eric and Dimitri for reviewing the patch!
WebKit Review Bot
Comment 11 2011-10-11 09:47:58 PDT
Comment on attachment 110473 [details] updated patch Clearing flags on attachment: 110473 Committed r97157: <http://trac.webkit.org/changeset/97157>
WebKit Review Bot
Comment 12 2011-10-11 09:48:03 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.