There are issues if you use position relative and percentage offsets in grid items. This has been already fixed in Blink: https://bugs.chromium.org/p/chromium/issues/detail?id=835607
Created attachment 352088 [details] Patch
Comment on attachment 352088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352088&action=review Looks good overall. However I am not sure whether this change could cause performance regressions. See bellow, have you observed any significant change in Blink? > Source/WebCore/rendering/RenderBoxModelObject.cpp:426 > + bool hasOverrideContainingBlockContent = hasOverrideContainingBlockContentHeight(); You're unconditionally adding a hashmap query here. I think it's better to have the call inside the if so we can shortcircuit it if any of the previous conditions is already true. On the other hand it might be the case that we need to call it twice in the worst case...
Comment on attachment 352088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352088&action=review >> Source/WebCore/rendering/RenderBoxModelObject.cpp:426 >> + bool hasOverrideContainingBlockContent = hasOverrideContainingBlockContentHeight(); > > You're unconditionally adding a hashmap query here. I think it's better to have the call inside the if so we can shortcircuit it if any of the previous conditions is already true. > > On the other hand it might be the case that we need to call it twice in the worst case... We should get rid of the hashmap to manage the RenderBox override size, as we do in Blink. About this specific case, I wouldn't complicate the code because of performance, since we'll probably get rid of the hashmap in the near future.
Comment on attachment 352088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352088&action=review >>> Source/WebCore/rendering/RenderBoxModelObject.cpp:426 >>> + bool hasOverrideContainingBlockContent = hasOverrideContainingBlockContentHeight(); >> >> You're unconditionally adding a hashmap query here. I think it's better to have the call inside the if so we can shortcircuit it if any of the previous conditions is already true. >> >> On the other hand it might be the case that we need to call it twice in the worst case... > > We should get rid of the hashmap to manage the RenderBox override size, as we do in Blink. > > About this specific case, I wouldn't complicate the code because of performance, since we'll probably get rid of the hashmap in the near future. We're getting some issues in Blink, so until we stablize the code there I don't think it makes sense to merge this on WebKit.
Created attachment 357871 [details] Rebased patch
Created attachment 357874 [details] Rebased patch
Created attachment 357875 [details] Apply suggested changes
Comment on attachment 352088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352088&action=review Thanks for the reviews. PTAL. >>>> Source/WebCore/rendering/RenderBoxModelObject.cpp:426 >>>> + bool hasOverrideContainingBlockContent = hasOverrideContainingBlockContentHeight(); >>> >>> You're unconditionally adding a hashmap query here. I think it's better to have the call inside the if so we can shortcircuit it if any of the previous conditions is already true. >>> >>> On the other hand it might be the case that we need to call it twice in the worst case... >> >> We should get rid of the hashmap to manage the RenderBox override size, as we do in Blink. >> >> About this specific case, I wouldn't complicate the code because of performance, since we'll probably get rid of the hashmap in the near future. > > We're getting some issues in Blink, so until we stablize the code there I don't think it makes sense to merge this on WebKit. The code in Blink seems to have stabilized. I'm applying the change suggested by Sergio on the last version of this patch.
Comment on attachment 357875 [details] Apply suggested changes Clearing flags on attachment: 357875 Committed r239502: <https://trac.webkit.org/changeset/239502>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46898273>