Closed
Bug 930904
Opened 11 years ago
Closed 11 years ago
Update/remove ifdef block once bug 732971 is landed
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: kats, Assigned: kats)
References
Details
(Whiteboard: [release28][qa-])
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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)
Assignee | ||
Comment 2•11 years ago
|
||
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)
Assignee | ||
Comment 3•11 years ago
|
||
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)
Assignee | ||
Comment 4•11 years ago
|
||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
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).
Attachment #825926 -
Flags: review?(tnikkel)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [release28]
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #827119 -
Flags: review?(tnikkel)
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #825926 -
Attachment is obsolete: true
Attachment #827120 -
Flags: review?(tnikkel)
Comment 9•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #827120 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 10•11 years ago
|
||
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•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f34a58fcc1a3
https://hg.mozilla.org/mozilla-central/rev/de45f494f6b2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•11 years ago
|
Whiteboard: [release28] → [release28][qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•