1321698 - Mark -webkit-box flex containers with a frame state bit

Status: RESOLVED FIXED

Description

[Daniel Holbert [:dholbert]]

We'll soon be adding a bit more -webkit-box specific behavior in nsFlexContainerFrame, over in bug 1320484.

While thinking about that fix, I realized we should just add a state bit so that we can easily/quickly distinguish between the two flavors of nsFlexContainerFrame (real & -webkit-box-emulating).

Right now, we do this distinguishing via a static function, nsFlexContainerFrame::IsLegacyBox().  I'm filing this bug on replacing that with a frame state bit.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1321698 part 1: Set a frame state bit on nsFlexContainerFrame if it's emulating -webkit-box.

Review commit: https://reviewboard.mozilla.org/r/97150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97150/

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1321698 part 2: Use the new frame state bit to check for -webkit-box containers.

Note that at the callsites in nsCSSFrameConstructor.cpp, we have to also check
the frame type (since the frame state bit is in a range of bits whose meaning
differs depending on frame type).  The first change in this patch is the
addition of a convenience fucntion that checks both the frame type as well as
the frame state bit.

Review commit: https://reviewboard.mozilla.org/r/97152/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97152/

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1321698 part 3: Fold nsFlexContainerFrame's old IsLegacyBox() function into its only remaining caller, nsFlexContainerFrame::Init().

This patch is to prevent any new callers of this API, since we can now rely on
the new frame state bit (NS_STATE_FLEX_IS_LEGACY_WEBKIT_BOX) to test for this.

Review commit: https://reviewboard.mozilla.org/r/97154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97154/

Comment 4

[Daniel Holbert [:dholbert]]

Each of these patches should be idempotent (i.e. shouldn't modify behavior).

Comment 5

[Mats Palmgren]

Comment on attachment 8816403 [details]
Bug 1321698 part 1: Set a frame state bit on nsFlexContainerFrame if it's emulating -webkit-box.

https://reviewboard.mozilla.org/r/97150/#review97488

::: layout/generic/nsFlexContainerFrame.cpp:2288
(Diff revision 1)
> +                           nsIFrame*         aPrevInFlow)
> +{
> +  nsContainerFrame::Init(aContent, aParent, aPrevInFlow);
> +
> +  if (nsFlexContainerFrame::IsLegacyBox(this)) {
> +    // Toggle frame state bit to indicate that this frame represents a

minor nit: "Toggle" means to me: "take the current value of X, calculate the opposite/other value from that, then assign that to X".  So I think s/Toggle/Set a/ is probably a better description here.

Comment 6

[Mats Palmgren]

Nevermind, I see that the code comment is indeed changed to use "set a" in the last part.

Comment 7

[Mats Palmgren]

Comment on attachment 8816404 [details]
Bug 1321698 part 2: Use the new frame state bit to check for -webkit-box containers.

https://reviewboard.mozilla.org/r/97152/#review97504

::: layout/generic/nsFlexContainerFrame.cpp:1881
(Diff revision 1)
> -  if (IsLegacyBox(containerRS->mFrame)) {
> +  const bool isLegacyBox =
> +    containerRS->mFrame->HasAnyStateBits(NS_STATE_FLEX_IS_LEGACY_WEBKIT_BOX);

nit: I think we should keep the IsLegacyBox convenience function, just change it to check the frame bit instead, because:
1. the old code is more readable
2. the intent of the caller is clearer
3. it avoids a few line breaks

If you agree, then perhaps make it a static function in this file instead, and assert that aFrame->GetType() is nsGkAtoms::flexContainerFrame?

Comment 8

[Mats Palmgren]

Comment on attachment 8816405 [details]
Bug 1321698 part 3: Fold nsFlexContainerFrame's old IsLegacyBox() function into its only remaining caller, nsFlexContainerFrame::Init().

https://reviewboard.mozilla.org/r/97154/#review97508

::: layout/generic/nsFlexContainerFrame.cpp:2260
(Diff revision 1)
>                             nsContainerFrame* aParent,
>                             nsIFrame*         aPrevInFlow)
>  {
>    nsContainerFrame::Init(aContent, aParent, aPrevInFlow);
>  
> -  if (nsFlexContainerFrame::IsLegacyBox(this)) {
> +  const nsStyleDisplay* styleDisp = mStyleContext->StyleDisplay();

nit: s/mStyleContext/StyleContext()/

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8816404 [details]
Bug 1321698 part 2: Use the new frame state bit to check for -webkit-box containers.

https://reviewboard.mozilla.org/r/97152/#review97504

> nit: I think we should keep the IsLegacyBox convenience function, just change it to check the frame bit instead, because:
> 1. the old code is more readable
> 2. the intent of the caller is clearer
> 3. it avoids a few line breaks
> 
> If you agree, then perhaps make it a static function in this file instead, and assert that aFrame->GetType() is nsGkAtoms::flexContainerFrame?

Makes sense -- I'll do that. Thanks!

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8816404 [details]
Bug 1321698 part 2: Use the new frame state bit to check for -webkit-box containers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97152/diff/1-2/

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8816405 [details]
Bug 1321698 part 3: Fold nsFlexContainerFrame's old IsLegacyBox() function into its only remaining caller, nsFlexContainerFrame::Init().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97154/diff/1-2/

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8816405 [details]
Bug 1321698 part 3: Fold nsFlexContainerFrame's old IsLegacyBox() function into its only remaining caller, nsFlexContainerFrame::Init().

https://reviewboard.mozilla.org/r/97154/#review97556

::: layout/generic/nsFlexContainerFrame.cpp:2260
(Diff revision 1)
>                             nsContainerFrame* aParent,
>                             nsIFrame*         aPrevInFlow)
>  {
>    nsContainerFrame::Init(aContent, aParent, aPrevInFlow);
>  
> -  if (nsFlexContainerFrame::IsLegacyBox(this)) {
> +  const nsStyleDisplay* styleDisp = mStyleContext->StyleDisplay();

OK - fixed in the new version of this patch.

Comment 13

[Daniel Holbert [:dholbert]]

OK - so now part 2 rewrites nsFlexContainerFrame like so:
 - Renames IsLegacyBox to IsLegacyBoxFOLD_ME() (doomed to be removed in the subsequent patch)
 - Creates a new (file-scoped) static function IsLegacyBox() which checks the frame state bit and asserts about the frame type.
 - Leaves most callers untouched -- they can still just call IsLegacyBox and benefit from the new version.  (I drop the "nsFlexContainerFrame::" prefix from the ones that had that, since it's a file-scoped static function now though.)

And as before, Part 3 is just a code-move, moving the contents of "IsLegacyBoxFOLD_ME()" into "Init()"

Not sure if you wanted to take another look after these changes (and MozReview won't let me re-request review anyway after r+ has been granted), so I'll just tag you for needinfo to be sure you're OK with these changes. :)  (I'm OK proceeding without additional review, but I don't want to be presumptive.)

Comment 14

[Mats Palmgren]

Comment on attachment 8816404 [details]
Bug 1321698 part 2: Use the new frame state bit to check for -webkit-box containers.

https://reviewboard.mozilla.org/r/97152/#review97564

::: layout/generic/nsFlexContainerFrame.cpp:2299
(Diff revision 2)
>                             nsContainerFrame* aParent,
>                             nsIFrame*         aPrevInFlow)
>  {
>    nsContainerFrame::Init(aContent, aParent, aPrevInFlow);
>  
> -  if (nsFlexContainerFrame::IsLegacyBox(this)) {
> +  if (IsLegacyBoxFOLD_ME(this)) {

Maybe you meant to remove the FOLD_ME before part 3?  Anyway, not super-important to fix unless you feel like it.

Otherwise, this looks good to me.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #14)
> Maybe you meant to remove the FOLD_ME before part 3?  Anyway, not
> super-important to fix unless you feel like it.

Nope, this is as-intended.

- Part 2 renames the old impl to a dummy name (with FOLD_ME) so we can swap in a new impl, and adds a single legitimate callsite of the old impl (in ::Init).
- Part 3 folds the old-impl into that one allowed callsite.

Thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15f43157a6e1
part 1: Set a frame state bit on nsFlexContainerFrame if it's emulating -webkit-box. r=mats
https://hg.mozilla.org/integration/autoland/rev/e38ccf521062
part 2: Use the new frame state bit to check for -webkit-box containers. r=mats
https://hg.mozilla.org/integration/autoland/rev/eeb34d6bc82c
part 3: Fold nsFlexContainerFrame's old IsLegacyBox() function into its only remaining caller, nsFlexContainerFrame::Init(). r=mats

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/15f43157a6e1
https://hg.mozilla.org/mozilla-central/rev/e38ccf521062
https://hg.mozilla.org/mozilla-central/rev/eeb34d6bc82c

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8816403 [details]
Bug 1321698 part 1: Set a frame state bit on nsFlexContainerFrame if it's emulating -webkit-box.

Requesting uplift approval to uplift the patches here to Aurora, since they're needed for bug 1320484 (which is what I'd really like to uplift).

Approval Request Comment
[Feature/Bug causing the regression]: N/A - this bug just doing some non-functional refactoring, in support of bug 1320484 (which I also want to uplift & which is a regression).
[User impact if declined]: We wouldn't be able to take bug 1320484 which means we'd regress our layout on some sites that use display:-webkit-box.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: N/A - this bug's changes should not change behavior in an observable way. (But these changes have been shipping in Nightly for a month, FWIW.)
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This fix isn't intended to change behavior at all -- it's just providing a lower-cost way to test for -webkit-box containers during layout.
[String changes made/needed]: None.

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 8816403 [details]
Bug 1321698 part 1: Set a frame state bit on nsFlexContainerFrame if it's emulating -webkit-box.

refactoring for -webkit-box handling, prerequisite for bug 1320484, aurora52+

Comment 20

[Daniel Holbert [:dholbert]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/181a7b3a8c42e00c34f875a5def13cab8f9faccc
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/bd10e45827131158e9dd3948396d8859b1674fad
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ae40c9ff8660cbe0239161cff7ec3db1faa4c468