Closed
Bug 1321698
Opened 4 years ago
Closed 4 years ago
Mark -webkit-box flex containers with a frame state bit
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(3 files)
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•4 years ago
|
||
Each of these patches should be idempotent (i.e. shouldn't modify behavior).
Comment 5•4 years ago
|
||
mozreview-review |
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+
Comment 6•4 years ago
|
||
Nevermind, I see that the code comment is indeed changed to use "set a" in the last part.
Comment 7•4 years ago
|
||
mozreview-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 ::: 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 8•4 years ago
|
||
mozreview-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+
Assignee | ||
Comment 9•4 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•4 years ago
|
||
mozreview-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/#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.
Assignee | ||
Comment 13•4 years ago
|
||
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 14•4 years ago
|
||
mozreview-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/#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.
Updated•4 years ago
|
Flags: needinfo?(mats)
Assignee | ||
Comment 15•4 years ago
|
||
(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•4 years ago
|
||
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 17•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/15f43157a6e1 https://hg.mozilla.org/mozilla-central/rev/e38ccf521062 https://hg.mozilla.org/mozilla-central/rev/eeb34d6bc82c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Comment 18•4 years ago
|
||
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 19•4 years ago
|
||
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+
Assignee | ||
Comment 20•4 years ago
|
||
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
status-firefox52:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•