984279 - when we use the view bounds to calculate the composition bounds we fail to make them relative to the reference frame

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Bug 935219 added this code. The composition bounds should be relative to the reference frame, but we don't add the offset to make that happen if we use the view bounds.

Comment 1

[Jason Smith [:jsmith]]

Do we need this on 1.3 or 1.4?

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 8392086 [details] [diff] [review]
patch

Seems reasonable to me, but please double-check with someone involved
in bug 935219.

Comment 3

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8392086 [details] [diff] [review]
patch

Ok. That would be kats or botond most likely, kats on pto -> botond.

We don't know if view bounds are the right thing here given bug 983208, but let's at least make the calculation correct. We can delete it later if that's what we find out we should do.

Comment 4

[Timothy Nikkel (:tnikkel)]

(In reply to Jason Smith [:jsmith] from comment #1)
> Do we need this on 1.3 or 1.4?

I don't think the situation this applies to comes up in b2g, but is low risk, we should probably just take it.

Comment 5

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3347b7314302

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3347b7314302

Comment 7

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8392086 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 935219
User impact if declined: no user visible problems encountered. the calculation is just wrong. bounds need to be relative to reference frame and not themselves. this is a situation that will we hit rarely, perhaps only on metro currently. it's better to keep this code correct and in-sync where it has landed.
Testing completed (on m-c, etc.): just landed m-c
Risk to taking this patch (and alternatives if risky): very low, riskier to keep the current behaviour
String or IDL/UUID changes made by this patch: none

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Timothy, do you confirm that it is only for beta? We merged aurora => beta yesterday.

Comment 9

[Timothy Nikkel (:tnikkel)]

I intend this for aurora (30), ie what just got merged from m-c. Bug 935219 is now on aurora and b2g28. I will get to a b2g28 request after a bit. Does that answer your question?

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Indeed. So, not 29.  Thanks

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2602ee663acf