[css-grid] Simplify the interface of GridResolvedPosition

[css-grid] Simplify the interface of GridResolvedPosition

The interface of GridResolvedPosition is full of static methods that are
used only internally, we should not expose them.

Apart from that the resolveXXX() methods return pointers because we wanted
to have a wait to notify the caller that the resolution did not succeed.
Instead of doing that, resolveGridPositionsFromStyle() do always return
a valid GridSpan from now on, meaning that the caller has to ensure that the
resolution does not require running the auto-placement algorithm. A new
class called GridUnresolvedSpan was added for that purpose.

[svillar, 2015-11-09 11:31:22]

svillar@igalia.com changed reviewers:
+ cbiesinger@chromium.org, jfernandez@igalia.com, rego@igalia.com

[svillar, 2015-11-09 11:31:22]

Sending for review

[Manuel Rego, 2015-11-10 09:08:03]

Non-owner LGTM.

Nice refactoring, just some small comments inline.

https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1450: offset =
LayoutUnit(0);
Why LayoutUnit(0)?
We're using LayoutUnit() all over the place.

https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1477: LayoutUnit start =
startIsAuto ? LayoutUnit() : (direction == ForColumns) ? 
m_columnPositions[initialPosition.toInt()] :
m_rowPositions[initialPosition.toInt()];
Now that we've isForColumns we can keep using it here and the next line too.

https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/GridPosition.h (right):

https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridPosition.h:52: 
Why are you moving the enum it's mostly used in GridResolvedPosition?

https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/GridResolvedPosition.h (right):

https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridResolvedPosition.h:120: static
PassOwnPtr<GridSpan> resolveNamedGridLinePositionAgainstOppositePosition(const
ComputedStyle&, const GridResolvedPosition& resolvedOppositePosition, const
GridPosition&, GridPositionSide);
Nit: Probably this should be plural ("Positions" insteasd of "Position")
like other methods.

[svillar, 2015-11-10 10:56:38]

Thanks for the review!

https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1450: offset =
LayoutUnit(0);
On 2015/11/10 09:08:03, Manuel Rego wrote:
> Why LayoutUnit(0)?
> We're using LayoutUnit() all over the place.

Yeah LayoutUnit() is more efficient, I thought it was the other way around.

https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1477: LayoutUnit start =
startIsAuto ? LayoutUnit() : (direction == ForColumns) ? 
m_columnPositions[initialPosition.toInt()] :
m_rowPositions[initialPosition.toInt()];
On 2015/11/10 09:08:03, Manuel Rego wrote:
> Now that we've isForColumns we can keep using it here and the next line too.

Acknowledged.

https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/GridPosition.h (right):

https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridPosition.h:52: 
On 2015/11/10 09:08:03, Manuel Rego wrote:
> Why are you moving the enum it's mostly used in GridResolvedPosition?

Definitions do not belong to the place they're used :)

IMHO this is a better place for it, it's something that belongs to GridPosition
semantics not GridResolvedPosition's.

https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/GridResolvedPosition.h (right):

https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridResolvedPosition.h:120: static
PassOwnPtr<GridSpan> resolveNamedGridLinePositionAgainstOppositePosition(const
ComputedStyle&, const GridResolvedPosition& resolvedOppositePosition, const
GridPosition&, GridPositionSide);
On 2015/11/10 09:08:03, Manuel Rego wrote:
> Nit: Probably this should be plural ("Positions" insteasd of "Position")
> like other methods.

Actually that method should not be there, we must not return PassOwnPtr anymore.

[svillar, 2015-11-11 11:12:41]

The CQ bit was checked by svillar@igalia.com

[svillar, 2015-11-11 11:12:41]

The patchset sent to the CQ was uploaded after l-g-t-m from rego@igalia.com
Link to the patchset: https://codereview.chromium.org/1424913009/#ps20001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-11-11 11:12:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424913009/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424913009/20001

[commit-bot: I haz the power, 2015-11-11 11:16:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-11 11:16:46]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[svillar, 2015-11-11 12:57:13]

Removed unused constructor

[svillar, 2015-11-11 13:45:49]

svillar@igalia.com changed reviewers:
+ rune@opera.com

[svillar, 2015-11-11 13:45:49]

rune@ could you please take a look? We need an OWNER for the core/style/
changes.

The patch looks more complicated than it should because it involves moving code
around but essentially is just about simplifying the interface removing
non-public methods and replacing the use of pointers by a new class
GridUnresolvedPosition.

[svillar, 2015-11-11 15:18:00]

svillar@igalia.com changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-11-11 18:02:01]

Can you do the move and the simplification in two patches?

[rune, 2015-11-11 18:16:52]

https://codereview.chromium.org/1424913009/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/GridResolvedPosition.cpp (right):

https://codereview.chromium.org/1424913009/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:13: static
GridSpan resolveNamedGridLinePositionAgainstOppositePosition(const
ComputedStyle& gridContainerStyle, const GridResolvedPosition&
resolvedOppositePosition, const GridPosition&, GridPositionSide);
Why did you need to declare this? Couldn't you just move the implementation
above where it is used?

https://codereview.chromium.org/1424913009/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:342: return
unresolvedSpan.adjustGridPositionsFromStyle(gridContainerStyle);
I would make adjustGridPositionsFromStyle return void and return unresolvedSpan
below to make it clear that it modifies the object and don't return a new value
based on the callee. Returning a non-const *this reference from
adjustGridPositionsFromStyle which in this case is a stack-allocated object
looks unnecessarily scary.

[Manuel Rego, 2015-11-12 10:01:24]

I've been thinking about all the placement structs/classes we currently have
and I'm not sure if they're the right structure anymore.

Since a while ago, the grid spec has changed and it allows now
to add implicit tracks before the explicit grid (on the top/left).
There's a bug to fix that, but now more issues are depending on it
as more parts of the spec need this behavior:
https://code.google.com/p/chromium/issues/detail?id=444011

We'd need to differentiate things like "grid-column: auto / 1;"
vs "grid-column: auto / 2;". And for that we might need to store
the grid lines instead of the GridResolvedPositions.

I'm still considering different alternatives, but if my current approach is
right
we might be refactoring these structures and code in a while.
So I'd keep this patch on hold until we clarify those issues,
and maybe reject it eventually.

I'd add more info in the bug commented above
explaining my conclusions on the topic.

[svillar, 2015-11-12 16:06:17]

On 2015/11/12 10:01:24, Manuel Rego wrote:
> I've been thinking about all the placement structs/classes we currently have
> and I'm not sure if they're the right structure anymore.
> 
> Since a while ago, the grid spec has changed and it allows now
> to add implicit tracks before the explicit grid (on the top/left).
> There's a bug to fix that, but now more issues are depending on it
> as more parts of the spec need this behavior:
> https://code.google.com/p/chromium/issues/detail?id=444011
> 
> We'd need to differentiate things like "grid-column: auto / 1;"
> vs "grid-column: auto / 2;". And for that we might need to store
> the grid lines instead of the GridResolvedPositions.
> 
> I'm still considering different alternatives, but if my current approach is
> right
> we might be refactoring these structures and code in a while.
> So I'd keep this patch on hold until we clarify those issues,
> and maybe reject it eventually.
> 
> I'd add more info in the bug commented above
> explaining my conclusions on the topic.

OK let's forget about this one then.