[webkit-reviews] review granted: [Bug 133543] [CSS Grid Layout] Simplify the named grid lines resolution algorithm : [Attachment 232559] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 133543: [CSS Grid Layout] Simplify the named grid lines resolution
algorithm
https://bugs.webkit.org/show_bug.cgi?id=133543

Attachment 232559: Patch
https://bugs.webkit.org/attachment.cgi?id=232559&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=232559&action=review


I’ll say review+, but the use of a vector and a std::sort each time we add
something is definitely not the right data structure / algorithm choice.

> Source/WebCore/css/StyleResolver.cpp:1862
> +    for (auto it = namedGridAreas.begin(), end = namedGridAreas.end(); it !=
end; ++it) {

This should use a modern for loop:

    for (auto& area : namedGridAreas)

Then below instead of "it->" it would be "area."

> Source/WebCore/css/StyleResolver.cpp:1868
> +	   {
> +	       NamedGridLinesMap::AddResult startResult =
namedGridLines.add(it->key + "-start", Vector<size_t>());
> +	      
startResult.iterator->value.append(areaSpan.initialPositionIndex);
> +	       std::sort(startResult.iterator->value.begin(),
startResult.iterator->value.end());
> +	   }

This code only uses the value, so I would write more like this:

    auto& startVector = namedGridLines.add(it->key + "-start",
Vector<size_t>()).iterator->value;
    startVector.append(areaSpan.initialPositionIndex);
    std::sort(vector.begin(), vector.end());

But also, to keep a vector sorted, it is *not* efficient to use keep adding
items to the end and then doing a std::sort on the whole vector each time.
There are webpages telling you not to do this, such as
<http://stackoverflow.com/questions/2710221/is-there-a-sorted-vector-class-whic
h-supports-insert-etc>. Please rethink the data structure here.

I also don’t understand the reason for the use of size_t for this. Is it the
size of some memory object? If not, then it should be some other type, maybe
unsigned or even uint64_t.

> Source/WebCore/css/StyleResolver.cpp:1873
> +	   {
> +	       NamedGridLinesMap::AddResult endResult =
namedGridLines.add(it->key + "-end", Vector<size_t>());
> +	       endResult.iterator->value.append(areaSpan.finalPositionIndex +
1);
> +	       std::sort(endResult.iterator->value.begin(),
endResult.iterator->value.end());
> +	   }

Same comment again.

> Source/WebCore/css/StyleResolver.cpp:2776
> +	   NamedGridLinesMap namedGridColumnLines =
state.style()->namedGridColumnLines();
> +	   NamedGridLinesMap namedGridRowLines =
state.style()->namedGridRowLines();

This looks kind of expensive. Do we really want to copy hash maps here?

> Source/WebCore/rendering/RenderGrid.cpp:988
> +	   const String namedGridLine = position.namedGridLine();

Not sure why this is "const String" instead of just String.

> Source/WebCore/rendering/RenderGrid.cpp:992
> +	   auto implicitLineIter =
gridLineNames.find(implicitNamedGridLineForSide(namedGridLine, side));

The use of "xxxIter" here is not a normal naming style we use in WebKit. I
think you could just leave off the Iter suffix, or use the entire word
Iterator, or think of another way to name it.