Bug 177130

Summary: [css-grid] Small refactoring adding RenderGrid::contentAlignment()
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: Layout and RenderingAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, buildbot, commit-queue, jfernandez, simon.fraser, svillar, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
=Patch for landing none

Description Manuel Rego Casasnovas 2017-09-19 03:43:43 PDT
[css-grid] Small refactoring adding RenderGrid::contentAlignment()
Comment 1 Manuel Rego Casasnovas 2017-09-19 03:44:38 PDT
Created attachment 321194 [details]
Patch
Comment 2 Build Bot 2017-09-19 03:46:45 PDT
Attachment 321194 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderGrid.cpp:1586:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Manuel Rego Casasnovas 2017-09-19 03:55:21 PDT
Created attachment 321195 [details]
Patch
Comment 4 Javier Fernandez 2017-09-19 05:29:31 PDT
Comment on attachment 321195 [details]
Patch

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

The change looks good to me.

> Source/WebCore/ChangeLog:10
> +        that is from RenderGrid and GridTrackSizingAlgorithm.

I think there is something missing in this sentence; perhaps "accessible" ?
Comment 5 Manuel Rego Casasnovas 2017-09-19 06:18:30 PDT
Comment on attachment 321195 [details]
Patch

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

Thanks for the review, uploading new version fixing ChangeLog.

>> Source/WebCore/ChangeLog:10
>> +        that is from RenderGrid and GridTrackSizingAlgorithm.
> 
> I think there is something missing in this sentence; perhaps "accessible" ?

Actually I was thinking in "that is used from", but indeed I didn't write it.
Comment 6 Manuel Rego Casasnovas 2017-09-19 07:06:37 PDT
Created attachment 321197 [details]
Patch
Comment 7 Sergio Villar Senin 2017-09-22 02:46:35 PDT
Comment on attachment 321197 [details]
Patch

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

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1078
> +        || (m_renderGrid->contentAlignment(m_direction).distribution() != ContentDistributionStretch))

Very nice. Do we still need resolvedJustifyContentPosition() and resolvedJustifyContentDistribution() ?
Comment 8 Javier Fernandez 2017-09-22 02:50:11 PDT
Comment on attachment 321197 [details]
Patch

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

>> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1078
>> +        || (m_renderGrid->contentAlignment(m_direction).distribution() != ContentDistributionStretch))
> 
> Very nice. Do we still need resolvedJustifyContentPosition() and resolvedJustifyContentDistribution() ?

Perhaps not, but I think they are both useful functions when only needing specific content alignment values.
Comment 9 Manuel Rego Casasnovas 2017-09-25 00:33:32 PDT
(In reply to Sergio Villar Senin from comment #7)
> Comment on attachment 321197 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321197&action=review
> 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1078
> > +        || (m_renderGrid->contentAlignment(m_direction).distribution() != ContentDistributionStretch))
> 
> Very nice. Do we still need resolvedJustifyContentPosition() and
> resolvedJustifyContentDistribution() ?

Some of these are still used by RenderFlexibleBox so I guess
it's not an issue to keep them.
Comment 10 WebKit Commit Bot 2017-09-25 00:34:36 PDT
Comment on attachment 321197 [details]
Patch

Rejecting attachment 321197 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 321197, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
71.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp.rej
patching file Source/WebCore/rendering/RenderGrid.cpp
patching file Source/WebCore/rendering/RenderGrid.h
patching file Source/WebCore/rendering/style/RenderStyle.cpp
patching file Source/WebCore/rendering/style/RenderStyle.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Sergio Villar Senin']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/4647172
Comment 11 Manuel Rego Casasnovas 2017-09-25 00:55:44 PDT
Created attachment 321673 [details]
=Patch for landing
Comment 12 WebKit Commit Bot 2017-09-25 01:39:52 PDT
Comment on attachment 321673 [details]
=Patch for landing

Clearing flags on attachment: 321673

Committed r222441: <http://trac.webkit.org/changeset/222441>
Comment 13 WebKit Commit Bot 2017-09-25 01:39:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2017-09-27 12:29:55 PDT
<rdar://problem/34693381>