Bug 190492 – [css-grid] Fix percentages in relative offsets for grid items

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Manuel Rego Casasnovas]

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

[Manuel Rego Casasnovas]

Created attachment 352088 [details]
Patch

[Sergio Villar Senin]

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...

[Javier Fernandez]

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.

[Manuel Rego Casasnovas]

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.

[Manuel Rego Casasnovas]

Created attachment 357871 [details]
Rebased patch

[Manuel Rego Casasnovas]

Created attachment 357874 [details]
Rebased patch

[Manuel Rego Casasnovas]

Created attachment 357875 [details]
Apply suggested changes

[Manuel Rego Casasnovas]

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.

[WebKit Commit Bot]

Comment on attachment 357875 [details]
Apply suggested changes

Clearing flags on attachment: 357875

Committed r239502: <https://trac.webkit.org/changeset/239502>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46898273>