[Webkit-unassigned] [Bug 6503] content property doesn't support quotes

[bugzilla-daemon at webkit.org]

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


eric at webkit.org changed:

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




------- Comment #11 from eric at webkit.org  2008-07-28 15:38 PDT -------
(From update of attachment 20959)
I am confused by:

1987     if (values->length()) {
 1988         addProperty(propId, values.release(), important);
 1989         if (value2)
 1990             valueList->next();
 1991         return true;
 1992     }

Couldn't that have been handled in the previous loop?

Also the previous loop:
 while (value1 = valueList->current()) {
 1974         if (value1->unit == CSSPrimitiveValue::CSS_STRING) {
 1975             value2 = valueList->next();
 1976             if (value2 && value2->unit == CSSPrimitiveValue::CSS_STRING)
 1977                 values->append(new QuotesValue(value1->string,
value2->string));
 1978             else
 1979                 break;
 1980         } else
 1981             break;
 1982 
 1983         value2 = 0;
 1984         valueList->next();
 1985     }

Would be easier to understand using an early-return.

if (value1->unit != CSSPrimitiveValue::CSS_STRING)
  break;

It's not clear to me why you advance valueList only if values->length() (my
first comment above).

* goes next to the type name in WebKit style:
 3577             CSSValueList *list = static_cast<CSSValueList*>(value);
That block has as bunch of misplaced *'s

Probably better to use a StringBuilder here:
47     String result("");
 48 
 49     result += quoteString(m_openQuote);
 50     result += " ";
 51     result += quoteString(m_closeQuote);

 44 void RenderQuote::computeLevel() const
looks horribly inefficient.  Can't it use the pre-computed levels from other
quotes it encounters?  I guess the question would be how to invalidate them
when the tree changes...
But if I'm reading that correctly, every RenderQuote will walk through *every*
RenderObject which corresponds to an element before it in a file.  Am I reading
that correctly?

* spacing:
 91     QuotesData *quotes = style()->quotes();

it seems this would be more efficient:
974     if (!m_quotes && o.m_quotes || m_quotes && !o.m_quotes)
 975         return false;
 976     if (m_quotes && o.m_quotes && (*m_quotes != *o.m_quotes))
 977         return false;
 978     return true;
written as:
if (m_quotes == o.m_quotes)
   return true;
return (m_quotes && o.m_quotes && (*m_quotes == *o.m_quotes));


This:
if (m_closeQuotes.size() <= level)
 1911         level = m_closeQuotes.size() - 1;
would read more clear to me as:
if (level >= m_closeQuotes.size())
   return m_closeQuotes.last();

spacing:
 1207     void addLevel(const AtomicString &open, const AtomicString &close);


You patch also needs a test case which covers syntax errors in "quotes"
property.

r- for the lack of syntax error test case.


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