Fix some reftest failures when enabling WebRenderBorderLayer

RESOLVED FIXED in Firefox 53

Status

()

Core
Graphics: WebRender
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ethlin, Assigned: ethlin)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
There is a simple case: layout/reftests/position-dynamic-changes/multiple-changes.
The test has reftest-wait and remove the class after changing the div's left position. With WebRenderBorderLayer, the snapshot gets the image before changing the left property. I added some logs and found the transaction with new position is later than snapshot command. 
I think the mechanism of reftest-wait should be fine since for other tests with WebRenderPaintedLayer are all passed.
(Assignee)

Updated

a year ago
See Also: → bug 1329609
(Assignee)

Comment 1

a year ago
The problem is when we use WRBorderLayer, the client rect[1] from MozAfterPaint is empty. So the reftest will not do DrawWindow to take the new snapshot. Currently FrameLayerBuilder has some mechanisms for paintedLayer[2] and imageLayer/colorLayer[3]. Maybe I should add something for borderLayer. Matt, do you have any idea for this problem?


[1] https://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-content.js#1135
[2] https://dxr.mozilla.org/mozilla-central/source/layout/painting/FrameLayerBuilder.cpp?q=path%3AFrameLayerBuilder.cpp&redirect_type=single#4535
[3] https://dxr.mozilla.org/mozilla-central/source/layout/painting/FrameLayerBuilder.cpp?q=path%3AFrameLayerBuilder.cpp&redirect_type=single#3183
Flags: needinfo?(matt.woodrow)
I think you need to add border (and text!) support within LayerTreeInvalidation.

http://searchfox.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.cpp#650
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 3

a year ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> I think you need to add border (and text!) support within
> LayerTreeInvalidation.
> 
> http://searchfox.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.
> cpp#650

Thanks! I should add one for border.
(Assignee)

Comment 4

a year ago
Created attachment 8828219 [details] [diff] [review]
add BorderLayerProperties

I add BorderLayerProperties for border layer though I'm not very sure if the calculation is correct. At least the test multiple-changes.html is passed now.
Attachment #8828219 - Flags: review?(matt.woodrow)
Comment on attachment 8828219 [details] [diff] [review]
add BorderLayerProperties

Review of attachment 8828219 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Do you want to do TextLayer while you're at it? :)
Attachment #8828219 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 6

a year ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Comment on attachment 8828219 [details] [diff] [review]
> add BorderLayerProperties
> 
> Review of attachment 8828219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> Do you want to do TextLayer while you're at it? :)

Okay! I'll have another bug for it.
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Comment 7

a year ago
Please push the attachment 8828219 [details] [diff] [review] to central.

Comment 8

a year ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d21bd74f1a20
Add BorderLayerProperties for border layer. r=mattwoodrow
Keywords: checkin-needed

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d21bd74f1a20
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.