Bug 217898 – Rename override sizes to overriding sizes

• Component: New Bugs
• Status: RESOLVED FIXED

[Sergio Villar Senin]

Rename override sizes to overriden sizes

[Sergio Villar Senin]

Created attachment 411731 [details]
Patch

[Sergio Villar Senin]

Gentle ping for reviewers.

[Darin Adler]

Since this is about naming, and it’s at my prompting, I should weigh in on the name semantics.

I think these are "sizes that should *override* the normal size computation".

When we use the term "overridden size" it sounds like a "size that *has been* overridden". That is backwards. The "size" is the thing that overrides, not the thing that gets overridden.

The problem with the original name "override size" is that it could easily be a flag that tells us *whether* to override a size or a function that you call to override, not a function that returns the value of the size that has the power to override.

We are looking for a term that means "size that has the power to override" or "size that is allowed to override the normal computation". That’s where my idea of using the term "overriding size" came from, but there probably are even better names.

So I’m not sure that this rename is right.

[Sergio Villar Senin]

(In reply to Darin Adler from comment #3)
> Since this is about naming, and it’s at my prompting, I should weigh in on
> the name semantics.
> 
> I think these are "sizes that should *override* the normal size computation".
> 
> When we use the term "overridden size" it sounds like a "size that *has
> been* overridden". That is backwards. The "size" is the thing that
> overrides, not the thing that gets overridden.
> 
> The problem with the original name "override size" is that it could easily
> be a flag that tells us *whether* to override a size or a function that you
> call to override, not a function that returns the value of the size that has
> the power to override.
> 
> We are looking for a term that means "size that has the power to override"
> or "size that is allowed to override the normal computation". That’s where
> my idea of using the term "overriding size" came from, but there probably
> are even better names.
> 
> So I’m not sure that this rename is right.

Fair enough, I'll use overriding size then.

I agree that there might be better names but we should also consider the cognitive effort to adapt to the new name. Folks working on rendering are quite used to "OverrideSize" so "OverridingSize" would be easier to learn than "SomeOtherThingWeMightComeUpWith".

[Sergio Villar Senin]

Created attachment 411969 [details]
Patch

[Darin Adler]

Comment on attachment 411969 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        does not work well as a noun. It'd be more correct to refer to them as "overriden" sizes.

Comment uses old name.

> Source/WebCore/rendering/RenderBlock.cpp:2429
> +                overrideHeight = Optional<LayoutUnit>(box.overridingLogicalHeight());

Should rename these local variables too, since they use the old "override" terminology, and the references in the comments above.

> Source/WebCore/rendering/RenderBox.cpp:1970
> +        if (auto overridingLogicalWidth = overridingContainingBlockContentLogicalWidth())
> +            return overridingLogicalWidth.value();

I think maybe just the word "width" is a better name for this very-local local variable.

Also, seems a little strange that we have to check wither the overriding width is present twice in two different ways. Might be worth coming back here later and figuring out whether the check for nullopt is even needed when has already returns true, or maybe it’s the "has" check that should be removed. Perhaps we are inheriting these separate has functions from a different code base where Optional isn’t used the same way?

If we know it can’t be null this should be more like:

    return *overridingContainingBlockContentLogicalWidth();

Rather than a two line if statement.

> Source/WebCore/rendering/RenderBox.cpp:1982
> +        if (auto overridingLogicalHeight = overridingContainingBlockContentLogicalHeight())
> +            return overridingLogicalHeight.value();

"height"

> Source/WebCore/rendering/RenderBox.cpp:2023
> +        if (auto overridingLogicalHeight = overridingContainingBlockContentLogicalHeight())
> +            return overridingLogicalHeight.value();

"height"

> Source/WebCore/rendering/RenderBox.cpp:3329
> +        if (auto overridingLogicalWidth = overridingContainingBlockContentLogicalWidth())
> +            return overridingLogicalWidth.value();

"width"

> Source/WebCore/rendering/RenderBox.cpp:3394
> +        if (auto overridingLogicalHeight = overridingContainingBlockContentLogicalHeight())
> +            return overridingLogicalHeight.value();

"height"

> Source/WebCore/rendering/RenderBoxModelObject.cpp:352
> +        LayoutUnit availableWidth = hasOverridingContainingBlockContentWidth()
> +            ? overridingContainingBlockContentWidth().valueOr(LayoutUnit()) : containingBlock()->availableWidth();

Same thing again, with the checking twice. Seems like this happens everywhere and could get cleaned up.

> Source/WebCore/rendering/RenderBoxModelObject.h:239
> +    virtual Optional<LayoutUnit> overridingContainingBlockContentWidth() const { ASSERT_NOT_REACHED(); return -1_lu; }
> +    virtual Optional<LayoutUnit> overridingContainingBlockContentHeight() const { ASSERT_NOT_REACHED(); return -1_lu; }
> +    virtual bool hasOverridingContainingBlockContentWidth() const { return false; }
> +    virtual bool hasOverridingContainingBlockContentHeight() const { return false; }

I guess here is the root of the design problem I see. Why have the value be Optional and have a *separate* has function? There could be a reason, but I can’t think of one. This looks like someone started using Optional but didn’t follow through all the way.

[Sergio Villar Senin]

Comment on attachment 411969 [details]
Patch

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

Thanks for the reviews!

>> Source/WebCore/rendering/RenderBoxModelObject.h:239
>> +    virtual bool hasOverridingContainingBlockContentHeight() const { return false; }
> 
> I guess here is the root of the design problem I see. Why have the value be Optional and have a *separate* has function? There could be a reason, but I can’t think of one. This looks like someone started using Optional but didn’t follow through all the way.

I can't think of any either. As far as I saw, there are no overridingContaining...() calls without a preceding hasOverriding...(). I've filled bug 218098 to deal with this (note that RenderBox.h replicates the same pattern).

[Sergio Villar Senin]

Committed r268919: <https://trac.webkit.org/changeset/268919>

[Radar WebKit Bug Importer]

<rdar://problem/70612084>