Bug 23727 – Implementaion of WCSS marquee extension relevant to XHML MP

• Component: CSS
• Status: RESOLVED FIXED

[yichao.yin]

As part of WCSS extensions, WCSS marquee extension is also relevant to XHMLMP, so that this bug depends on bug 23452

[yichao.yin]

Created attachment 27310 [details]
inital patch for WCSS marquee support

[yichao.yin]

Without the XHMLMP patch filed in bug 23452, this patch won't work because WCSS is supposed to used with XHMLMP document

[yichao.yin]

Created attachment 27651 [details]
updated patch for WCSS marquee extension


split the initial patch into two parts: one is for changed code, the other is for layout tests.

[yichao.yin]

Created attachment 27652 [details]
updated patch for layout tests

[yichao.yin]

Created attachment 30322 [details]
Updated patch for adding tests

Latest tests for WCSS marquee extension support

[yichao.yin]

Created attachment 30323 [details]
Updated patch for code changes

[yichao.yin]

Created attachment 30444 [details]
Latest Updated patch for code changes

Add copyright messages for code changes

[George Staikos]

Comment on attachment 30322 [details]
Updated patch for adding tests

They should also be added to Qt skipped list, but okay.

[George Staikos]

Checked in as 43888.

[Eric Seidel (no email)]

This change caused 31 tests to fail.  Dave Levin is about to roll it out.

http://build.webkit.org/builders/Tiger%20Intel%20Release/builds/1207

[David Levin]

As Eric noted, this has been rolled out due to many failing layout test: http://trac.webkit.org/changeset/43889

[Maciej Stachowiak]

The problem is likely due to new CSS property names and values being added unconditionally to the .in files.

I think it would be best to have separate .in files for the WAP-related CSS properties, the same way we have SVGCSSPropertyNames.in, SVGCSSValueKeywords.in and DashboardSupportCSSPropertyNames.in.

[David Levin]

fyi, since I was looking at this change due to the test break, I noticed some style nits in the change:

In WebCore/css/CSSParser.cpp
    FInteger|FNonNeg 
 should be 
    FInteger | FNonNeg

    FTime|FInteger|FNonNeg
 same as above.

In WebCore/css/CSSStyleSelector.cpp

    EOverflow o = OMARQUEE;
 This variable appears unused.


   switch (primitiveValue->getIdent()) {
       case CSSValueLtr:
The case should align with the switch -- the statements in the case should be indented.


WebCore/rendering/RenderMarquee.cpp
   if (m_totalLoops == 0)
should be
   if (!m_totalLoops)

Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.

[Mark Rowe (bdash)]

Even if they had been added conditionally the change still would have been wrong, but only for builds where the new code was enabled.  The problem is that a new value keyword was added to CSSValueKeywords.in and the comment at the top of the file was ignored:

# WARNING:
# --------
#
# The Values are sorted according to the properties they belong to,
# and have to be in the same order as the enums in RenderStyle.h.
#
# If not, the optimizations in the cssparser and style selector will fail,
# and produce incorrect results.


This patch results in the values for the display property being out of sync with the values of the EDisplay enum in RenderStyleConstants.h, which leads to the incorrect results that the comment warns of.

[Mark Rowe (bdash)]

Comment on attachment 30322 [details]
Updated patch for adding tests

Clearing review since patch was landed.

[Mark Rowe (bdash)]

Comment on attachment 30444 [details]
Latest Updated patch for code changes

Clearing review flag since the patch was rolled out.

[George Staikos]

Seems to be a symptom of bit rot on the old patch.  We'll have to fix it up and try again today.

[George Staikos]

(In reply to comment #14)
> Even if they had been added conditionally the change still would have been
> wrong, but only for builds where the new code was enabled.  The problem is that
> a new value keyword was added to CSSValueKeywords.in and the comment at the top
> of the file was ignored:

  Not ignored.  It's not surprising this happened since this code long long long predates that optimization, and even the initial rewrite to try to get it merged predates the optimization.  Likewise the test tree predates it.  A good reason why patches should not be left to rot for so long.

[George Staikos]

(In reply to comment #18)
> (In reply to comment #14)
> > Even if they had been added conditionally the change still would have been
> > wrong, but only for builds where the new code was enabled.  The problem is that
> > a new value keyword was added to CSSValueKeywords.in and the comment at the top
> > of the file was ignored:
> 
>   Not ignored.  It's not surprising this happened since this code long long
> long predates that optimization, and even the initial rewrite to try to get it
> merged predates the optimization.  Likewise the test tree predates it.  A good
> reason why patches should not be left to rot for so long.

   Actually not so sure about how this happened after all.  Too many changes happened since the original patch.

[yichao.yin]

Created attachment 30536 [details]
Latest patch for code changes

Thank all of you for the comments.

This patch tries to fix the issues mentioned by you guys.
I haven't verified it on tiger since I just have Qt+Linux environment.
George, please help to verify it. thanks!

[Maciej Stachowiak]

Comment on attachment 30536 [details]
Latest patch for code changes

It looks like the new -wap keywords are still being added unconditionally to the .in files. That's wrong, especially in the case of the addition to CSSValueKeywords.in.If you add -wap-marquee to the .in file unconditionally, but only add it to the EDisplay enum conditionally, the tests will break on platforms with WCSS enabled the same way as before.

[yichao.yin]

(In reply to comment #21)
> (From update of attachment 30536 [details] [review])
> It looks like the new -wap keywords are still being added unconditionally to
> the .in files. That's wrong, especially in the case of the addition to
> CSSValueKeywords.in.If you add -wap-marquee to the .in file unconditionally,
> but only add it to the EDisplay enum conditionally, the tests will break on
> platforms with WCSS enabled the same way as before.

Thanks, you are right. will fix it soon.

[yichao.yin]

Created attachment 30576 [details]
Updated patch for code changes


According to Maciej's comment, fixed the issue of without conditionalization constrains for WAP marquee css properties and values

[Nikolas Zimmermann]

(In reply to comment #23)
> Created an attachment (id=30576) [review]
> Updated patch for code changes
> 
> 
> According to Maciej's comment, fixed the issue of without conditionalization
> constrains for WAP marquee css properties and values.

Patch looks great! Three issues remaining, otherwhise I'd r+ it:

1) Maciej commented before that he'd like a seperated .in file for WCSS before. Or is it fine to add these properties in ENABLE(WML) blocks to the CSS*.in files directly?

2) Hardcoded changes in CSSStyleSelector:

> +            if (primitiveValue->getIdent() == CSSValueWapMarquee) {
> +                // Initialize Wap Marquee style 
> +                m_style->setOverflowX(OMARQUEE);
> +                m_style->setOverflowY(OMARQUEE);
> +                m_style->setWhiteSpace(NOWRAP);
> +                m_style->setMarqueeDirection(MLEFT);
> +                m_style->setMarqueeSpeed(85); // Normal speed 
> +                m_style->setMarqueeLoopCount(1);
> +                m_style->setMarqueeBehavior(MSCROLL);

Hm, in WML the marquee mode is enabled using the display="-wap-marquee" _attribute_ right?
Then you could move this to a WCSS specific .css file, somthing along the lines:
*[display="-wap-marquee"] { overflow: ... }

The universal selector combined with a specific attribute selector would do.

3) The changes in RenderMarquee concern me a bit, as they'd also affect the HTML marquee implementation, if WML is enabled. Is that fine? Or shouldn't we better check if we're operating on a WML document?

> Index: WebCore/rendering/RenderMarquee.cpp
> ===================================================================
> --- WebCore/rendering/RenderMarquee.cpp	(revision 44034)
> +++ WebCore/rendering/RenderMarquee.cpp	(working copy)
> @@ -1,5 +1,6 @@
>  /*
>   * Copyright (C) 2006, 2007, 2008 Apple Inc. All rights reserved.
> + * Copyright (C) 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
>   *
>   * Portions are Copyright (C) 1998 Netscape Communications Corporation.
>   *
> @@ -196,6 +197,12 @@ void RenderMarquee::updateMarqueePositio
>      if (activate) {
>          EMarqueeBehavior behavior = m_layer->renderer()->style()->marqueeBehavior();
>          m_start = computePosition(direction(), behavior == MALTERNATE);
> +#if ENABLE(WCSS)
> +        // As per the spec WAP-239-WCSS-20011026-a.pdf, if the '-wap-marquee-loop' is '0', no looping occurs.
> +        if (!m_totalLoops)
> +            m_end = m_start;
> +        else 
> +#endif
>          m_end = computePosition(reverseDirection(), behavior == MALTERNATE || behavior == MSLIDE);
>          if (!m_stopped)
>              start();
> @@ -298,6 +305,10 @@ void RenderMarquee::timerFired(Timer<Ren
>          m_currentLoop++;
>          if (m_totalLoops > 0 && m_currentLoop >= m_totalLoops)
>              m_timer.stop();
> +#if ENABLE(WCSS)
> +        else if (!m_totalLoops && m_timer.isActive())
> +            m_timer.stop();
> +#endif
>          else if (s->marqueeBehavior() != MALTERNATE)
>              m_reset = true;
>      }

Have a nice day,
Niko

[Maciej Stachowiak]

(In reply to comment #24)
> (In reply to comment #23)
> > Created an attachment (id=30576) [review] [review]
> > Updated patch for code changes
> > 
> > 
> > According to Maciej's comment, fixed the issue of without conditionalization
> > constrains for WAP marquee css properties and values.
> 
> Patch looks great! Three issues remaining, otherwhise I'd r+ it:
> 
> 1) Maciej commented before that he'd like a seperated .in file for WCSS before.
> Or is it fine to add these properties in ENABLE(WML) blocks to the CSS*.in
> files directly?
> 

For the values, it turns out that's not possible, since the patch adds a new value for the 'display' property.

r- for the remaining issues Niko cited, please update and repost.

[yichao.yin]

(In reply to comment #24)
> (In reply to comment #23)
> > Created an attachment (id=30576) [review] [review]
> > Updated patch for code changes
> > 
> > 
> > According to Maciej's comment, fixed the issue of without conditionalization
> > constrains for WAP marquee css properties and values.
> Patch looks great! Three issues remaining, otherwhise I'd r+ it:
> 1) Maciej commented before that he'd like a seperated .in file for WCSS before.
> Or is it fine to add these properties in ENABLE(WML) blocks to the CSS*.in
> files directly?

ENABLE(WML)? you mean ENABLE(WCSS)? As for WAP marquee, it is not required by WML 1.3 but WML 2.0.
And current WebKit just supports WML 1.3. right? 
Like Maciej said, it's not fine to add the property to CSS*.in files directly.


> 2) Hardcoded changes in CSSStyleSelector:
> > +            if (primitiveValue->getIdent() == CSSValueWapMarquee) {
> > +                // Initialize Wap Marquee style 
> > +                m_style->setOverflowX(OMARQUEE);
> > +                m_style->setOverflowY(OMARQUEE);
> > +                m_style->setWhiteSpace(NOWRAP);
> > +                m_style->setMarqueeDirection(MLEFT);
> > +                m_style->setMarqueeSpeed(85); // Normal speed 
> > +                m_style->setMarqueeLoopCount(1);
> > +                m_style->setMarqueeBehavior(MSCROLL);
> Hm, in WML the marquee mode is enabled using the display="-wap-marquee"
> _attribute_ right?

Actually, WCSS marquee extension is for XHMLMP. and not WML1.3 but WML2.0 is required to support it.

> Then you could move this to a WCSS specific .css file, somthing along the
> lines:
> *[display="-wap-marquee"] { overflow: ... }
> The universal selector combined with a specific attribute selector would do.

Yeah, sounds better, I will try to reimplement it. thanks. :)


> 3) The changes in RenderMarquee concern me a bit, as they'd also affect the
> HTML marquee implementation, if WML is enabled. Is that fine? Or shouldn't we
> better check if we're operating on a WML document?

Like I said above, now we needn't consider WML. But you are right, I should 
consider checking if it's an XHTMLMP document to avoid to affect HTML marquee.

> > Index: WebCore/rendering/RenderMarquee.cpp
> > ===================================================================
> > --- WebCore/rendering/RenderMarquee.cpp	(revision 44034)
> > +++ WebCore/rendering/RenderMarquee.cpp	(working copy)
> > @@ -1,5 +1,6 @@
> >  /*
> >   * Copyright (C) 2006, 2007, 2008 Apple Inc. All rights reserved.
> > + * Copyright (C) 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
> >   *
> >   * Portions are Copyright (C) 1998 Netscape Communications Corporation.
> >   *
> > @@ -196,6 +197,12 @@ void RenderMarquee::updateMarqueePositio
> >      if (activate) {
> >          EMarqueeBehavior behavior = m_layer->renderer()->style()->marqueeBehavior();
> >          m_start = computePosition(direction(), behavior == MALTERNATE);
> > +#if ENABLE(WCSS)
> > +        // As per the spec WAP-239-WCSS-20011026-a.pdf, if the '-wap-marquee-loop' is '0', no looping occurs.
> > +        if (!m_totalLoops)
> > +            m_end = m_start;
> > +        else 
> > +#endif
> >          m_end = computePosition(reverseDirection(), behavior == MALTERNATE || behavior == MSLIDE);
> >          if (!m_stopped)
> >              start();
> > @@ -298,6 +305,10 @@ void RenderMarquee::timerFired(Timer<Ren
> >          m_currentLoop++;
> >          if (m_totalLoops > 0 && m_currentLoop >= m_totalLoops)
> >              m_timer.stop();
> > +#if ENABLE(WCSS)
> > +        else if (!m_totalLoops && m_timer.isActive())
> > +            m_timer.stop();
> > +#endif
> >          else if (s->marqueeBehavior() != MALTERNATE)
> >              m_reset = true;
> >      }
> Have a nice day,
> Niko
Thanks for your comments, Niko!
Have a nice day! :)
Yichao

[Charles Wei]

Created attachment 34119 [details]
new patch with seperate .in file for the new WCSS properties.

This is a new patch for the code changes, the most important change is that it seperates the WCSS specific CSS properties in a seperate .in file, and can be conditionally picked up by webkit with ENABLE(WCSS) .

[Nikolas Zimmermann]

Comment on attachment 34119 [details]
new patch with seperate .in file for the new WCSS properties.

Hi Charles,

r-, because the old review comments still apply. Still hardcoded things in CSSStyleSelector that should go in a .css file, and the RenderMarquee changes that affect HTML. These are the only remaining issues, the introduction of the new .in files look good.

Have a nice day,
NIko

[Charles Wei]

Created attachment 34648 [details]
resubmit the patch to address comments

This patch differes from the patch submitted on Aug 8 , 2009 is that :

For the changes for RenderMarquee,  it only applys when the WCSS and XHTMLMP are enabled, and the document is of XHTMLMP type,  no impacts to the HTML documents. 

For the other comment from Niko to have a .css file to replace the hard-coded codes that initializes the marquee status with :

*[display="-wap-marquee"] {overflow=...}

Actually this doesn't work. The above universal selector works for something like display as an attribute like:

<a display="-wap-marquee">

There's no such an attribute for HTML/xHTML tags,  actually the WCSS marquee is an extension to WCSS display and it works like this : 

<a href="..." style="display:-wap-marquee;-wap-marquee-loop:xxx;-wap-marquee-dir:xxx;-wap-marquee-style:xxx;-wap-marquee-speed:xxx">

So the universal selector  *[display="-wap-marquee"] {...} doesn't work.

[Nikolas Zimmermann]

Comment on attachment 34648 [details]
resubmit the patch to address comments

Hi Charles,

patch looks great, along with your comments (regarding the impossiblity to not-hardcode the stuff.)

One last question:

> Index: WebCore/css/WCSSValueKeywords.in
> ===================================================================
> --- WebCore/css/WCSSValueKeywords.in	(revision 0)
> +++ WebCore/css/WCSSValueKeywords.in	(revision 0)
> @@ -0,0 +1 @@
> +# place holder for all WCSS specific CSS value keywords

Do we need this file? I suspect you _may_ need it for the build process?
If not, please remove this file completly, and reupload a patch, then I'll set "commit-queue +" as well.

[Eric Seidel (no email)]

Comment on attachment 34648 [details]
resubmit the patch to address comments


 157     if (m_layer->renderer()->document()->isXHTMLMPDocument()) {
 158         if (m_timer.isActive() || m_layer->renderer()->style()->marqueeIncrement().isZero() || !m_layer->renderer()->style()->marqueeLoopCount())    
 159             return;
 160     } else {
 161         if (m_timer.isActive() || m_layer->renderer()->style()->marqueeIncrement().isZero())
 162             return;
 163     }
 164     

I would have written that as:

161         if (m_timer.isActive() || m_layer->renderer()->style()->marqueeIncrement().isZero() || (m_layer->renderer()->document()->isXHTMLMPDocument() && !m_layer->renderer()->style()->marqueeLoopCount())
 162             return;

in fact, that can seasily just be added as part of the existing if:

     if (m_timer.isActive() || m_layer->renderer()->style()->marqueeIncrement().isZero()
#if WCSS
|| (m_layer->renderer()->document()->isXHTMLMPDocument() && !m_layer->renderer()->style()->marqueeLoopCount()
#endif
}
156167         return;

[George Staikos]

(In reply to comment #31)
> (From update of attachment 34648 [details])
>
> in fact, that can seasily just be added as part of the existing if:
> 
>      if (m_timer.isActive() ||
> m_layer->renderer()->style()->marqueeIncrement().isZero()
> #if WCSS
> || (m_layer->renderer()->document()->isXHTMLMPDocument() &&
> !m_layer->renderer()->style()->marqueeLoopCount()
> #endif
> }
> 156167         return;


I'm already on it.  Fixed a few other minor style issues and this one, but in the end took your implementation as above.

I'm still concerned about the unconditional addition to CSSValueKeywords.in.  How can we deal with that, or is it just fine to include it?

[George Staikos]

Checked in as r47176.  If the CSSValueKeywords.in change is an issue, we should back it out and find a solution but right now I can't find a reason against it.

[Nikolas Zimmermann]

(In reply to comment #31)
> (From update of attachment 34648 [details])
> 
>  157     if (m_layer->renderer()->document()->isXHTMLMPDocument()) {
>  158         if (m_timer.isActive() ||
> m_layer->renderer()->style()->marqueeIncrement().isZero() ||
> !m_layer->renderer()->style()->marqueeLoopCount())    
>  159             return;
>  160     } else {
>  161         if (m_timer.isActive() ||
> m_layer->renderer()->style()->marqueeIncrement().isZero())
>  162             return;
>  163     }
>  164     
> 
> I would have written that as:
> 
> 161         if (m_timer.isActive() ||
> m_layer->renderer()->style()->marqueeIncrement().isZero() ||
> (m_layer->renderer()->document()->isXHTMLMPDocument() &&
> !m_layer->renderer()->style()->marqueeLoopCount())
>  162             return;
> 
> in fact, that can seasily just be added as part of the existing if:
> 
>      if (m_timer.isActive() ||
> m_layer->renderer()->style()->marqueeIncrement().isZero()
> #if WCSS
> || (m_layer->renderer()->document()->isXHTMLMPDocument() &&
> !m_layer->renderer()->style()->marqueeLoopCount()
> #endif
> }
> 156167         return;

Okay, but that doesn't satifsy a r- in my opinion. It could be done before landing.
Any other reason for that?

[Nikolas Zimmermann]

(In reply to comment #33)
> Checked in as r47176.  If the CSSValueKeywords.in change is an issue, we should
> back it out and find a solution but right now I can't find a reason against it.

What about the empty WCSSValueKeywords.in file? No way to remove it?

[George Staikos]

(In reply to comment #35)
> (In reply to comment #33)
> > Checked in as r47176.  If the CSSValueKeywords.in change is an issue, we should
> > back it out and find a solution but right now I can't find a reason against it.
> 
> What about the empty WCSSValueKeywords.in file? No way to remove it?

Harmless and I think it's to be used later?  Otherwise I'll remove it.

[Nikolas Zimmermann]

(In reply to comment #36)
> (In reply to comment #35)
> > (In reply to comment #33)
> > > Checked in as r47176.  If the CSSValueKeywords.in change is an issue, we should
> > > back it out and find a solution but right now I can't find a reason against it.
> > 
> > What about the empty WCSSValueKeywords.in file? No way to remove it?
> 
> Harmless and I think it's to be used later?  Otherwise I'll remove it.
Oh, if there are follow-up patches, that may add content there, I see no problem.

[Eric Seidel (no email)]

(In reply to comment #34)
> Okay, but that doesn't satifsy a r- in my opinion. It could be done before
> landing.
> Any other reason for that?

AFAIK Charles Wei is not a committer, so it wouldn't be possible for him to fix those before landing anyway.  I guess I view "r-" as more "not quite perfect, try again" instead of "completely rejected, go home". :)

[Charles Wei]

We have two other patches for other (In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #35)
> > > (In reply to comment #33)
> > > > Checked in as r47176.  If the CSSValueKeywords.in change is an issue, we should
> > > > back it out and find a solution but right now I can't find a reason against it.
> > > 
> > > What about the empty WCSSValueKeywords.in file? No way to remove it?
> > 
> > Harmless and I think it's to be used later?  Otherwise I'll remove it.
> Oh, if there are follow-up patches, that may add content there, I see no
> problem.


We have two other patches for other WCSS features, which might put CSS values in WCSSVAlueKeywords.in ,  that's why we keep it as a placeholder.