102441 2012-11-15 15:56:45 -0800 RenderGrid should have a function to resolve grid position 2012-11-16 05:32:56 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 60731 1 jchaffraix jchaffraix eric ojan tony webkit.review.bot xan.lopez oldest_to_newest 768591 0 jchaffraix 2012-11-15 15:56:45 -0800 Currently mapping grid-row or grid-column (grid-position-row and grid-position-col in the latest spec) to the actual position on the grid is done in an implicit way inside RenderGrid::findChildLogicalPosition. In order to add support for grid areas, we need a way to get the grid items' explicit position which is where this function would come in handy. Patch forthcoming. 768598 1 174539 jchaffraix 2012-11-15 16:07:23 -0800 Created attachment 174539 Proposed refactoring. 768602 2 174539 ojan 2012-11-15 16:11:03 -0800 Comment on attachment 174539 Proposed refactoring. View in context: https://bugs.webkit.org/attachment.cgi?id=174539&action=review > Source/WebCore/rendering/RenderGrid.cpp:178 > +size_t RenderGrid::resolveGridPosition(const Length& position) const Isn't this the grid size? resolveGridExtent would be more consistent with what we call these in the rest of the codebase. Also, should this return a LayoutUnit? 768626 3 174539 jchaffraix 2012-11-15 16:20:47 -0800 Comment on attachment 174539 Proposed refactoring. View in context: https://bugs.webkit.org/attachment.cgi?id=174539&action=review >> Source/WebCore/rendering/RenderGrid.cpp:178 >> +size_t RenderGrid::resolveGridPosition(const Length& position) const > > Isn't this the grid size? resolveGridExtent would be more consistent with what we call these in the rest of the codebase. Also, should this return a LayoutUnit? OK, here are the answers in order: * No, the grid size is the max of the grid items' grid-position-row and grid-position-column, including the spans. * I don't think 'extend' is the right word here as it's not a sizing function: it is meant to return the physical position on the 2D grid. * Returning LayoutUnit makes no sense, we can't place an item at the (1/60, 1/60) position on the grid. If you have some suggestion on how to better convey this idea, I would love to update the patch. 768645 4 174539 ojan 2012-11-15 16:26:59 -0800 Comment on attachment 174539 Proposed refactoring. View in context: https://bugs.webkit.org/attachment.cgi?id=174539&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:178 >>> +size_t RenderGrid::resolveGridPosition(const Length& position) const >> >> Isn't this the grid size? resolveGridExtent would be more consistent with what we call these in the rest of the codebase. Also, should this return a LayoutUnit? > > OK, here are the answers in order: > * No, the grid size is the max of the grid items' grid-position-row and grid-position-column, including the spans. > * I don't think 'extend' is the right word here as it's not a sizing function: it is meant to return the physical position on the 2D grid. > * Returning LayoutUnit makes no sense, we can't place an item at the (1/60, 1/60) position on the grid. > > If you have some suggestion on how to better convey this idea, I would love to update the patch. oic. It's confusing that we using Length to represent these. I think we need a new value type. 768662 5 174539 jchaffraix 2012-11-15 16:36:33 -0800 Comment on attachment 174539 Proposed refactoring. View in context: https://bugs.webkit.org/attachment.cgi?id=174539&action=review >>>> Source/WebCore/rendering/RenderGrid.cpp:178 >>>> +size_t RenderGrid::resolveGridPosition(const Length& position) const >>> >>> Isn't this the grid size? resolveGridExtent would be more consistent with what we call these in the rest of the codebase. Also, should this return a LayoutUnit? >> >> OK, here are the answers in order: >> * No, the grid size is the max of the grid items' grid-position-row and grid-position-column, including the spans. >> * I don't think 'extend' is the right word here as it's not a sizing function: it is meant to return the physical position on the 2D grid. >> * Returning LayoutUnit makes no sense, we can't place an item at the (1/60, 1/60) position on the grid. >> >> If you have some suggestion on how to better convey this idea, I would love to update the patch. > > oic. It's confusing that we using Length to represent these. I think we need a new value type. I agree: I used Length as a temporary type (as it supports 'auto' and <integer>) until we actually get to the point where we require this new type. It is orthogonal to this refactoring though and I will be more than happy to clean this up in a follow-up bug. 769149 6 174539 webkit.review.bot 2012-11-16 05:32:53 -0800 Comment on attachment 174539 Proposed refactoring. Clearing flags on attachment: 174539 Committed r134935: <http://trac.webkit.org/changeset/134935> 769150 7 webkit.review.bot 2012-11-16 05:32:56 -0800 All reviewed patches have been landed. Closing bug. 174539 2012-11-15 16:07:23 -0800 2012-11-16 05:32:53 -0800 Proposed refactoring. bug-102441-20121115160520.patch text/plain 4716 jchaffraix U3VidmVyc2lvbiBSZXZpc2lvbjogMTM0Mjk1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNjgyN2Y0ZGFkODk1MTNl NTg3MDcyMDUxNGFlN2RlODA1ZDAyOGRjNi4uMjg4YmI5MjRlMTkyODAxNmVmMjdhMWVjNDA0MWJj ZDg4Y2U3Y2ZiOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI2IEBACisyMDEyLTExLTE1ICBKdWxp ZW4gQ2hhZmZyYWl4ICA8amNoYWZmcmFpeEB3ZWJraXQub3JnPgorCisgICAgICAgIFJlbmRlckdy aWQgc2hvdWxkIGhhdmUgYSBmdW5jdGlvbiB0byByZXNvbHZlIGdyaWQgcG9zaXRpb24KKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwMjQ0MQorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBjb2RlIHdhcyBk b2luZyB0aGlzIGNvbnZlcnNpb24gaW1wbGljaXRseSBpbnNpZGUgUmVuZGVyR3JpZDo6ZmluZENo aWxkTG9naWNhbFBvc2l0aW9uLgorICAgICAgICBBbHNvIG5vdGUgdGhhdCB3ZSBhbHNvIHByb3Zp ZGVkIGEgZmFsbGJhY2sgYnkgcmV0dXJuaW5nIExheW91dFBvaW50KCkgKGllIHRoZSAoMCwgMCkg cG9zaXRpb24KKyAgICAgICAgb24gdGhlIGdyaWQpIGlmIHdlIGNvdWxkbid0IGhhbmRsZSB0aGUg dmFsdWUuIFRoZSBleHBsaWNpdCBjb252ZXJzaW9uIGlzIG5lZWRlZCBpbiBvcmRlciB0bworICAg ICAgICBzdXBwb3J0IHJlbmRlciBhcmVhcyBhbmQgYWRkIGEgcHJvcGVyIGdyaWQgbW9kZWwgdG8g UmVuZGVyR3JpZC4KKworICAgICAgICBObyBleHBlY3RlZCBjaGFuZ2UgaW4gYmVoYXZpb3IuCisK KyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyR3JpZC5oOgorICAgICAgICAqIHJlbmRlcmluZy9S ZW5kZXJHcmlkLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlckdyaWQ6OnJlc29sdmVHcmlk UG9zaXRpb24pOgorICAgICAgICBBZGRlZCB0aGlzIG5ldyBmdW5jdGlvbiB0byBoYW5kbGUgdGhl IGNvbnZlcnNpb24uIFdlIHJlLXVzZSBMZW5ndGggYnV0IHNob3VsZCBuZXZlciBzZWUKKyAgICAg ICAgYSBsb3Qgb2YgdGhlIDxsZW5ndGg+IHZhbHVlcyBzbyBJIGFkZGVkIHNvbWUgQVNTRVJUcyB0 byBlbmZvcmNlIGFuZCBjYXRjaCB0aGF0LgorCisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJHcmlk OjpmaW5kQ2hpbGRMb2dpY2FsUG9zaXRpb24pOgorICAgICAgICBTaW1wbGlmaWVkIHRoZSBmdW5j dGlvbiBub3cgdGhhdCBpdCBqdXN0IHVzZSByZXNvbHZlR3JpZFBvc2l0aW9uLgorCiAyMDEyLTEx LTEyICBDaHJpc3RvcGhlIER1bWV6ICA8Y2hyaXN0b3BoZS5kdW1lekBpbnRlbC5jb20+CiAKICAg ICAgICAgRml4IG1lbW9yeSBsZWFrIGluIGNyZWF0ZVN1cmZhY2VGb3JCYWNraW5nU3RvcmUoKQpk aWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckdyaWQuY3BwIGIvU291 cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckdyaWQuY3BwCmluZGV4IDZlMjkzY2MwYzQ4OTVh ZjU5ZTg1ODkxZDEyNzkyNzU5YzllYjhjMjguLmM1YjkwM2MzMDVkZDdlMTkwNmRjNDZkMmE3ZjMx NmU3MTQxMmMxNTUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJH cmlkLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyR3JpZC5jcHAKQEAg LTE3NSwyMyArMTc1LDQ3IEBAIHZvaWQgUmVuZGVyR3JpZDo6bGF5b3V0R3JpZEl0ZW1zKCkKICAg ICAgICAgc2V0TG9naWNhbEhlaWdodChsb2dpY2FsSGVpZ2h0KCkgKyByb3dUcmFja3NbaV0ubV91 c2VkQnJlYWR0aCk7CiB9CiAKLUxheW91dFBvaW50IFJlbmRlckdyaWQ6OmZpbmRDaGlsZExvZ2lj YWxQb3NpdGlvbihSZW5kZXJCb3gqIGNoaWxkLCBjb25zdCBWZWN0b3I8R3JpZFRyYWNrPiYgY29s dW1uVHJhY2tzLCBjb25zdCBWZWN0b3I8R3JpZFRyYWNrPiYgcm93VHJhY2tzKQorc2l6ZV90IFJl bmRlckdyaWQ6OnJlc29sdmVHcmlkUG9zaXRpb24oY29uc3QgTGVuZ3RoJiBwb3NpdGlvbikgY29u c3QKIHsKLSAgICBMZW5ndGggY29sdW1uID0gY2hpbGQtPnN0eWxlKCktPmdyaWRJdGVtQ29sdW1u KCk7Ci0gICAgTGVuZ3RoIHJvdyA9IGNoaWxkLT5zdHlsZSgpLT5ncmlkSXRlbVJvdygpOwotCi0g ICAgLy8gRklYTUU6IFdoYXQgZG9lcyBhIG5vbi1wb3NpdGl2ZSBpbnRlZ2VyIG1lYW4gZm9yIGEg Y29sdW1uL3Jvdz8KLSAgICBpZiAoIWNvbHVtbi5pc1Bvc2l0aXZlKCkgfHwgIXJvdy5pc1Bvc2l0 aXZlKCkpCi0gICAgICAgIHJldHVybiBMYXlvdXRQb2ludCgpOwotCiAgICAgLy8gRklYTUU6IEhh bmRsZSBvdGhlciB2YWx1ZXMgZm9yIGdyaWQte3Jvdyxjb2x1bW59IGxpa2UgcmFuZ2VzIG9yIGxp bmUgbmFtZXMuCi0gICAgaWYgKCFjb2x1bW4uaXNGaXhlZCgpIHx8ICFyb3cuaXNGaXhlZCgpKQot ICAgICAgICByZXR1cm4gTGF5b3V0UG9pbnQoKTsKKyAgICBzd2l0Y2ggKHBvc2l0aW9uLnR5cGUo KSkgeworICAgIGNhc2UgRml4ZWQ6CisgICAgICAgIC8vIEZJWE1FOiBXaGF0IGRvZXMgYSBub24t cG9zaXRpdmUgaW50ZWdlciBtZWFuIGZvciBhIGNvbHVtbi9yb3c/CisgICAgICAgIGlmICghcG9z aXRpb24uaXNQb3NpdGl2ZSgpKQorICAgICAgICAgICAgcmV0dXJuIDA7CisKKyAgICAgICAgcmV0 dXJuIHBvc2l0aW9uLmludFZhbHVlKCkgLSAxOworICAgIGNhc2UgQXV0bzoKKyAgICAgICAgLy8g RklYTUU6IFdlIHNob3VsZCBmb2xsb3cgJ2dyaWQtYXV0by1mbG93JyBmb3IgcmVzb2x1dGlvbi4K KyAgICAgICAgLy8gVW50aWwgdGhlbiwgd2UgdXNlIHRoZSAnZ3JpZC1hdXRvLWZsb3c6IG5vbmUn IGJlaGF2aW9yICh3aGljaCBpcyB0aGUgZGVmYXVsdCkKKyAgICAgICAgLy8gYW5kIHJlc29sdmUg J2F1dG8nIGFzIHRoZSBmaXJzdCByb3cgLyBjb2x1bW4uCisgICAgICAgIHJldHVybiAwOworICAg IGNhc2UgUmVsYXRpdmU6CisgICAgY2FzZSBQZXJjZW50OgorICAgIGNhc2UgSW50cmluc2ljOgor ICAgIGNhc2UgTWluSW50cmluc2ljOgorICAgIGNhc2UgTWluQ29udGVudDoKKyAgICBjYXNlIE1h eENvbnRlbnQ6CisgICAgY2FzZSBGaWxsQXZhaWxhYmxlOgorICAgIGNhc2UgRml0Q29udGVudDoK KyAgICBjYXNlIENhbGN1bGF0ZWQ6CisgICAgY2FzZSBWaWV3cG9ydFBlcmNlbnRhZ2VXaWR0aDoK KyAgICBjYXNlIFZpZXdwb3J0UGVyY2VudGFnZUhlaWdodDoKKyAgICBjYXNlIFZpZXdwb3J0UGVy Y2VudGFnZU1pbjoKKyAgICBjYXNlIFVuZGVmaW5lZDoKKyAgICAgICAgYnJlYWs7CisgICAgfQor ICAgIEFTU0VSVF9OT1RfUkVBQ0hFRCgpOworICAgIHJldHVybiAwOworfQogCi0gICAgc2l6ZV90 IGNvbHVtblRyYWNrID0gc3RhdGljX2Nhc3Q8c2l6ZV90Pihjb2x1bW4uaW50VmFsdWUoKSkgLSAx OwotICAgIHNpemVfdCByb3dUcmFjayA9IHN0YXRpY19jYXN0PHNpemVfdD4ocm93LmludFZhbHVl KCkpIC0gMTsKK0xheW91dFBvaW50IFJlbmRlckdyaWQ6OmZpbmRDaGlsZExvZ2ljYWxQb3NpdGlv bihSZW5kZXJCb3gqIGNoaWxkLCBjb25zdCBWZWN0b3I8R3JpZFRyYWNrPiYgY29sdW1uVHJhY2tz LCBjb25zdCBWZWN0b3I8R3JpZFRyYWNrPiYgcm93VHJhY2tzKQoreworICAgIHNpemVfdCBjb2x1 bW5UcmFjayA9IHJlc29sdmVHcmlkUG9zaXRpb24oY2hpbGQtPnN0eWxlKCktPmdyaWRJdGVtQ29s dW1uKCkpOworICAgIHNpemVfdCByb3dUcmFjayA9IHJlc29sdmVHcmlkUG9zaXRpb24oY2hpbGQt PnN0eWxlKCktPmdyaWRJdGVtUm93KCkpOwogCiAgICAgTGF5b3V0UG9pbnQgb2Zmc2V0OworICAg IC8vIEZJWE1FOiB8Y29sdW1uVHJhY2t8IGFuZCB8cm93VHJhY2t8IHNob3VsZCBiZSBzbWFsbGVy IHRoYW4gb3VyIGNvbHVtbiAvIHJvdyBjb3VudC4KICAgICBmb3IgKHNpemVfdCBpID0gMDsgaSA8 IGNvbHVtblRyYWNrICYmIGkgPCBjb2x1bW5UcmFja3Muc2l6ZSgpOyArK2kpCiAgICAgICAgIG9m ZnNldC5zZXRYKG9mZnNldC54KCkgKyBjb2x1bW5UcmFja3NbaV0ubV91c2VkQnJlYWR0aCk7CiAg ICAgZm9yIChzaXplX3QgaSA9IDA7IGkgPCByb3dUcmFjayAmJiBpIDwgcm93VHJhY2tzLnNpemUo KTsgKytpKQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckdyaWQu aCBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJHcmlkLmgKaW5kZXggZTUwMTdmYWUw ZmRhYWNjOGNlZWZmYzgwMWRkYWQxMmQ3NzU4ZjU4My4uYjNmNGNhNTBmMTNlNzkxMTExYzlkZmY1 NmFlNjhhNmUwMmE0NWI3MSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1Jl bmRlckdyaWQuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyR3JpZC5oCkBA IC01MCw2ICs1MCw3IEBAIHByaXZhdGU6CiAgICAgdm9pZCBsYXlvdXRHcmlkSXRlbXMoKTsKIAog ICAgIExheW91dFBvaW50IGZpbmRDaGlsZExvZ2ljYWxQb3NpdGlvbihSZW5kZXJCb3gqLCBjb25z dCBWZWN0b3I8R3JpZFRyYWNrPiYgY29sdW1uVHJhY2tzLCBjb25zdCBWZWN0b3I8R3JpZFRyYWNr PiYgcm93VHJhY2tzKTsKKyAgICBzaXplX3QgcmVzb2x2ZUdyaWRQb3NpdGlvbihjb25zdCBMZW5n dGgmKSBjb25zdDsKIH07CiAKIH0gLy8gbmFtZXNwYWNlIFdlYkNvcmUK