Rename override sizes to overriden sizes
Created attachment 411731 [details] Patch
Gentle ping for reviewers.
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.
(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".
Created attachment 411969 [details] Patch
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.
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).
Committed r268919: <https://trac.webkit.org/changeset/268919>
<rdar://problem/70612084>