[webkit-reviews] review denied: [Bug 6503] content property doesn't support quotes : [Attachment 20365] Final patch

[bugzilla-daemon at webkit.org]

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

Attachment 20365: Final patch
http://bugs.webkit.org/attachment.cgi?id=20365&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
There seem to be a bunch of little issues in this patch.

We'll see how many I get this first round.  I fear we may have to got a couple
rounds.

1.  There are a bunch of style guideline violations re: placement of "*"
http://webkit.org/coding/coding-style.html

No { } here:
 1921		  if (value2 && value2->unit == CSSPrimitiveValue::CSS_STRING)
{
 1922		      values->append(new QuotesValue(value1->string,
value2->string));
 1923		  } else

or here:

 3449	      if (primitiveValue && primitiveValue->getIdent() == CSSValueNone)
{
 3450		  m_style->setQuotes(0);
 3451	      } else {


I'm confused by this?
/ FIXME: we should use the version present in with CSSPrimitiveValue.cpp
 37 static String quoteString(const String& string)
 

Again, {}
 66	if (m_quote == CLOSE_QUOTE || m_quote == NO_CLOSE_QUOTE) {
 67	    --level;
 68	}


This seems hackish?

 111 void RenderQuote::calcPrefWidths(int lead)
 112 {
 113	 setTextInternal(originalText());
 114	 RenderText::calcPrefWidths(lead);
 115 }

(the setTextInternal during a calcPrefsWidths call).  Why is that
needed/correct?

0 <= m_level (for isVisible) would be more clear as m_level >= 0 IMO.


Why can't:
 839 bool StyleRareInheritedData::quotesDataEquivalent(const
StyleRareInheritedData& o) const

just be
return m_quotes == o.m_quotes && *m_quotes == *o.m_quotes ?
yes, that depends on the compiler short-circuting the &&, but that's safe.


Bah.  I wish the various setContent calls could share more code.

Any time you're returning an AtomicString, return emptyAtom instead of "";
there are at least 2 times where you could do that.

Generally each initializer gets its own line:
 1068	  QuotesData(const QuotesData& o)
 1069	      : m_openQuotes(o.m_openQuotes), m_closeQuotes(o.m_closeQuotes) {
}


I think m_quotes is leaking when set.  Can't m_quotes just be an OwnPtr?  I
don't think it needs to be a DataRef, but that would be another option.

In general the patch looks good.  r- for the various issues above.