getComputedStyle(gridEl).gridTemplateRows now returns resolved values for the grid-template-rows property, which is going to be pretty useful for displaying css grid information in devtools (see bug 1181227). For instance, setting this CSS value: > [gutter] 25px repeat(3, [row] auto [gutter] 25px) could result in the following resolved value: > [gutter] 25px [row] 78px [gutter] 25px [row] 26.5px [gutter] 25px [row] 26.5px [gutter] 25px And a value like this: > minmax(100px, max-content) repeat(auto-fill, 200px) 20% could result in: > 100px 200px 89.4px 26px 26px 26px 0px This is nice for devtools because that means we can use these resolved values to draw a grid overlay on top of the page. However, it still comes only as a string that needs to be parsed. Parsing this in javascript is definitely doable but bug prone (especially when that string contains line names (e.g. [linename1 linename2])) and knowing that the information is available on the platform, it would make sense to add a chrome-only API to get something like: data = getGridData(gridElement) with data being an object like: { rows: [{ type: "line", number: 1, names: ["linename1", "linename2"] position: 0 }, { type: "track", position: 0, size: 45 }, { type: "line", number: 2, names: [], position: 45 }, ...], cols: [...] areas: ... } This is just a proposal to illustrate the type of information that would be helpful to devtools, I haven't spent too much time looking at the details (I'm also not sure what the template-areas data would look like).

Hi Mats, I spent some time hacking on a very simple devtools prototype for a css grid devtools inspector, and comment 0 summarizes what I think I'd need on the platform side to make it happen. Does it make sense? Sounds feasible?

That looks good to me. Perhaps we can punt on "areas" until you know what you want to present about them in the UI? Should I include the implicit line names that comes from 'grid-template-areas' in the "names:" array above? e.g. for 'grid-template-areas: ". a a .";' there will be a "a-start" line name for line 2 and a "a-end" for line 4. Or do you want to overlay those yourself? (perhaps with some special styling to differentiate them from line names that comes from 'grid-template-rows/columns')

(In reply to Mats Palmgren (:mats) from comment #2) > That looks good to me. Perhaps we can punt on "areas" until you know what > you > want to present about them in the UI? Sounds good to me, we can do this in a later bug. > Should I include the implicit line names that comes from > 'grid-template-areas' in > the "names:" array above? e.g. for 'grid-template-areas: ". a a .";' > there will be a "a-start" line name for line 2 and a "a-end" for line 4. > Or do you want to overlay those yourself? (perhaps with some special styling > to differentiate them from line names that comes from > 'grid-template-rows/columns') Interesting. I think we'd want to know *all* line names, both explicit and implicit. If a-start and a-end are names that authors can use in their styles, then these names should be displayed so authors can know they even exist. Displaying these names with some special styling sounds like a good idea, I will need to investigate, but if we want to do this, then we'd need this API to give us a type for each line name. Maybe something like: { type: "line", number: 2, names: [{name: "my-name", type: "user"}, {name: "a-start", type: "area"}], position: 45 }

Here's some webidl interfaces that I think should be fairly easy to fill in using data from layout. Does this look sufficient for the Grid overlay? I guess you also want the actual grid area for a grid item, but that's just the four line numbers I imagine.

Now that we support Grid fragmentation I guess the easiest way to support that in this API is to make Element.grid and array of Grid, one for each fragment.

Jet / Johnny, can you help find someone with webidl + DOM C++ code skills that can take this bug? I think that if we want to have a grid visualizer in devtools by the time we enable grid by default, then this work needs to start soon.

(In reply to Mats Palmgren (:mats) from comment #6) > Jet / Johnny, can you help find someone with webidl + DOM C++ code skills > that can take this bug? > I think that if we want to have a grid visualizer in devtools by the time > we enable grid by default, then this work needs to start soon. The proposed WebIDL doesn't look too hard to fill in (assuming you've already got enough Layout done to return the data.) This could be a good first bug for someone, no?

The DOM part seems like the hard part to me. I can take on adding methods to nsGridContainerFrame to provide the actual data if needed, but I don't think the DOM work needs to block on that - it can just return some mockup values instead (which probably helps the devtools guys start on the overlay).

My first time exporting an object via WebIDL. It seems that the proposed WebIDL classes (Grid, GridDimension, GridTracks, etc.) would necessitate the creation of a lot of C++ classes to support what is otherwise read-only plain-old-data. What's the reason to pursue this approach, instead of a single Grid class that delivers an appopriately-structured JSObject on demand?

No particular reason, so if it's easier to implement an untyped variant using arrays and dictionaries etc, that's fine with me.

Feedback needed on this partial implementation of mats' proposed webIDL classes. No tests in the patch. In particular, please check if my use of IDL interfaces to support the [ArrayClass] directives is correct.

Comment on attachment 8754122 [details] [diff] [review] Made a new stub Grid class, to expose grid CSS properties for all Elements I don't know enough about webidl implementation stuff to give advice here I'm afraid. Ms2ger, can you give feedback, or know someone else who can? Thanks!

Comment on attachment 8754122 [details] [diff] [review] Made a new stub Grid class, to expose grid CSS properties for all Elements Review of attachment 8754122 [details] [diff] [review]: ----------------------------------------------------------------- This seems okay as far as a WIP goes, but I haven't done much Gecko work in the past few years. Do make sure to check for trailing whitespace. ::: dom/interfaces/grid/moz.build @@ +4,5 @@ > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +XPIDL_SOURCES += [ > + 'nsIDOMGridLine.idl', I don't think there's a need to have any of those.

Requesting feedback from khuey, also.

Comment on attachment 8754122 [details] [diff] [review] Made a new stub Grid class, to expose grid CSS properties for all Elements Review of attachment 8754122 [details] [diff] [review]: ----------------------------------------------------------------- You don't need any of the nsIDOMGrid* stuff. You can rip it all out and things will be fine, I think. You also have a lot of trailing whitespace in this patch. You may need to reconfigure your editor. ::: Grid.patch @@ +1,1 @@ > +diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp For obvious reasons, you don't want to check in a copy of the patch ;) ::: dom/base/Element.h @@ +1873,5 @@ > { \ > return nsINode::QuerySelectorAll(aSelector, aReturn); \ > } > > + nit: extra \n ::: dom/grid/Grid.cpp @@ +14,5 @@ > + > +NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(Grid, AddRef) > +NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(Grid, Release) > + > +Grid::Grid(nsISupports *parent) *s to the left, and aParent. ::: dom/grid/Grid.h @@ +16,5 @@ > +class Grid > + : public nsWrapperCache > +{ > +public: > + Grid(nsISupports *parent); *s to the left, and 'a' prefixes on arguments (so aParent). @@ +35,5 @@ > + GridDimension* Rows() const; > + GridDimension* Cols() const; > + > +protected: > + nsCOMPtr<nsISupports> mParent; In general, it would be nice if you used the concrete types here if they're always the same. So mParent would be an nsCOMPtr<Element> ::: dom/grid/GridDimension.cpp @@ +14,5 @@ > + > +NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(GridDimension, AddRef) > +NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(GridDimension, Release) > + > +GridDimension::GridDimension(nsISupports *parent) ibid ::: dom/grid/GridDimension.h @@ +36,5 @@ > + GridLines* Lines() const; > + GridTracks* Tracks() const; > + > +protected: > + nsCOMPtr<nsISupports> mParent; Similarly this would be a RefPtr<Grid> ::: dom/webidl/Element.webidl @@ +159,5 @@ > + // Support reporting of Grid properties > + > + // gridNumber is just a test to ensure we can reflect a simple property > + //[ChromeOnly] > + readonly attribute unsigned long gridNumber; I assume this is going away. @@ +162,5 @@ > + //[ChromeOnly] > + readonly attribute unsigned long gridNumber; > + // grid is the real deal -- the computed CSS grid layout for this element > + //[ChromeOnly] > + readonly attribute Grid? grid; And that this is actually going to become ChromeOnly

In particular the [ArrayClass] stuff looks reasonable.

> + readonly attribute Grid? grid; We should make this sequence<Grid> or some such, so that we can return one Grid per fragment, similar to getClientRects.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #15) > You don't need any of the nsIDOMGrid* stuff. You can rip it all out and > things will be fine, I think. I'd like to remove the use of nsISupports, but https://dxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py#12114 is putting a static assert into the generated bindings.cpp files for GridLines and GridTracks since they have the [ArrayClass] property. It seems that deriving from nsISupports is mandatory for such webIDLs. I'm making all changes proposed above, with this exception.

Right, you need nsISupports in that case. But you don't need the nsIDOMBlah anywhere.

Got it. I thought nsISupports implied the nsIDOM stuff, but I'll work to understand it better then remove the nsIDOM interfaces.

Addressed previous feedback, except need to support grid fragmentation. I woudl appreciate insight into the proper way to associate the data in nsGridContainerFrame with the new Grid classes.

(In reply to Mats Palmgren (:mats) from comment #17) > > + readonly attribute Grid? grid; > > We should make this sequence<Grid> or some such, so that we can return one > Grid > per fragment, similar to getClientRects. Note that in that case, it'll need to be a method, not an attribute. (A sequence return type implies returning a new object every time, which is considered poor design for an attribute.)

Comment on attachment 8755637 [details] [diff] [review] Revised proposed code implementing webIDL classes for Grid CSS properties. All prior feedback addressed, but still no tests, and no data filled in from nsGridContainerFrame. For track position/sizes you can look at DoGetGridTemplateColumns (which implements the computed value for the 'grid-template-columns' property): https://hg.mozilla.org/mozilla-central/annotate/5511d54a3f17/layout/style/nsComputedDOMStyle.cpp#l2746 it gets the data from GetComputedTemplateColumns: https://hg.mozilla.org/mozilla-central/annotate/5511d54a3f17/layout/generic/nsGridContainerFrame.h#l97 That only reports the column sizes, but the positions are readily available in nsGridContainerFrame so I think you can extend ComputedGridTrackInfo with those. The data is set up here: https://hg.mozilla.org/mozilla-central/annotate/5511d54a3f17/layout/generic/nsGridContainerFrame.cpp#l4988 (nsGridContainerFrame::TrackSize::mPosition is the position) The track State/Type can probably be computed from existing data as well. Extend ComputedGridTrackInfo with enum classes for those? If it's an explicit/implicit track can be determined from nsGridContainerFrame::TrackSizingFunctions::mExplicitGridOffset and NumExplicitTracks(). State can probably mostly be derived from TrackSizingFunctions, but "removed" is tricky. We remove empty 'repeat' tracks here: https://hg.mozilla.org/mozilla-central/annotate/5511d54a3f17/layout/generic/nsGridContainerFrame.cpp#l3152 So I don't think we really have any data about those. So maybe we should just skip the "removed" state and not return any track data about those? (I don't see why DevTools would be interested in them anyway) Line names only exist in style data, nsGridContainerFrame doesn't record anything about those. We need to write a function that takes an explicit line number and returns a set of names. The style data that contains line names are nsStylePosition::mGridTemplateColumns/Rows and lines names derived from areas in nsStylePosition::mGridTemplateAreas. The line start/breadth is simply the spacing between the tracks, so the first line's start is the same as the first explicit track's start and has zero breadth, the next line starts at the first track's start+breadth, and so on, until the last line which has zero breadth. Except that we need to account for the removed tracks here - if a track was removed then next line starts where the previous line ended and has zero breadth. To determine which tracks were removed in the repeated range we need to look at the GridItemInfo which says where each item starts/ends. If we find an item covering some tracks in the repeat-range then those tracks exist, tracks not covered were removed. We could store the line data in a frame property, like we do for ComputedGridTrackInfo about the tracks, but it seems a bit wasteful when the start/breadth can be deduced from the track data. If we store "number of lines at the track start" in ComputedGridTrackInfo we don't need separate data about lines.

Reproduction to hit assert at https://dxr.mozilla.org/mozilla-central/source/mfbt/LinkedList.h#329: 1) Load html at /dom/grid/test/chrome/test_grid_object.html. 2) Open Web Console. 3) Force the creation of the Grid object by entering: "wrapper.grid" in the Web Console. 4) Quit Nightly. Assert will be hit during shutdown.

Comment on attachment 8756586 [details] [diff] [review] Grid property returns a mostly-empty object for display:grid elements, and null otherwise. A basic test has been created. I don't see anything obviously wrong here, unfortunately. The assertion seems to indicate that we're destroying a compartment (a container for a Javascript global object and all of its associated objects/strings/etc) but there's something left inside it. I don't see any weird stuff with wrappers here though. I wonder if it's related to the web console.

Changes are getting rangy, so I want to land this partial implementation, if acceptable. Summary of changes: Element gets a new function getGridFragments() which returns an array of Grid objects. Grid objects accurately return: 1) Track sizes 2) Track positions 3) Track type (implicit/explicit) Still to be implemented: a) Track states (static/repeat) b) All line properties c) All area properties

I've built the patch to have a play with it, and it looks really good! Can't wait to have the rest implemented (a, b and c in comment 26), but it's already really useful for devtools to start playing with a grid tool. Thanks!

Comment on attachment 8758920 [details] [diff] [review] gridPatch4.patch I should probably review the nsGridContainerFrame.cpp/h changes here. >+ // The number of repeat tracks at the end, removed because they were empty. >+ uint32_t mRepeatRemovedEnd; This doesn't seem to be used anywhere, so please remove it. >+ for (const TrackSize& sz : gridReflowState.mRows.mSizes) { The state for each grid fragment actually have the full grid data for all tracks in its mCols/mRows. The state has a mStartRow which says which row is the first in this fragment. I suspect we need to add a mEndRow there too. Note that fragments at the end may have mStartRow == mRows.mSizes.Length() which means there is no row in those fragments. Also, the first fragment may have mStartRow==0 but no tracks (because the first track may end up in the second fragment if it doesn't fit and there's a break opportunity before it). Anyway, feel free to address that in a follow-up bug if you want. >+ rowTrackSizes.AppendElement(sz.mBase); >+ bool isRepeat = ((col >= gridReflowState.mColFunctions.mRepeatAutoStart) && >+ (col < gridReflowState.mColFunctions.mRepeatAutoEnd)); Copy-paste error? 'col' should be 'row' here. Please add some fragmented grids to the tests (even though the row results are slightly wrong ATM. (You can find plenty of reftests in layout/reftests/css-grid/grid-fragmentation-* that does grid fragmentation inside column layout that you can copy.) r=mats on nsGridContainerFrame.cpp/h with the above addressed.

Comment on attachment 8758920 [details] [diff] [review] gridPatch4.patch Some drive-by nits on the other files: >if (primaryFrame != nullptr) { The general rule is to not write out explicit nullptr pointer checks. Instead, just use "if (primaryFrame)" or "if (!primaryFrame)". > for (nsIFrame* frame = primaryFrame->FirstInFlow(); This is correct, but it's an invariant that the primary frame is also FirstInFlow(), so you should just do an assignment here. > if (frame->GetType() == nsGkAtoms::gridContainerFrame) It's also an invariant that each fragment that is returned by GetNextInFlow() have the same frame type. So testing each by calling GetType() is unnecessary (GetType() is virtual so we should try to avoid that cost). > for (size_t i = 0; i < mNames.Length(); ++i) nsTArray have iterator methods so in most cases it's better to use for (auto name : mNames) ... In this particular case, appending elements from one array to another, you should probably use AppendElements instead. (or just assignment?) >+GridLines::GridLines(GridDimension* aParent) >+ : mParent(aParent) Three space indent. >+GridTrack::SetTrackValues( >+ double aStart, >+ double aBreadth, >+ GridTrackType aType, >+ GridTrackState aState >+) { The ')' should go after 'aState'. >+GridTracks::GridTracks(GridDimension *parent) >+ : mParent(parent) Three space indent. >+ * If this element has a display:grid or display:grid-inline style, s/grid-inline/inline-grid/

Updated to address most of the issues raised in mats review in comment 28, except: 1) No tests of grid fragementation yet -- I need to understand the spec and behavior better to craft the tests. 2) No attempt to fix the row fragmentation potential mis-reporting for the same reason.

Some decisions ahead of us on the proposed track states of static/repeat/removed... We're trending to drop removed since the current implementation actually does remove the tracks and therefore has no correllated object to host the "removed" state. So first question: Is this removed state valuable enough to dev tools that we change the rest of the object model to report 0-width tracks with state=removed? On to repeat state. With CSS > grid-template-columns: 50px repeat(auto-fill, 100px) track 0 has state=static and tracks 1..n have state=repeat. But with CSS > grid-template-columns: 50px repeat(4, 100px) track 1 has state=static since the repeat directive is processed within the CSS parser. This seems inconsistent given the similarities with the prior CSS. Second question: What is the dev tools value of the repeat state for tracks, and should it apply to ALL tracks generated from "repeat()" styling, or only tracks generated from "repeat(auto-fill, ...)" or "repeat(auto-fit, ...)"?

(In reply to Brad Werth [:bradwerth] from comment #31) > We're trending to drop removed since the current implementation actually > does remove the tracks and therefore has no correllated object to host the > "removed" state. So first question: > Is this removed state valuable enough to dev tools that we change the rest > of the object model to report 0-width tracks with state=removed? I don't actually know what it means for a track to be removed and when that occurs. Does telling authors when tracks are removed help them understand layout issues? If it helps, then perhaps instead of showing information about that in the grid tool, we could emit a CSS warning in the console. This way there's no need to keep them in memory if gecko don't use them anyway. I think the grid tool (which will be an overlay on top of the page) is going to be a pretty busy UI as it is. I'm not even sure we'd have a good way to represent removed tracks anyway. > On to repeat state. With CSS > > grid-template-columns: 50px repeat(auto-fill, 100px) > track 0 has state=static and tracks 1..n have state=repeat. But with CSS > > grid-template-columns: 50px repeat(4, 100px) > track 1 has state=static since the repeat directive is processed within the > CSS parser. This seems inconsistent given the similarities with the prior > CSS. Second question: > What is the dev tools value of the repeat state for tracks, and should it > apply to ALL tracks generated from "repeat()" styling, or only tracks > generated from "repeat(auto-fill, ...)" or "repeat(auto-fit, ...)"? I think it makes sense for authors to understand that repeat(4, 100px) is just syntax sugar to avoid having to write 10px 4 times, but that the 4 columns are actually static. If we do display static tracks differently in devtools, I think it totally makes sense for these 4 columns to be displayed as static. That's what they are. And then we can display those auto-fit/auto-fill tracks differently from the static ones. So, short answer: repeat state should apply only to auto-fill/auto-fit.

Additive to patch4.1 in comment 30. Additionally supports line postions, sizes, and names.

Additive patch to handle grid fragmentation and stub out GridAreas

Comment on attachment 8760889 [details] [diff] [review] Additve patch that adds line positions, sizes, names >+bool nsGridContainerFrame::LineNameMap::Contains( >+ uint32_t aIndex, >+ const nsString& aName) const >+{ >+ return NamesAtIndex(aIndex).Contains(aName); Creating an array of strings of all possible names and then searching that for each lookup is going to be slow. Please keep the current line name lookup code as is. NamesAtIndex will have a bit of code duplication I guess, but that's fine. I'd like to avoid exposing LineNameMap as a public interface. LineNameMap has members that references into style data which may become stale, so it's risky to keep LineNameMap alive past reflow. LineNameMap was only intended to be transient object - all the data it has is already present in TrackSizingFunctions (except mTemplateLinesEnd which is easily derived). We don't need to store the style data references, so I think we should create a new struct for this that just copies the values we need from aState.m[Row|Col]Functions at the end of reflow, and store that on frame properties. Do we actually need to store anything else but a mNumRepeatTracks? For the API, maybe we could add [Col|Row]NamesAtIndex methods on nsGridContainerFrame to hide the internal stuff? They would call your NamesAtIndex code, which can be a static function that take some style data refs and the saved property value as args. It can probably derive most values it needs by itself, see TrackSizingFunctions::SetNumRepeatTracks. (the pre-calculated members in LineNameMap are just for speed)

Perhaps you could try the approach I suggested in bug 1229180? (I'd really like to know if that approach works) Then the CSSOM code would look something like this: GridContainerFrame* f = node->GetPrimaryFrame(); f->AddStateBits(/*some new bit*/); f->FrameNeedsReflow(...); shell->FlushPendingNotifications(Flush_Layout); f = node->GetPrimaryFrame(); for each fragment of f: for each line: nsTArray<nsString> names = fragment->ColNamesAtIndex(line); nsGridContainerFrame::Reflow would look for the new frame bit and save the data on frame properties when it's set.

Comment on attachment 8762162 [details] [diff] [review] gridPatch6.patch FYI, I'm adding a GridReflowState::mNextFragmentStartRow member in bug 1151204 (part 3). It's like a sentinel, so the number of rows in this fragment is: mNextFragmentStartRow - mStartRow, which might be zero. That might help to avoid doing this at all? (i.e. just store data about the rows in this fragment)

(In reply to Mats Palmgren (:mats) from comment #36) > Perhaps you could try the approach I suggested in bug 1229180? If you do, and if it seems to work in the non-fragmented case, then we need to add a nsGridContainerFrame::Init method that propagates the bit from aPrevInFlow.

Fixed comprehensive 4.1 patch, addressing most issues in comment 28.

Comment on attachment 8763054 [details] [diff] [review] gridPatch4.1.patch Additional review needed from Kyle to approve the WebIDL changes.

Comment on attachment 8763054 [details] [diff] [review] gridPatch4.1.patch Review of attachment 8763054 [details] [diff] [review]: ----------------------------------------------------------------- This is generally looking good. Some comments below. ::: dom/base/Element.cpp @@ +146,5 @@ > #include "mozilla/IntegerPrintfMacros.h" > #include "mozilla/Preferences.h" > #include "nsComputedDOMStyle.h" > > +#include "mozilla/dom/Grid.h" Nit: I would just choose a clump of #includes to add this one too. The organisation of #includes in this file isn't great, but no need to add another section. ::: dom/grid/Grid.cpp @@ +6,5 @@ > + > +#include "Grid.h" > +#include "mozilla/dom/GridBinding.h" > +#include "GridDimension.h" > +#include "nsGridContainerFrame.h" Nit: keep these sorted (but with Grid.h at the top, possible with a blank line between it and the rest). @@ +25,5 @@ > + , mFrame(aFrame) > + , mRows(new GridDimension(this)) > + , mCols(new GridDimension(this)) > +{ > + if (mFrame) { It looks like we only ever create Grid objects when we do have an nsGridContainerFrame. Let's assert mFrame, and not bother checking it in here. This is important from the perspective of the Web IDL, where the rows and cols IDL attributes are defined to be of type |GridDimension| and not |GridDimension?|, and thus are not allowed to return null (which, if aFrame were allowed to be null, we could return). @@ +26,5 @@ > + , mRows(new GridDimension(this)) > + , mCols(new GridDimension(this)) > +{ > + if (mFrame) { > + if (mRows) { No need to null-check mRows and mCols here. operator new is infallible by default. ::: dom/grid/Grid.h @@ +9,5 @@ > + > +#include "nsISupports.h" > +#include "nsWrapperCache.h" > +#include "mozilla/dom/Element.h" > +#include "nsGridContainerFrame.h" Nit: keep these #includes sorted. (And in other .h and .cpp files in this patch.) @@ +17,5 @@ > + > +class GridDimension; > + > +class Grid > + : public nsISupports Nit: while I don't there's a style guide rule for it, it's probably more consistent with other code to put the " : public nsISupports" on the previous line. (And for the other added .h files in this patch.) @@ +42,5 @@ > + GridDimension* Cols() const; > + > +protected: > + nsCOMPtr<Element> mParent; > + nsGridContainerFrame* mFrame; Is it possible for mFrame to dangle here? Imagine if you have a display:grid element, you call element.getGridFragments() and store away the result, and then you make the element display:none. I think the frame will be destroyed. But are there any callers of the Frame() accessor method? I can't find any in the patch, so if there aren't, and since we only use the frame during the constructor, we shouldn't need to store it. ::: dom/grid/GridDimension.cpp @@ +25,5 @@ > +GridDimension::GridDimension(Grid* aParent) > + : mParent(aParent) > + , mLines(new GridLines(this)) > + , mTracks(new GridTracks(this)) > +{ Let's assert aParent. @@ +51,5 @@ > + return mTracks; > +} > + > +void > +GridDimension::SetTrackInfo(const mozilla::ComputedGridTrackInfo* aTrackInfo) Can drop "mozilla::" here too. @@ +53,5 @@ > + > +void > +GridDimension::SetTrackInfo(const mozilla::ComputedGridTrackInfo* aTrackInfo) > +{ > + if (mTracks) { No need to null check mTracks. @@ +62,5 @@ > +void > +GridDimension::SetLineInfo(const mozilla::ComputedGridTrackInfo* aTrackInfo, > + const nsGridContainerFrame::LineNameMap* aLineInfo) > +{ > + if (mLines) { No need to null check mLines. ::: dom/grid/GridDimension.h @@ +41,5 @@ > + > + GridLines* Lines() const; > + GridTracks* Tracks() const; > + > + void SetTrackInfo(const mozilla::ComputedGridTrackInfo* aTrackInfo); You can drop the "mozilla::" here since we're within namespace mozilla. ::: dom/grid/GridLine.cpp @@ +18,5 @@ > + NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY > + NS_INTERFACE_MAP_ENTRY(nsISupports) > +NS_INTERFACE_MAP_END > + > +GridLine::GridLine(GridLines *parent) aParent @@ +33,5 @@ > +void > +GridLine::GetNames(nsTArray<nsString>& aNames) const > +{ > + aNames.Clear(); > + aNames.AppendElements(mNames); You can replace these two lines with |aNames = mNames;|. @@ +45,5 @@ > + > +double > +GridLine::Start() const > +{ > + return mStart; FWIW for simple accessors like these feel free to write them inline in the .h file. @@ +68,5 @@ > + const nsTArray<nsString>& aNames) > +{ > + mStart = aStart; > + mBreadth = aBreadth; > + mNumber = aNumber; I don't see mNumber declared anywhere. What is it? @@ +70,5 @@ > + mStart = aStart; > + mBreadth = aBreadth; > + mNumber = aNumber; > + mNames.Clear(); > + mNames.AppendElements(aNames); As above, just make this |mNames = aNames|. ::: dom/grid/GridLines.cpp @@ +58,5 @@ > + return mLines[aIndex]; > +} > + > +void > +GridLines::SetLineInfo(const mozilla::ComputedGridTrackInfo* aTrackInfo, Can drop "mozilla::" here. @@ +63,5 @@ > + const nsGridContainerFrame::LineNameMap* aLineInfo) > +{ > + mLines.Clear(); > + > + if (aTrackInfo) { Might be a stylistic preference, but would prefer an early return if !aTrackInfo so that we avoid indenting the rest of the method. @@ +64,5 @@ > +{ > + mLines.Clear(); > + > + if (aTrackInfo) { > + uint32_t trackCount = aTrackInfo->mEndFragmentTrack - I can't find where mEndFragmentTrack is defined? ::: dom/grid/GridLines.h @@ +39,5 @@ > + uint32_t Length() const; > + GridLine* Item(uint32_t aIndex); > + GridLine* IndexedGetter(uint32_t aIndex, bool& aFound); > + > + void SetLineInfo(const mozilla::ComputedGridTrackInfo* aTrackInfo, Can drop "mozilla::" here. ::: dom/grid/GridTrack.cpp @@ +63,5 @@ > +} > + > +void > +GridTrack::SetTrackValues( > + double aStart, Nit: it's probably more consistent with other code to format this with the first argument on the first line, and then all arguments aligned: GridTrack::SetTrackValues(double aString, double aBreadth, ...) { ::: dom/grid/GridTrack.h @@ +19,5 @@ > + : public nsISupports > + , public nsWrapperCache > +{ > +public: > + GridTrack(GridTracks *parent); aParent ::: dom/grid/GridTracks.cpp @@ +21,5 @@ > + NS_INTERFACE_MAP_ENTRY(nsISupports) > +NS_INTERFACE_MAP_END > + > + > +GridTracks::GridTracks(GridDimension *parent) aParent @@ +59,5 @@ > + return mTracks[aIndex]; > +} > + > +void > +GridTracks::SetTrackInfo(const mozilla::ComputedGridTrackInfo* aTrackInfo) Drop "mozilla::". @@ +68,5 @@ > + if (aTrackInfo) { > + for (size_t i = aTrackInfo->mStartFragmentTrack; > + i < aTrackInfo->mEndFragmentTrack; > + i++) { > + GridTrack* track = new GridTrack(this); While it's OK here, the pattern of creating a new refcounted object and assigning it to a raw pointer and then calling methods on it (or calling other functions and passing it in) can be unsafe if those methods will AddRef and Release the object. So let's either move the mTracks.AppendElement(track) call in here so that we can be sure the object won't go down to refcnt = 0 somehow during the SetTrackValues call if we make changes to it later, or do: RefPtr<GridTrack> track = new GridTrack(this); ... mTracks.AppendElement(Move(track)); ::: dom/grid/test/chrome.ini @@ +1,1 @@ > +[DEFAULT] Can drop this section if there are no settings inside it. ::: dom/interfaces/base/domstubs.idl @@ +81,5 @@ > interface nsIDOMFontFaceList; > + > +// Grid > +interface nsIDOMGridLine; > +interface nsIDOMGridLines; I think these can be dropped now that we don't have these XPIDL interfaces. ::: dom/webidl/Grid.webidl @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + * > + * The origin of this IDL file is > + * https://drafts.csswg.org/css-grid/#grid-definition I think this line is only used for pointing to where the IDL was copied from, but since we're not copying it from the spec, let's leave this out. @@ +9,5 @@ > +[ChromeOnly] > +interface Grid > +{ > + readonly attribute GridDimension rows; > + readonly attribute GridDimension cols; Since the Rows() and Cols() methods on mozilla::Grid can return null (if you call getGrid @@ +19,5 @@ > + readonly attribute GridLines lines; > + readonly attribute GridTracks tracks; > +}; > + > +[ChromeOnly, ArrayClass] As someone mentioned, [ArrayClass] probably isn't necessary for these interfaces. (Here it would make GridLines.prototype inherit from Array.prototype rather than Object.prototype, and thus make available methods like concat(), join(), etc.) So I think we could drop it; the indexed access through the "getter" would still work.

Comment on attachment 8760889 [details] [diff] [review] Additve patch that adds line positions, sizes, names Review of attachment 8760889 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay Brad. I have tested the API locally and was able to build a grid highlighter really quickly with it. With a combination of getBoxQuads and getGridFragments, we have everything we need in devtools to have something really cool. I did notice something that looked like a bug: if there are more than 1 fragment (grid split on multiple columns), then the line.start and track.start of the second fragment is offset vertically: https://www.dropbox.com/s/qq9pjn0k3ez184w/Screenshot%202016-06-20%2009.51.49.png?dl=0 As you can see in the previous screenshot, the first row line of the 2nd and 3rd fragments are ok (they are at the top of the container), but the lines are this start after the last row line of the first fragment instead of at the top. Same for the tracks (which are represented with their breadth label on the side, "20px". Apart from this, everything seemed to work really nicely. I think we're just missing line numbers and areas now. About line names: I didn't test this specifically, do we have names generated by areas already? I think lines can end up with many names that weren't defined in the grid-template-row/column property. It would help if we could differentiate them from authored names in the API.

(In reply to Patrick Brosset <:pbro> from comment #42) > I did notice something that looked like a bug: > if there are more than 1 fragment (grid split on multiple columns), then the > line.start and track.start of the second fragment is offset vertically: > https://www.dropbox.com/s/qq9pjn0k3ez184w/Screenshot%202016-06-20%2009.51.49. > png?dl=0 I'll sort that out. Would you please send me the HTML/CSS you used for the example in the screenshot? I'm not so adept at triggering fragmentation on demand.

Additive patch to attachment 8763054 [details] [diff] [review] that addresses the issues raised in commment 41. If this is accepted in review, I'll submit a comprehensive patch and ask for checkin. It will also become the head for addressing the issues raised by mats in comment 35 and by pbro in comment 42.

Comment on attachment 8764069 [details] [diff] [review] Additive patch to address issues in heycam's review Review of attachment 8764069 [details] [diff] [review]: ----------------------------------------------------------------- r=me on those changes. ::: dom/grid/GridLines.cpp @@ +93,5 @@ > + nsPresContext::AppUnitsToDoubleCSSPixels(endOfLastTrack), > + nsPresContext::AppUnitsToDoubleCSSPixels(startOfNextTrack - > + endOfLastTrack), > + i + 1, > + nsTArray<nsString>() Is it that we don't support grid line names yet, or is this going to be filled in in a later patch? ::: dom/grid/GridTracks.cpp @@ +90,3 @@ > GridTrackType::Implicit : > + GridTrackType::Explicit > + ), I feel this might be a little clearer written as: (i < aTrackInfo->mNumLeadingImplicitTracks) || (i >= aTrackInfo->mNumLeadingImplicitTracks + aTrackInfo->mNumberExplicitTracks) ? GridTrackType::Implicit : GridTrackType::Explicit;

This patch rolls up the changes that have been r=heycam into one patch. It supplies the grid track properties only.

This proposed patch address Mats' feedback in comment 35 by re-architecting the line name retrieval to cache the names on reflow, without exposing the existing LineNameMap class. It has a stub to set a "generate grid properties" flag on reflow, but the flag is not yet defined or checked.

has problems to apply: 1 out of 2 hunks FAILED -- saving rejects to file dom/base/Element.cpp.rej unable to find 'dom/grid/Grid.cpp' for patching 1 out of 1 hunks FAILED -- saving rejects to file dom/grid/Grid.cpp.rej unable to find 'dom/grid/Grid.h' for patching 2 out of 2 hunks FAILED -- saving rejects to file dom/grid/Grid.h.rej unable to find 'dom/grid/GridDimension.cpp' for patching 3 out of 3 hunks FAILED -- saving rejects to file dom/grid/GridDimension.cpp.rej unable to find 'dom/grid/GridDimension.h' for patching 2 out of 2 hunks FAILED -- saving rejects to file dom/grid/GridDimension.h.rej unable to find 'dom/grid/GridLine.cpp' for patching 2 out of 2 hunks FAILED -- saving rejects to file dom/grid/GridLine.cpp.rej unable to find 'dom/grid/GridLine.h' for patching 2 out of 2 hunks FAILED -- saving rejects to file dom/grid/GridLine.h.rej unable to find 'dom/grid/GridLines.cpp' for patching 2 out of 2 hunks FAILED -- saving rejects to file dom/grid/GridLines.cpp.rej unable to find 'dom/grid/GridLines.h' for patching 2 out of 2 hunks FAILED -- saving rejects to file dom/grid/GridLines.h.rej unable to find 'dom/grid/GridTrack.cpp' for patching 3 out of 3 hunks FAILED -- saving rejects to file dom/grid/GridTrack.cpp.rej unable to find 'dom/grid/GridTrack.h' for patching 1 out of 1 hunks FAILED -- saving rejects to file dom/grid/GridTrack.h.rej unable to find 'dom/grid/GridTracks.cpp' for patching 2 out of 2 hunks FAILED -- saving rejects to file dom/grid/GridTracks.cpp.rej unable to find 'dom/grid/GridTracks.h' for patching 1 out of 1 hunks FAILED -- saving rejects to file dom/grid/GridTracks.h.rej unable to find 'dom/grid/test/chrome.ini' for patching 1 out of 1 hunks FAILED -- saving rejects to file dom/grid/test/chrome.ini.rej patching file dom/interfaces/base/domstubs.idl Hunk #1 FAILED at 73 1 out of 1 hunks FAILED -- saving rejects to file dom/interfaces/base/domstubs.idl.rej unable to find 'dom/webidl/Grid.webidl' for patching 1 out of 1 hunks FAILED -- saving rejects to file dom/webidl/Grid.webidl.rej patching file layout/generic/nsGridContainerFrame.cpp Hunk #1 FAILED at 5601 Hunk #2 FAILED at 5624 2 out of 2 hunks FAILED -- saving rejects to file layout/generic/nsGridContainerFrame.cpp.rej patching file layout/generic/nsGridContainerFrame.h Hunk #1 FAILED at 24 1 out of 1 hunks FAILED -- saving rejects to file layout/generic/nsGridContainerFrame.h.rej unable to find 'dom/grid/GridTracks.cpp' for patching 1 out of 1 hunks FAILED -- saving rejects to file dom/grid/GridTracks.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh grid4.3.patch

(In reply to Brad Werth [:bradwerth] from comment #43) > (In reply to Patrick Brosset <:pbro> from comment #42) > > I did notice something that looked like a bug: > > if there are more than 1 fragment (grid split on multiple columns), then the > > line.start and track.start of the second fragment is offset vertically: > > https://www.dropbox.com/s/qq9pjn0k3ez184w/Screenshot%202016-06-20%2009.51.49. > > png?dl=0 > > I'll sort that out. Would you please send me the HTML/CSS you used for the > example in the screenshot? I'm not so adept at triggering fragmentation on > demand. https://dl.dropboxusercontent.com/u/714210/filter-editor.html (the number of columns depend on the viewport size, so resize your window until you do have multiple columns).

Comment on attachment 8765018 [details] [diff] [review] Proposed patch to add line names >dom/base/Element.cpp > Element::GetGridFragments(nsTArray<RefPtr<Grid>>& aResult) > { > nsIFrame* frame = GetPrimaryFrame(); > if (frame && (frame->GetType() == nsGkAtoms::gridContainerFrame)) { I think we need additional code here to pierce through scroll frames and such... something like: nsIFrame* frame = GetPrimaryFrame(); if (frame) { frame = frame->GetContentInsertionFrame(); } if (frame && (frame->GetType() == nsGkAtoms::gridContainerFrame)) { ... You can test this by setting overflow:hidden on a grid container. >+ // Trigger a reflow that generates additional grid property data. >+ nsIPresShell* shell = frame->PresContext()->PresShell(); >+ //frame->AddStateBits(/*some new bit*/); >+ shell->FrameNeedsReflow(frame, nsIPresShell::eResize, NS_FRAME_IS_DIRTY); >+ shell->FlushPendingNotifications(Flush_Layout); >+ frame = GetPrimaryFrame(); Yeah, this should work I think. If we use eResize (and I agree it should be enough) then refetching the frame shouldn't be necessary. Did you verify you got a Reflow call from the above? Feel free to add a new grid container bit here: http://searchfox.org/mozilla-central/rev/ef24c234ed53b3ba50a1734f6b946942e4434b5b/layout/generic/nsFrameStateBits.h#311 > for (; frame != nullptr; frame = frame->GetNextInFlow()) { nit: "!= nullptr" is redundant >layout/generic/nsGridContainerFrame.cpp >+ // Generate the line info properties. We need to provide the number of >+ // repeat tracks produced in the reflow. >+ >+ // Generate column lines first. >+ uint32_t repeatTrackCount = (gridReflowState.mColFunctions.mRepeatAutoEnd - >+ gridReflowState.mColFunctions.mRepeatAutoStart); >+ nsTArray<nsTArray<nsString>> columnLineNames; We could give a capacity hint here (it doesn't need to be exact): auto capacity = repeatTrackCount + gridReflowState.mCols.mSizes.Length(); nsTArray<nsTArray<nsString>> columnLineNames(capacity); >+ for (col = 0; col <= gridReflowState.mCols.mSizes.Length(); col++) { >+ nsTArray<nsString>* names = columnLineNames.AppendElements(1); >+ if (names) { nsTArray is infallible so null-checking is unnecessary. nit: auto type might be more readable. >+ TemplateLineNamesAtIndex(StylePosition()->mGridTemplateColumns, Can TemplateLineNamesAtIndex be static? If so, then it might be better to move this code to a method on nsGridContainerFrame::Tracks? Something like this: nsTArray<nsTArray<nsString>> columnLineNames = gridReflowState.mCols->GetLineNames(gridReflowState.mColFunctions, gridReflowState.mGridStyle->mGridTemplateColumns); Assuming we can avoid array copying by RVO. >+nsGridContainerFrame::TemplateLineNamesAtIndex( ... >+ if (!hasRepeatAuto) { >+ if (aIndex < lineNameLists.Length()) { >+ aLineNames.AppendElements(lineNameLists[aIndex]); >+ } The aIndex check doesn't seem necessary there. With no repeat(auto) the style data always has one line more than the number of tracks, IIRC.

Figured out the export to supply the changeset diffs in the proper order, ensuring the patch applies cleanly.

Addresses many of the issues raised in comment 50, with these caveats: > Feel free to add a new grid container bit here: I added a new bit, but for now I only apply it to the generation of the line name structures. I'll expand it to cover more of the calculations as I evaluate the places where the ComputedGridTrackInfo structures are used, and make sure reflow is triggered with the new bit as appropriate. > Can TemplateLineNamesAtIndex be static? If so, then it might be > better to move this code to a method on nsGridContainerFrame::Tracks? I moved the function into Tracks struct, and once that was done, there seemed to be no need for a static helper function at all. > >+nsGridContainerFrame::TemplateLineNamesAtIndex( > ... > >+ if (!hasRepeatAuto) { > >+ if (aIndex < lineNameLists.Length()) { > >+ aLineNames.AppendElements(lineNameLists[aIndex]); > >+ } > > The aIndex check doesn't seem necessary there. > With no repeat(auto) the style data always has one line more than > the number of tracks, IIRC. The array bounds check appears to be necessary to handle the case of tracks being generated to cover auto-positioned grid items, like extra rows at the bottom. The indicies iterate over computed track sizes, which may grow indefinitely, and the line name lists correctly end at the last explicit track.

Comment on attachment 8766115 [details] [diff] [review] Updated incremental patch to add line names > dom/base/Element.cpp > shell->FrameNeedsReflow(frame, nsIPresShell::eResize, NS_FRAME_IS_DIRTY); > shell->FlushPendingNotifications(Flush_Layout); > frame = GetPrimaryFrame(); Now that I think about it, we should do a GetContentInsertionFrame() and check GetType() on the new frame here too. While eResize can't mess with that, we could have an unrelated pending style changes that Flush_Layout will execute. It probably makes sense to factor this out to a static GetGridContainerFrame function and use it in both places. > The array bounds check appears to be necessary to handle the case > of tracks being generated to cover auto-positioned grid items Ah, makes sense. I was thinking we didn't call this method with an index beyond the last explicit line, because there are no names to be found there. So yeah, this is fine as is.

Addresses issues raised in comment 53. Computed grid info structures are now wholly protected behind the NS_STATE_GRID_GENERATE_COMPUTED_VALUES bit, and that bit is propegated to next-in-flow frames. The triggering of that reflow is fairly aggressive with a check added to the property getters GetComputedTemplateColumns(), etc. Mats: if this is too heavyweight, please advise a better location to call the new GenerateComputedGridInfoIfNecessary() function. Also, now that these patches are messing with reflow and not just pulling data out of reflow, please advise which mochitest directories I should be running before submitting patches for review.

whats needed to be checked-in here, both patches ?

Comment on attachment 8766576 [details] [diff] [review] Further revised incremental patch to add support for line names, and computing info structures only wheen needed > dom/base/Element.cpp > Element::GetGridContainerFrame(Element* aElement) nit: this should probably just be a static function at file scope (or a lambda in GetGridFragments) to avoid adding to the Element API. > layout/generic/nsGridContainerFrame.h > const ComputedGridTrackInfo* GetComputedTemplateColumns() > { >+ GenerateComputedGridInfoIfNecessary(); > return Properties().Get(GridColTrackInfo()); This will crash if 'this' was destroyed by a pending style change that GenerateComputedGridInfoIfNecessary flushed. Currently, that's unlikely to happen since Element::GetGridFragments already did that, but I think the idea here doesn't work in general. I don't see why we need this though. I think we should just MOZ_ASSERT that we have a non-null value here, and add a doc comment above these methods that says the caller is assumed to have requested the data by setting the relevant frame bit and triggered a reflow, with a reference to GetGridFragments. So I think we should move the flush code back to GetGridFragments.

Only attachment 8765953 [details] [diff] [review], gridTracks.patch is ready for check-in.

This is an incremental patch on top of attachment 8765953 [details] [diff] [review] that adds support of line names and is comprehensive of mats' feedback and also fixes the row position offset problem noted in comment 42. https://treeherder.mozilla.org/#/jobs?repo=try&revision=44057417ad15

Comment on attachment 8766897 [details] [diff] [review] gridLines.patch Please add a @note in the doc comment on GenerateComputedGridInfoIfNecessary saying that this method may destroy 'this' and other layout data since it may do a FlushPendingNotifications(Flush_Layout) With that, this looks ready to land. Nice work Brad, thanks!

Incremental patch to attachment 8765953 [details] [diff] [review] that adds support for grid line names. Addresses issues in comments 42 and 60. Added the reflow call to nsComputedDOMStyle::DoGetGridTemplateColumns, etc, to ensure that the layout/style/test/test_grid_computed_values.html works again (as shown failing in the treeherder build).

seems need rebasing patching file dom/moz.build Hunk #1 FAILED at 57 1 out of 1 hunks FAILED -- saving rejects to file dom/moz.build.rej patching file layout/generic/nsGridContainerFrame.cpp Hunk #3 FAILED at 5575 1 out of 3 hunks FAILED -- saving rejects to file layout/generic/nsGridContainerFrame.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh gridTracks.patch

Brad, can you attach a squashed rollup patch please? It's hard to see what the sum of the changes are with the attached patches. (i.e. "hg diff -r qparent" if you're using MQ)

Rebased patch, still r=heycam.

Incremental patch to attachment 8767254 [details] [diff] [review] Adds support for grid lines, and corrects some behavior with grid fragmentation.

Comment on attachment 8767260 [details] [diff] [review] gridLines3.patch >dom/base/Element.cpp >+GetGridContainerFrame(Element* aElement) This function isn't needed, right? >layout/generic/nsGridContainerFrame.cpp >+ ComputedGridTrackInfo* revisedPriorRowInfo = new ComputedGridTrackInfo( >+ priorRowInfo->mNumLeadingImplicitTracks, >+ priorRowInfo->mNumExplicitTracks, >+ priorRowInfo->mStartFragmentTrack, >+ gridReflowState.mStartRow, >+ Move(priorRowInfo->mPositions), >+ Move(priorRowInfo->mSizes), >+ Move(priorRowInfo->mStates)); >+ prevInFlow->Properties().Set(GridRowTrackInfo(), revisedPriorRowInfo); I think all of that can be replaced with: priorRowInfo->mEndFragmentTrack = gridReflowState.mStartRow; >+/** >+ * Create the properties that contain the computed grid info values. >+ * >+ * @note May force reflow which could delete this. If return value is true, >+ * ensure that the frame still exists. >+ */ >+bool >+nsGridContainerFrame::GenerateComputedGridInfoIfNecessary() Please move this comment to the header file instead, We generally don't have documentation in both places since they easily diverge. Also, the "May force reflow which could delete this" is misleading since reflow never deletes 'this', it's the FlushPendingNotifications(Flush_Layout) that might do that. >layout/style/nsComputedDOMStyle.cpp >nsComputedDOMStyle::DoGetGridTemplateColumns() > gridFrame = GetGridContainerFrame(mInnerFrame); 'mInnerFrame' might be destroyed here - use mContent->GetPrimaryFrame() instead. It might be worth adding a new protected method on nsComputedDOMStyle that does all this so we don't have to repeat it in *Rows(): /** * If we have a nsGridContainerFrame then ensure that is has generated * computed data. * @return nullptr if we're not a grid container, or our frame was destroyed * @note this might destroy layout/style data such as mInnerFrame/mOuterFrame */ nsGridContainerFrame* EnsureComputedGridInfo(); Then DoGetGridTemplateColumns/Rows() becomes nsGridContainerFrame* gridFrame = EnsureComputedGridInfo(); if (gridFrame) { info = gridFrame->GetComputedTemplateColumns/Rows(); } return ... Actually, it might be even better to make EnsureComputedGridInfo a static method on nsGridContainerFrame instead, so it can be used by Element::GetGridFragments too. i.e. static nsGridContainerFrame* EnsureComputedGridInfo(nsIFrame* aFrame); and call it with GetPrimaryFrame() in GetGridFragments, and with 'mInnerFrame' in DoGetGridTemplateColumns/Rows. WDYT? (feel free to do this in a follow-up bug though)

(In reply to Brad Werth [:bradwerth] from comment #54) > Also, now that these patches are messing with reflow and not just pulling > data out of reflow, please advise which mochitest directories I should be > running before submitting patches for review. I forgot to answer this bit. I think mochitests and reftests are appropriate. While you're not changing the rendering in any way, most Grid tests are reftests so they trigger a lot more edge cases than are covered by the mochitests, so it's probably a good idea to check that they don't crash/assert. Except now that this code is behind a frame bit, you might want to push this to Try with an extra test patch that unconditionally set the frame bit in Reflow to exercise this code on every reflow.

Incremental patch to attachment 8767254 [details] [diff] [review], comprehensive of all feedback up through comment 66. Adds support for grid lines and corrects behavior of generating info for fragmented grids.

Comment on attachment 8767434 [details] [diff] [review] gridLines4.patch Redirecting this review to Mats. Though one thing I noticed is that this brace should be on its own line: >+GetGridContainerFrame(nsIFrame* aFrame) {

Comment on attachment 8767434 [details] [diff] [review] gridLines4.patch > dom/base/Element.cpp >+GetGridContainerFrame(Element* aElement) This function can be removed now? > dom/base/Element.h >+class nsGridContainerFrame; This doesn't seem necessary. > layout/generic/nsGridContainerFrame.cpp >+ auto GetGridContainerFrame = [](nsIFrame *aFrame)->nsGridContainerFrame* { nit: spelling out the return type isn't really needed here, just makes it harder to read IMO > nsGridContainerFrame::GetGridFrameWithComputedInfo nit: I'd prefer s/outputFrame/gridFrame/ > layout/generic/nsGridContainerFrame.h >+ * These properties are created by a call to >+ * nsGridContainerFrame::GenerateComputedGridInfoIfNecessary, typically from >+ * Element::GetGridFragments. Asserts are in place to report a null property. s/GenerateComputedGridInfoIfNecessary/GetGridFrameWithComputedInfo/ nit: "Asserts are in place to report a null property." seems a bit superfluous since they are in plain view. >+ * @note this might destroy layout/style data such as mInnerFrame/mOuterFrame "such as mInnerFrame/mOuterFrame" doesn't make sense here. Probably a good idea to spell out the reason: @note this might destroy layout/style data since it may flush layout > layout/style/nsComputedDOMStyle.cpp >+GetGridContainerFrame(nsIFrame* aFrame) { This function can be removed now? Also, don't forget to update the commit message to describe all the changes in this patch, with the bug number and list of reviewers.

need also dom peer review for the first patch since i run into emote: WebIDL file dom/webidl/Element.webidl altered in changeset 912e1f6b27c4 without DOM peer review remote: WebIDL file dom/webidl/Grid.webidl altered in changeset 912e1f6b27c4 without DOM peer review r

Incremental to attachment 8767254 [details] [diff] [review]. Adds grid line properties and resolves problems with property generation for fragmented grids. r=mats

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3de5b79d7373 Expose decoded CSS grid track properties in a Chrome API. r=heycam, r=khuey https://hg.mozilla.org/integration/mozilla-inbound/rev/e98d1ac584d6 Expose decoded CSS grid line properties via a Chrome API. r=mats

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=31257799&repo=mozilla-inbound

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/75880714f9d8 Backed out changeset e98d1ac584d6 https://hg.mozilla.org/integration/mozilla-inbound/rev/623f0d0a8703 Backed out changeset 3de5b79d7373 for bustage

Only attachment 8767254 [details] [diff] [review] is ready for check-in. Looks like the problem appeared with the other patch, which is still being worked.

Updated to change all new constructors to explicit to sidestep an OS X static analysis error.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3396152c5e11 Expose decoded CSS grid track properties in a Chrome API. r=heycam, r=khuey

This is incremental to attachment 8768521 [details] [diff] [review]. It adds line names, corrects the behavior on fragmented grids, and contains explicit constructors in order to pass the static analysis checks on Treeherder. It also passed an "always-on" grid info generation test build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc46c8b62ebd. Will mark for checkin when attachment 8768521 [details] [diff] [review] lands.

Will do the checkin. Brad can you make sure next time the bug number is in the commit message :) this makes checkin-needed a little more easier for us :) thanks!

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4686084769 Expose decoded CSS grid line properties via a Chrome API. r=mats

Adds grid areas to exposed css grid properties, consolidates the implicit/explicit "type" of tracks, lines, and areas, and adds new tests. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1d4e428a092

Last try failed in an unclear way... here's a new one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2de83e2994fe

For tracking purposes, and to avoid having many separate discussions in the same bug, it's probably better to close this bug as fixed and open new bug(s) for additional work.

Created new bug 1289200 to track the effort of adding grid areas to these properties. Closing this bug as FIXED.

Comment on attachment 8770641 [details] [diff] [review] gridAreas1.patch Sorry for the really long delay. I was on PTO for a while. Anyway, I think the work on grid area has moved to another bug. And Gabriel Luong is a better person to ask now that he has formally started the development of the grid inspector.

Comment on attachment 8770641 [details] [diff] [review] gridAreas1.patch (this part moved to bug 1289200)