[webkit-reviews] review denied: [Bug 15602] Quirksmode: CSS1: WebKit fails dynamic :first-letter test : [Attachment 457268] Update first-letter-block-change-expected files

[bugzilla-daemon at webkit.org]

Joone Hur <joone at webkit.org> has denied Joone Hur <joone at webkit.org>'s request
for review:
Bug 15602: Quirksmode: CSS1: WebKit fails dynamic :first-letter test
https://bugs.webkit.org/show_bug.cgi?id=15602

Attachment 457268: Update first-letter-block-change-expected files

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




--- Comment #52 from Joone Hur <joone at webkit.org> ---
Comment on attachment 457268
  --> https://bugs.webkit.org/attachment.cgi?id=457268
Update first-letter-block-change-expected files

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

>> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:171
>> +	RenderElement* firstLetter = firstLetterText.parent();
> 
> auto*

Done.

>> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:174
>> +	RenderElement* firstLetterContainer = firstLetter->parent();
> 
> auto*
> 
> But also, this local variable doesn’t need to exist. We only use it one place
below and we could write firstLetter->parent() there. Unless it’s here because
we want to write assertions?

Removed this line.

>> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:175
>> +	ASSERT(firstLetter->isFloating() || firstLetter->isInline());
> 
> Can we move this ASSERT up? It’s asserting things about firstLetter, not
firstLettterContainer.

Done.

>> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:177
>> +	// Check if the first letter was part of the remaning text. If not,
> 
> Typo here: "remaning".

Done.

>> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:179
>> +	RenderObject* remainingText = firstLetter->nextSibling();
> 
> auto*

Done.

>> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:180
>> +	if (remainingText && firstLetterText.node() != remainingText->node()) {
> 
> Maybe we could do early return instead of nesting for this? We use early
return for what’s next.

Done.

>> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:184
>> +	    if (RenderTextFragment* oldRemainingText =
downcast<RenderBoxModelObject>(*firstLetter).firstLetterRemainingText()) {
> 
> auto*
> 
> And consider early return.

Done.

>> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:186
>> +		// and create a RenderText with the original text recovered
from the corresponding DOM node.
> 
> This comment seems to be a line early.

Done.

>> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:187
>> +		if (Text* text = oldRemainingText->textNode()) {
> 
> auto*
> 
> And consider early return.

Done.

>> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:188
>> +		    RenderPtr<RenderText> originalText =
createRenderer<RenderText>(*text, text->data());
> 
> auto

Done.