[Webkit-unassigned] [Bug 64230] REGRESSION (r73385): Marquee with behavior="alternate" is not working

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=64230


Andy Estes <aestes at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #122757|review?                     |review-
               Flag|                            |




--- Comment #20 from Andy Estes <aestes at apple.com>  2012-01-17 16:04:15 PST ---
(From update of attachment 122757)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.