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

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

Index: third_party/WebKit/Source/core/style/GridCoordinate.h
diff --git a/third_party/WebKit/Source/core/style/GridCoordinate.h b/third_party/WebKit/Source/core/style/GridCoordinate.h
index bd1592f545e972c8a90b04c2ccdb6dff4efb3f72..ad9514db219500ea09a0e55f6a6eb5c3063303d6 100644
--- a/third_party/WebKit/Source/core/style/GridCoordinate.h
+++ b/third_party/WebKit/Source/core/style/GridCoordinate.h
@@ -50,6 +50,11 @@ 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);
@@ -72,16 +77,29 @@ public:
         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;
     }
 
@@ -112,20 +130,51 @@ public:
         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;
 };