123033 2013-10-18 11:47:37 -0700 [CSS Shapes] Match adjustLogicalLineTopAndLogicalHeightIfNeeded's implementation with Blink's 2013-10-22 15:05:52 -0700 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 zoltan zoltan commit-queue esprehn+autocc glenn kondapallykalyan oldest_to_newest 941285 0 zoltan 2013-10-18 11:47:37 -0700 In blink I made this function in a bit different way. This change modifies it to be identical, which helps a lot in the future cross-merging patches. 941286 1 214591 zoltan 2013-10-18 11:50:24 -0700 Created attachment 214591 Patch 941290 2 214591 darin 2013-10-18 12:03:32 -0700 Comment on attachment 214591 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214591&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1472 > - if (shapeInsideInfo && !wordMeasurements.size() && containsFloats()) { > - lastFloatFromPreviousLine = m_floatingObjects->set().last().get(); > - LayoutUnit floatLogicalTopOffset = shapeInsideInfo->computeFirstFitPositionForFloat(logicalSizeForFloat(lastFloatFromPreviousLine)); > - if (logicalHeight() < floatLogicalTopOffset) > - adjustedLogicalLineTop = floatLogicalTopOffset; > + if (shapeInsideInfo) { > + lastFloatFromPreviousLine = (containsFloats()) ? m_floatingObjects->set().last().get() : 0; > + if (!wordMeasurements.size() && lastFloatFromPreviousLine) { > + LayoutUnit floatLogicalTopOffset = shapeInsideInfo->computeFirstFitPositionForFloat(logicalSizeForFloat(lastFloatFromPreviousLine)); > + if (logicalHeight() < floatLogicalTopOffset) > + adjustedLogicalLineTop = floatLogicalTopOffset; > + } > } I like the old version better than the new version. The use of ? : and 0 is not as nice as regular if statement. Could you merge in the other direction instead? 941310 3 214597 zoltan 2013-10-18 13:21:35 -0700 Created attachment 214597 Patch 941312 4 zoltan 2013-10-18 13:24:26 -0700 (In reply to comment #2) > (From update of attachment 214591 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=214591&action=review > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1472 > > - if (shapeInsideInfo && !wordMeasurements.size() && containsFloats()) { > > - lastFloatFromPreviousLine = m_floatingObjects->set().last().get(); > > - LayoutUnit floatLogicalTopOffset = shapeInsideInfo->computeFirstFitPositionForFloat(logicalSizeForFloat(lastFloatFromPreviousLine)); > > - if (logicalHeight() < floatLogicalTopOffset) > > - adjustedLogicalLineTop = floatLogicalTopOffset; > > + if (shapeInsideInfo) { > > + lastFloatFromPreviousLine = (containsFloats()) ? m_floatingObjects->set().last().get() : 0; > > + if (!wordMeasurements.size() && lastFloatFromPreviousLine) { > > + LayoutUnit floatLogicalTopOffset = shapeInsideInfo->computeFirstFitPositionForFloat(logicalSizeForFloat(lastFloatFromPreviousLine)); > > + if (logicalHeight() < floatLogicalTopOffset) > > + adjustedLogicalLineTop = floatLogicalTopOffset; > > + } > > } > > I like the old version better than the new version. The use of ? : and 0 is not as nice as regular if statement. Could you merge in the other direction instead? I updated the patch. What do you think about the new version? I'm working on the refactoring of the RenderBlockLineLayout's nextSegmentBreak method, and because of that change I need to update the lastFloatFromPreviousLine variable in the shapeInsideInfo case. That's the reason because I want to do the merge this direction. This one looks better now, I think. 941334 5 zoltan 2013-10-18 14:24:41 -0700 (In reply to comment #4) > I'm working on the refactoring of the RenderBlockLineLayout's nextSegmentBreak method, and because of that change I need to update the lastFloatFromPreviousLine variable in the shapeInsideInfo case. That's the reason because I want to do the merge this direction. This one looks better now, I think. patch preview on bug #123038 942408 6 214597 hyatt 2013-10-22 14:21:06 -0700 Comment on attachment 214597 Patch r=me 942429 7 zoltan 2013-10-22 15:05:52 -0700 Committed r157820: <http://trac.webkit.org/changeset/157820> 214591 2013-10-18 11:50:24 -0700 2013-10-18 13:21:32 -0700 Patch bug-123033-20131018115023.patch text/plain 2475 zoltan U3VidmVyc2lvbiBSZXZpc2lvbjogMTU3NjQxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDJkOWQ1NzI2OWQ2YjUy MWY2MjVhZThjODIxZjZjNDBlZWIzYzMxNS4uOGExZDkxMmE3Y2UyNTY3YjBjY2Q5YzQ4NDNlOGMw ODc3NDM4NGZkNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEzLTEwLTE4ICBab2x0 YW4gSG9ydmF0aCAgPHpvbHRhbkB3ZWJraXQub3JnPgorCisgICAgICAgIFtDU1MgU2hhcGVzXSBN YXRjaCBhZGp1c3RMb2dpY2FsTGluZVRvcEFuZExvZ2ljYWxIZWlnaHRJZk5lZWRlZCdzIGltcGxl bWVudGF0aW9uIHdpdGggQmxpbmsncworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTIzMDMzCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgSW4gQmxpbmsgSSBtYWRlIHRoaXMgZnVuY3Rpb24gaW4gYSBiaXQgZGlm ZmVyZW50IHdheS4gVGhpcyBjaGFuZ2UgbW9kaWZpZXMgaXQKKyAgICAgICAgdG8gYmUgaWRlbnRp Y2FsLCB3aGljaCBoZWxwcyBhIGxvdCBpbiB0aGUgZnV0dXJlIGNyb3NzLW1lcmdpbmcgcGF0Y2hl cy4KKworICAgICAgICBObyBuZXcgdGVzdHMsIGNvdmVyZWQgYnkgZXhpc3RpbmcgdGV4dHMuCisK KyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyQmxvY2tMaW5lTGF5b3V0LmNwcDoKKyAgICAgICAg KFdlYkNvcmU6OlJlbmRlckJsb2NrRmxvdzo6YWRqdXN0TG9naWNhbExpbmVUb3BBbmRMb2dpY2Fs SGVpZ2h0SWZOZWVkZWQpOgorCiAyMDEzLTEwLTE4ICBDaGFuZ1Nlb2sgT2ggIDxjaGFuZ3Nlb2su b2hAY29sbGFib3JhLmNvbT4KIAogICAgICAgICBVbnJldmlld2VkIGJ1aWxkIGZpeCBmb3IgLS1u by1zdmcgb3B0aW9uLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRl ckJsb2NrTGluZUxheW91dC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQmxv Y2tMaW5lTGF5b3V0LmNwcAppbmRleCAyNzVkNDc4M2MxMTYzZDk5Zjc3OWIzN2Q0YzEyZTExZTQ5 ZDQxZTcwLi5lMjNlOTBjNjU3NWYzZWIzYTQ5YjlhM2MzNGFiNjFmZjczOWNlOWU2IDEwMDY0NAot LS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQmxvY2tMaW5lTGF5b3V0LmNwcAor KysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQmxvY2tMaW5lTGF5b3V0LmNwcApA QCAtMTQ2MiwxMSArMTQ2MiwxMyBAQCBib29sIFJlbmRlckJsb2NrRmxvdzo6YWRqdXN0TG9naWNh bExpbmVUb3BBbmRMb2dpY2FsSGVpZ2h0SWZOZWVkZWQoU2hhcGVJbnNpZGVJbgogewogICAgIExh eW91dFVuaXQgYWRqdXN0ZWRMb2dpY2FsTGluZVRvcCA9IGFkanVzdExvZ2ljYWxMaW5lVG9wKHNo YXBlSW5zaWRlSW5mbywgcmVzb2x2ZXIucG9zaXRpb24oKSwgZW5kLCB3b3JkTWVhc3VyZW1lbnRz KTsKIAotICAgIGlmIChzaGFwZUluc2lkZUluZm8gJiYgIXdvcmRNZWFzdXJlbWVudHMuc2l6ZSgp ICYmIGNvbnRhaW5zRmxvYXRzKCkpIHsKLSAgICAgICAgbGFzdEZsb2F0RnJvbVByZXZpb3VzTGlu ZSA9IG1fZmxvYXRpbmdPYmplY3RzLT5zZXQoKS5sYXN0KCkuZ2V0KCk7Ci0gICAgICAgIExheW91 dFVuaXQgZmxvYXRMb2dpY2FsVG9wT2Zmc2V0ID0gc2hhcGVJbnNpZGVJbmZvLT5jb21wdXRlRmly c3RGaXRQb3NpdGlvbkZvckZsb2F0KGxvZ2ljYWxTaXplRm9yRmxvYXQobGFzdEZsb2F0RnJvbVBy ZXZpb3VzTGluZSkpOwotICAgICAgICBpZiAobG9naWNhbEhlaWdodCgpIDwgZmxvYXRMb2dpY2Fs VG9wT2Zmc2V0KQotICAgICAgICAgICAgYWRqdXN0ZWRMb2dpY2FsTGluZVRvcCA9IGZsb2F0TG9n aWNhbFRvcE9mZnNldDsKKyAgICBpZiAoc2hhcGVJbnNpZGVJbmZvKSB7CisgICAgICAgIGxhc3RG bG9hdEZyb21QcmV2aW91c0xpbmUgPSAoY29udGFpbnNGbG9hdHMoKSkgPyBtX2Zsb2F0aW5nT2Jq ZWN0cy0+c2V0KCkubGFzdCgpLmdldCgpIDogMDsKKyAgICAgICAgaWYgKCF3b3JkTWVhc3VyZW1l bnRzLnNpemUoKSAmJiBsYXN0RmxvYXRGcm9tUHJldmlvdXNMaW5lKSB7CisgICAgICAgICAgICBM YXlvdXRVbml0IGZsb2F0TG9naWNhbFRvcE9mZnNldCA9IHNoYXBlSW5zaWRlSW5mby0+Y29tcHV0 ZUZpcnN0Rml0UG9zaXRpb25Gb3JGbG9hdChsb2dpY2FsU2l6ZUZvckZsb2F0KGxhc3RGbG9hdEZy b21QcmV2aW91c0xpbmUpKTsKKyAgICAgICAgICAgIGlmIChsb2dpY2FsSGVpZ2h0KCkgPCBmbG9h dExvZ2ljYWxUb3BPZmZzZXQpCisgICAgICAgICAgICAgICAgYWRqdXN0ZWRMb2dpY2FsTGluZVRv cCA9IGZsb2F0TG9naWNhbFRvcE9mZnNldDsKKyAgICAgICAgfQogICAgIH0KIAogICAgIGlmICgh YWRqdXN0ZWRMb2dpY2FsTGluZVRvcCkK 214597 2013-10-18 13:21:35 -0700 2013-10-22 14:21:06 -0700 Patch bug-123033-20131018132134.patch text/plain 2365 zoltan U3VidmVyc2lvbiBSZXZpc2lvbjogMTU3NjQxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDJkOWQ1NzI2OWQ2YjUy MWY2MjVhZThjODIxZjZjNDBlZWIzYzMxNS4uOGExZDkxMmE3Y2UyNTY3YjBjY2Q5YzQ4NDNlOGMw ODc3NDM4NGZkNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEzLTEwLTE4ICBab2x0 YW4gSG9ydmF0aCAgPHpvbHRhbkB3ZWJraXQub3JnPgorCisgICAgICAgIFtDU1MgU2hhcGVzXSBN YXRjaCBhZGp1c3RMb2dpY2FsTGluZVRvcEFuZExvZ2ljYWxIZWlnaHRJZk5lZWRlZCdzIGltcGxl bWVudGF0aW9uIHdpdGggQmxpbmsncworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTIzMDMzCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgSW4gQmxpbmsgSSBtYWRlIHRoaXMgZnVuY3Rpb24gaW4gYSBiaXQgZGlm ZmVyZW50IHdheS4gVGhpcyBjaGFuZ2UgbW9kaWZpZXMgaXQKKyAgICAgICAgdG8gYmUgaWRlbnRp Y2FsLCB3aGljaCBoZWxwcyBhIGxvdCBpbiB0aGUgZnV0dXJlIGNyb3NzLW1lcmdpbmcgcGF0Y2hl cy4KKworICAgICAgICBObyBuZXcgdGVzdHMsIGNvdmVyZWQgYnkgZXhpc3RpbmcgdGV4dHMuCisK KyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyQmxvY2tMaW5lTGF5b3V0LmNwcDoKKyAgICAgICAg KFdlYkNvcmU6OlJlbmRlckJsb2NrRmxvdzo6YWRqdXN0TG9naWNhbExpbmVUb3BBbmRMb2dpY2Fs SGVpZ2h0SWZOZWVkZWQpOgorCiAyMDEzLTEwLTE4ICBDaGFuZ1Nlb2sgT2ggIDxjaGFuZ3Nlb2su b2hAY29sbGFib3JhLmNvbT4KIAogICAgICAgICBVbnJldmlld2VkIGJ1aWxkIGZpeCBmb3IgLS1u by1zdmcgb3B0aW9uLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRl ckJsb2NrTGluZUxheW91dC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQmxv Y2tMaW5lTGF5b3V0LmNwcAppbmRleCAyNzVkNDc4M2MxMTYzZDk5Zjc3OWIzN2Q0YzEyZTExZTQ5 ZDQxZTcwLi4xOTgwMGI1MDJhYTdjYjk0NjBjODU0MWRjY2I4OWY5NmU1ZjJlODExIDEwMDY0NAot LS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQmxvY2tMaW5lTGF5b3V0LmNwcAor KysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQmxvY2tMaW5lTGF5b3V0LmNwcApA QCAtMTQ2MiwxMSArMTQ2MiwxMyBAQCBib29sIFJlbmRlckJsb2NrRmxvdzo6YWRqdXN0TG9naWNh bExpbmVUb3BBbmRMb2dpY2FsSGVpZ2h0SWZOZWVkZWQoU2hhcGVJbnNpZGVJbgogewogICAgIExh eW91dFVuaXQgYWRqdXN0ZWRMb2dpY2FsTGluZVRvcCA9IGFkanVzdExvZ2ljYWxMaW5lVG9wKHNo YXBlSW5zaWRlSW5mbywgcmVzb2x2ZXIucG9zaXRpb24oKSwgZW5kLCB3b3JkTWVhc3VyZW1lbnRz KTsKIAotICAgIGlmIChzaGFwZUluc2lkZUluZm8gJiYgIXdvcmRNZWFzdXJlbWVudHMuc2l6ZSgp ICYmIGNvbnRhaW5zRmxvYXRzKCkpIHsKKyAgICBpZiAoc2hhcGVJbnNpZGVJbmZvICYmIGNvbnRh aW5zRmxvYXRzKCkpIHsKICAgICAgICAgbGFzdEZsb2F0RnJvbVByZXZpb3VzTGluZSA9IG1fZmxv YXRpbmdPYmplY3RzLT5zZXQoKS5sYXN0KCkuZ2V0KCk7Ci0gICAgICAgIExheW91dFVuaXQgZmxv YXRMb2dpY2FsVG9wT2Zmc2V0ID0gc2hhcGVJbnNpZGVJbmZvLT5jb21wdXRlRmlyc3RGaXRQb3Np dGlvbkZvckZsb2F0KGxvZ2ljYWxTaXplRm9yRmxvYXQobGFzdEZsb2F0RnJvbVByZXZpb3VzTGlu ZSkpOwotICAgICAgICBpZiAobG9naWNhbEhlaWdodCgpIDwgZmxvYXRMb2dpY2FsVG9wT2Zmc2V0 KQotICAgICAgICAgICAgYWRqdXN0ZWRMb2dpY2FsTGluZVRvcCA9IGZsb2F0TG9naWNhbFRvcE9m ZnNldDsKKyAgICAgICAgaWYgKCF3b3JkTWVhc3VyZW1lbnRzLnNpemUoKSkgeworICAgICAgICAg ICAgTGF5b3V0VW5pdCBmbG9hdExvZ2ljYWxUb3BPZmZzZXQgPSBzaGFwZUluc2lkZUluZm8tPmNv bXB1dGVGaXJzdEZpdFBvc2l0aW9uRm9yRmxvYXQobG9naWNhbFNpemVGb3JGbG9hdChsYXN0Rmxv YXRGcm9tUHJldmlvdXNMaW5lKSk7CisgICAgICAgICAgICBpZiAobG9naWNhbEhlaWdodCgpIDwg ZmxvYXRMb2dpY2FsVG9wT2Zmc2V0KQorICAgICAgICAgICAgICAgIGFkanVzdGVkTG9naWNhbExp bmVUb3AgPSBmbG9hdExvZ2ljYWxUb3BPZmZzZXQ7CisgICAgICAgIH0KICAgICB9CiAKICAgICBp ZiAoIWFkanVzdGVkTG9naWNhbExpbmVUb3ApCg==