[webkit-reviews] review denied: [Bug 6503] content property doesn't support quotes : [Attachment 20959] Patch with some fix

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 28 15:38:53 PDT 2008


Eric Seidel <eric at webkit.org> has denied mitz at webkit.org's request for review:
Bug 6503: content property doesn't support quotes
https://bugs.webkit.org/show_bug.cgi?id=6503

Attachment 20959: Patch with some fix
https://bugs.webkit.org/attachment.cgi?id=20959&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.


More information about the webkit-reviews mailing list