Closed Bug 1321698 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(3 files)

Bug 1321698 part 1: Set a frame state bit on nsFlexContainerFrame if it's emulating -webkit-box.
58 bytes, text/x-review-board-request
mats
: review+
jcristau
: approval-mozilla-aurora+
Details
Bug 1321698 part 2: Use the new frame state bit to check for -webkit-box containers.
58 bytes, text/x-review-board-request
mats
: review+
Details
Bug 1321698 part 3: Fold nsFlexContainerFrame's old IsLegacyBox() function into its only remaining caller, nsFlexContainerFrame::Init().
58 bytes, text/x-review-board-request
mats
: review+
Details 
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.
Each of these patches should be idempotent (i.e. shouldn't modify behavior).
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.
Attachment #8816403 - Flags: review?(mats) → review+
Nevermind, I see that the code comment is indeed changed to use "set a" in the last part.
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?
Attachment #8816404 - Flags: review?(mats) → review+
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()/
Attachment #8816405 - Flags: review?(mats) → review+
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 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.
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.)
Flags: needinfo?(mats)
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.
Flags: needinfo?(mats)
(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!
Pushed by dholbert@mozilla.com:
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 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.
Attachment #8816403 - Flags: approval-mozilla-aurora?
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+
Attachment #8816403 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.