[webkit-reviews] review granted: [Bug 64059] REGRESSION (r88913):=?UTF-8?Q?=20Preview=20in=20Safari=E2=80=99s=20snippet=20editor=20has=20a=20fixed=20height=20instead=20of=20filling=20the=20entire=20pane=20?=: [Attachment 100976] Patch v6

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has granted  review:
Bug 64059: REGRESSION (r88913): Preview in Safari’s snippet editor has a fixed
height instead of filling the entire pane
https://bugs.webkit.org/show_bug.cgi?id=64059

Attachment 100976: Patch v6
https://bugs.webkit.org/attachment.cgi?id=100976&action=review

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=100976&action=review


r+ after the explanation, and assuming no regressions? Have a look before
landing that the if conditions are ordered according to the chance they are
hit, so if one of them is more likely to occur than the others, test it first.

>>> Source/WebCore/ChangeLog:16
>>> +	     wheter the height property is set on the containing block, there
are other ways to implicitly specify
>> 
>> wheter -> whether
> 
> Fixed.

wheter -> whether

>>> Source/WebCore/rendering/RenderReplaced.cpp:323
>>> +		 return false;
>> 
>> Can you tell me how the above while relates to this for? For instance where
does the isTableCell check come from?
> 
> Short version: if you leave out the table cell check you'll see regressions.
> 
> Long version: The code is inspired by RenderBlock::isSelfCollapsingBlock,
which contains similar (but not identical) logic to determine whether a block
has auto height or not:
> 
>     Length logicalHeightLength = style()->logicalHeight();
>     bool hasAutoHeight = logicalHeightLength.isAuto();
>     if (logicalHeightLength.isPercent() && !document()->inQuirksMode()) {
>	  hasAutoHeight = true;
>	  for (RenderBlock* cb = containingBlock(); !cb->isRenderView(); cb =
cb->containingBlock()) {
>	      if (cb->style()->logicalHeight().isFixed() || cb->isTableCell())
>		  hasAutoHeight = false;
>	  }
>     }
> 
> I'm determining whether a replaced object has auto height or not, and can't
reuse the code as we have have to take more things into account for replaced
elements, as indicated in my comment:
> 
>     // For percentage heights: The percentage is calculated with respect to
the height of the generated box's
>     // containing block. If the height of the containing block is not
specified explicitly (i.e., it depends
>     // on content height), and this element is not absolutely positioned, the
value computes to 'auto'.
> 
> Hope that resolves the question?

Can you tell me how the above while relates to this for? For instance where
does the isTableCell check come from?