Bug 218975 - [css-flex] Images as flex items should use the overridingLogicalHeight when defined to compute the logical width
Summary: [css-flex] Images as flex items should use the overridingLogicalHeight when d...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-16 02:56 PST by Sergio Villar Senin
Modified: 2020-11-20 01:34 PST (History)
13 users (show)

See Also:

Attachments
Patch (12.72 KB, patch)
2020-11-16 03:07 PST, Sergio Villar Senin 		no flags Details | Formatted Diff | Diff
Patch (12.94 KB, patch)
2020-11-17 02:13 PST, Sergio Villar Senin 		no flags Details | Formatted Diff | Diff
Patch (12.99 KB, patch)
2020-11-18 05:32 PST, Sergio Villar Senin 		rego: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2020-11-16 02:56:21 PST 
RenderReplaced should use the overridingLogicalHeight when defined to compute the logical width
Comment 1 Sergio Villar Senin 2020-11-16 03:07:29 PST 
Created attachment 414213 [details]
Patch
Comment 2 Sergio Villar Senin 2020-11-17 02:13:56 PST 
Created attachment 414322 [details]
Patch
Comment 3 Manuel Rego Casasnovas 2020-11-17 13:19:45 PST 
Comment on attachment 414322 [details]
Patch

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

> Source/WebCore/rendering/RenderReplaced.cpp:527
> +            ASSERT(intrinsicRatio);

intrisicRatio can be zero, so this ASSERT will crash (like it's happening on the debug EWS), Shouldn't we add it in the previous check?
Comment 4 Sergio Villar Senin 2020-11-18 02:28:53 PST 
(In reply to Manuel Rego Casasnovas from comment #3)
> Comment on attachment 414322 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414322&action=review
> 
> > Source/WebCore/rendering/RenderReplaced.cpp:527
> > +            ASSERT(intrinsicRatio);
> 
> intrisicRatio can be zero, so this ASSERT will crash (like it's happening on
> the debug EWS), Shouldn't we add it in the previous check?

I'm trying to know how intrinsicRatio is zero given that both the intrinsic width and the intrinsic height are > 0. I thought it won't be possible, that's why I added the ASSERT.
Comment 5 Sergio Villar Senin 2020-11-18 05:17:03 PST 
(In reply to Sergio Villar Senin from comment #4)
> (In reply to Manuel Rego Casasnovas from comment #3)
> > Comment on attachment 414322 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=414322&action=review
> > 
> > > Source/WebCore/rendering/RenderReplaced.cpp:527
> > > +            ASSERT(intrinsicRatio);
> > 
> > intrisicRatio can be zero, so this ASSERT will crash (like it's happening on
> > the debug EWS), Shouldn't we add it in the previous check?
> 
> I'm trying to know how intrinsicRatio is zero given that both the intrinsic
> width and the intrinsic height are > 0. I thought it won't be possible,
> that's why I added the ASSERT.

Right so for example an iframe has indeed a intrinsic size but not necessarily an aspect ratio. Will change the condition and remove the assert
Comment 6 Sergio Villar Senin 2020-11-18 05:32:52 PST 
Created attachment 414442 [details]
Patch
Comment 7 Manuel Rego Casasnovas 2020-11-19 01:42:34 PST 
Comment on attachment 414442 [details]
Patch

r=me
Comment 8 Sergio Villar Senin 2020-11-20 01:33:52 PST 
Committed r270073: <https://trac.webkit.org/changeset/270073>
Comment 9 Radar WebKit Bug Importer 2020-11-20 01:34:40 PST 
<rdar://problem/71620147>