Bug 23727 - Implementaion of WCSS marquee extension relevant to XHML MP
Summary: Implementaion of WCSS marquee extension relevant to XHML MP
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 23452
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-04 02:07 PST by yichao.yin
Modified: 2009-08-13 18:28 PDT (History)
8 users (show)

See Also:


Attachments
inital patch for WCSS marquee support (14.35 KB, patch)
2009-02-04 02:14 PST, yichao.yin
no flags Details | Formatted Diff | Diff
updated patch for WCSS marquee extension (9.39 KB, patch)
2009-02-13 02:38 PST, yichao.yin
no flags Details | Formatted Diff | Diff
updated patch for layout tests (6.75 KB, patch)
2009-02-13 02:43 PST, yichao.yin
no flags Details | Formatted Diff | Diff
Updated patch for adding tests (6.85 KB, patch)
2009-05-14 02:47 PDT, yichao.yin
no flags Details | Formatted Diff | Diff
Updated patch for code changes (9.23 KB, patch)
2009-05-14 02:48 PDT, yichao.yin
no flags Details | Formatted Diff | Diff
Latest Updated patch for code changes (9.94 KB, patch)
2009-05-18 02:00 PDT, yichao.yin
no flags Details | Formatted Diff | Diff
Latest patch for code changes (12.63 KB, patch)
2009-05-21 03:03 PDT, yichao.yin
mjs: review-
Details | Formatted Diff | Diff
Updated patch for code changes (12.69 KB, patch)
2009-05-22 00:50 PDT, yichao.yin
mjs: review-
Details | Formatted Diff | Diff
new patch with seperate .in file for the new WCSS properties. (18.54 KB, patch)
2009-08-05 00:28 PDT, Charles Wei
zimmermann: review-
Details | Formatted Diff | Diff
resubmit the patch to address comments (18.49 KB, patch)
2009-08-12 02:36 PDT, Charles Wei
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yichao.yin 2009-02-04 02:07:45 PST
As part of WCSS extensions, WCSS marquee extension is also relevant to XHMLMP, so that this bug depends on bug 23452
Comment 1 yichao.yin 2009-02-04 02:14:05 PST
Created attachment 27310 [details]
inital patch for WCSS marquee support
Comment 2 yichao.yin 2009-02-04 02:17:04 PST
Without the XHMLMP patch filed in bug 23452, this patch won't work because WCSS is supposed to used with XHMLMP document
Comment 3 yichao.yin 2009-02-13 02:38:14 PST
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.
Comment 4 yichao.yin 2009-02-13 02:43:42 PST
Created attachment 27652 [details]
updated patch for layout tests
Comment 5 yichao.yin 2009-05-14 02:47:29 PDT
Created attachment 30322 [details]
Updated patch for adding tests

Latest tests for WCSS marquee extension support
Comment 6 yichao.yin 2009-05-14 02:48:44 PDT
Created attachment 30323 [details]
Updated patch for code changes
Comment 7 yichao.yin 2009-05-18 02:00:25 PDT
Created attachment 30444 [details]
Latest Updated patch for code changes

Add copyright messages for code changes
Comment 8 George Staikos 2009-05-19 20:38:08 PDT
Comment on attachment 30322 [details]
Updated patch for adding tests

They should also be added to Qt skipped list, but okay.
Comment 9 George Staikos 2009-05-19 21:35:16 PDT
Checked in as 43888.
Comment 10 Eric Seidel (no email) 2009-05-19 23:57:41 PDT
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
Comment 11 David Levin 2009-05-20 00:28:31 PDT
As Eric noted, this has been rolled out due to many failing layout test: http://trac.webkit.org/changeset/43889
Comment 12 Maciej Stachowiak 2009-05-20 00:34:29 PDT
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.

Comment 13 David Levin 2009-05-20 00:40:20 PDT
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.
Comment 14 Mark Rowe (bdash) 2009-05-20 00:43:13 PDT
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 15 Mark Rowe (bdash) 2009-05-20 00:43:32 PDT
Comment on attachment 30322 [details]
Updated patch for adding tests

Clearing review since patch was landed.
Comment 16 Mark Rowe (bdash) 2009-05-20 00:43:45 PDT
Comment on attachment 30444 [details]
Latest Updated patch for code changes

Clearing review flag since the patch was rolled out.
Comment 17 George Staikos 2009-05-20 06:12:05 PDT
Seems to be a symptom of bit rot on the old patch.  We'll have to fix it up and try again today.
Comment 18 George Staikos 2009-05-20 06:19:24 PDT
(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.
Comment 19 George Staikos 2009-05-20 11:01:29 PDT
(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.
Comment 20 yichao.yin 2009-05-21 03:03:55 PDT
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 21 Maciej Stachowiak 2009-05-22 00:10:29 PDT
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.
Comment 22 yichao.yin 2009-05-22 00:20:39 PDT
(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.
Comment 23 yichao.yin 2009-05-22 00:50:31 PDT
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
Comment 24 Nikolas Zimmermann 2009-05-23 07:43:20 PDT
(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
Comment 25 Maciej Stachowiak 2009-05-23 17:25:06 PDT
(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.
Comment 26 yichao.yin 2009-05-25 20:01:08 PDT
(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

Comment 27 Charles Wei 2009-08-05 00:28:17 PDT
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 28 Nikolas Zimmermann 2009-08-05 16:21:44 PDT
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
Comment 29 Charles Wei 2009-08-12 02:36:11 PDT
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 30 Nikolas Zimmermann 2009-08-12 06:13:54 PDT
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 31 Eric Seidel (no email) 2009-08-12 15:15:14 PDT
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;
Comment 32 George Staikos 2009-08-12 15:26:24 PDT
(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?
Comment 33 George Staikos 2009-08-12 19:58:48 PDT
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.
Comment 34 Nikolas Zimmermann 2009-08-13 05:54:16 PDT
(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?
Comment 35 Nikolas Zimmermann 2009-08-13 05:55:55 PDT
(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?
Comment 36 George Staikos 2009-08-13 06:54:21 PDT
(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.
Comment 37 Nikolas Zimmermann 2009-08-13 06:59:57 PDT
(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.
Comment 38 Eric Seidel (no email) 2009-08-13 08:59:37 PDT
(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". :)
Comment 39 Charles Wei 2009-08-13 18:28:02 PDT
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.