Bug 58157 - Need more efficient ways to set inline CSS styles
Summary: Need more efficient ways to set inline CSS styles
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on: 53020
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-08 13:17 PDT by Dimitri Glazkov (Google)
Modified: 2011-10-11 09:48 PDT (History)
3 users (show)

See Also:


Attachments
patch for discussion (8.50 KB, patch)
2011-10-04 06:51 PDT, Arun Patole
no flags Details | Formatted Diff | Diff
updated patch (10.33 KB, patch)
2011-10-10 22:05 PDT, Arun Patole
no flags Details | Formatted Diff | Diff
updated patch (10.33 KB, patch)
2011-10-10 22:09 PDT, Arun Patole
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 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.
Comment 1 Dimitri Glazkov (Google) 2011-04-08 15:29:04 PDT
Also, "show()" actually means removal of display:none style, which makes it confusingly named.
Comment 2 Arun Patole 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.
Comment 3 Arun Patole 2011-10-04 06:51:42 PDT
Created attachment 109616 [details]
patch for discussion
Comment 4 Arun Patole 2011-10-10 21:45:15 PDT
Adding Eric in cc...
Comment 5 Arun Patole 2011-10-10 22:05:10 PDT
Created attachment 110471 [details]
updated patch
Comment 6 WebKit Review Bot 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.
Comment 7 Arun Patole 2011-10-10 22:09:32 PDT
Created attachment 110473 [details]
updated patch
Comment 8 Eric Carlson 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!
Comment 9 Dimitri Glazkov (Google) 2011-10-11 08:43:46 PDT
Comment on attachment 110473 [details]
updated patch

awesome.
Comment 10 Arun Patole 2011-10-11 08:48:45 PDT
Thanks Eric and Dimitri for reviewing the patch!
Comment 11 WebKit Review Bot 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>
Comment 12 WebKit Review Bot 2011-10-11 09:48:03 PDT
All reviewed patches have been landed.  Closing bug.