[webkit-reviews] review granted: [Bug 209871] [css-flexbox] align-content should apply even when there's just a single line : [Attachment 400916] Patch

[bugzilla-daemon at webkit.org]

Manuel Rego Casasnovas <rego at igalia.com> has granted  review:
Bug 209871: [css-flexbox] align-content should apply even when there's just a
single line
https://bugs.webkit.org/show_bug.cgi?id=209871

Attachment 400916: Patch

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




--- Comment #15 from Manuel Rego Casasnovas <rego at igalia.com> ---
Comment on attachment 400916
  --> https://bugs.webkit.org/attachment.cgi?id=400916
Patch

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

r=me

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:365
>>> +	 if (!isMultiline() && !lineContexts.isEmpty())
>> 
>> We might want to move this condition to a separated method with a
descriptive name to be called here and later.
> 
> Hmm the condition here and the other one are not the same.
> 
>  (lineContexts.isEmpty() || !isMultiline())

Ok, true, I didn't realize sorry.

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:486
>>> +	 });
>> 
>> What does these lines do? Wouldn't be the same if we just this this two
lines before the return?
> 
> Yes, it's another way to do it. Do you prefer the other way?

makeScopeExit() is not used at all in WebCore/rendering/, and it's not like
this is a lot of code and you lose the context, so I'd prefer the other way
around.