See bug 1428491 comment 15 & co. In particular, we set the pres context visible area _before_ processing restyles. This causes inconsistencies when resolving viewport units. In particular, the resulting style tree will have some units resolved in terms of the old size, and some in terms of the new size, depending on whatever is dirty. This stroke me in bug 1428491, when the MediaQueryChange stuff that was posted were resolved, causing the whole document to be restyle. I don't have a good sense of what's the best thing to do here, to be honest... There are callers who pass NS_UNCONSTRAINEDSIZE heights to this function, so we end up with crazy sizes in layout...

I think the sane thing to do here would be to flush styles _before_ SetVisibleArea but...

Ok, I got a sense of what should be done now...

I don't understand why you want to flush before SetVisibleArea rather than after. Wouldn't we want the SetVisibleArea first so that size-related media queries are correct when we flush style? It would be good to see tests in this patch for the things you're fixing. (And maybe also tests for the other media queries case.)

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #5) > I don't understand why you want to flush before SetVisibleArea rather than > after. Wouldn't we want the SetVisibleArea first so that size-related media > queries are correct when we flush style? Right, the patch flushes before only when we shrink-to-fit. In that case, the bsize is NS_UNCONSTRAINEDHEIGHT, and you don't really want to flush against it (first because it causes all sorts of crazy sizes, as in the blocked bug, second because the visible area will change again anyway after reflowing). For the non-shrink-to-fit case, we do flush after SetVisibleArea, which has the nice property that you mention.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6) > (In reply to David Baron :dbaron: ⌚️UTC-8 from comment #5) > > I don't understand why you want to flush before SetVisibleArea rather than > > after. Wouldn't we want the SetVisibleArea first so that size-related media > > queries are correct when we flush style? > > Right, the patch flushes before only when we shrink-to-fit. In that case, > the bsize is NS_UNCONSTRAINEDHEIGHT, and you don't really want to flush > against it (first because it causes all sorts of crazy sizes, as in the > blocked bug, second because the visible area will change again anyway after > reflowing). To clarify, s/is/would become after SetVisibleArea/ in that sentence.

Err, UNCONSTRAINEDSIZE, I mean.

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #5) > It would be good to see tests in this patch for the things you're fixing. > (And maybe also tests for the other media queries case.) Regarding tests, I agree it would be good, but I cannot think of a way to test it. In particular, I think the posted mediaquerychange that wasn't flushed before this patch causes us to recascade the whole document anyway. And I don't know how to test the shrink-to-fit thing easily, since it has only one caller... I can add some assertions though.

I suspect the extra restyles caused by this may impact stylo-chrome perf (which is slower recascading the whole document).

Here's a green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc303fbf546c34d6a315b0229f156de3f77f7221 (ignore the ::slotted failure, it's related to a patch in the same queue)

> And I don't know how to test the shrink-to-fit thing easily, since it has only one caller... That being sizeToContent? Does the getContentSizeConstrained() call path avoid shrink-to-fit?

Comment on attachment 8942979 [details] Bug 1430844: Make resize reflow sound re. viewport units. https://reviewboard.mozilla.org/r/213244/#review219142 ::: layout/base/PresShell.cpp:1983 (Diff revision 2) > > RefPtr<nsViewManager> viewManager = mViewManager; > nsCOMPtr<nsIPresShell> kungFuDeathGrip(this); > > - if (!GetPresContext()->SuppressingResizeReflow()) { > - // Have to make sure that the content notifications are flushed before we > + if (!suppressingResizeReflow && shrinkToFit) { > + // Make sure that style is flushed before setting the pres context So the only shrinkToFit direct caller is nsDocumentViewer::GetContentSizeInternal which is already doing `mDocument->FlushPendingNotifications(FlushType::Layout);` before calling into us. Can we take that layout flush out now? I'd think we can, but followup for that to contain risk. ::: layout/base/PresShell.cpp:1989 (Diff revision 2) > - // start messing with the frame model; otherwise we can get content doubling. > - mDocument->FlushPendingNotifications(FlushType::ContentAndNotify); > + // VisibleArea if we're shrinking to fit. > + // > + // Otherwise we may end up with bogus viewport units resolved against the > + // unconstrained bsize, or restyling the whole document resolving viewport > + // units against targetWidth, which may end up doing wasteful work. > + mDocument->FlushPendingNotifications(FlushType::Frames); Is this really conceptually a Frames flush, or a Style flush? This does more work than we used to; in particular it will flush layout on ancestors. I expect this is not used on non-root documents much, though (again, not sure about the webextension callsite). ::: layout/base/PresShell.cpp:1992 (Diff revision 2) > + // unconstrained bsize, or restyling the whole document resolving viewport > + // units against targetWidth, which may end up doing wasteful work. > + mDocument->FlushPendingNotifications(FlushType::Frames); > + } > > - // Make sure style is up to date > + mPresContext->SetVisibleArea(nsRect(0, 0, targetWidth, targetHeight)); Might want to guard this on mIsDestroying, since we now do a full-up style flush before this line in the shrink-to-fit case. ::: layout/base/PresShell.cpp:1995 (Diff revision 2) > + } > > - // Make sure style is up to date > - { > - nsAutoScriptBlocker scriptBlocker; > - mPresContext->RestyleManager()->ProcessPendingRestyles(); > + mPresContext->SetVisibleArea(nsRect(0, 0, targetWidth, targetHeight)); > + if (!suppressingResizeReflow) { > + if (!shrinkToFit) { > + // Flush styles _now_ (with the correct visible area) if not shrinking. s/shrinking/computing the shrink-to-fit size/?

Comment on attachment 8943002 [details] Bug 1430844: Add assertions that would've caught this. https://reviewboard.mozilla.org/r/213272/#review219144 r=me

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13) > > And I don't know how to test the shrink-to-fit thing easily, since it has only one caller... > > That being sizeToContent? > > Does the getContentSizeConstrained() call path avoid shrink-to-fit? No, it still shrinks if possible, but constrains the content width if necessary before reflowing to calculate the preferred height. This is all pretty terrible, by the way. The layout engine is smart enough to figure out how to size content within a root element that can grow or shrink, with minimum and maximum dimensions, without a bunch of extra reflows after every DOM change. The problem is extending that to actually resizing a window. I've been wanting to replace this with a cleaner solution for a long time.

Comment on attachment 8942979 [details] Bug 1430844: Make resize reflow sound re. viewport units. https://reviewboard.mozilla.org/r/213244/#review219142 > So the only shrinkToFit direct caller is nsDocumentViewer::GetContentSizeInternal which is already doing `mDocument->FlushPendingNotifications(FlushType::Layout);` before calling into us. Can we take that layout flush out now? I'd think we can, but followup for that to contain risk. Filed bug 1431046. > Is this really conceptually a Frames flush, or a Style flush? > > This does more work than we used to; in particular it will flush layout on ancestors. I expect this is not used on non-root documents much, though (again, not sure about the webextension callsite). It is a style flush, but we do poke at the frames afterwards... I guess is a "flush styles in this document and make sure that frames are constructed". For now flush styles works for that purpose, though it won't if we start deferring change hint processing, so will use that for now with a comment.

> I guess is a "flush styles in this document and make sure that frames are constructed". In that case, flushing "Frames" is correct.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7317de4672a8 Make resize reflow sound re. viewport units. r=bz https://hg.mozilla.org/integration/autoland/rev/83d6c4007b51 Add assertions that would've caught this. r=bz

https://hg.mozilla.org/mozilla-central/rev/7317de4672a8 https://hg.mozilla.org/mozilla-central/rev/83d6c4007b51