Closed Bug 1267471 Opened 4 years ago Closed 4 years ago

Check snap info in ScrollMetadata ==

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Keywords: correctness, Whiteboard: [gfx-noted])

Attachments

(1 file)

There's a TODO at [1] that needs to be fixed. Without it, checks like [2] don't update the layer with snap info if it changes and nothing else in the metadata changes.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/FrameMetrics.h?rev=97daab2fd231#845
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.h?rev=97daab2fd231#875
Heh, I was just looking at that TODO! Sorry for overlooking it before landing bug 1219296.
Attached patch PatchSplinter Review
rs=botond over IRC
Attachment #8745123 - Flags: review+
(In reply to Botond Ballo [:botond] from comment #1)
> Heh, I was just looking at that TODO! Sorry for overlooking it before
> landing bug 1219296.

No worries, I should have caught it in review. Came back to bite me, as is just and fair :)
https://hg.mozilla.org/mozilla-central/rev/faf1c8eb9261
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Target Milestone: mozilla48 → mozilla49
Comment on attachment 8745123 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 1219296
[User impact if declined]: in some cases scroll snapping with APZ might not work properly
[Describe test coverage new/current, TreeHerder]: there is a test in bug 1265510 (not yet landed) which exercises this code and requires this fix in order to pass
[Risks and why]: low risk, it was a TODO that was meant to be implemented but was overlooked
[String/UUID change made/needed]: none
Attachment #8745123 - Flags: approval-mozilla-aurora?
Comment on attachment 8745123 [details] [diff] [review]
Patch

See comment 6 for risk assessment. I'm requesting uplift to beta because I'm also requesting uplift of bug 1219296 to beta, which in turn is needed to reduce the risk when uplifting bug 1255823 (beta topcrasher fix).
Attachment #8745123 - Flags: approval-mozilla-beta?
Comment on attachment 8745123 [details] [diff] [review]
Patch

We need this fix for addressing the crash spike seen in bug 1268881, I've been told this is the least risky option and this affects e10s only mode, Beta47+, Aurora48+
Attachment #8745123 - Flags: approval-mozilla-beta?
Attachment #8745123 - Flags: approval-mozilla-beta+
Attachment #8745123 - Flags: approval-mozilla-aurora?
Attachment #8745123 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.