930904 - Update/remove ifdef block once bug 732971 is landed

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

There is an #ifdef at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=a61d898ea4fa#703 which will need to be removed or updated once bug 732971 is landed. Because the resolution is set on a layer higher in the layer tree than the scrollable layer I'm not 100% sure the comment there is right, but it should be easy enough to check if that statement is a no-op and remove it if it is.

Comment 1

[Mats Palmgren (inactive)]

Bug 732971 has landed, so it seems like a good idea to sort this out now rather than later.
kats, can you take this?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Yup I'll take this.

For the record I did check to see if the statement in question was a no-op and I don't think it is. At least the values on the LHS and RHS of the assignment are not the same. I didn't check to see if it has effects later in the code but I assume it does. Removing the ifdef won't be as trivial as I thought.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Merge the ifdef into the condition that determines the resolution. Now that bug 732971 is landed, this has no effect on B2G and Fennec. With my current patch queue for bug 902505, I was seeing the same problem on Metro that I was seeing previously on Fennec (i.e. all of the FrameMetrics had a mResolution == 1.0) and this patch fixes that too.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

https://tbpl.mozilla.org/?tree=Try&rev=e81ef6d46fb9

Comment 5

[Timothy Nikkel (:tnikkel)]

Comment on attachment 825926 [details] [diff] [review]
Patch

This is kind of nitty but would it be better to split this into fix + remove the hack in two patches? Will make it clearer in the history, and since we've had a lot of charges here recently, that would probably be appreciated.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 825926 [details] [diff] [review]
Patch

Yeah I'll split it into two. Also as discussed on IRC I want to populate the FrameMetrics for all subdocument container layers, and so this will be affected by that as well (I'll want to set the mResolution only on the root frame of presShells rather than the rootScrollFrame).

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Set the resolution on the root scrollable layer for a presshell

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Remove ifdef

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 827119 [details] [diff] [review]
Part 1 - Set the resolution on the root scrollable layer for a presshell

This makes sense. Can you explain what exactly goes wrong when looking for the root scroll id versus checking if we have the root scroll frame directly?

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Wrote a small essay into the commit message of the first patch and landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f34a58fcc1a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/de45f494f6b2

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f34a58fcc1a3
https://hg.mozilla.org/mozilla-central/rev/de45f494f6b2