[webkit-reviews] review denied: [Bug 23477] Support for WCSS extensions : [Attachment 26929] initial patch for WCSS marquee styles

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 23 13:50:49 PST 2009

Darin Adler <darin at apple.com> has denied maheshK <mahesh.kulkarni at nokia.com>'s
request for review:
Bug 23477: Support for WCSS extensions

Attachment 26929: initial patch for WCSS marquee styles 

------- Additional Comments from Darin Adler <darin at apple.com>
I think Hyatt should review this.

> +    RenderStyle* s = m_layer->renderer()->style();
> +    
> +    if(s->marqueeLoopCount() == 0 && s->display() == WAP_MARQUEE)

WebKit coding style would use a longer name, perhaps "style" for the local

WebKit coding style would include a space after the word "if" before the "(".

WebKit coding style would normally use "!" rather than "== 0".

This patch adds lots of "wap" CSS properties, but it adds them unconditionally.
That means they can be tested with regression tests. So this needs to include
some regression tests demonstrating these properties are present.

This patch adds lots of "wap" CSS properties that it doesn't implement. Is that
really the right way to do this?

This patch adds some properties, like -wap-marquee-speed, without adding
computed style support. It's unfortunate that there are so many old properties
where we don't do computed style correctly. We don't want to make things even
worse by adding new properties that also don't have computed style support. So
that should be included with any new CSS properties.

I'm going to say review-, but Hyatt needs to review for the more basic
questions like, "Should we have these '-wap' properties unconditionally in all
WebKit-based browsers?"

More information about the webkit-reviews mailing list