[webkit-reviews] review denied: [Bug 64230] REGRESSION (r73385): Marquee with behavior="alternate" is not working : [Attachment 122757] Updated Patch

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has denied Parag Radke <nrqv63 at motorola.com>'s
request for review:
Bug 64230: REGRESSION (r73385): Marquee with behavior="alternate" is not
working
https://bugs.webkit.org/show_bug.cgi?id=64230

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

------- Additional Comments from Andy Estes <aestes at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=122757&action=review


I don't think you need to add a new image for your test. There are numerous
images already in LayoutTests that could be reused for this purpose. I also
don't think it's okay to add an image of Apple's trademarked logo without
permission from the copyright holder.

r- because of this. I have some additional comments and questions below.

> Source/WebCore/rendering/RenderBlock.cpp:4936
> +    if (!isTableCell() && style()->logicalWidth().isFixed() &&
style()->logicalWidth().value() > 0 && style()->marqueeBehavior() !=
MALTERNATE)

Your ChangeLog says that alternating marquees are a special case when
calculating logical width, but you don't explain why, and I don't understand
myself why this is the case. A previous version of your patch didn't have this
check, so I'd like to see more of a "why" explanation.

> LayoutTests/ChangeLog:9
> +	   and smaller testcase execution time.

You don't need to say "...and smaller testcase execution time." The slower test
was never checked in, so nobody will know that some previous iteration of this
patch included a slower test case.

> LayoutTests/fast/html/marquee-alternate-expected.txt:4
> +PASS on Initial Position
> +PASS on after half Cycle Completion
> +PASS on after full Cycle Completion

The phrases "Initial Position" and "Cycle Completion" do not need to be
capitalized.

> LayoutTests/fast/html/marquee-alternate.html:10
> +var halfCycle = 75;
> +var fullCycle = 150;

The terms "halfCycle" and "fullCycle" are inaccurate. A full cycle of the
alternating marquee takes 2ms, not 150ms. 150ms would be 75 cycles. Is it
possible for this entire test to run in 2ms instead?