mozilla :: #layout

14 Jul 2017
02:07heycamdbaron: I got myself into a situation where it looks like I'm reflowing forever:
02:07heycamdbaron: have you seen something like this lately?
02:08dbaronI don't think so
02:08* dbaron loads the link
02:09dbaronheycam: no, haven't seen anything like that lately
02:09heycamdbaron: ok
02:12heycamdbaron: reproducible with stylo enabled it seems
02:12* heycam looks into it
02:16heycamah, fixed after updating to today's nightly
20:52bradwerthdholbert: my shape-outside reflow changes led to a mochitest failure on linux:
20:52dholbertbradwerth: only on linux?
20:53bradwerthdholbert: same test passes fine on my local macOS build
20:53bradwerthI can trigger some others on try server, I suppose
20:54dholbertbradwerth: I'll bet it's more likely to be an issue with the existing implementation, which your change revealed
20:54bradwerthdholbert: i'm simplifying the test locally, but of course it still passes locally, on macOS
20:55bradwerthdholbert: I'll spam some other builds on tryserver and see what comes up
20:57dholbertbradwerth: no need probably - you've got the Treeherder run for when it landed
20:57bradwerthdholbert: though it looks like the mochitest suite that contains that test, M-14, is run on few testing platfomrs
20:57dholbertbradwerth: the number is arbitrary
20:57dholbertbradwerth: It's M-5 on windows, looks like (in the try run aryx posted)
20:58bradwerthdholbert: ahh, I didn't know they got regrouped... I thought it was a stealth constant grouping thing
20:58dholbertbradwerth: "Windows 7 opt" shows the failure in TC-M (5) there
20:59dholbertbradwerth: (the number is just the bucketizing, and we use different numbers of buckets on different platforms/configs)
21:00dholbertbradwerth: it looks like maybe your run didn't get all tests on all platforms, so maybe worth doing an all-mochitests run on win/mac/linux after all
21:00bradwerthdholbert: do you see anything unexpected in the prop definition?
21:03dholbertbradwerth: not offhand
21:04bradwerthdholbert: what would be your strategy for debugging this unexpected behavior
21:05dholbertbradwerth: I'd first try to figure out how to reproduce it locally, probably (and if not, then maybe try to debug on the one-click loaner UI, though I haven't used that before so I don't know how much work it is to get a debugger working nicely there)
21:07dholbertbradwerth: I'm seeing if I can reproduce locally, as an additional data point
21:07bradwerthdholbert: thank you for that. what platform?
21:07dholbertlinux debug
21:09bradwerthdholbert: look at this change since your last review. The code does hint |= nsChangeHint_ReconstructFrame, instead of straight setting it to nsChangeHint_ReconstructFrame as is done earlier in the function. Would that cause problems?
21:12dholbertbradwerth: I can reproduce locally (with your "part 1" applied)
21:12bradwerthdholbert: okay, I'll see if there's something odd going on with my local build (do a rebuild, etc) and try again
21:12bradwerthdholbert: thanks for your help
21:14dholbertbradwerth: sure! RE the ReconstructFrame hint: I don't know if that change would cause problems, but I don't think it makes sense to combine that with reflow hints
21:15dholbertbradwerth: you probably just want to return that change hint directly, assuming we do really need to reconstruct
21:15dholbertbecause a freshly-reconstructed subtree will already need a dirty reflow for itself, so all the other reflow-related change hints are redundant
21:16bradwerthdholbert: okay, though the earliest case does not return early: Once I can reproduce, I'll try both ways
21:18dholbertbradwerth: hmm
21:20dholbertbradwerth: that might be an oversight -- that seems to be the only "hint |= nsChangeHint_ReconstructFrame;" in that file
21:21dholbert(I found one other spot in nsStyleVisibility::CalcDifference, but that one effectively returns immediately if we hit it and doesn't set any other bits)
21:23dholbertbradwerth: anyway, I'm not 100% sure that'd cause this problem
21:24dholbertbradwerth: (also, that change probably should've gotten another round of reflow, since it was changing the patch to do something else. In any case, I imagine it will now, since you'll need to be changing something else too :))
21:24dholbert(er "another round of review")
21:25dholbertbradwerth: (note also that the commit message, "Force changes to shape-outside to trigger float reflow", no longer matches what the patch does, too)
21:25bradwerthdholbert: understood. How do i trigger a new review in mozreview from same person when I already have an r+ ?
21:25dholbertbradwerth: good question :( I don't think it's technically possible. co-opt "needinfo", probably
21:25dholbertbradwerth: but also, I think ReconstructFrame is probably overkill here
21:26dholbertI'm sure it works, but I'm not sure we need it... as I recall when I was testing the testcase, we'd get the correct layout if I resized my window horizontall
21:26dholbertwhich indicates that some amount of dirty reflow (no reconstruction needed) was sufficient to produce the correct layout
21:27bradwerthdholbert: I'll look for a narrower set of hints. nsChangeHint_AllReflowHints wasn't cutting it. The textnodes within the sibling div in the test were reflowing correct at the "top" of the circle but not at the "bottom".
21:29dholbertbradwerth: All of the text adjusts nicely for me (at the top and bottom of the circle), if I resize the browser window horizontally
21:29dholbert(in stock nightly with the shape-outside pref toggled)
21:29dholbertbradwerth: so some amount of reflow is sufficient, it seems
22:25dholbertbradwerth: *facepalm* I know what's wrong I think
22:26bradwerthdholbert: I am all ears
22:26dholbertbradwerth: so if "shape-outside" changes...
22:26dholbert...and "float" is unset...
22:26dholbertyour latest patch will return nsChangeHint(0)
22:27bradwerth... in that test, that sequence could happen I suppose
22:27dholbertand that makes us think the old style struct & the new style struct are equivalent
22:27bradwerthdholbert: so I need to drop the qualifier float != none
22:27dholbertnot exactly
22:27dholbertif float != none, we need "some reflow" (yet to be defined)
22:28dholbertotherwise, we need to return nsChangeHint_NeutralChange
22:28dholbert(which is distinct from nsChangeHint(0)
22:28bradwerthokay, I'll give that a shot. the to-be-determined reflow I am trying next is nsChangeHint_ReflowHintsForBSizeChange
22:29bradwerthdholbert: thank you for continuing thinking about it, and for coming up with that insight
22:29dholbertThe above ^^ should fix your mochitest failure, I'm pretty sure. (dunno for the reflowing-sufficiently though)
22:29dholbert(the above = what I said, not the ReflowHintsForBSizeChange)
22:36dholbertbradwerth: I'm going to update the code patch to be "r-" only so that you have the ability to re-request review & it'll make it into my review queue, etc :)
15 Jul 2017
No messages
Last message: 69 days and 10 hours ago