Closed Bug 930904 Opened 8 years ago Closed 8 years ago

Update/remove ifdef block once bug 732971 is landed


(Core :: Layout, defect)

Not set





(Reporter: kats, Assigned: kats)



(Whiteboard: [release28][qa-])


(2 files, 1 obsolete file)

There is an #ifdef at 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.
Bug 732971 has landed, so it seems like a good idea to sort this out now rather than later.
kats, can you take this?
Flags: needinfo?(bugmail.mozilla)
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.
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(bugmail.mozilla)
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #825926 - Flags: review?(tnikkel)
Comment on attachment 825926 [details] [diff] [review]

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 on attachment 825926 [details] [diff] [review]

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).
Attachment #825926 - Flags: review?(tnikkel)
Whiteboard: [release28]
Attachment #825926 - Attachment is obsolete: true
Attachment #827120 - Flags: review?(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?
Attachment #827119 - Flags: review?(tnikkel) → review+
Attachment #827120 - Flags: review?(tnikkel) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [release28] → [release28][qa-]
You need to log in before you can comment on or make changes to this bug.