Bug 165006 - [css-grid] Isolate size of internal representation from actual grid size
Summary: [css-grid] Isolate size of internal representation from actual grid size
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on:
Blocks: 165007
  Show dependency treegraph
 
Reported: 2016-11-21 03:08 PST by Sergio Villar Senin
Modified: 2016-11-22 06:35 PST (History)
10 users (show)

See Also:


Attachments
Patch (7.24 KB, patch)
2016-11-21 03:13 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (7.45 KB, patch)
2016-11-22 02:29 PST, Sergio Villar Senin
rego: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2016-11-21 03:08:50 PST
[css-grid] Isolate size of internal representation from actual grid size
Comment 1 Sergio Villar Senin 2016-11-21 03:13:58 PST
Created attachment 295289 [details]
Patch
Comment 2 Javier Fernandez 2016-11-21 03:31:30 PST
Comment on attachment 295289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295289&action=review

In the ChangeLog it's explained very well the problem, but not so clear, IMO, why this patch solves the issue. I'd add some lines to show that we have now 2 different functions to get the size of the grid, one using the pysical representation and the other one, using the logical one.

Other than that, the change looks good.

> Source/WebCore/ChangeLog:13
> +        memory allocations are done to create a grid representation it as it is not needed.

typo: "representation *it as it*" -> "representation *as it*
Comment 3 Sergio Villar Senin 2016-11-22 02:29:02 PST
Created attachment 295333 [details]
Patch
Comment 4 Javier Fernandez 2016-11-22 02:47:47 PST
Comment on attachment 295333 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295333&action=review

> Source/WebCore/rendering/RenderGrid.cpp:2713
> +unsigned RenderGrid::numTracks(GridTrackSizingDirection direction) const

As we talked privately, what do you think about figuring out a more descriptive name for this function and the one we already have, gridColumnCount ? The difference between them is not obvious at all.
Comment 5 Sergio Villar Senin 2016-11-22 03:13:43 PST
(In reply to comment #4)
> Comment on attachment 295333 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295333&action=review
> 
> > Source/WebCore/rendering/RenderGrid.cpp:2713
> > +unsigned RenderGrid::numTracks(GridTrackSizingDirection direction) const
> 
> As we talked privately, what do you think about figuring out a more
> descriptive name for this function and the one we already have,
> gridColumnCount ? The difference between them is not obvious at all.

Right, I agree the names are not very good, but I'd prefer to change them later after finishing the refactoring if you don't feel strongly about it (I have a lot of dependent patches in my queue, so this kind of changes will force me to do a lot of conflict resolution).
Comment 6 Javier Fernandez 2016-11-22 04:16:09 PST
(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 295333 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=295333&action=review
> > 
> > > Source/WebCore/rendering/RenderGrid.cpp:2713
> > > +unsigned RenderGrid::numTracks(GridTrackSizingDirection direction) const
> > 
> > As we talked privately, what do you think about figuring out a more
> > descriptive name for this function and the one we already have,
> > gridColumnCount ? The difference between them is not obvious at all.
> 
> Right, I agree the names are not very good, but I'd prefer to change them
> later after finishing the refactoring if you don't feel strongly about it (I
> have a lot of dependent patches in my queue, so this kind of changes will
> force me to do a lot of conflict resolution).

Sure, no problem with waiting until the refactoring is completed.
Comment 7 Manuel Rego Casasnovas 2016-11-22 04:20:45 PST
Comment on attachment 295333 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295333&action=review

LGTM.

> Source/WebCore/rendering/RenderGrid.h:195
>      unsigned gridColumnCount() const;
>      unsigned gridRowCount() const;
> +    unsigned numTracks(GridTrackSizingDirection) const;

I'd add a FIXME on these 3 methods to look for better names once the refactoring is done.
Comment 8 Sergio Villar Senin 2016-11-22 06:35:17 PST
Committed r208962: <http://trac.webkit.org/changeset/208962>