As part of WCSS extensions, WCSS marquee extension is also relevant to XHMLMP, so that this bug depends on bug 23452
Created attachment 27310 [details] inital patch for WCSS marquee support
Without the XHMLMP patch filed in bug 23452, this patch won't work because WCSS is supposed to used with XHMLMP document
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.
Created attachment 27652 [details] updated patch for layout tests
Created attachment 30322 [details] Updated patch for adding tests Latest tests for WCSS marquee extension support
Created attachment 30323 [details] Updated patch for code changes
Created attachment 30444 [details] Latest Updated patch for code changes Add copyright messages for code changes
Comment on attachment 30322 [details] Updated patch for adding tests They should also be added to Qt skipped list, but okay.
Checked in as 43888.
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
As Eric noted, this has been rolled out due to many failing layout test: http://trac.webkit.org/changeset/43889
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.
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.
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.
Comment on attachment 30322 [details] Updated patch for adding tests Clearing review since patch was landed.
Comment on attachment 30444 [details] Latest Updated patch for code changes Clearing review flag since the patch was rolled out.
Seems to be a symptom of bit rot on the old patch. We'll have to fix it up and try again today.
(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.
(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.
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!
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.
(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.
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
(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
(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.
(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
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) .
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
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.
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.
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;
(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?
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.
(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?
(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?
(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.
(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.
(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". :)
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.