160252 2016-07-27 08:10:09 -0700 [css-grid] The isValidTransition function must not alter the GridSizingData object 2016-07-28 02:02:27 -0700 1 1 1 Unclassified WebKit Layout and Rendering WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jfernandez jfernandez commit-queue darin esprehn+autocc glenn jfernandez kondapallykalyan rego simon.fraser svillar oldest_to_newest 1214897 0 jfernandez 2016-07-27 08:10:09 -0700 Small refactoring of the states-machine we use in the grid tracks sizing algorithm. 1214899 1 284697 jfernandez 2016-07-27 08:17:36 -0700 Created attachment 284697 Patch 1214951 2 284697 darin 2016-07-27 10:05:38 -0700 Comment on attachment 284697 Patch Change looks fine, although I would not exactly call this "refactoring". 1214952 3 284697 darin 2016-07-27 10:06:05 -0700 Comment on attachment 284697 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. 1214970 4 284697 jfernandez 2016-07-27 11:08:57 -0700 Comment on attachment 284697 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. 1214971 5 jfernandez 2016-07-27 11:09:57 -0700 (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. 1214975 6 284711 jfernandez 2016-07-27 11:30:21 -0700 Created attachment 284711 Patch 1214976 7 284711 darin 2016-07-27 11:35:52 -0700 Comment on attachment 284711 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. 1214980 8 284711 commit-queue 2016-07-27 11:47:55 -0700 Comment on attachment 284711 Patch Clearing flags on attachment: 284711 Committed r203785: <http://trac.webkit.org/changeset/203785> 1214981 9 commit-queue 2016-07-27 11:48:01 -0700 All reviewed patches have been landed. Closing bug. 1215177 10 284711 svillar 2016-07-28 02:02:27 -0700 Comment on attachment 284711 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. 284697 2016-07-27 08:17:36 -0700 2016-07-27 11:30:14 -0700 Patch bug-160252-20160727171624.patch text/plain 2411 jfernandez U3VidmVyc2lvbiBSZXZpc2lvbjogMjAzNzcxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYThhNjY5ODUzYzVkMTRl MzE3ZjMyZjFkMDRlOTIzOWIwZDUwNmEwZC4uNWY5NDUyZWJkOTBhYjBhMDFiMjVkYTA5NTY4Nzg4 MDdlODRkNjc1NiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDI2IEBACiAyMDE2LTA3LTI3ICBKYXZp ZXIgRmVybmFuZGV6ICA8amZlcm5hbmRlekBpZ2FsaWEuY29tPgogCisgICAgICAgIFtjc3MtZ3Jp ZF0gUmVmYWN0b3Jpbmcgb2YgdHJhY2sgc2l6aW5nIHN0YXRlIG1hY2hpbmUKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE2MDI1MgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFNvbWUgb2YgdGhlIHN0YXRlIHRy YW5zaXRpb24gdmFsaWRhdGlvbiB3YXMgbm90IHVzZWZ1bCwgc2luY2UgdGhlIHRyYWNrCisgICAg ICAgIHNpemluZyBhbGdvcml0aG0gbmV2ZXIgZ29lcyB0aHJvdWdoIHRoYXQuIEl0IHdhcyBpbnRl bmRlZCBmb3IgcmVwZWF0IHRoZQorICAgICAgICBmaXJzdCBpdGVyYXRpb24gb2YgdGhlIHJvdyB0 cmFjayBzaXppbmcgaW4gY2FzZSBvZiBhbiBpbmRlZmluaXRlIGhlaWdodAorICAgICAgICBncmlk IGNvbnRhaW5lciwgYnV0IHNpbmNlIGludHJpbnNpYyBoZWlnaHQgY29tcHV0YXRpb24gZG9lcyBu b3QgdG91Y2gKKyAgICAgICAgdGhlIHN0YXRlLCBzdWNoIHJlcGVhdGVkIHRyYW5zaXRpb24gaXMg bm90IG5lZWRlZC4KKworICAgICAgICBBZGRpdGlvbmFsbHksIGlzIG5vdCBjb3JyZWN0IHRoYXQg YSBmdW5jdGlvbiB3aGljaCBwdXJwb3NlIGlzIHRvIGNoZWNrCisgICAgICAgIG91dCB0aGUgdmFs aWRpdHkgb2YgYSBzdGF0ZSBtb2RpZmllcyBzdWNoIHN0YXRlLgorCisgICAgICAgIE5vIG5ldyB0 ZXN0cywgaXQncyBqdXN0IGEgcmVmYWN0b3Jpbmcgd2l0aCBubyBmdW5jdGlvbmFsaXR5IGNoYW5n ZS4KKworICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJHcmlkLmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OlJlbmRlckdyaWQ6OkdyaWRTaXppbmdEYXRhOjppc1ZhbGlkVHJhbnNpdGlvbik6CisKKzIw MTYtMDctMjcgIEphdmllciBGZXJuYW5kZXogIDxqZmVybmFuZGV6QGlnYWxpYS5jb20+CisKICAg ICAgICAgW2Nzcy1ncmlkXSBIYW5kbGUgYWxpZ25tZW50IHdpdGggb3J0aG9nb25hbCBmbG93cwog ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU5Mjk1CiAK ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJHcmlkLmNwcCBiL1Nv dXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJHcmlkLmNwcAppbmRleCAwMmQ5ODA1MTc1YzY1 MGIzMzllOTRiNGRiYzhlMjZiMTk2OTNiZWNlLi41ZjhhNjk5ZDBhMDhhNzE3MWFkYTgyZmM4MTY3 NWQ1OWNjOWI5OTg1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVy R3JpZC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckdyaWQuY3BwCkBA IC0yNjEsMTcgKzI2MSwxMyBAQCBwdWJsaWM6CiAgICAgICAgIEFTU0VSVF9OT1RfUkVBQ0hFRCgp OwogICAgICAgICBzaXppbmdTdGF0ZSA9IENvbHVtblNpemluZ0ZpcnN0SXRlcmF0aW9uOwogICAg IH0KLSAgICBib29sIGlzVmFsaWRUcmFuc2l0aW9uKEdyaWRUcmFja1NpemluZ0RpcmVjdGlvbiBk aXJlY3Rpb24pCisgICAgYm9vbCBpc1ZhbGlkVHJhbnNpdGlvbihHcmlkVHJhY2tTaXppbmdEaXJl Y3Rpb24gZGlyZWN0aW9uKSBjb25zdAogICAgIHsKICAgICAgICAgc3dpdGNoIChzaXppbmdTdGF0 ZSkgewogICAgICAgICBjYXNlIENvbHVtblNpemluZ0ZpcnN0SXRlcmF0aW9uOgorICAgICAgICBj YXNlIENvbHVtblNpemluZ1NlY29uZEl0ZXJhdGlvbjoKICAgICAgICAgICAgIHJldHVybiBkaXJl Y3Rpb24gPT0gRm9yQ29sdW1uczsKICAgICAgICAgY2FzZSBSb3dTaXppbmdGaXJzdEl0ZXJhdGlv bjoKLSAgICAgICAgICAgIHJldHVybiBkaXJlY3Rpb24gPT0gRm9yUm93czsKLSAgICAgICAgY2Fz ZSBDb2x1bW5TaXppbmdTZWNvbmRJdGVyYXRpb246Ci0gICAgICAgICAgICBpZiAoZGlyZWN0aW9u ID09IEZvclJvd3MpCi0gICAgICAgICAgICAgICAgc2l6aW5nU3RhdGUgPSBSb3dTaXppbmdGaXJz dEl0ZXJhdGlvbjsKLSAgICAgICAgICAgIHJldHVybiB0cnVlOwogICAgICAgICBjYXNlIFJvd1Np emluZ1NlY29uZEl0ZXJhdGlvbjoKICAgICAgICAgICAgIHJldHVybiBkaXJlY3Rpb24gPT0gRm9y Um93czsKICAgICAgICAgfQo= 284711 2016-07-27 11:30:21 -0700 2016-07-27 11:47:55 -0700 Patch bug-160252-20160727202909.patch text/plain 2579 jfernandez U3VidmVyc2lvbiBSZXZpc2lvbjogMjAzNzcxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYThhNjY5ODUzYzVkMTRl MzE3ZjMyZjFkMDRlOTIzOWIwZDUwNmEwZC4uZjJjYTQxMGJmNmI0ODdmNWE0MTA0NTYyMjc2Zjli Y2ZhYzc4ZmFlZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDMwIEBACiAyMDE2LTA3LTI3ICBKYXZp ZXIgRmVybmFuZGV6ICA8amZlcm5hbmRlekBpZ2FsaWEuY29tPgogCisgICAgICAgIFtjc3MtZ3Jp ZF0gVGhlIGlzVmFsaWRUcmFuc2l0aW9uIGZ1bmN0aW9uIG11c3Qgbm90IGFsdGVyIHRoZSBHcmlk U2l6aW5nRGF0YSBvYmplY3QKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTE2MDI1MgorCisgICAgICAgIFJldmlld2VkIGJ5IERhcmluIEFkbGVyLgorCisg ICAgICAgIEl0J3Mgbm90IGNvcnJlY3QgdGhhdCBhIGZ1bmN0aW9uIHdoaWNoIHB1cnBvc2UgaXMg dG8gY2hlY2sgb3V0IHRoZQorICAgICAgICB2YWxpZGl0eSBvZiBhIHN0YXRlIG1vZGlmaWVzIHN1 Y2ggc3RhdGUuCisKKyAgICAgICAgVGhhdCBjb2RlIHdhcyB0aGVyZSB0byBhbGxvdyB0aGUgZXh0 cmEgcm93IHRyYWNrIHNpemluZyBpdGVyYXRpb24KKyAgICAgICAgaW4gdGhlIGNhc2Ugb2YgYSBn cmlkIGNvbnRhaW5lciB3aXRoIGluZGVmaW5pdGUgaGVpZ2h0LiBXZSBuZWVkIHRvCisgICAgICAg IGRvIHRoYXQgaW4gb3JkZXIgdG8gZmlndXJlIG91dCBpdHMgaW50cmluc2ljIGhlaWdodCwgd2hp Y2ggd2lsbCBiZQorICAgICAgICB1c2VkIHRoZW4gdG8gcHJvcGVybHkgc2l6aW5nIHRoZSByb3cg dHJhY2tzLgorCisgICAgICAgIFNpbmNlIHRoZSBpbnRyaW5zaWMgaGVpZ2h0IGNvbXB1dGF0aW9u IHVzZXMgZGlyZWN0bHkgdGhlCisgICAgICAgIGNvbXB1dGVVc2VkQnJlYWR0aE9mR3JpZFRyYWNr cyBmdW5jdGlvbiwgaXQgZG9lcyBub3QgYWx0ZXIgdGhlCisgICAgICAgIGFsZ29yaXRobSBzdGF0 ZS1tYWNoaW5lLCBoZW5jZSwgd2UgY2FuIHNhZmVseSByZW1vdmUgdGhpcyBjb2RlLAorICAgICAg ICB3aGljaCB3YXMgaW5jb3JyZWN0IGluIGFueSBjYXNlLgorCisgICAgICAgIE5vIG5ldyB0ZXN0 cywgaXQncyBqdXN0IGEgcmVmYWN0b3Jpbmcgd2l0aCBubyBmdW5jdGlvbmFsaXR5IGNoYW5nZS4K KworICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJHcmlkLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6 OlJlbmRlckdyaWQ6OkdyaWRTaXppbmdEYXRhOjppc1ZhbGlkVHJhbnNpdGlvbik6CisKKzIwMTYt MDctMjcgIEphdmllciBGZXJuYW5kZXogIDxqZmVybmFuZGV6QGlnYWxpYS5jb20+CisKICAgICAg ICAgW2Nzcy1ncmlkXSBIYW5kbGUgYWxpZ25tZW50IHdpdGggb3J0aG9nb25hbCBmbG93cwogICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU5Mjk1CiAKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJHcmlkLmNwcCBiL1NvdXJj ZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJHcmlkLmNwcAppbmRleCAwMmQ5ODA1MTc1YzY1MGIz MzllOTRiNGRiYzhlMjZiMTk2OTNiZWNlLi41ZjhhNjk5ZDBhMDhhNzE3MWFkYTgyZmM4MTY3NWQ1 OWNjOWI5OTg1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyR3Jp ZC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckdyaWQuY3BwCkBAIC0y NjEsMTcgKzI2MSwxMyBAQCBwdWJsaWM6CiAgICAgICAgIEFTU0VSVF9OT1RfUkVBQ0hFRCgpOwog ICAgICAgICBzaXppbmdTdGF0ZSA9IENvbHVtblNpemluZ0ZpcnN0SXRlcmF0aW9uOwogICAgIH0K LSAgICBib29sIGlzVmFsaWRUcmFuc2l0aW9uKEdyaWRUcmFja1NpemluZ0RpcmVjdGlvbiBkaXJl Y3Rpb24pCisgICAgYm9vbCBpc1ZhbGlkVHJhbnNpdGlvbihHcmlkVHJhY2tTaXppbmdEaXJlY3Rp b24gZGlyZWN0aW9uKSBjb25zdAogICAgIHsKICAgICAgICAgc3dpdGNoIChzaXppbmdTdGF0ZSkg ewogICAgICAgICBjYXNlIENvbHVtblNpemluZ0ZpcnN0SXRlcmF0aW9uOgorICAgICAgICBjYXNl IENvbHVtblNpemluZ1NlY29uZEl0ZXJhdGlvbjoKICAgICAgICAgICAgIHJldHVybiBkaXJlY3Rp b24gPT0gRm9yQ29sdW1uczsKICAgICAgICAgY2FzZSBSb3dTaXppbmdGaXJzdEl0ZXJhdGlvbjoK LSAgICAgICAgICAgIHJldHVybiBkaXJlY3Rpb24gPT0gRm9yUm93czsKLSAgICAgICAgY2FzZSBD b2x1bW5TaXppbmdTZWNvbmRJdGVyYXRpb246Ci0gICAgICAgICAgICBpZiAoZGlyZWN0aW9uID09 IEZvclJvd3MpCi0gICAgICAgICAgICAgICAgc2l6aW5nU3RhdGUgPSBSb3dTaXppbmdGaXJzdEl0 ZXJhdGlvbjsKLSAgICAgICAgICAgIHJldHVybiB0cnVlOwogICAgICAgICBjYXNlIFJvd1Npemlu Z1NlY29uZEl0ZXJhdGlvbjoKICAgICAgICAgICAgIHJldHVybiBkaXJlY3Rpb24gPT0gRm9yUm93 czsKICAgICAgICAgfQo=