Closed Bug 1009282 Opened 6 years ago Closed 6 years ago

[css-grid] Add primitive support for implicit named areas

Categories

(Core :: Layout, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch fix (obsolete) — Splinter Review
Very much a temporary solution, but it's good enough for now and it'll
allow me to write tests sooner for upcoming grid item placement code.
(I'm using nsCheapSets::Clear() so it depends on bug 1009263)

I think eventually this code should probably be moved to the style system.
We can put it behind a getter method to populate it lazily.
Attachment #8421390 - Flags: review?(dholbert)
Depends on: 1009263
Comment on attachment 8421390 [details] [diff] [review]
fix

>Bug 1009282 - Add primitive support for implicit named areas.  r=dholbert

Probably worth mentioning "grid" somewhere in the commit message.

>diff --git a/layout/generic/nsGridContainerFrame.cpp b/layout/generic/nsGridContainerFrame.cpp
>+static bool
>+IsNameWithSuffix(const nsString& aString, const char* aSuffix, int32_t* aIndex)
>+{
>+  int32_t index = aString.RFind(aSuffix);
>+  if (index <= 0) {
>+    // No match or nothing before the suffix.
>+    return false;
>+  }

Why RFind? We're looking for a suffix, so isn't it more efficient (and more correct) to just directly check the end of the string for equality with aSuffix?

>+void
>+AddImplicitNamedAreas(const nsTArray<nsTArray<nsString>>& aLineNameLists,
>+                      nsCheapSet<nsStringHashKey>& aImplicitAreaNames)
>+{
>+  // XXX this just checks x-start .. x-end in one dimension and there's
>+  // no other error checking.  A few wrong cases (maybe):
>+  // (x-start x-end)
>+  // (x-start) 0 (x-start) 0 (x-end)
>+  // (x-end) 0 (x-start) 0 (x-end)
>+  // (x-start) 0 (x-end) 0 (x-start) 0 (x-end)
>+  const uint32_t len = aLineNameLists.Length();
>+  nsTHashtable<nsStringHashKey> currentStarts;
>+  for (uint32_t i = 0; i < len; ++i) {
>+    const nsTArray<nsString>& names(aLineNameLists[i]);
>+    const uint32_t len = names.Length();
>+    for (uint32_t i = 0; i < len; ++i) {

I don't love the |len| & |i| variable-shadowing here... Won't this trigger -Wshadow GCC build warnings? I guess those aren't enabled right now, but it looks like dbaron wants them to be, per bug 563195.

So, maybe use differentiated variable names...

>+      int32_t index;
>+      if (::IsNameWithStartSuffix(name, &index)) {
>+        currentStarts.PutEntry(nsDependentSubstring(name, 0, index));
>+      } else if (::IsNameWithEndSuffix(name, &index)) {
>+        nsDependentSubstring area(name, 0, index);
>+        if (currentStarts.Contains(area)) {
>+          aImplicitAreaNames.Put(area);
>+        }

Looks like we're discarding the start/end index of the implicit named area, and then we have to re-derive that later (in FindNamedLine in attachment 8421973 [details] [diff] [review]).

Wouldn't it be better to just map the name to the start/end row/col, so that we don't have to do another walk through the lines every time an area-name is used?

> void
>+nsGridContainerFrame::InitImplicitNamedAreas()
>+{
>+  // http://dev.w3.org/csswg/css-grid/#implicit-named-areas
>+  mImplicitAreaNames.Clear();
>+  const nsStylePosition& pos = *StylePosition();
>+  ::AddImplicitNamedAreas(pos.mGridTemplateColumns.mLineNameLists,
>+                          mImplicitAreaNames);
>+  ::AddImplicitNamedAreas(pos.mGridTemplateRows.mLineNameLists,
>+                          mImplicitAreaNames);

I think it's a bit more efficient to grab the StylePosition() off of the nsHTMLReflowState. (It's got a cached pointer.)

So, perhaps have this method take a 'const nsStylePosition*', and have the caller pass in nsHTMLReflowState.StylePosition()?

(I think we're also pretty consistent about dealing with style structs via pointers, not references; seems like we might as well stick with that convention here.)

>+ private:
>+  /**
>+   * The implicit area names that comes from x-start .. x-end lines in
>+   * grid-template-columns / grid-template-rows.
>+   */
>+  nsCheapSet<nsStringHashKey> mImplicitAreaNames;

I'm not sure nsCheapSet is what we want here. For one thing, I think we might want to be mapping to the line start/end, as noted above (so -- we'd need a map, not a set). And space-wise, we could still optimize for emptiness-being-the-common-case by putting the set in the frame's property table, I think.

What do you think?
(Just verified with a local c++ testcase that a nested loop, w/ "len" & "i" being shadowed, does indeed trigger build warnings if you build with -Wshadow.)
(and it looks like -Wshadow is turned on by default, for GCC, in layout/style at least:
  http://mxr.mozilla.org/mozilla-central/source/layout/style/Makefile.in#6
though not in layout/generic for now.)
Comment on attachment 8421390 [details] [diff] [review]
fix

>diff --git a/layout/generic/nsGridContainerFrame.h b/layout/generic/nsGridContainerFrame.h
> protected:
[...]
> #if DEBUG
>   void SanityCheckAnonymousGridItems() const;
> #endif // DEBUG
>+
>+ private:
>+  /**
>+   * The implicit area names that comes from x-start .. x-end lines in
>+   * grid-template-columns / grid-template-rows.
>+   */
>+  nsCheapSet<nsStringHashKey> mImplicitAreaNames;

One other nit: no need to add a "private" section here -- this is already in the 'protected' section, and the class is MOZ_FINAL, which means protected is the same as private.

(I'd be fine with changing the existing 'protected' section to 'private', if you like, though.)
[adding dependency on bug 1009272, since the patch here layers on top of that bug's added code, in the .h file's context in particular]
Depends on: 1009272
Attached patch fix, v2Splinter Review
(In reply to Daniel Holbert [:dholbert] from comment #2)
> Probably worth mentioning "grid" somewhere in the commit message.

Fixed.

> Why RFind? We're looking for a suffix, [...]

Right, finding the right method to use down in xpcom string hell can be
a bit of struggle sometimes :-)  I eventually found StringEndsWith() in
nsReadableUtils.h which I couldn't find initially.

> I don't love the |len| & |i| variable-shadowing here...

Fair enough, renamed.

> Looks like we're discarding the start/end index of the implicit named area,
> and then we have to re-derive that later [...]

Yeah, we could optimize this I guess, but I plan to move this into some
style system struct anyway at some point and I don't want to spend time
on it right now.  If you don't mind, I'll file a bug on it and punt this
optimization until later.

FYI, my priority at this point is to get the item placement plus some
some primitive track sizing landed.  At that point I can write some
regression tests so we can detect errors going forward with more advanced
layout.

> I think it's a bit more efficient to grab the StylePosition() off of the
> nsHTMLReflowState. (It's got a cached pointer.)
> 
> So, perhaps have this method take a 'const nsStylePosition*', and have the
> caller pass in nsHTMLReflowState.StylePosition()?

Fixed as suggested.

> I'm not sure nsCheapSet is what we want here. For one thing, I think we
> might want to be mapping to the line start/end, as noted above (so -- we'd
> need a map, not a set). And space-wise, we could still optimize for
> emptiness-being-the-common-case by putting the set in the frame's property
> table, I think.

OK, I changed it to use a Hashtable in a frame property instead.

> One other nit: no need to add a "private" section here -- this is already in
> the 'protected' section, and the class is MOZ_FINAL, which means protected
> is the same as private.

True, but the public/protected/private sections are nice for a documentation
purpose as well since they convey the author's idea of which properties belongs
where, even if for final classes the protected part isn't really "used".
I think I prefer it the way it is for that reason.  But yeah, I guess I could
change s/protected/private/ so that there are two private sections if you
think a protected section might be confusing to someone.
Attachment #8421390 - Attachment is obsolete: true
Attachment #8421390 - Flags: review?(dholbert)
Attachment #8544704 - Flags: review?(dholbert)
(In reply to Mats Palmgren (:mats) from comment #7)
> True, but the public/protected/private sections are nice for a documentation
> purpose as well since they convey the author's idea of which properties
> belongs
> where, even if for final classes the protected part isn't really "used".
> I think I prefer it the way it is for that reason.

(I'm not sure I understand what sort of distinction there would be, for a class that's not meant to be subclassed.  But it's a moot point now, since the private member-var from the previous patch-version is now gone [replaced with a frame property].)
Comment on attachment 8544704 [details] [diff] [review]
fix, v2

Review of attachment 8544704 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following:

::: layout/generic/nsGridContainerFrame.cpp
@@ +15,5 @@
>  
>  using namespace mozilla;
>  
> +// Return true if aString ends in aSuffix, and if so, assign aIndex to where
> +// the suffix starts.

The first part ("Return true if aString ends in aSuffix") isn't quite true -- this function also requires that there are characters before the suffix. That's worth mentioning in this comment, I think.

@@ +18,5 @@
> +// Return true if aString ends in aSuffix, and if so, assign aIndex to where
> +// the suffix starts.
> +static bool
> +IsNameWithSuffix(const nsString& aString, const nsString& aSuffix,
> +                 int32_t* aIndex)

The aIndex parameter here should be uint32_t* -- not int32t* -- because:
 (a) it doesn't actually need to be able to represent negative values (it's an index in a string)
 (b) at the callsite, it ends up being passed to the nsDependentSubstring constructor, which requires a uint32_t (not an int32_t).  So right now it's doing an implicit conversion there, and it'd be better not to have that implicit conversion.

@@ +22,5 @@
> +                 int32_t* aIndex)
> +{
> +  if (StringEndsWith(aString, aSuffix)) {
> +    *aIndex = aString.Length() - aSuffix.Length();
> +    return *aIndex > 0 ? true : false;

Note: StringEndsWith ensures that aString is at least as long as aSuffix (it bails early if not), so we don't actually need to check for negative values here.  (This is why it's OK for aIndex to become unsigned, in my above comment; this subtraction will only produce nonnegative values.)

So, maybe this ternary-condition check should be "!= 0" instead of "> 0", to avoid giving the impression that we're screening for negative values?  (particularly after aIndex becomes unsigned & can no longer represent negative values -- at which point ">0" will appear to be a potentially-buggy check)

@@ +90,5 @@
> +    for (uint32_t j = 0; j < jLen; ++j) {
> +      const nsString& name = names[j];
> +      int32_t index;
> +      if (::IsNameWithStartSuffix(name, &index)) {
> +        currentStarts.PutEntry(nsDependentSubstring(name, 0, index));

per above, 'index' should be a uint32_t here.

(Also, maybe worth asserting that 'index' is nonzero here, & at the other nsDependentString instantiation, to make it clear that we can't end up with the empty string as an area-name)

::: layout/generic/nsGridContainerFrame.h
@@ +45,5 @@
>  
> +  /**
> +   * XXX temporary - move the ImplicitNamedAreas stuff to the style system.
> +   * The implicit area names that comes from x-start .. x-end lines in
> +   * grid-template-columns / grid-template-rows is stored in this frame

Two grammar nits:
 s/comes from/come from/
 s/is stored/are stored/

(The sentence subject is plural -- "The implicit area names")
Attachment #8544704 - Flags: review?(dholbert) → review+
Good points, thanks.  With the nits above fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a69b0f98e791
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/a69b0f98e791
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.