Bug 217479 – Sanitize the usage of override sizes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

Sanitize the usage of override sizes

[Sergio Villar Senin]

Created attachment 410852 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 410852 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410852&action=review

Thanks for doing this, I believe this was really needed as it was a mess what we were storing on those overrides.

There's one test failing yet, so we need to check what's going on before merging this.

> Source/WebCore/rendering/RenderBox.cpp:95
>  // Used by flexible boxes when flexing this element and by table cells.

Nit: Maybe we want to update this comment, as grid layout also uses these.

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:170
> +    return std::max<LayoutUnit>(0, widthForChild(child) - child->borderAndPaddingLogicalWidth());

Don't we need to subsctract scrollbar sizes here?

[Sergio Villar Senin]

Created attachment 411222 [details]
Patch

[Darin Adler]

Note the point here, but:

I’m not a big fan of the broken grammar of naming something "override size" or a "size override". The word "override" is a verb and it’s not very good as a noun. It’s the kind of poor grammar that programmers sometimes use that in the end makes it hard to translate between software and spoken English, making it just that little bit harder to think about a program.

If I was creating something new I might call it an "overriding size" or an "explicitly set size" or an "explicit size" or search for other terminology that matches how people would speak about this if explaining it to another person.

Possibly also some additional consideration needs to be applied since we want this to be English that works well for many different types of English speakers. Sometimes people argue in favor of jargon because of that; not sure if that applies here.

[zalan]

(In reply to Darin Adler from comment #4)
> Note the point here, but:
> 
> I’m not a big fan of the broken grammar of naming something "override size"
> or a "size override". The word "override" is a verb and it’s not very good
> as a noun. It’s the kind of poor grammar that programmers sometimes use that
> in the end makes it hard to translate between software and spoken English,
> making it just that little bit harder to think about a program.
> 
> If I was creating something new I might call it an "overriding size" or an
> "explicitly set size" or an "explicit size" or search for other terminology
> that matches how people would speak about this if explaining it to another
> person.
> 
> Possibly also some additional consideration needs to be applied since we
> want this to be English that works well for many different types of English
> speakers. Sometimes people argue in favor of jargon because of that; not
> sure if that applies here.
Agree and it's also contagious. I started using it in LFC without even giving it a thought (bug 217657).

[Sergio Villar Senin]

(In reply to zalan from comment #5)
> (In reply to Darin Adler from comment #4)
> > Note the point here, but:
> > 
> > I’m not a big fan of the broken grammar of naming something "override size"
> > or a "size override". The word "override" is a verb and it’s not very good
> > as a noun. It’s the kind of poor grammar that programmers sometimes use that
> > in the end makes it hard to translate between software and spoken English,
> > making it just that little bit harder to think about a program.
> > 
> > If I was creating something new I might call it an "overriding size" or an
> > "explicitly set size" or an "explicit size" or search for other terminology
> > that matches how people would speak about this if explaining it to another
> > person.
> > 
> > Possibly also some additional consideration needs to be applied since we
> > want this to be English that works well for many different types of English
> > speakers. Sometimes people argue in favor of jargon because of that; not
> > sure if that applies here.
> Agree and it's also contagious. I started using it in LFC without even
> giving it a thought (bug 217657).

Fair enough. I could replace it to OverriddenXXX to make it consistent with Zalan's changes but I wonder if we should do it as part of this patch or in a follow up in order not to make it more difficult to understand for future readers.

[Darin Adler]

(In reply to Sergio Villar Senin from comment #6)
> Fair enough. I could replace it to OverriddenXXX to make it consistent with
> Zalan's changes but I wonder if we should do it as part of this patch or in
> a follow up in order not to make it more difficult to understand for future
> readers.

I don’t feel strongly about that either way. What I care about is the future, not how quickly we make a change.

[Radar WebKit Bug Importer]

<rdar://problem/70338631>

[Sergio Villar Senin]

(In reply to Manuel Rego Casasnovas from comment #2)
> Comment on attachment 410852 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410852&action=review
> 
> Thanks for doing this, I believe this was really needed as it was a mess
> what we were storing on those overrides.

Thanks for the review!

> There's one test failing yet, so we need to check what's going on before
> merging this.

Already fixed in the new patch, I forgot to add the border and padding when storing the overriden size in the case of border-fit: line.

> > Source/WebCore/rendering/RenderBox.cpp:95
> >  // Used by flexible boxes when flexing this element and by table cells.
> 
> Nit: Maybe we want to update this comment, as grid layout also uses these.

I've finally decided to remove it. The overriden sizes mechanism shouldn't be attached to flex or grid, it's mean to be general usage.

> > Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:170
> > +    return std::max<LayoutUnit>(0, widthForChild(child) - child->borderAndPaddingLogicalWidth());
> 
> Don't we need to subsctract scrollbar sizes here?

I wondered the same but the current code does not take scrollbars into account, for example:

static LayoutUnit contentWidthForChild(RenderBox* child)
{
    if (child->hasOverrideContentLogicalWidth())
        return child->overrideContentLogicalWidth();
    return child->logicalWidth() - child->borderAndPaddingLogicalWidth();
}

so I'm basically respecting that the current code is doing. Maybe there is a bug but that would be orthogonal to this bug IMO.

[Manuel Rego Casasnovas]

(In reply to Sergio Villar Senin from comment #9)
> > > Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:170
> > > +    return std::max<LayoutUnit>(0, widthForChild(child) - child->borderAndPaddingLogicalWidth());
> > 
> > Don't we need to subsctract scrollbar sizes here?
> 
> I wondered the same but the current code does not take scrollbars into
> account, for example:
> 
> static LayoutUnit contentWidthForChild(RenderBox* child)
> {
>     if (child->hasOverrideContentLogicalWidth())
>         return child->overrideContentLogicalWidth();
>     return child->logicalWidth() - child->borderAndPaddingLogicalWidth();
> }
> 
> so I'm basically respecting that the current code is doing. Maybe there is a
> bug but that would be orthogonal to this bug IMO.

I agree this can be done in a separated issue, so please check if this is a bug or not, and report it if that's the case.

[Manuel Rego Casasnovas]

Comment on attachment 411222 [details]
Patch

r=me, but please do the suggested renaming in a follow-up patch.

[Sergio Villar Senin]

Committed r268666: <https://trac.webkit.org/changeset/268666>

[zalan]

This caused the following regression: bug 220524

[Brent Fulgham]

We should roll this out until they have a good patch for this. The regression is too severe to keep in the tree.

[zalan]

auto-revert fails with bunch of conflicts. Sergio could you revert this patch soon?

[Sergio Villar Senin]

(In reply to zalan from comment #15)
> auto-revert fails with bunch of conflicts. Sergio could you revert this
> patch soon?

I'll try to fix the regression ASAP. Otherwise I'll try to revert it indeed.

[Sergio Villar Senin]

(In reply to Sergio Villar Senin from comment #16)
> (In reply to zalan from comment #15)
> > auto-revert fails with bunch of conflicts. Sergio could you revert this
> > patch soon?
> 
> I'll try to fix the regression ASAP. Otherwise I'll try to revert it indeed.

Should be fixed now. Thanks for reporting!

[Darin Adler]

Wanted to figure out what you mean. Are saying you this was fixed by r271436 or by something else?

[Darin Adler]

Oh, I see there’s a reference above to the bug number.