I would like a method that I can call for flexbox containers such a getFlexItems that will return the properties for the flex items: - order - flex-grow - flex-shrink - flex-basis - flex - align-self In addition, I would like the rect informatiom of flex items that can tell me the following: - position of the flex item - cross size (width or height of a flex item) I think this will evolve as I learn more about flexbox and their main/cross properties. I would also keep in mind that knowing the properties for the flex container is just as important if this is possible: - flex-direction - flex-wrap - flex-flow - justify-content - align-items - align-content

Comment on attachment 8919358 [details] Bug 1409083 Part 2: Stub webidl definitions to support flex container/item properties. https://reviewboard.mozilla.org/r/190212/#review195586 C/C++ static analysis found 2 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/flex/Flex.cpp:18 (Diff revision 2) > +namespace dom { > + > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(Flex, mParent) > +NS_IMPL_CYCLE_COLLECTING_ADDREF(Flex) > +NS_IMPL_CYCLE_COLLECTING_RELEASE(Flex) > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Flex) Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return] ::: dom/flex/Flex.cpp:31 (Diff revision 2) > +{ > + MOZ_ASSERT(aFrame, > + "Should never be instantiated with a null nsFlexContainerFrame"); > +} > + > +Flex::~Flex() Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

Comment on attachment 8919358 [details] Bug 1409083 Part 2: Stub webidl definitions to support flex container/item properties. https://reviewboard.mozilla.org/r/190212/#review195640 C/C++ static analysis found 2 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/flex/Flex.cpp:18 (Diff revision 2) > +namespace dom { > + > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(Flex, mParent) > +NS_IMPL_CYCLE_COLLECTING_ADDREF(Flex) > +NS_IMPL_CYCLE_COLLECTING_RELEASE(Flex) > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Flex) Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return] ::: dom/flex/Flex.cpp:31 (Diff revision 2) > +{ > + MOZ_ASSERT(aFrame, > + "Should never be instantiated with a null nsFlexContainerFrame"); > +} > + > +Flex::~Flex() Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

Comment on attachment 8919528 [details] Bug 1409083 Part 1: Capture computed flex data for use by devtools. https://reviewboard.mozilla.org/r/190364/#review195642 C/C++ static analysis found 2 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/flex/FlexItem.cpp:17 (Diff revision 1) > +namespace dom { > + > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(FlexItem, mParent) > +NS_IMPL_CYCLE_COLLECTING_ADDREF(FlexItem) > +NS_IMPL_CYCLE_COLLECTING_RELEASE(FlexItem) > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(FlexItem) Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return] ::: dom/flex/FlexLine.cpp:18 (Diff revision 1) > +namespace dom { > + > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(FlexLine, mParent, mItems) > +NS_IMPL_CYCLE_COLLECTING_ADDREF(FlexLine) > +NS_IMPL_CYCLE_COLLECTING_RELEASE(FlexLine) > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(FlexLine) Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment on attachment 8919358 [details] Bug 1409083 Part 2: Stub webidl definitions to support flex container/item properties. https://reviewboard.mozilla.org/r/190212/#review195646 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/flex/Flex.cpp:18 (Diff revision 3) > +namespace dom { > + > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(Flex, mParent) > +NS_IMPL_CYCLE_COLLECTING_ADDREF(Flex) > +NS_IMPL_CYCLE_COLLECTING_RELEASE(Flex) > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Flex) Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment on attachment 8919528 [details] Bug 1409083 Part 1: Capture computed flex data for use by devtools. https://reviewboard.mozilla.org/r/190364/#review195648 C/C++ static analysis found 2 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/flex/FlexItem.cpp:17 (Diff revision 2) > +namespace dom { > + > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(FlexItem, mParent) > +NS_IMPL_CYCLE_COLLECTING_ADDREF(FlexItem) > +NS_IMPL_CYCLE_COLLECTING_RELEASE(FlexItem) > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(FlexItem) Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return] ::: dom/flex/FlexLine.cpp:18 (Diff revision 2) > +namespace dom { > + > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(FlexLine, mParent, mItems) > +NS_IMPL_CYCLE_COLLECTING_ADDREF(FlexLine) > +NS_IMPL_CYCLE_COLLECTING_RELEASE(FlexLine) > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(FlexLine) Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Okay, still need tests, but I've built this out to provide a bunch of values I could find on flex lines and flex items that I'm confident are not already exposed as style data. They include: Flex Lines: * growing: did it stretch items (true) or shrink them? (false) * crossSize: useful if we ever wanted to visually draw lines independent of the items they contain (items may be offset, etc.) * firstBaselineOffset: measure from start of writing direction of distance to first baseline. * lastBaselineOffset: measure from start of writing direction of distance to last baseline. Flex Items: * weight: factor determining how line stretch/shrink amount was applied to this item, relative to siblings. * mainMinSize * mainMaxSize * crossMinSize * crossMaxSize I'll add tests. Now it's easy to add or change properties as needed. Gabe, any values you need but you can't get from these properties + style data?

Comment on attachment 8920739 [details] Bug 1409083 Part 4: Actually set values for exposed Flex properties. https://reviewboard.mozilla.org/r/191754/#review196962 C/C++ static analysis found 2 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/flex/Flex.cpp:38 (Diff revision 1) > + aFrame->GetFlexContainerInfo(); > + MOZ_ASSERT(containerInfo, "Should only be passed a frame with info."); > + > + LinkedList<nsFlexContainerFrame::FlexLine> lines; > + > + for (auto l = containerInfo->mLines.begin(); Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert] ::: dom/flex/FlexLine.cpp:37 (Diff revision 1) > + mGrowing = aLine->mGrowing; > + mCrossSize = aLine->mCrossSize; > + mFirstBaselineOffset = aLine->mFirstBaselineOffset; > + mLastBaselineOffset = aLine->mLastBaselineOffset; > + > + for (auto i = aLine->mItems.begin(); i != aLine->mItems.end(); ++i) { Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

Patrick and I agreed in conversation that it is necessary for the Flex Item structure to also container a pointer to the corresponding DOM node.

(In reply to Brad Werth [:bradwerth] from comment #14) > Flex Lines: > * growing: did it stretch items (true) or shrink them? (false) Lines without growing or shrinking have growing:true. > * lastBaselineOffset: measure from start of writing direction of distance to > last baseline. This is measured from the cross-end direction (from bottom up in left-to-right writing). > Flex Items: > * weight: factor determining how line stretch/shrink amount was applied to > this item, relative to siblings. I took this out; it was merely reporting the flex-grow value for lines that are growing, or flex-shrink * flex-basis for lines that are not growing. Since that value isn't seemingly relevant for overlays, it probably is better to rely on the three properties directly.

Comment on attachment 8919358 [details] Bug 1409083 Part 2: Stub webidl definitions to support flex container/item properties. https://reviewboard.mozilla.org/r/190212/#review205114 ::: dom/base/Element.cpp:3715 (Diff revision 7) > { > OwnerDoc()->RequestPointerLock(this, aCallerType); > } > > +Flex* > +Element::GetAsFlexContainer() Please make this return already_AddRefed<Flex> ::: dom/base/Element.cpp:3725 (Diff revision 7) > + // that annotated version of the frame. > + nsFlexContainerFrame* flexFrame = > + nsFlexContainerFrame::GetFlexFrameWithComputedInfo(frame); > + > + if (flexFrame) { > + return new Flex(this, flexFrame); and then here do something like RefPtr<Flex> flex = new .... return flex.forget(); ::: dom/flex/Flex.h:25 (Diff revision 7) > + > +class Flex : public nsISupports > + , public nsWrapperCache > +{ > +public: > + explicit Flex(nsISupports* aParent, nsFlexContainerFrame* aFrame); Just pass Element* aParent ::: dom/flex/Flex.cpp:26 (Diff revision 7) > + NS_INTERFACE_MAP_ENTRY(nsISupports) > +NS_INTERFACE_MAP_END > + > +Flex::Flex(nsISupports* aParent, > + nsFlexContainerFrame* aFrame) > + : mParent(do_QueryInterface(aParent)) ...Then you don't need this QI call ::: dom/flex/FlexItem.cpp:34 (Diff revision 7) > +{ > + return FlexItemBinding::Wrap(aCx, this, aGivenProto); > +} > + > +mozilla::dom::Element* > +FlexItem::Element() const Something missing here? I assume some other patch changes this ::: dom/webidl/Flex.webidl:45 (Diff revision 7) > +}; > + > +[ChromeOnly] > +interface FlexItem > +{ > + readonly attribute Element element; Is it guaranteed the element can't be null. If not, use Element? as the type. But if it is never null without ? is fine.

Comment on attachment 8919358 [details] Bug 1409083 Part 2: Stub webidl definitions to support flex container/item properties. https://reviewboard.mozilla.org/r/190212/#review205114 > Something missing here? I assume some other patch changes this Yeah, I didn't have you review the part where "real" values are returned. I figured you wanted to stay focused on the webidl changes and less on the implementation files. Next time I'll have you review both (and probably collapse the changesets together). > Is it guaranteed the element can't be null. If not, use Element? as the type. But if it is never null without ? is fine. The complete implementation always returns a non-null value.

Gabe is now a reviewer for tests; clearing the needinfo.

To visualize the flex lines, I am looking for the start position of the flex lines in the cross axis, and the crossSize will give us the end position of the lines. I would also like to expose more information about the flexibility of the flex items: - how much the item has grown or shrank - order - align-self

Comment on attachment 8925746 [details] Bug 1409083 Part 5: Add tests of new Flex API. Clearing review flags while patch is being updated to capture properties requested in comment 34.

(In reply to Gabriel [:gl] (ΦωΦ) from comment #34) > I would also like to expose more information about the flexibility of the > flex items: > - order I've confirmed that the FlexItems are added to the container in the order specified by their "order" property (then in order by their position in the source). So an effective "order" value can be calculated by iterating over lines and then items, incrementing by 1 for each item. Given this, I don't know that it's valuable to report an order property per item. The upcoming patches omit this property for this reason.

> incrementing by 1 for each item The 'order' specified by the author may not be consecutive. That said, I don't see why we need to report the 'order' value through this API. It can already be read using getComputedStyle(). (Ditto for 'align-self'.)

(In reply to Mats Palmgren (:mats) from comment #37) > > incrementing by 1 for each item > > The 'order' specified by the author may not be consecutive. I agree. My point is that whatever the computed order is, that's the order that the flex items are added to flex lines. The iterator at https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/layout/generic/nsFlexContainerFrame.cpp#3569 adds them in that order and is the source of truth for computed order. So if there's a need for an "effective order" then that can be derived by the order of the flex items themselves.

No, that's not how it works. If you have two items, with _specified values_ 1 and 333, then that's also the _computed values_. The _used values_ are also 1 and 333, not 1 and 2. (_underscored_ terms are CSS spec terms for clarity)

I'm sorry to create confusion here. I understand what you are saying and I agree. The value of the "order" property on the flex items can be retrieved directly using getComputedStyle() as you point out in comment 37. I believe Gabe also is clear on this. I have only been investigating if there is any utility in reporting an "effective order" of flex items in the container. It's possible this came from conversations with Gabe that aren't reflected here in the bug. My comment 36 is just pointing out that such an order is implicit in the order of the flex items themselves.

> My comment 36 is just pointing out that such an order is implicit in the order of the flex items themselves. Ah, now I see what you're saying. Hmm, is this what the user of this API wants though? It might make more sense to return flex items in the DOM order of the corresponding content... In any case, you probably want to change FlexItem::element from Element to Node, since items can also be created for text nodes, e.g. <div style="display:flex">a<item>b</item></div> In fact, a *single* item can be created for *multiple* DOM nodes, see nsCSSFrameConstructor::CreateNeededAnonFlexOrGridItems for details. Items can also be created for content not in the DOM, e.g. ::before/::after pseudo elements.

(In reply to Mats Palmgren (:mats) from comment #41) > In any case, you probably want to change FlexItem::element from Element > to Node, since items can also be created for text nodes, e.g. > <div style="display:flex">a<item>b</item></div> Yes, that was pointed out to me by :pbro. The approach I'm taking in the forthcoming patch is two members; one "element" and one "text". Only one of which will have a value. If that's considered an anti-pattern, then it's easy enough to have one Node member. I'll let Gabe comment on this as he reviews the tests. > In fact, a *single* item can be created for *multiple* DOM nodes, see > nsCSSFrameConstructor::CreateNeededAnonFlexOrGridItems for details. > > Items can also be created for content not in the DOM, e.g. ::before/::after > pseudo elements. I think the intended use of this API means both of these issues are not problems. The intent of the API is to have properties for all of the FlexItems, regardless of how they were generated. With the element/text properties, it's possible to correlate those FlexItems back to the content that generated them. I'll expand the tests to make these outcomes explicit.

Comment on attachment 8919528 [details] Bug 1409083 Part 1: Capture computed flex data for use by devtools. https://reviewboard.mozilla.org/r/190364/#review209036 ::: layout/generic/nsFlexContainerFrame.h:23 (Diff revision 8) > } // namespace mozilla > > nsContainerFrame* NS_NewFlexContainerFrame(nsIPresShell* aPresShell, > nsStyleContext* aContext); > > +struct ComputedFlexItemInfo Could you add a comment above these structs, to document their purpose? (One comment that covers all three is fine.) It should mention devtools, probably. ::: layout/generic/nsFlexContainerFrame.h:34 (Diff revision 8) > +struct ComputedFlexLineInfo > +{ > + bool mGrowing; > + nscoord mCrossSize; > + nscoord mFirstBaselineOffset; > + nscoord mLastBaselineOffset; > + nsTArray<ComputedFlexItemInfo> mItems; nit: we tend to place bools & other smaller objects at the end of structs, rather than the beginning, so that smaller things are more likely to pack into packing space left behind by medium-sized things before them. (And packing space tends to be be trailing rather than being awkwardly inserted in the middle of the struct.) So: - let's bump mItems to be the first member here. - let's bump mGrowing to be the last member here. That way, we've got a shot at having mGrowing pack nicely into the empty packing-space after the three 32-bit nscoord values (there'll be some packing space there, on 64-bit systems). ::: layout/generic/nsFlexContainerFrame.cpp:952 (Diff revision 8) > } > > // Runs the "Resolving Flexible Lengths" algorithm from section 9.7 of the > // CSS flexbox spec to distribute aFlexContainerMainSize among our flex items. > - void ResolveFlexibleLengths(nscoord aFlexContainerMainSize); > + void ResolveFlexibleLengths(nscoord aFlexContainerMainSize, > + ComputedFlexLineInfo *aLineInfo); Nit: shift the "*" to the left side of the space (to hug the type rather than the variable name) ::: layout/generic/nsFlexContainerFrame.cpp:2385 (Diff revision 8) > } > } > > void > -FlexLine::ResolveFlexibleLengths(nscoord aFlexContainerMainSize) > +FlexLine::ResolveFlexibleLengths(nscoord aFlexContainerMainSize, > + ComputedFlexLineInfo *aLineInfo) shift the "*" to the left. ::: layout/generic/nsFlexContainerFrame.cpp:2397 (Diff revision 8) > - if (mNumFrozenItems == mNumItems) { > + if ((mNumFrozenItems == mNumItems) && !aLineInfo) { > // All our items are frozen, so we have no flexible lengths to resolve. > return; Please add a comma somewhere to explain the aLineInfo usage here. e.g. maybe just extend this existing comment to say "(though if we're populating data for devtools, we need to proceed through all the items)" ::: layout/generic/nsFlexContainerFrame.cpp:2401 (Diff revision 8) > > - if (mNumFrozenItems == mNumItems) { > + if ((mNumFrozenItems == mNumItems) && !aLineInfo) { > // All our items are frozen, so we have no flexible lengths to resolve. > return; > } > - MOZ_ASSERT(!IsEmpty(), "empty lines should take the early-return above"); > + MOZ_ASSERT(!IsEmpty() || aLineInfo, "empty lines should take the early-return above"); Please wrap this line after the comma, to keep it under 80 characters. ::: layout/generic/nsFlexContainerFrame.cpp:4167 (Diff revision 8) > +nsFlexContainerFrame::GetFlexFrameWithComputedInfo(nsIFrame* aFrame) > +{ > + MOZ_ASSERT(aFrame); > + > + // Prepare a lambda function that we may need to call multiple times. > + auto GetFlexContainerFrame = [](nsIContent *aContent) { shift the "*" to the left ::: layout/generic/nsFlexContainerFrame.cpp:4168 (Diff revision 8) > + // Return the aContent primary frame, iff it is > + // a flex container. Nit: This comment could fit on one line -- no need for it to wrap halfway through. ::: layout/generic/nsFlexContainerFrame.cpp:4170 (Diff revision 8) > + nsFlexContainerFrame* flexFrame = nullptr; > + > + if (aContent) { > + nsIFrame* primaryFrame = aContent->GetPrimaryFrame(); > + if (primaryFrame && (primaryFrame->IsFlexContainerFrame())) { > + flexFrame = static_cast<nsFlexContainerFrame*>(primaryFrame); > + } > + } > + return flexFrame; If you like, you could simplify/shorten this a little by getting rid of flexFrame, and just having two separate return statements: return static_cast<nsFlexContainerFrame*>(primaryFrame); } } return nullptr; }; It's also fine as-is, though. ::: layout/generic/nsFlexContainerFrame.cpp:4174 (Diff revision 8) > + // a flex container. > + nsFlexContainerFrame* flexFrame = nullptr; > + > + if (aContent) { > + nsIFrame* primaryFrame = aContent->GetPrimaryFrame(); > + if (primaryFrame && (primaryFrame->IsFlexContainerFrame())) { Drop the unnecessary parens around (primaryFrame->IsFlexContainerFrame()) please. ::: layout/generic/nsFlexContainerFrame.cpp:4181 (Diff revision 8) > + } > + } > + return flexFrame; > + }; > + > + nsIContent* content = aFrame->GetContent(); "content" needs to be a nsCOMPtr<nsIContent> here. (otherwise it's possible that the object it's pointing to might've be destroyed by the time GetFlexContainerFrame(content) gets called a second time further down, after the layout flush -- since layout flushes can potentially trigger arbitrary script to run.) ::: layout/generic/nsFlexContainerFrame.cpp:4184 (Diff revision 8) > + // If any of our properties are missing, generate them. > + bool reflowNeeded = (!flexFrame->HasProperty(FlexContainerInfo())); "any of our properties" and "them" are a bit confusing here, since (a) easy to confuse with CSS properties, and (b) there's only one entry here -- it's not a plural thing. How about: // Generate the FlexContainerInfo data, if it's not already there. ::: layout/generic/nsFlexContainerFrame.cpp:4199 (Diff revision 8) > + // Assert the grid properties are present > + MOZ_ASSERT(!flexFrame || > + flexFrame->HasProperty(FlexContainerInfo())); "grid" is clearly a typo here. But really, the comment isn't so useful right now (it doesn't add much info on top of the MOZ_ASSERT statement itself). It should really be converted into a more self-justifying message in the MOZ_ASSERT itself, since messageless MOZ_ASSERTs are kinda evil. How about this, to make it more self-documenting (and make it produce a more-useful error message if it ever fails): MOZ_ASSERT(!flexFrame || flexFrame->HasProperty(FlexContainerInfo()), "The state bit should've made our forced-reflow " "generate a FlexContainerInfo object"); ::: layout/generic/nsFrameStateBits.h:326 (Diff revision 8) > // True if the container has no flex items; may lie if there is a pending reflow > FRAME_STATE_BIT(FlexContainer, 22, NS_STATE_FLEX_SYNTHESIZE_BASELINE) > > +// True iff computed flex values should be generated on the next reflow > +FRAME_STATE_BIT(FlexContainer, 23, NS_STATE_FLEX_GENERATE_COMPUTED_VALUES) Extreme nit: Could you insert this new bit as #22, and bump the existing SYNTHESIZE_BASELINE bit to be #23? That'll make these bits line up with grid, which would be nice for consistency & debuggability. (More-importantly, we'll have consistent ordering; and less-importantly, they'll actually have the same values, too.) https://dxr.mozilla.org/mozilla-central/rev/cad9c9573579698c223b4b6cb53ca723cd930ad2/layout/generic/nsFrameStateBits.h#343-347

Comment on attachment 8919528 [details] Bug 1409083 Part 1: Capture computed flex data for use by devtools. https://reviewboard.mozilla.org/r/190364/#review209166 ::: layout/generic/nsFlexContainerFrame.h:32 (Diff revision 9) > + * the structures are attached to the nsFlexContainerFrame via the > + * FlexContainerInfo property. > + */ > +struct ComputedFlexItemInfo > +{ > + nsINode* mNode; I'm somewhat uncomfortable with this as a raw nsINode* pointer here. Do we plan to dereference the pointer anywhere, or just use it for equality comparisons? If we dereference it, do we have a guarantee that the thing it's pointing to is still alive at that point? ::: layout/generic/nsFlexContainerFrame.h:173 (Diff revision 9) > + /** > + * Return a containing grid frame, and ensure it has computed grid info > + * @return nullptr if aFrame has no grid container, or frame was destroyed > + * @note this might destroy layout/style data since it may flush layout s/grid/flex/ (twice) Also, "returns nullptr if aFrame has no grid|flex container" is a bit too hand-wavy. It makes it sound like aFrame may or may not be a nsFlexContainerFrame. But really, in practice, it looks like it is always a nsFlexContainerFrame...? At least, the callsite in Part 2 has a "frame->IsFlexContainerFrame()" check before calling this method. Maybe this method should really take a nsFlexContainerFrame* as its type, to make that expectation clearer? (And in part 2, we can static_cast inside of the frame->IsFlexContainerFrame() check to satisfy that type-requirement). And this documentation needs an update to make the real reasons why we might return nullptr a bit clearer, because "has no flex container" is kinda meaningless if our arg is guaranteed to be a nsFlexContainerFrame... ::: layout/generic/nsFlexContainerFrame.cpp:4012 (Diff revision 9) > + if (HasAnyStateBits(NS_STATE_FLEX_GENERATE_COMPUTED_VALUES)) { > + // This state bit will never be cleared. That's acceptable because > + // it's only set in a Chrome API invoked by devtools, and won't > + // impact normal browsing. > + ComputedFlexContainerInfo* containerInfo = new ComputedFlexContainerInfo(); > + SetProperty(FlexContainerInfo(), containerInfo); > + } Perhaps we should check if we already have a ComputedFlexContainerInfo() struct and re-use it (clearing out its mLines) if it already exists? It seems wasteful to allocate a brand-new one every time, when most/much of the time we'll have an existing one sitting in our property table already. ::: layout/generic/nsFlexContainerFrame.cpp:4182 (Diff revision 9) > + nsCOMPtr<nsIContent> content = aFrame->GetContent(); > + nsFlexContainerFrame* flexFrame = GetFlexContainerFrame(content); > + if (flexFrame) { Why do we need to bother with calling the lambda here, when we already know that (a) aFrame is a nsFlexContainerFrame, and (b) it is the primary frame for its content? (based on how it's called in part 2's "Element::GetAsFlexContainer" function -- which seems do do the same thing that the lambda does) I think maybe this usage of the lambda is unnecessary...? Also, to the extent that we need the lambda's logic (after the FlushPendingNotifications call), perhaps we should share that logic with the similar logic in Part 2 "Element::GetAsFlexContainer"? (Also, does this handle the case of an element with "display:flex;overflow:scroll" correctly? I forget which frame is the primary frame -- the outer nsHTMLScrollFrame vs. the inner nsFlexContainerFrame -- but if we're going to add logic for that, I want to be sure we only have to add that logic in one place rather than in two places (i.e. I don't want to duplicate those checks in the lambda + Element::GetAsFlexContainer, hypothetically) ::: layout/generic/nsFlexContainerFrame.cpp:4248 (Diff revision 9) > + MOZ_ASSERT(containerInfo, "Reflow should have created container info struct."); > + MOZ_ASSERT(containerInfo->mLines.Length() == 0, "Shouldn't have lines yet."); > + > + for (const FlexLine* line = lines.getFirst(); line; line = line->getNext()) { These 3 lines are all longer than 80 characters and need to be wrapped somewhere. Also: - The first assert probably wants s/Reflow/::Reflow()/ in its message to make it clearer that it's talking about the specific reflow method on this here class, rather than the process of reflow (which this code here is part of) - The second assert could be shortened (and avoid the need to linewrap) by using IsEmpty() rather than Length() == 0. ::: layout/generic/nsFlexContainerFrame.cpp:4254 (Diff revision 9) > + // All of the lineInfo properties will be filled out at the end of this > + // function, when we have real values. But we still add all the items > + // here, so we can capture computed data for each item. > + for (const FlexItem* item = line->GetFirstItem(); item; > + item = item->getNext()) { > + nsIFrame* frame = item->Frame(); > + nsIContent* content = frame->GetContent(); > + if (content) { > + ComputedFlexItemInfo* itemInfo = lineInfo->mItems.AppendElement(); > + > + itemInfo->mNode = content; If we have some FlexItem that happens to have a null "GetContent()" result (for whatever reason), It looks like that'll means we'll skip that item when populating this ComputedFlexItemInfo array. So we might have fewer ComputedFlexItemInfo entries than flex items (and they point where they stop mapping directly might be buried somewhere in the middle). Is that OK? Or do we need there to be a 1:1 correspondence? (i.e. is there going to be code somewhere that iterates across all N of our FlexItems, and expects to have a ComputedFlexItemInfo entry for each one?) ::: layout/generic/nsFlexContainerFrame.cpp:4608 (Diff revision 9) > + lineInfo->mGrowing = line->GetTotalOuterHypotheticalMainSize() < > + aContentBoxMainSize; I'm not sure what this "mGrowing" member-var represents or why we're storing it... I think it might want to be renamed & get some documentation up in the struct definition. (Or perhaps removed?) Right now, it seems to be storing the result of the question: "Is the flex container's main-size larger than the sum of this flex line's base-sizes?" But I'm not sure why that's a useful piece of data to reason about at the granularity of a flex line, or to surface to devtools. In particular: - we might get mGrowing=true but have our flex items all have "flex:none". In that case, no items are growing and "mGrowing=true" is kind of misleading. - From a symmetry perspective, if we're storing some sort of "is this line growing" bool, it seems like we'd also want to be able to differentiate the not-growing vs. actively-shrinking states (e.g. if GetTotalOuterHypotheticalMainSize() is *greater* than aContentBoxMainSize)

Comment on attachment 8919528 [details] Bug 1409083 Part 1: Capture computed flex data for use by devtools. https://reviewboard.mozilla.org/r/190364/#review209480 ::: layout/generic/nsFlexContainerFrame.h:32 (Diff revision 9) > + * the structures are attached to the nsFlexContainerFrame via the > + * FlexContainerInfo property. > + */ > +struct ComputedFlexItemInfo > +{ > + nsINode* mNode; Changed it to an nsCOMPtr to address these points. ::: layout/generic/nsFlexContainerFrame.cpp:4182 (Diff revision 9) > + nsCOMPtr<nsIContent> content = aFrame->GetContent(); > + nsFlexContainerFrame* flexFrame = GetFlexContainerFrame(content); > + if (flexFrame) { I've significantly reworked this function to eliminate the lambda, and to not store a pointer to the content when we don't need it. I'll update the tests to confirm the "display:flex;overflow:scroll" case. ::: layout/generic/nsFlexContainerFrame.cpp:4254 (Diff revision 9) > + // All of the lineInfo properties will be filled out at the end of this > + // function, when we have real values. But we still add all the items > + // here, so we can capture computed data for each item. > + for (const FlexItem* item = line->GetFirstItem(); item; > + item = item->getNext()) { > + nsIFrame* frame = item->Frame(); > + nsIContent* content = frame->GetContent(); > + if (content) { > + ComputedFlexItemInfo* itemInfo = lineInfo->mItems.AppendElement(); > + > + itemInfo->mNode = content; Good point; I've changed this to always create a ComputedFlexItemInfo for each FlexItem. If there is no content, then the node will be null. ::: layout/generic/nsFlexContainerFrame.cpp:4608 (Diff revision 9) > + lineInfo->mGrowing = line->GetTotalOuterHypotheticalMainSize() < > + aContentBoxMainSize; This value really means "not shrinking". I captured it initially because it was necessary to support the capture of the "weight" of the flex item via GetWeight(), which has a parameter aIsUsingFlexGrow. That parameter informed the name of this value as "growing". Now, the weight value became less interesting once we started capturing size deltas per item -- who cares about the numeric weight when we can skip to the results? I have so far kept growing in, in case devtools wants to do any overlays on whole lines that have shrunken items (and doesn't want to iterate the items to determine this). What I'd like to do is raise this issue to gabe and have him evaluate it in the context of the tests. The tests use it to make some sanity checks of what is heppening to items on the line. If we don't need shrinking line overlays, then I'll remove the property entirely. If we need both shrinking and growing line overlays, I'll make this an enum property instead.

(In reply to Brad Werth [:bradwerth] from comment #69) > > Now, the weight value became less interesting once we started capturing size > deltas per item -- who cares about the numeric weight when we can skip to > the results? I have so far kept growing in, in case devtools wants to do any > overlays on whole lines that have shrunken items (and doesn't want to > iterate the items to determine this). > > What I'd like to do is raise this issue to gabe and have him evaluate it in > the context of the tests. The tests use it to make some sanity checks of > what is heppening to items on the line. If we don't need shrinking line > overlays, then I'll remove the property entirely. If we need both shrinking > and growing line overlays, I'll make this an enum property instead. Gabe, as you review the tests, please make a determination if the FlexLine.growing property is useful to devtools, presumably for the purposes of visually highlighting flex lines that have changed size of their items. If it is useful, I'll change the property to an enum for three-state values (shrinking, unchanged, growing). If it's not useful, I'll remove it.

Comment on attachment 8919528 [details] Bug 1409083 Part 1: Capture computed flex data for use by devtools. Clearing review flags while fixing support for "overflow:scroll" flex elements.

(In reply to Brad Werth [:bradwerth] from comment #70) > (In reply to Brad Werth [:bradwerth] from comment #69) > > > > Now, the weight value became less interesting once we started capturing size > > deltas per item -- who cares about the numeric weight when we can skip to > > the results? I have so far kept growing in, in case devtools wants to do any > > overlays on whole lines that have shrunken items (and doesn't want to > > iterate the items to determine this). > > > > What I'd like to do is raise this issue to gabe and have him evaluate it in > > the context of the tests. The tests use it to make some sanity checks of > > what is heppening to items on the line. If we don't need shrinking line > > overlays, then I'll remove the property entirely. If we need both shrinking > > and growing line overlays, I'll make this an enum property instead. > > Gabe, as you review the tests, please make a determination if the > FlexLine.growing property is useful to devtools, presumably for the purposes > of visually highlighting flex lines that have changed size of their items. > If it is useful, I'll change the property to an enum for three-state values > (shrinking, unchanged, growing). If it's not useful, I'll remove it. Yes, I am definitely looking for this 3-state value for FlexLines. In terms of my priorities, it is to get the FlexLine properties first - growing, crossStart, and crossSize. I would like to prototype around the FlexLine visualization first and work on FlexItems afterwards.

Comment on attachment 8925746 [details] Bug 1409083 Part 5: Add tests of new Flex API. https://reviewboard.mozilla.org/r/196892/#review210772

Comment on attachment 8919528 [details] Bug 1409083 Part 1: Capture computed flex data for use by devtools. https://reviewboard.mozilla.org/r/190364/#review211172 r=me with nits: ::: layout/generic/nsFlexContainerFrame.h:33 (Diff revision 11) > + * FlexContainerInfo property. > + */ > +struct ComputedFlexItemInfo > +{ > + nsCOMPtr<nsINode> mNode; > + nscoord mMainBaseSize; Please either rename this to mFlexBaseSize (which AFAICT is the spec-concept that it represents) -- or if you'd rather keep the main prefix, maybe just add a comment clarifying: // This is what the spec calls "flex base size" ::: layout/generic/nsFlexContainerFrame.h:165 (Diff revision 11) > + /** > + * These properties are created by a call to > + * nsFlexContainerFrame::GetFlexFrameWithComputedInfo. > + */ > + NS_DECLARE_FRAME_PROPERTY_DELETABLE(FlexContainerInfo, ComputedFlexContainerInfo) > + const ComputedFlexContainerInfo* GetFlexContainerInfo() > + { > + const ComputedFlexContainerInfo* info = GetProperty(FlexContainerInfo()); > + MOZ_ASSERT(info, "Property generation wasn't requested."); > + return info; > + } Two things: (1) s/These properties are/This property is/ (There's only one, AFAICT) (2) Please add a second sentence to emphasize *under what conditions we can be sure it's safe to call this function on a given nsFlexContainerFrame*. I think really, it's only ever valid to call this on a flex container that has *just been returned to us* by a call to GetFlexFrameWithComputedInfo, right? (At least, that's the use-case that we're expecting & intending to support?) And if that's the case, I wonder if this getter should just be folded into GetFlexFrameWithComputedInfo, to be returned as an outparam... (I'm assuming every GetFlexFrameWithComputedInfo caller will *also* want to request the computeddata right away.) I'm slightly uneasy with the current setup because it feels like a bit of a footgun to have this public API on nsFlexContainerFrame, which will fatally assert if someone comes along and calls it on a nsFlexContainerFrame that they happen to be working with. Anyway -- for now, perhaps best to just document that it's only to be called on the result of a GetFlexFrameWithComputedInfo() call (if that's the right restriction), and then down the line we can perhaps fold the two APIs together if that's appropriate? ::: layout/generic/nsFlexContainerFrame.cpp:2626 (Diff revision 11) > + nscoord deltaSize = item->GetMainSize() - > + aLineInfo->mItems[itemIndex].mMainBaseSize; Please add a comment here, noting that this is only the delta to the *tentative flexed size* for this item, in this particular trial-run of the layout algorithm. In particular: - this item's size may be clamped+frozen to a different size (unrelated to this delta) in the code below, if it has a min/max violations. - or, this item may be re-flexed and this delta may change, if some other element gets frozen and we have to restart this layout algorithm. ::: layout/generic/nsFlexContainerFrame.cpp:2630 (Diff revision 11) > + // If any item on the line is growing, mark the aLineInfo > + // structure; likewise if any item is shrinking. Items in > + // a line can't be both growing and shrinking. > + if (deltaSize > 0) { > + MOZ_ASSERT(aLineInfo->mGrowthState != > + ComputedFlexLineInfo::GrowthState::SHRINKING); Note: there's some overlap between this state and the "isUsingFlexGrow" boolean (local variable declared way at the top of this function), aside from the fact that your version is persistent & slightly more expressive with the third "unchanged" default state. Could you please add an additional assertion here, to document/enforce that overlap? i.e. in the first case, add: MOZ_ASSERT(isUsingFlexGrow, "only way to get positive delta is with flex-grow"); ...and in the second case: MOZ_ASSERT(isUsingFlexGrow, "only way to get negative delta is with flex-shrink"); ::: layout/generic/nsFlexContainerFrame.cpp:4207 (Diff revision 11) > + }; > + > + nsFlexContainerFrame* flexFrame = GetFlexContainerFrame(aFrame); > + if (flexFrame) { > + // Generate the FlexContainerInfo data, if it's not already there. > + bool reflowNeeded = (!flexFrame->HasProperty(FlexContainerInfo())); (The outer parens seem unnecessary here; but I'm OK keeping them if you prefer to have them for some stylistic reason.) ::: layout/generic/nsFlexContainerFrame.cpp:4279 (Diff revision 11) > + MOZ_ASSERT(containerInfo->mLines.IsEmpty(), > + "Shouldn't have lines yet."); This latter assertion isn't necessarily valid. But we can make it so! :) Here's why it's not valid yet: it's possible for Reflow to call DoFlexReflow twice (specifically, if we've got some flex items with visibility:collapse, which prompt us to layout twice, and have these items collapsed to "struts" on the second run). And on the second run, containerInfo->mLines *will not be empty* with your current patch. So, let's make it empty. Something like: if (!aStruts.IsEmpty()) { // We restarted DoFlexLayout, and may have stale mLines to clear: containerInfo->mLines.Clear(); } else { MOZ_ASSERT(containerInfo->mLines.IsEmpty(), "Shouldn't have lines yet."); } ::: layout/generic/nsFlexContainerFrame.cpp:4637 (Diff revision 11) > FinishReflowWithAbsoluteFrames(aPresContext, aDesiredSize, > aReflowInput, aStatus); > > NS_FRAME_SET_TRUNCATION(aStatus, aReflowInput, aDesiredSize) > + > + // Finally update our line properties in our containerInfo. s/properties/sizing results/ perhaps? Or "member-vars"? ("properties" is a very overloaded term in layout, and this doesn't seem to be using one of the common meanings -- css properties + frame properties.)

Comment on attachment 8931178 [details] Bug 1409083 Part 3: Add a GetFirstNonAnonBoxDescendant function to unpack anonymous flex items. https://reviewboard.mozilla.org/r/202260/#review211186 ::: commit-message-e5d69:1 (Diff revision 5) > +Bug 1409083 Part 3: Add a GetFirstNonAnonBoxDescendant function to unpack anonymous flex items. Please add a second line to the commit message, saying e.g.: "This patch is effectively just resurrecting a function that was previously removed in https://hg.mozilla.org/mozilla-central/rev/173a4f49dfe3#l1.96 " (That'll help in case someone wants to find out hg blame for this code -- they'll have a hint at how to proceed past this changeset to get to the older version) ::: layout/generic/nsFlexContainerFrame.cpp:183 (Diff revision 5) > + if (aFrame->IsTableWrapperFrame()) { > + nsIFrame* captionDescendant = > + GetFirstNonAnonBoxDescendant(aFrame->GetChildList(kCaptionList).FirstChild()); > + if (captionDescendant) { > + return captionDescendant; > + } > + } else if (aFrame->IsTableFrame()) { Please bring back the MOZ_UNLIKELY that we had in the old version of this code ( https://hg.mozilla.org/mozilla-central/rev/173a4f49dfe3#l1.119 ), on the IsTableWrapperFrame and IsTableFrame checks. Two reasons: - that makes it a more direct restoration of the deleted code - we *do know* that tables & table-wrappers are uncommon here, per the "SPECIAL CASE" / "USUAL CASE" comments. We'd like to help the compiler optimize for the usual case.

Comment on attachment 8920739 [details] Bug 1409083 Part 4: Actually set values for exposed Flex properties. https://reviewboard.mozilla.org/r/191754/#review211190 r=me with nits addressed, assuming you move the nsFlexContainerFrame.cpp chunk to another patch (e.g. part 3 or part 3.5) ::: dom/flex/Flex.cpp:37 (Diff revision 11) > + for (auto&& l : containerInfo->mLines) { > + FlexLine* line = new FlexLine(this, &l); > + mLines.AppendElement(line); Since you know the line-count up-front here, let's pre-allocate enough space in mLines, by doing something like the following before you append anything to it: mLines->EnsureCapacity(containerInfo->mLines.Length()); That way we don't have to rely on AppendElement doing lazy-allocation and buffer-copying. ::: dom/flex/FlexItem.h:11 (Diff revision 11) > > #ifndef mozilla_dom_FlexItem_h > #define mozilla_dom_FlexItem_h > > #include "mozilla/dom/FlexBinding.h" > +#include "nsFlexContainerFrame.h" I don't think you need this #include. It looks like you added it to provide the definition for ComputedFlexItemInfo, but I don't think you need the definition since we only have ComputedFlexItemInfo* as an arg here. I think you can just forward-declare that type -- inside of "namespace mozilla {", just before "namespace dom {". ::: dom/flex/FlexItem.h:51 (Diff revision 11) > double CrossMaxSize() const; > > protected: > RefPtr<FlexLine> mParent; > + RefPtr<nsINode> mNode; > + Whitespace on blank line. ::: dom/flex/FlexItem.h:52 (Diff revision 11) > + double mMainBaseSize; > + double mMainDeltaSize; > + double mMainMinSize; > + double mMainMaxSize; > + double mCrossMinSize; > + double mCrossMaxSize; Could you add a comment before these, noting that they're in units of CSS Pixels? (since their analogous members in layout are in a different coordinate space) ::: dom/flex/FlexItem.cpp:22 (Diff revision 11) > -FlexItem::FlexItem(FlexLine* aParent) > +FlexItem::FlexItem(FlexLine* aParent, > + const ComputedFlexItemInfo* aItem) > : mParent(aParent) > { This is kind of a nit on a different patch (part 2 maybe?), but -- "mParent"/"aParent" feels like it's a wrong name here. The real "parent" of a flex item would generally be its DOM-tree or frame-tree parent. So I think these might want to be named aLine / mLine, perhaps? This can be fixed after-the-fact, though -- no need to bitrot yourself -- so maybe make a note-to-self to do that renaming in a followup, if you agree. ::: dom/flex/FlexItem.cpp:29 (Diff revision 11) > + // Eagerly create property values from aItem, because we're not > + // going to keep it around. > + mNode = aItem->mNode; s/create property values/copy member-data/, or similar. (since this code isn't really "creating" anything -- no allocation/constructors are happening -- and no CSS/frame/DOM properties are involved here) ::: dom/flex/FlexItem.cpp:60 (Diff revision 11) > double > FlexItem::MainBaseSize() const > { > - return 0; > + return mMainBaseSize; > } As in part 1, this might want to be named "mFlexBaseSize" / GetFlexBaseSize. Or at least, if you don't rename it, we should document it (in the .h file) with a mention of the term "flex base size", to make it clearer that this getter's result maps directly to that concept from the spec. ::: dom/flex/FlexLine.h:11 (Diff revision 11) > > #ifndef mozilla_dom_FlexLine_h > #define mozilla_dom_FlexLine_h > > #include "mozilla/dom/FlexBinding.h" > +#include "nsFlexContainerFrame.h" As above, please replace this #include with a forward-decl for ComputedFlexLineInfo. ::: dom/flex/FlexLine.cpp:30 (Diff revision 11) > : mParent(aParent) > { > + MOZ_ASSERT(aLine, > + "Should never be instantiated with a null ComputedFlexLineInfo."); > + > + // Eagerly create property values from aLine, because we're not As above, s/create property values/copy member-data/ ::: dom/flex/FlexLine.cpp:45 (Diff revision 11) > + // Convert all the app unit values into css pixels. > + mCrossSize = nsPresContext::AppUnitsToDoubleCSSPixels( > + aLine->mCrossSize); > + mFirstBaselineOffset = nsPresContext::AppUnitsToDoubleCSSPixels( > + aLine->mFirstBaselineOffset); > + mLastBaselineOffset = nsPresContext::AppUnitsToDoubleCSSPixels( > + aLine->mLastBaselineOffset); I'd prefer that we do trivial assignments like this in the init list (and then mark the member-var as 'const' so that it's easier to reason about its lifetime & immutability), but this is OK too. ::: dom/flex/FlexLine.cpp:53 (Diff revision 11) > + for (auto&& i : aLine->mItems) { > + FlexItem* item = new FlexItem(this, &i); > + mItems.AppendElement(item); Use EnsureCapacity to make mItems preallocate enough space for all the items here. ::: layout/generic/nsFlexContainerFrame.cpp:4337 (Diff revision 11) > // add all the items here, so we can capture computed data for > // each item. > for (const FlexItem* item = line->GetFirstItem(); item; > item = item->getNext()) { > + nsIFrame* frame = item->Frame(); > + > + // The frame may be for an element, or it may be for an This whole chunk (the nsFlexContainerFrame.cpp changes) feel out of place in this patch -- the rest of this patch is entirely on the devtools-structs data-copying side of things, vs. over here we're doing some box-unwrapping inside of flex layout. Maybe this piece wants to be merged into the Part 3? (so that we're resurrecting and using GetFirstNonAnonBoxDescendant in the same patch) I suspect that might be what you intended to do... Alternately, you could make this its own part (call it "3.5" if you like) In any case, I'm including a few review nits on this chunk, for you to address when you move it, but I do think it needs to move out of this patch. ::: layout/generic/nsFlexContainerFrame.cpp:4343 (Diff revision 11) > + // The frame may be for an element, or it may be for an > + // anonymous text node. To distinguish between them, we get > + // the first non-anonymous descendant (inclusive of the > + // starting frame). I think "anonymous text node" is the wrong term here (I don't know if the text node itself is necessarily anonymous -- and in any case, the text node's anonymity is unrelated to the box's anonymity). I think you really want to say: "This may be an *anonymous* flex item, e.g. wrapping one or more text nodes. DevTools wants the content node for the *actual child* in the DOM tree, so we descend through anonymous boxes." ::: layout/generic/nsFlexContainerFrame.cpp:4350 (Diff revision 11) > + // Skip over content that is only whitespace, which might > + // have been broken off from an anonymous text node which > + // is our real target. Again, I'm not sure "anonymous text node" is the right term to use here. that makes me think you're talking about some special anonymous content, as opposed to just regular text nodes (which I think is what you're meaning to describe)...

Comment on attachment 8919528 [details] Bug 1409083 Part 1: Capture computed flex data for use by devtools. https://reviewboard.mozilla.org/r/190364/#review211172 > This latter assertion isn't necessarily valid. But we can make it so! :) > > Here's why it's not valid yet: it's possible for Reflow to call DoFlexReflow twice (specifically, if we've got some flex items with visibility:collapse, which prompt us to layout twice, and have these items collapsed to "struts" on the second run). And on the second run, containerInfo->mLines *will not be empty* with your current patch. > > So, let's make it empty. Something like: > > if (!aStruts.IsEmpty()) { > // We restarted DoFlexLayout, and may have stale mLines to clear: > containerInfo->mLines.Clear(); > } else { > MOZ_ASSERT(containerInfo->mLines.IsEmpty(), > "Shouldn't have lines yet."); > } Thanks for spotting this; I do not really understand struts and this did not occur to me.

Comment on attachment 8919528 [details] Bug 1409083 Part 1: Capture computed flex data for use by devtools. https://reviewboard.mozilla.org/r/190364/#review212114 ::: layout/generic/nsFlexContainerFrame.cpp:2630 (Diff revision 11) > + // If any item on the line is growing, mark the aLineInfo > + // structure; likewise if any item is shrinking. Items in > + // a line can't be both growing and shrinking. > + if (deltaSize > 0) { > + MOZ_ASSERT(aLineInfo->mGrowthState != > + ComputedFlexLineInfo::GrowthState::SHRINKING); I attempted this change, but hit serious problems because isUsingFlexGrow is set **per iteration** of the flex sizing algorithm. So an item that grew and froze in the first loop might be marked again in the second loop (fine) but the line is no longer isUsingFlexGrow. It seemed my options were to: 1) Change the assert to only trigger when the deltaSize value was different from the existing mMainDeltaSize value. 2) Detect and pass over items that were frozen on previous loops. 3) Remove this proposed assert. I chose to remove the proposed assert.

Pushed by bwerth@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b21e6a795493 Part 1: Capture computed flex data for use by devtools. r=dholbert https://hg.mozilla.org/integration/autoland/rev/b21b06a24705 Part 2: Stub webidl definitions to support flex container/item properties. r=smaug https://hg.mozilla.org/integration/autoland/rev/574cd09aad41 Part 3: Add a GetFirstNonAnonBoxDescendant function to unpack anonymous flex items. r=dholbert https://hg.mozilla.org/integration/autoland/rev/660e79af5c93 Part 4: Actually set values for exposed Flex properties. r=dholbert https://hg.mozilla.org/integration/autoland/rev/9de539be3665 Part 5: Add tests of new Flex API. r=gl

Backed out in https://hg.mozilla.org/integration/autoland/rev/2bf149f66160 for ESLint failures, https://treeherder.mozilla.org/logviewer.html#?job_id=150671147&repo=autoland

I don't understand this eslint failure it all. The first failure is that SimpleTest isn't defined, but the script include at the top is the same one I see in other mochitests. > <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script> Another try push confirmed the eslint failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1c4d8263db6&selectedJob=150711621 I'll try some different things to get this landed, but I'm not sure what I'm up against at all...

(In reply to Brad Werth [:bradwerth] from comment #96) > I don't understand this eslint failure it all. The first failure is that > SimpleTest isn't defined, but the script include at the top is the same one > I see in other mochitests. Phil, any thoughts on how to diagnose this problem? I can't see the issue that's causing the eslint to fail.

I'm probably the last person I would ask about eslint: I certainly haven't written a test during the eslint era, since I probably haven't written one this decade. But the first two things that I'd try are removing one of your two </head>s, since that might be confusing to it, and putting a type="application/javascript" on your <script> since it might passively enforce that by using some different rules within untyped script.

Ah, it appears the "solution" is adding "dom/flex/**" to the .eslintignore file -- as all other dom directories seem to do. I guess lint doesn't like the simpletest directives? New patch arriving shortly.

Pushed by bwerth@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d9bb470ce2eb Part 1: Capture computed flex data for use by devtools. r=dholbert https://hg.mozilla.org/integration/autoland/rev/3f762f13c075 Part 2: Stub webidl definitions to support flex container/item properties. r=smaug https://hg.mozilla.org/integration/autoland/rev/db843359f5a8 Part 3: Add a GetFirstNonAnonBoxDescendant function to unpack anonymous flex items. r=dholbert https://hg.mozilla.org/integration/autoland/rev/3c2ef8c6a04f Part 4: Actually set values for exposed Flex properties. r=dholbert https://hg.mozilla.org/integration/autoland/rev/e06bb30b6f5f Part 5: Add tests of new Flex API. r=gl

Comment on attachment 8919528 [details] Bug 1409083 Part 1: Capture computed flex data for use by devtools. https://reviewboard.mozilla.org/r/190364/#review212114 > I attempted this change, but hit serious problems because isUsingFlexGrow is set **per iteration** of the flex sizing algorithm. So an item that grew and froze in the first loop might be marked again in the second loop (fine) but the line is no longer isUsingFlexGrow. > > It seemed my options were to: > 1) Change the assert to only trigger when the deltaSize value was different from the existing mMainDeltaSize value. > 2) Detect and pass over items that were frozen on previous loops. > 3) Remove this proposed assert. > > I chose to remove the proposed assert. > hit serious problems because isUsingFlexGrow is set *per iteration* > [...] item that grew and froze in the first loop might be marked > again in the second loop (fine) but the line is no longer isUsingFlexGrow. > [...] I chose to remove the proposed assert. This sounds fine, but I'm also not sure this explanation is correct (or I might be missing some part of the problem here). In particular, I don't understand your note that "isUsingFlexGrow is set *per iteration*". isUsingFlexGrow is a const bool which is set at the *very beginning* of the FlexLine::ResolveFlexibleLengths method, *before* we enter the loop: https://dxr.mozilla.org/mozilla-central/rev/457b0fe91e0d49a5bc35014fb6f86729cd5bac9b/layout/generic/nsFlexContainerFrame.cpp#2383-2389 So it's not reset per iteration -- it should be the same value across the whole loop. (And the spec requires this -- note "1. Determine the used flex factor" comes before "4. Loop:" at https://drafts.csswg.org/css-flexbox-1/#resolve-flexible-lengths) So I'm unclear on how we could have isUsingFlexGrow disagreeing with deltaSize... Maybe there's an edge case I'm not seeing, but even then, we may want a subtler assertion rather than no-assertion-at-all.

Comment on attachment 8919528 [details] Bug 1409083 Part 1: Capture computed flex data for use by devtools. https://reviewboard.mozilla.org/r/190364/#review212114 > > hit serious problems because isUsingFlexGrow is set *per iteration* > > [...] item that grew and froze in the first loop might be marked > > again in the second loop (fine) but the line is no longer isUsingFlexGrow. > > [...] I chose to remove the proposed assert. > > This sounds fine, but I'm also not sure this explanation is correct (or I might be missing some part of the problem here). > > In particular, I don't understand your note that "isUsingFlexGrow is set *per iteration*". isUsingFlexGrow is a const bool which is set at the *very beginning* of the FlexLine::ResolveFlexibleLengths method, *before* we enter the loop: > https://dxr.mozilla.org/mozilla-central/rev/457b0fe91e0d49a5bc35014fb6f86729cd5bac9b/layout/generic/nsFlexContainerFrame.cpp#2383-2389 > > So it's not reset per iteration -- it should be the same value across the whole loop. (And the spec requires this -- note "1. Determine the used flex factor" comes before "4. Loop:" at https://drafts.csswg.org/css-flexbox-1/#resolve-flexible-lengths) So I'm unclear on how we could have isUsingFlexGrow disagreeing with deltaSize... Maybe there's an edge case I'm not seeing, but even then, we may want a subtler assertion rather than no-assertion-at-all. Ah -- I think your suggested "2. Detect and pass over items that were frozen on previous loops" is perhaps really the correct solution here -- and for simplicity, you could just "pass them over" in the assertion by extending the assert-condition. Such items are frozen *because they violated their min/max size constraints*, by definition -- which means they may indeed have grown/shrunk in the opposite direction that would be suggested by the isUsingFlexGrow bool. So, it makes sense to exempt them from the assertion. So I think you could safely reintroduce the assertions as MOZ_ASSERT(item->IsFrozen() || isUsingFlexGrow, "...") MOZ_ASSERT(item->IsFrozen() || !isUsingFlexGrow, "...") If that makes sense to you, could you try that locally to sanity-check that it works, and perhaps file a one-off bug with a followup? (I think that's the only straightforward way to do followups in non-bustage/backout scenarios on autoland...)

(In reply to Daniel Holbert [:dholbert] from comment #108) > Such items are frozen *because they violated their min/max size > constraints*, by definition -- which means they may indeed have grown/shrunk > in the opposite direction that would be suggested by the isUsingFlexGrow > bool. So, it makes sense to exempt them from the assertion. So I think you > could safely reintroduce the assertions as > MOZ_ASSERT(item->IsFrozen() || isUsingFlexGrow, "...") > MOZ_ASSERT(item->IsFrozen() || !isUsingFlexGrow, "...") Ugh. Sorry for the mis-diagnosis. I agree with your conclusion here. Thanks for thinking it through. I will file a follow-up bug with the enhanced assertion.

No worries -- sounds good, thanks!

https://hg.mozilla.org/mozilla-central/rev/d9bb470ce2eb https://hg.mozilla.org/mozilla-central/rev/3f762f13c075 https://hg.mozilla.org/mozilla-central/rev/db843359f5a8 https://hg.mozilla.org/mozilla-central/rev/3c2ef8c6a04f https://hg.mozilla.org/mozilla-central/rev/e06bb30b6f5f