Closed Bug 1155772 Opened 8 years ago Closed 3 years ago

Convert flex item margin/border/padding computations to use logical axes

Categories

(Core :: Layout: Flexbox, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox40 --- wontfix
firefox74 --- fixed

People

(Reporter: dholbert, Assigned: TYLin)

References

(Blocks 2 open bugs)

Details

Attachments

(16 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Bug 1152913 left behind physical axes in otherwise-logical structs FlexboxAxisTracker & PositionTracker, for use in some margin/border/padding computations.

I'm filing this bug on converting these computations to use logical axes & LogicalMargin, so that we don't need to know the physical axes in these structs anymore.

I look at this a bit, I think we'll need to

  1. Change FlexItem::{mBorderPadding,mMargin} to store in LogicalMargin. I'm not sure which writing-mode we should use to store the LogicalMargin . It's straitforward to use flex item's writing-mode because it is from ReflowInput, but flex algorithm seems need to use flex container's writing-mode most of the cases.
  2. Eliminate AxisOrientationType and FlexboxAxisTracker::{GetMainAxis(),GetCrossAxis()}, and use FlexboxAxisTracker::{IsRowOriented(),IsColumnOriented} instead.
  3. Search for XXXdholbert [BEGIN DEPRECATED], and delete those code.

Did I miss anything?

Flags: needinfo?(dholbert)

(In reply to Ting-Yu Lin [:TYLin] (UTC-8) (Traveling Nov 23 - Dec 15) from comment #1)

I look at this a bit, I think we'll need to

  1. Change FlexItem::{mBorderPadding,mMargin} to store in LogicalMargin. I'm not sure which writing-mode we should use to store the LogicalMargin . It's straitforward to use flex item's writing-mode because it is from ReflowInput, but flex algorithm seems need to use flex container's writing-mode most of the cases.

Yeah, we should pick whichever is more convenient in terms of usage. I suspect the flex container's WM is the more commonly-needed one. This will involve minor tweaks/rewriting of a bunch of small APIs, I think, but hopefully they're all pretty straightforward.

  1. Eliminate AxisOrientationType and FlexboxAxisTracker::{GetMainAxis(),GetCrossAxis()}, and use `FlexboxAxisTracker::{IsRowOriented(),IsColumnOriented} instead.

(Also the kAxisOrientationToSidesMap struct, which is indexed in part by AxisOrientationType and is currently only used for physical-margin/border-side related stuff, I believe. Probably all/most of its usages will be removed in (1).)

  1. Search for XXXdholbert [BEGIN DEPRECATED], and delete those code.

Yeah -- I think that should cover it for this bug! Thanks for taking a look.

Flags: needinfo?(dholbert)

Fix "warning: deleted member function should be public", which can be
reproduced by
./mach static-analysis check layout/generic/nsFlexContainerFrame.cpp

If a private delete method is called, the compiler shows two error
messages: the method is deleted, and a private methods is called. By
declaring the method as public, only the former message is shown, which
should be more readable.

Assignee: nobody → aethanyc
Status: NEW → ASSIGNED

I'm going to add logical accessors and variables in the next patch, so
it's better the rename the old ones first.

Depends on D59043

This includes renaming aAxis to aPhysicalAxis in PositionTracker's
constructor.

Depends on D59045

These new added member variables are going to be used in the following
parts that convert border/padding/margin to use logical layout, so they
are added to the place prior to mBorderPadding and mMargin.

Also, move flex item's own writing-mode mWM for readability.

For "strut" constructor, we can use the default values for mMainAxis,
mCrossAxis, and mIsInlineAxisMainAxis initialized in the class
declaration.

Depends on D59047

Also, use GetPhysicalMargin() instead of mMargin in
FlexItem::GetBaselineOffsetFromOuterCrossEdge() to ease the transition
when converting FlexItem::mMargin to LogicalMargin in the later parts.

Depends on D59049

Note that I return LogicalMargin by value for GetMargin(). It is
considered a suboptimal practice to return a constant reference to a
member variable. We won't gain performance by returning a reference
because the compiler can optimize the return value. If it were assigned
to a new variable, the copy/move constructor is called anyway.

It can also break encapsulation if the caller removes the const
deliberately by something like

    const_cast<LogicalMargin&>(item->GetMargin()) = LogicalMargin(...);

As noted in FIXME(TYLin), the AxisOrientationType argument in
GetMarginBorderPaddingSizeInMainAxis() and many other methods can be
removed. However, to make the patch size manageable, I'll remove them in
a later part.

Depends on D59050

Also, remove unused FlexboxAxisTracker argument from
ResolveStretchedCrossSize().

Depends on D59052

The original physical axis AxisOrientationType contains direction
information, which can be replaced by physical start side
straightforwardly.

Depends on D59054

Attachment #9119260 - Attachment description: Bug 1155772 Part 6 - Cache flex container's writing-mode, main / cross axis in FlexItem. r=mats → Bug 1155772 Part 6 - Cache flex container's writing-mode and main axis in FlexItem. r=mats
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7677362c44b3
Part 1 - Move delete methods to public section for various flex classes. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/11f020664664
Part 2 - Add "Physical" to main/cross axis accessor and variable names in FlexboxAxisTracker (idempotent patch). r=mats
https://hg.mozilla.org/integration/autoland/rev/ce108df4d23c
Part 3 - Add logical main/cross axis accessors and variables to FlexboxAxisTracker. r=mats
https://hg.mozilla.org/integration/autoland/rev/abc7f4471cbf
Part 4 - Add "Physical" to the axis accessor and variable names in PositionTracker (idempotent patch). r=mats
https://hg.mozilla.org/integration/autoland/rev/b3b3dfa8ac4e
Part 5 - Cache flex container's logical axis and writing-mode in PositionTracker. r=mats
https://hg.mozilla.org/integration/autoland/rev/32f209c8856c
Part 6 - Cache flex container's writing-mode and main axis in FlexItem. r=mats
https://hg.mozilla.org/integration/autoland/rev/83da4fc71357
Part 7 - Convert FlexItem::mBorderPadding from nsMargin to LogicalMargin. r=mats
https://hg.mozilla.org/integration/autoland/rev/7c92f5b6d190
Part 8 - Rename GetMargin() to GetPhysicalMargin() in FlexItem (idempotent patch). r=mats
https://hg.mozilla.org/integration/autoland/rev/981e290de2af
Part 9 - Convert FlexItem::mMargin to LogicalMargin. r=mats
https://hg.mozilla.org/integration/autoland/rev/6cc7c04bf567
Part 10 - Remove unused AxisOrientationType argument. r=mats
https://hg.mozilla.org/integration/autoland/rev/a5cb718ed15d
Part 11 - Convert FlexItem::GetNumAutoMarginsInAxis() to use logical axis. r=mats
https://hg.mozilla.org/integration/autoland/rev/ad2edd6baa2f
Part 12 - Use LogicalMargin for EnterMargin / ExitMargin in PositionTracker. r=mats
https://hg.mozilla.org/integration/autoland/rev/3cd8558625a2
Part 13 - Add start side and end side accessors in FlexboxAxisTracker, and use them to replace AxisOrientationType usages. r=mats
https://hg.mozilla.org/integration/autoland/rev/1e9c57134aae
Part 14 - Remove physical axis usage from GetBaselineOffsetFromOuterCrossEdge(). r=mats
https://hg.mozilla.org/integration/autoland/rev/2a2e5970cf88
Part 15 - Remove mPhysicalAxis from PositionTracker. r=mats
https://hg.mozilla.org/integration/autoland/rev/21ab70ca4496
Part 16 - Remove mPhysicalMainAxis and mPhysicalAxis from FlexboxAxisTracker, and all the other helper enums and functions. r=mats

Backed out for bustages on WritingModes.h

backout: https://hg.mozilla.org/integration/autoland/rev/5914beb11d5c24fc83bc1e2bfb28281b42facf9f

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=285822980&revision=21ab70ca44968f47755aa1bcf6d309d927c43376&searchStr=build&group_state=expanded

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285822980&repo=autoland&lineNumber=43427

[task 2020-01-21T20:15:07.759Z] 20:15:07 INFO - In file included from /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:59:0,
[task 2020-01-21T20:15:07.759Z] 20:15:07 INFO - from /builds/worker/workspace/build/src/layout/generic/ScrollbarActivity.cpp:11,
[task 2020-01-21T20:15:07.759Z] 20:15:07 INFO - from Unified_cpp_layout_generic1.cpp:11:
[task 2020-01-21T20:15:07.759Z] 20:15:07 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/WritingModes.h: In member function 'nscoord& mozilla::LogicalMargin::Side(mozilla::LogicalSide, mozilla::WritingMode)':
[task 2020-01-21T20:15:07.759Z] 20:15:07 ERROR - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/WritingModes.h:1253:3: error: control reaches end of non-void function [-Werror=return-type]
[task 2020-01-21T20:15:07.759Z] 20:15:07 INFO - }
[task 2020-01-21T20:15:07.759Z] 20:15:07 INFO - ^
[task 2020-01-21T20:15:07.759Z] 20:15:07 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/WritingModes.h: In member function 'nscoord mozilla::LogicalMargin::Side(mozilla::LogicalSide, mozilla::WritingMode) const':
[task 2020-01-21T20:15:07.759Z] 20:15:07 ERROR - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/WritingModes.h:1241:3: error: control reaches end of non-void function [-Werror=return-type]

Flags: needinfo?(aethanyc)

OK. This is a build failure for warnings on only g++. For a code snippet like the following, clang is smart enough to know that we have handled all the cases in LogicalSide enum in the switch-statement, so the code flow never reaches after the switch, but g++ does not aware of this.

  nscoord Side(LogicalSide aSide, WritingMode aWM) const {
    switch (aSide) {
      case eLogicalSideBStart:
        return BStart(aWM);
      case eLogicalSideBEnd:
        return BEnd(aWM);
      case eLogicalSideIStart:
        return IStart(aWM);
      case eLogicalSideIEnd:
        return IEnd(aWM);
    }

Both Part 9 & Part 13 contain such switch statements that can trigger this kind of warnings.

I'll add an assertion and dumb return value at the end of the function.

    MOZ_ASSERT_UNREACHABLE("We should handle all sides!");
    return BStart(aWM);
Flags: needinfo?(aethanyc)
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8503dcdda2c4
Part 1 - Move delete methods to public section for various flex classes. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/c887483fa25b
Part 2 - Add "Physical" to main/cross axis accessor and variable names in FlexboxAxisTracker (idempotent patch). r=mats
https://hg.mozilla.org/integration/autoland/rev/075afaa25ee6
Part 3 - Add logical main/cross axis accessors and variables to FlexboxAxisTracker. r=mats
https://hg.mozilla.org/integration/autoland/rev/e42d916c6011
Part 4 - Add "Physical" to the axis accessor and variable names in PositionTracker (idempotent patch). r=mats
https://hg.mozilla.org/integration/autoland/rev/fd73bbfddf8c
Part 5 - Cache flex container's logical axis and writing-mode in PositionTracker. r=mats
https://hg.mozilla.org/integration/autoland/rev/2babcfb213c5
Part 6 - Cache flex container's writing-mode and main axis in FlexItem. r=mats
https://hg.mozilla.org/integration/autoland/rev/bfd7c1c2accc
Part 7 - Convert FlexItem::mBorderPadding from nsMargin to LogicalMargin. r=mats
https://hg.mozilla.org/integration/autoland/rev/ea81b8962a19
Part 8 - Rename GetMargin() to GetPhysicalMargin() in FlexItem (idempotent patch). r=mats
https://hg.mozilla.org/integration/autoland/rev/d03d935a66df
Part 9 - Convert FlexItem::mMargin to LogicalMargin. r=mats
https://hg.mozilla.org/integration/autoland/rev/d4e98d652bc3
Part 10 - Remove unused AxisOrientationType argument. r=mats
https://hg.mozilla.org/integration/autoland/rev/54e1f82768fd
Part 11 - Convert FlexItem::GetNumAutoMarginsInAxis() to use logical axis. r=mats
https://hg.mozilla.org/integration/autoland/rev/ba772f194d56
Part 12 - Use LogicalMargin for EnterMargin / ExitMargin in PositionTracker. r=mats
https://hg.mozilla.org/integration/autoland/rev/0e8361f9069e
Part 13 - Add start side and end side accessors in FlexboxAxisTracker, and use them to replace AxisOrientationType usages. r=mats
https://hg.mozilla.org/integration/autoland/rev/615c3a987954
Part 14 - Remove physical axis usage from GetBaselineOffsetFromOuterCrossEdge(). r=mats
https://hg.mozilla.org/integration/autoland/rev/38150b46f038
Part 15 - Remove mPhysicalAxis from PositionTracker. r=mats
https://hg.mozilla.org/integration/autoland/rev/d3fd7085836b
Part 16 - Remove mPhysicalMainAxis and mPhysicalAxis from FlexboxAxisTracker, and all the other helper enums and functions. r=mats
Regressions: 1613380
Component: Layout → Layout: Flexbox
You need to log in before you can comment on or make changes to this bug.