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

[bugzilla-daemon at webkit.org]

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


eric at webkit.org changed:

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




------- Comment #7 from eric at webkit.org  2008-04-30 23:45 PDT -------
(From update of attachment 20365)
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.


-- 
Configure bugmail: http://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.