1574016 - patch from bug 1547277 contains logic error

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

bwerth noticed this. On this line

https://searchfox.org/mozilla-central/rev/3366c3d24f1c3818df37ec0818833bf085e41a53/layout/base/nsDocumentViewer.cpp#2100

we are missing a negation. This still fixed the bug because we seem to get another SetBounds call right after with the same bounds. But it does mean that we are probably invalidating when we don't need to.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1574016. Fix logic error in patch from bug 1547277. r?bradwerth

The fixed still worked because we get another SetBounds call right after with the same bounds. But it does mean we are doing some useless invalidation.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5cc355f51aa
Fix logic error in patch from bug 1547277. r=bradwerth

Comment 3

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f5cc355f51aa

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Is this worth backporting to Beta? I'm thinking no for ESR if it's just causing wasted work but no correctness issues.

Comment 5

[Brad Werth [:bradwerth]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #4)

Is this worth backporting to Beta? I'm thinking no for ESR if it's just causing wasted work but no correctness issues.

I am in favor of backporting. Although we don't have a testcase for it, it seems possible that this would fail to invalidate a frame with differing bounds where the rootView has already been resized.

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9085632 [details]
Bug 1574016. Fix logic error in patch from bug 1547277. r?bradwerth

Beta/Release Uplift Approval Request

• User impact if declined: slightly more invalidating in some cases, possibly not painting a popup in a rare case
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): just invalidates in the correct case instead of accidentally getting the invalidation correct
• String changes made/needed:

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9085632 [details]
Bug 1574016. Fix logic error in patch from bug 1547277. r?bradwerth

Fixes an invalidation bug, approved for 69.0b16.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3749ea604ad9