Bug 154100 - [ThreadedCompositor] position:fixed elements do not have their own layers when threaded compositor is enabled.
Summary: [ThreadedCompositor] position:fixed elements do not have their own layers whe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: ChangSeok Oh
URL:
Keywords:
Depends on:
Blocks: 154066
  Show dependency treegraph
 
Reported: 2016-02-10 21:38 PST by ChangSeok Oh
Modified: 2016-02-15 21:40 PST (History)
6 users (show)

See Also:


Attachments
a test case (237 bytes, text/html)
2016-02-10 21:38 PST, ChangSeok Oh
no flags Details
Patch (2.51 KB, patch)
2016-02-12 01:22 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (2.23 KB, patch)
2016-02-12 02:00 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ChangSeok Oh 2016-02-10 21:38:24 PST
Created attachment 271043 [details]
a test case

SSIA.
Run the attached test case on threaded compositor enabled webkit build.
Comment 1 ChangSeok Oh 2016-02-12 01:22:48 PST
Created attachment 271150 [details]
Patch
Comment 2 Gwang Yoon Hwang 2016-02-12 01:44:44 PST
Comment on attachment 271150 [details]
Patch

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

Nice patch, thanks!
Some nitpiks in the changelog..

> Source/WebKit2/ChangeLog:9
> +        embeded environment. Probably main concern was increasing video memory for fixed elements.

s/embeded/embedded/,
I'm not sure whether is it correct reason or not. As we talked in IRC, it is because the fixed element is not considered to be a new stacking context at first. This feature was added later because of the scrolling performance in the embedded system.

> Source/WebKit2/ChangeLog:10
> +        However, it is time to rethink spliting the elements out to separate layers. Currently the fixed

spliting/splitting
and how about adding a comma after "Currently" ?

> Source/WebKit2/ChangeLog:11
> +        elements are redrawn to a backing store every time layout or scroll happens. It results in bad

Also maybe you missed "the" before bad effect
Comment 3 ChangSeok Oh 2016-02-12 01:58:35 PST
Comment on attachment 271150 [details]
Patch

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

Thanks for your review!

>> Source/WebKit2/ChangeLog:9
>> +        embeded environment. Probably main concern was increasing video memory for fixed elements.
> 
> s/embeded/embedded/,
> I'm not sure whether is it correct reason or not. As we talked in IRC, it is because the fixed element is not considered to be a new stacking context at first. This feature was added later because of the scrolling performance in the embedded system.

I see. Then Let's get rid of the whole lines.

>> Source/WebKit2/ChangeLog:10
>> +        However, it is time to rethink spliting the elements out to separate layers. Currently the fixed
> 
> spliting/splitting
> and how about adding a comma after "Currently" ?

Yep.

>> Source/WebKit2/ChangeLog:11
>> +        elements are redrawn to a backing store every time layout or scroll happens. It results in bad
> 
> Also maybe you missed "the" before bad effect

Done.
Comment 4 ChangSeok Oh 2016-02-12 02:00:25 PST
Created attachment 271154 [details]
Patch
Comment 5 ChangSeok Oh 2016-02-15 01:16:05 PST
Any concern?
Comment 6 Gwang Yoon Hwang 2016-02-15 01:20:20 PST
I'm okay with this patch. Maybe cgarcia can help you. :)
Comment 7 ChangSeok Oh 2016-02-15 20:49:48 PST
Comment on attachment 271154 [details]
Patch

Thanks.
Comment 8 WebKit Commit Bot 2016-02-15 21:39:56 PST
Comment on attachment 271154 [details]
Patch

Clearing flags on attachment: 271154

Committed r196623: <http://trac.webkit.org/changeset/196623>
Comment 9 WebKit Commit Bot 2016-02-15 21:40:02 PST
All reviewed patches have been landed.  Closing bug.