Bug 147627 - [CoordinatedGraphics] Fixed position element didn't make the compositing layer in WTR.
Summary: [CoordinatedGraphics] Fixed position element didn't make the compositing laye...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hunseop Jeong
URL:
Keywords:
Depends on:
Blocks: 147019
  Show dependency treegraph
 
Reported: 2015-08-04 05:19 PDT by Hunseop Jeong
Modified: 2015-09-23 22:56 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.70 KB, patch)
2015-08-04 05:27 PDT, Hunseop Jeong
simon.fraser: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hunseop Jeong 2015-08-04 05:19:07 PDT
compositing/fixed-with-fixed-layout.html failed after r186911 because

# 49 line in LayoutTests/compositing/fixed-with-fixed-layout.html 
if (window.internals) {
    internals.settings.setAcceleratedCompositingForFixedPositionEnabled(true);

'internals.settings.setAcceleratedCompositingForFixedPositionEnabled(true)' didn't work properly in WTR.

CoordinatedGrapihcs set the 'AcceleratedCompositingForFixedPosition' in WebPage::setUseFixedLayout normally.
But only 'frameView->setUseFixedLayout()' was invoked in WTR if I use the 'internals.settings.setAcceleratedCompositingForFixedPositionEnabled' in test.
So we need to set the AcceleratedCompositingForFixedPositionEnabled in Internals::setUseFixedLayout() to work correctly.
Comment 1 Hunseop Jeong 2015-08-04 05:27:38 PDT
Created attachment 258171 [details]
Patch
Comment 2 Csaba Osztrogonác 2015-08-04 07:56:39 PDT
I have some questions about this change:
- How does this change affect the layout test results? 
- Will we have more passing tests? If yes, we should unskip them.
- Do we reset this setting to the default between each test?
Comment 3 Simon Fraser (smfr) 2015-08-04 08:37:06 PDT
Comment on attachment 258171 [details]
Patch

I don't think this makes sense. fixed layout and these two settings should be orthogonal.
Comment 4 Tim Horton 2015-08-04 11:07:06 PDT
(In reply to comment #3)
> Comment on attachment 258171 [details]
> Patch
> 
> I don't think this makes sense. fixed layout and these two settings should
> be orthogonal.

Right.

This:

> CoordinatedGrapihcs set the 'AcceleratedCompositingForFixedPosition' in WebPage::setUseFixedLayout normally.

Is totally bizarre and wrong, and needs fixing/deconflating.
Comment 5 Hunseop Jeong 2015-08-04 19:32:11 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 258171 [details]
> > Patch
> > 
> > I don't think this makes sense. fixed layout and these two settings should
> > be orthogonal.
> 
> Right.
> 
> This:
> 
> > CoordinatedGrapihcs set the 'AcceleratedCompositingForFixedPosition' in WebPage::setUseFixedLayout normally.
> 
> Is totally bizarre and wrong, and needs fixing/deconflating.

You mean, 'setAcceleratedCompositingForFixedPositionEnabled' & 'setFixedPositionCreatesStackingContext' isn't related with fixedLayout?

Now CoordinatedGraphics turn on these options in WebPage::setUseFixedLayout.
On mac, these options was set in RemoteLayerTreeDrawingArea::updatePreferences.
Could I separate these options from the fixedLayout?
Comment 6 Hunseop Jeong 2015-08-04 22:02:54 PDT
(In reply to comment #2)
> I have some questions about this change:
> - How does this change affect the layout test results? 
This patch can composite the layer for the element that has the fixed position.
CoordinatedGraphics didn't composite the fixed position element in WTR. 
Is it expected result?
> - Will we have more passing tests? If yes, we should unskip them.
It makes about 30 tests passed, but sadly 200 more tests failed.
If compositing the fixed position element is right, we need to rebaseline the expected results. 
> - Do we reset this setting to the default between each test?
Comment 7 Tim Horton 2015-08-05 11:26:24 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Comment on attachment 258171 [details]
> > > Patch
> > > 
> > > I don't think this makes sense. fixed layout and these two settings should
> > > be orthogonal.
> > 
> > Right.
> > 
> > This:
> > 
> > > CoordinatedGrapihcs set the 'AcceleratedCompositingForFixedPosition' in WebPage::setUseFixedLayout normally.
> > 
> > Is totally bizarre and wrong, and needs fixing/deconflating.
> 
> You mean, 'setAcceleratedCompositingForFixedPositionEnabled' &
> 'setFixedPositionCreatesStackingContext' isn't related with fixedLayout?

Yes, that's what I mean. These are totally unrelated settings which the CoordinatedGraphics code for whatever reason conflates.

> Now CoordinatedGraphics turn on these options in WebPage::setUseFixedLayout.
> On mac, these options was set in
> RemoteLayerTreeDrawingArea::updatePreferences.

Turning it on in the drawing areas is still a little weird, but a thousand times better than turning it on because another unrelated setting (fixed layout) was turned on.

> Could I separate these options from the fixedLayout?

Could, and should!
Comment 8 Hunseop Jeong 2015-09-23 22:56:08 PDT
gyuyoung, Do you know the reason that setAcceleratedCompositingForFixedPositionEnabled() and setFixedPositionCreatesStackingContext() was tied with fixedLayout?

In mac, set the setting values of FixedPositionCreatesStackingContext() and AcceleratedCompositingForFixedPositionEnabled() in updatePreferences() of the DrawingArea.

I think CoordinatedGraphics also need to set the FixedPositionCreatesStackingContext and AcceleratedCompositingForFixedPositionEnabled with true for the fixed position element to composite the layer.

If I move the setAcceleratedCompositingForFixedPositionEnabled() and setFixedPositionCreatesStackingContext() from WebPage::setUseFixedLayout() to CoordinatedDrawingArea::updatePreferences(), is it possible to make some problems?