Move mask invalid region computing from nsDisplaySVGEffects to nsDisplayMask

RESOLVED FIXED in Firefox 52

Status

()

Core
Layout
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: u459114, Assigned: u459114)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

2 years ago
As title, a thing that should be done in bug 1295094 bug missed.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8800555 - Flags: review?(mstange)
Attachment #8800556 - Flags: review?(mstange)
Attachment #8800557 - Flags: review?(mstange)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8800555 - Attachment is obsolete: true
Attachment #8800555 - Flags: review?(mstange)
(Assignee)

Updated

2 years ago
Attachment #8800556 - Attachment is obsolete: true
Attachment #8800556 - Flags: review?(mstange)
(Assignee)

Updated

2 years ago
Attachment #8800557 - Attachment is obsolete: true
Attachment #8800557 - Flags: review?(mstange)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8800658 - Attachment is obsolete: true
Attachment #8800658 - Flags: review?(mstange)
(Assignee)

Updated

2 years ago
Attachment #8800659 - Attachment is obsolete: true
Attachment #8800659 - Flags: review?(mstange)
(Assignee)

Updated

2 years ago
Attachment #8800660 - Attachment is obsolete: true
Attachment #8800660 - Flags: review?(mstange)
(Assignee)

Updated

2 years ago
Attachment #8800661 - Flags: review?(mstange)
Attachment #8800662 - Flags: review?(mstange)
Attachment #8800663 - Flags: review?(mstange)
Do reftests pass with these changes? I was under the impression that these invalidations are also needed for filters, not just for masks, and layout/reftests/invalidation/filter-userspace-offset.svg has a few filters, so if there is a problem, that test should catch them.
I'm just going to review these patches under the assumption that that test still passes and that you have an explanation for why the geometry is not needed for filters.

Comment 12

2 years ago
mozreview-review
Comment on attachment 8800661 [details]
Bug 1309804 - Part 1. Rename nsDisplaySVGEffectsGeometry to nsDisplayMaskGeometry.

https://reviewboard.mozilla.org/r/85542/#review84240

typos in the commit message: Geomerty -> Geometry
Attachment #8800661 - Flags: review?(mstange) → review+

Comment 13

2 years ago
mozreview-review
Comment on attachment 8800662 [details]
Bug 1309804 - Part 2. Split invalid region computation into nsDisplayMask & nsDisplayFilter.

https://reviewboard.mozilla.org/r/85544/#review84280

::: layout/base/nsDisplayList.cpp:7036
(Diff revision 1)
> +  bool snap;
> +  nsRect bounds = GetBounds(aBuilder, &snap);
> +  if (geometry->mFrameOffsetToReferenceFrame != ToReferenceFrame() ||
> +      geometry->mUserSpaceOffset != UserSpaceOffset() ||
> +      !geometry->mBBox.IsEqualInterior(BBoxInUserSpace())) {
> +    // Filter and mask output can depend on the location of the frame's user

Remove filter from this comment.
Attachment #8800662 - Flags: review?(mstange) → review+

Comment 14

2 years ago
mozreview-review
Comment on attachment 8800663 [details]
Bug 1309804 - Part 3. Fix trailing space.

https://reviewboard.mozilla.org/r/85546/#review84282

"trailing spaces"
Attachment #8800663 - Flags: review?(mstange) → review+
(Assignee)

Comment 15

2 years ago
(In reply to Markus Stange [:mstange] from comment #10)
> Do reftests pass with these changes? I was under the impression that these
> invalidations are also needed for filters, not just for masks, and
> layout/reftests/invalidation/filter-userspace-offset.svg has a few filters,
> so if there is a problem, that test should catch them.
I thought this change is too trivial so I didn't actually run through reftest
Sorry, like you said, filter invalidation test failed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=67559d79b5d2&selectedJob=29146738

Both user-space-offset and bbox geometry change need to be handled in invalid region computing in nsDisplayFilter too. I am going to slightly change part 2.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Ok, looks good with that change.

Comment 20

2 years ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/560e6555c82b
Part 1. Rename nsDisplaySVGEffectsGeometry to nsDisplayMaskGeometry. r=mstange
https://hg.mozilla.org/integration/autoland/rev/a5fa27d68f33
Part 2. Split invalid region computation into nsDisplayMask & nsDisplayFilter. r=mstange
https://hg.mozilla.org/integration/autoland/rev/3acec32fd8f4
Part 3. Fix trailing space. r=mstange

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/560e6555c82b
https://hg.mozilla.org/mozilla-central/rev/a5fa27d68f33
https://hg.mozilla.org/mozilla-central/rev/3acec32fd8f4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
You need to log in before you can comment on or make changes to this bug.