[css-grid] Initial support for implicit grid before explicit grid

third_party/WebKit/Source/core/style/GridCoordinate.h

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 32 matching lines @@
 // Recommended maximum size for both explicit and implicit grids.
 const size_t kGridMaxTracks = 1000000;
 
 // A span in a single direction (either rows or columns). Note that |resolvedInitialPosition|
 // and |resolvedFinalPosition| are grid lines' indexes.
 // Iterating over the span shouldn't include |resolvedFinalPosition| to be correct.
 struct GridSpan {
     USING_FAST_MALLOC(GridSpan);
 public:
 
+    static GridSpan untranslatedGridSpan(int resolvedInitialPosition, int resolvedFinalPosition)

[jfernandez, 2015/12/18 14:33:21]

I find the 'unstranslated" concept a bit obscure. Also, I'd expect something not
translated as the regular thing, so why adding a prefix for defining it ? 

[Manuel Rego, 2015/12/18 22:42:30]

Untranslated is only used during the code to resolve positions.
The rest (most) of the code use the translated positions.

Probably you can find more context in the following document:
http://bit.ly/1j3CRdi

+    {
+        return GridSpan(resolvedInitialPosition, resolvedFinalPosition, Untranslated);
+    }
+
     static GridSpan definiteGridSpan(size_t resolvedInitialPosition, size_t resolvedFinalPosition)
     {
         return GridSpan(resolvedInitialPosition, resolvedFinalPosition, Definite);
     }
 
     static GridSpan indefiniteGridSpan()
     {
         return GridSpan(0, 1, Indefinite);
     }
 
     bool operator==(const GridSpan& o) const
     {
         return m_type == o.m_type && m_resolvedInitialPosition == o.m_resolvedInitialPosition && m_resolvedFinalPosition == o.m_resolvedFinalPosition;
     }
 
     size_t integerSpan() const
     {
         ASSERT(isDefinite());
         ASSERT(m_resolvedFinalPosition > m_resolvedInitialPosition);
         return m_resolvedFinalPosition - m_resolvedInitialPosition;
     }
 
+    int untranslatedResolvedInitialPosition() const
+    {
+        ASSERT(m_type == Untranslated);
+        return m_resolvedInitialPosition;
+    }
+
+    int untranslatedResolvedFinalPosition() const
+    {
+        ASSERT(m_type == Untranslated);
+        return m_resolvedFinalPosition;
+    }
+
     size_t resolvedInitialPosition() const
     {
         ASSERT(isDefinite());
+        ASSERT(m_resolvedInitialPosition >= 0);
         return m_resolvedInitialPosition;
     }
 
     size_t resolvedFinalPosition() const
     {
         ASSERT(isDefinite());
-        ASSERT(m_resolvedFinalPosition);
+        ASSERT(m_resolvedFinalPosition > 0);
         return m_resolvedFinalPosition;
     }
 
     struct GridSpanIterator {
         GridSpanIterator(size_t v) : value(v) {}
 
         size_t operator*() const { return value; }
         size_t operator++() { return value++; }
         bool operator!=(GridSpanIterator other) const { return value != other.value; }
 
@@ skipping 10 matching lines @@
     {
         ASSERT(isDefinite());
         return m_resolvedFinalPosition;
     }
 
     bool isDefinite() const
     {
         return m_type == Definite;
     }
 
+    bool isIndefinite() const
+    {
+        return m_type == Indefinite;
+    }

[svillar, 2015/12/18 13:08:30]

I don't like this API because is super-confusing. 

If isDefinite() returns false then most of developers will think that the
position is definite. It's quite weird that !isDefinite() != isIndefinite(),
this could be a source of programming errors.

Also it looks to me that we need some more ASSERTs to check that we're not using
untranslated positions like for example when using and iterator

[Manuel Rego, 2015/12/18 22:42:30]

I agree this is confusing.

So I've changed the possible types of GridSpan, now the possible types are:
UntranslatedDefinite, TranslatedDefinite and Indefinite.

Indefinite are unknown positions so we don't need to know
if they're untranslated or translated, those don't make sense
for indefinite positions.

The general idea is that during the position resolution code,
we always work with UntranslatedDefinite or Indefinite positions.
Then, in placeItemsOnGrid() we translate those positions to
TransaltedDefinite and the rest of the code always use
translated positions.

I've also renamed isDefinite() to isTranslatedDefinite() so the meaning is
clear.

I hope this changes make the patch more readable
but any other suggestion is welcomed.
We already had ASSERTs in begin() and end() checking
that we're working with translated positions.

+
+    void translate(size_t offset)
+    {
+        ASSERT(m_type == Untranslated);
+
+        m_type = Definite;
+        m_resolvedInitialPosition += offset;
+        m_resolvedFinalPosition += offset;
+

[svillar, 2015/12/18 13:08:30]

Looks like it's missing the kGridMaxTrack checks?

[Manuel Rego, 2015/12/18 22:42:30]

I don't think we need them here.
We've already the checks in GridSpan constructor,
so we know we've as maximum kGridMaxTrack * 2 tracks.

This will be just translating the positions to positive values,
but not creating extra tracks.

+        ASSERT(m_resolvedInitialPosition >= 0);
+        ASSERT(m_resolvedFinalPosition > 0);

[jfernandez, 2015/12/18 14:33:21]

I don't get the purpose of these asserts. If 'offset' is an unsigned value, how
this operation may result in a m_resolvedXXX fields attributes negative or zero
? 

[Manuel Rego, 2015/12/18 22:42:30]

I think you're missing the fact that m_resolvedInitialPosition can be negative.
Imagine that m_resolvedInitialPosition is "-5", and we pass an offest of "2"
(because of we calculate it wrong).
We'll end up in "-3" which is not a translated position (it should be >= 0).

+    }
+
 private:
 
-    enum GridSpanType {Definite, Indefinite};
+    enum GridSpanType {Untranslated, Definite, Indefinite};
 
-    GridSpan(size_t resolvedInitialPosition, size_t resolvedFinalPosition, GridSpanType type)
-        : m_resolvedInitialPosition(std::min(resolvedInitialPosition, kGridMaxTracks - 1))
-        , m_resolvedFinalPosition(std::min(resolvedFinalPosition, kGridMaxTracks))
-        , m_type(type)
+    GridSpan(int resolvedInitialPosition, int resolvedFinalPosition, GridSpanType type)
+        : m_type(type)
     {
+#if ENABLE(ASSERT)
         ASSERT(resolvedInitialPosition < resolvedFinalPosition);
+        if (type == Definite) {
+            ASSERT(resolvedInitialPosition >= 0);
+            ASSERT(resolvedFinalPosition > 0);
+        }
+#endif
+
+        if (resolvedInitialPosition >= 0)
+            m_resolvedInitialPosition = std::min(resolvedInitialPosition, static_cast<int>(kGridMaxTracks) - 1);

[svillar, 2015/12/18 13:08:30]

Just convert kGridMaxTracks to int and remove the casts.

[jfernandez, 2015/12/18 14:33:21]

I think many other casts have been added because of this; we may want to remove
them all.

[Manuel Rego, 2015/12/18 22:42:30]

Done.

[Manuel Rego, 2015/12/18 22:42:30]

I've converted it to int so I think we don't have issues anymore.

+        else
+            m_resolvedInitialPosition = std::max(resolvedInitialPosition, -static_cast<int>(kGridMaxTracks));
+
+        if (resolvedFinalPosition >= 0)
+            m_resolvedFinalPosition = std::min(resolvedFinalPosition, static_cast<int>(kGridMaxTracks));
+        else
+            m_resolvedFinalPosition = std::max(resolvedFinalPosition, -static_cast<int>(kGridMaxTracks) + 1);
     }
 
-    size_t m_resolvedInitialPosition;
-    size_t m_resolvedFinalPosition;
+    int m_resolvedInitialPosition;
+    int m_resolvedFinalPosition;
     GridSpanType m_type;
 };
 
 // This represents a grid area that spans in both rows' and columns' direction.
 struct GridCoordinate {
     USING_FAST_MALLOC(GridCoordinate);
 public:
     // HashMap requires a default constuctor.
     GridCoordinate()
         : columns(GridSpan::indefiniteGridSpan())
@@ skipping 19 matching lines @@
 
     GridSpan columns;
     GridSpan rows;
 };
 
 typedef HashMap<String, GridCoordinate> NamedGridAreaMap;
 
 } // namespace blink
 
 #endif // GridCoordinate_h