976411 - Use ConstructFramesFromItemList in more places, and some minor cleanup

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

We have three places with the same code that can be shared.

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8381128 [details] [diff] [review]
fix

So this is losing some useful asserts from ConstructFramesFromItemList, no?

Also, why can the other two callsites not call ConstructFramesFromItemList?

Comment 3

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #2)
> So this is losing some useful asserts from ConstructFramesFromItemList, no?

It looks to me like all three call sites currently assert:
  iter.item().DesiredParentType() == GetParentType(aParentFrame)
which is what the new method also assert.
ConstructFramesFromItemList do run that in a separate loop but I don't
think it makes a difference if it's in the loop that creates the frames.
So, AFAICT, the patch is idempotent.

> Also, why can the other two callsites not call ConstructFramesFromItemList?

The flexbox stuff in ConstructFrame:
http://hg.mozilla.org/mozilla-central/annotate/1507f021ac93/layout/base/nsCSSFrameConstructor.cpp#l5062
looks different then the flexbox stuff in ConstructFramesFromItemList:
http://hg.mozilla.org/mozilla-central/annotate/1507f021ac93/layout/base/nsCSSFrameConstructor.cpp#l9173

Maybe Daniel remembers what the difference is and if they could be
replaced with something that works for both call sites?

Other than that, I think it should be OK - I'd expect the
CreateNeededTablePseudos to be a NOP from the other two call
sites.  The last assertion about mHavePendingPopupgroup
hopefully holds too.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> So, AFAICT, the patch is idempotent.

Er, right you are.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #3)
> The flexbox stuff in ConstructFrame:
> http://hg.mozilla.org/mozilla-central/annotate/1507f021ac93/layout/base/nsCSSFrameConstructor.cpp#l5062
> looks different then the flexbox stuff in ConstructFramesFromItemList:
> http://hg.mozilla.org/mozilla-central/annotate/1507f021ac93/layout/base/nsCSSFrameConstructor.cpp#l9173
> 
> Maybe Daniel remembers what the difference is and if they could be
> replaced with something that works for both call sites?

The second one is easy to explain -- in ConstructFramesFromItemList(), if the items' parent is a flex container, we need to create anonymous flex items (around runs of text) as-needed. That's all.

The first one is a little more subtle. In ConstructFrame(), even if our parent *looks* like a flex container (i.e. has "display:flex"), we actually really know it's *NOT* a flex container, because ConstructFrame is only used for native anonymous content (e.g. scroll widgets), which should never be direct children of a flex container.

We can get into weird situations in ConstructFrame() if we have an element with a mandated frame-type (e.g. <legend>) that has "display:flex" -- we'll create a nsLegendFrame (before we hit ConstructFrame), but then when we go to create any anonymous child content (e.g. scroll widgets, if this legend has 'overflow: scroll'), then that anonymous child content will see its parent as having "display:flex", and its display type will get converted to "block" (in ApplyStyleFixups), and then we'll create the wrong type of children. This is problematic, since we never had a flex container in the first place, since <legend> has a mandated frame type.

To prevent this problem, we instantiate a AutoFlexItemStyleFixupSkipper, which suppresses the flexbox chunk in ApplyStyleFixups. (preventing us from blockifying the display values of the children)

> if they could be
> replaced with something that works for both call sites?

Possibly. We just need to...
 (a) make sure that ConstructFramesFromItemList's invocation of "CreateNeededAnonFlexItems" stays dependent on the *parent frame Type()*, not on its 'display' value. (since the 'display' value can't necessarily be trusted)

 (b) probably bump ConstructFrame()'s AutoFlexItemStyleFixupSkipper up one stack-level, to be instantiated in CreateAnonymousFrames instead, to leave ConstructFrame() free of flexbox special-cases.

(NOTE: In a draft patch, I originally instantiated the AutoFlexItemStyleFixupSkipper in CreateAnonymousFrames(), as I'm suggesting in (b) above, but bz requested that I make it more targeted and put it in ConstructFrame()  (since ConstructFrame is only used for native anonymous content anyway). See bug 812822 comment 32 and bug 812822 comment 34.  But if we're now wanting to merge ConstructFrame with some non-anonymous-content functionality, we should be able to move the AutoFlexItemStyleFixupSkipper back into CreateAnonymousFrames().)

Comment 6

[Daniel Holbert [:dholbert]]

> We can get into weird situations in ConstructFrame

er, I meant to say we *used to be able to* get into weird situations, as happened in bug 812822. (The AutoFlexItemStyleFixupSkipper should prevent us from getting into those situations now, though.)

Comment 7

[Mats Palmgren (inactive)]

Attachment: fix, v2

Thanks for the info Daniel.  I moved FlexItemStyleFixupSkipper as
suggested, and with Boris' suggestion to just calling
ConstructFramesFromItemList instead that made ConstructFrame
pretty pointless so I removed it.  I also moved the frame type
check to CreateNeededAnonFlexItems itself rather than doing that
at the call site, it seems more logical and more consistent with
CreateNeededTablePseudos.  (It also allowed a minor optimization
of not calling the virtual GetType() when the item list is empty)

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8382399 [details] [diff] [review]
fix, v2

>-                 "Needed pseudos didn't get created; expect bad things");
>+                 "This is not going to work");

I somewhat prefer the old assertion text here.

r=me

Comment 9

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc48f71b864e

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cc48f71b864e