Bug 160252 – [css-grid] The isValidTransition function must not alter the GridSizingData object

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Javier Fernandez]

Small refactoring of the states-machine we use in the grid tracks sizing algorithm.

[Javier Fernandez]

Created attachment 284697 [details]
Patch

[Darin Adler]

Comment on attachment 284697 [details]
Patch

Change looks fine, although I would not exactly call this "refactoring".

[Darin Adler]

Comment on attachment 284697 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:264
> +    bool isValidTransition(GridTrackSizingDirection direction) const

Instead of a const member function, this should be a static member function, since it’s not dependent on the state of the object at all.

[Javier Fernandez]

Comment on attachment 284697 [details]
Patch

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

>> Source/WebCore/rendering/RenderGrid.cpp:264
>> +    bool isValidTransition(GridTrackSizingDirection direction) const
> 
> Instead of a const member function, this should be a static member function, since it’s not dependent on the state of the object at all.

It depends on the current object's state, defined in its "sizingState" attribute. This change, precisely, removes the code which was altering the value of the sizingState attribute when the object it's at the ColumnSizingSecondIteration and we are going to perform a transition in the ForRows direction. By adding 'const' I'm trying to avoid the same mistake and state clearly that this function performs some read-only checks.

[Javier Fernandez]

(In reply to comment #2)
> Comment on attachment 284697 [details]
> Patch
> 
> Change looks fine, although I would not exactly call this "refactoring".

Perhaps I should change the description, indeed.

[Javier Fernandez]

Created attachment 284711 [details]
Patch

[Darin Adler]

Comment on attachment 284711 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:264
> +    bool isValidTransition(GridTrackSizingDirection direction) const

Hmm, I set commit-queue+ on this, but this is still using a const member function instead of a static member function.

[WebKit Commit Bot]

Comment on attachment 284711 [details]
Patch

Clearing flags on attachment: 284711

Committed r203785: <http://trac.webkit.org/changeset/203785>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sergio Villar Senin]

Comment on attachment 284711 [details]
Patch

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

>> Source/WebCore/rendering/RenderGrid.cpp:264
>> +    bool isValidTransition(GridTrackSizingDirection direction) const
> 
> Hmm, I set commit-queue+ on this, but this is still using a const member function instead of a static member function.

And it should because it's checking the value of sizingState which is a member of the GridSizingData struct (it does not have the m_ as it's a struct).

That'll become less confusing after the refactoring and cleanup of that struct which is becoming a C++ class.