Bug 160252 - [css-grid] The isValidTransition function must not alter the GridSizingData object
Summary: [css-grid] The isValidTransition function must not alter the GridSizingData o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-27 08:10 PDT by Javier Fernandez
Modified: 2016-07-28 02:02 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.35 KB, patch)
2016-07-27 08:17 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (2.52 KB, patch)
2016-07-27 11:30 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2016-07-27 08:10:09 PDT
Small refactoring of the states-machine we use in the grid tracks sizing algorithm.
Comment 1 Javier Fernandez 2016-07-27 08:17:36 PDT
Created attachment 284697 [details]
Patch
Comment 2 Darin Adler 2016-07-27 10:05:38 PDT
Comment on attachment 284697 [details]
Patch

Change looks fine, although I would not exactly call this "refactoring".
Comment 3 Darin Adler 2016-07-27 10:06:05 PDT
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.
Comment 4 Javier Fernandez 2016-07-27 11:08:57 PDT
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.
Comment 5 Javier Fernandez 2016-07-27 11:09:57 PDT
(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.
Comment 6 Javier Fernandez 2016-07-27 11:30:21 PDT
Created attachment 284711 [details]
Patch
Comment 7 Darin Adler 2016-07-27 11:35:52 PDT
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.
Comment 8 WebKit Commit Bot 2016-07-27 11:47:55 PDT
Comment on attachment 284711 [details]
Patch

Clearing flags on attachment: 284711

Committed r203785: <http://trac.webkit.org/changeset/203785>
Comment 9 WebKit Commit Bot 2016-07-27 11:48:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Sergio Villar Senin 2016-07-28 02:02:27 PDT
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.