Closed Bug 1114336 Opened 8 years ago Closed 8 years ago

Progressive tiling code has a typo with possibly bad consequences

Categories

(Core :: Graphics: Layers, defect)

Other
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
blocking-b2g 2.1+
Tracking Status
firefox34 --- wontfix
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- fixed
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: kats, Assigned: nical)

Details

Attachments

(1 file)

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/TiledContentClient.cpp?rev=aa785fd0b16a#219 is missing the <= 2 check

Saw this while reviewing the new contributor patch in bug 1113774. Don't have time at the moment to dig into the history or implications of this bug but i can do it tomorrow if nobody gets to it before me.
Attached patch Fix the typo.Splinter Review
Attachment #8540174 - Flags: review?(bugmail.mozilla)
Comment on attachment 8540174 [details] [diff] [review]
Fix the typo.

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

This might need uplifting, please check to see which branches this affects and uplift as needed.
Attachment #8540174 - Flags: review?(bugmail.mozilla) → review+
Assignee: nobody → nical.bugzilla
The typo landed a year ago so pretty much all branches are affected. I'll let it bake on central for a day or two and see how things go. Not sure whether we want to uplift all the way to beta considering we haven't seen that it caused issues so far (or haven't made the connection).
It might be related to the things benwa is trying to fix in bug 1112332.
Comment on attachment 8540174 [details] [diff] [review]
Fix the typo.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: We can get into incorrect situations with low precision tiling, currently defaults on B2G and Mac.
[Describe test coverage new/current, TBPL]:
[Risks and why]: Low risk.
[String/UUID change made/needed]:
Attachment #8540174 - Flags: approval-mozilla-beta?
Attachment #8540174 - Flags: approval-mozilla-b2g34?
Attachment #8540174 - Flags: approval-mozilla-aurora?
[Blocking Requested - why for this release]:
We heavily use tiling, including low precision on B2G, every time we scroll.  This problem can lead to noticeable but hard to diagnose correctness problems.
blocking-b2g: --- → 2.1?
blocking-b2g: 2.1? → 2.1+
https://hg.mozilla.org/mozilla-central/rev/0076eb0ed38c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attachment #8540174 - Flags: approval-mozilla-beta?
Attachment #8540174 - Flags: approval-mozilla-beta+
Attachment #8540174 - Flags: approval-mozilla-aurora?
Attachment #8540174 - Flags: approval-mozilla-aurora+
Is this bad enough to warrant 2.0 blocking status (and a b2g32 approval request)?
Flags: needinfo?(nical.bugzilla)
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #10)
> Is this bad enough to warrant 2.0 blocking status (and a b2g32 approval
> request)?

I don't know how bad things need to be to warrant 2.0 blocking status, so I don't think I am the best person to answer this question. The fix is rather simple and low-risk so I would argue if favour of taking it to 2.0. forwarding the needinfo to Kats.
Flags: needinfo?(nical.bugzilla) → needinfo?(bugmail.mozilla)
I would say no. I don't know how many people are running 2.0 within Mozilla anymore and if this patch were to cause regressions it's unlikely it would be noticed before it shipped to users. Based on the testing so far we don't have any concrete evidence that this typo actually manifests as something bad so I would say to leave it out of 2.0.
Flags: needinfo?(bugmail.mozilla)
Attachment #8540174 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
You need to log in before you can comment on or make changes to this bug.