[webkit-reviews] review granted: [Bug 217898] Rename override sizes to overriding sizes : [Attachment 411969] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 217898: Rename override sizes to overriding sizes
https://bugs.webkit.org/show_bug.cgi?id=217898

Attachment 411969: Patch

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




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 411969
  --> https://bugs.webkit.org/attachment.cgi?id=411969
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.