Bug 6503 - content property doesn't support quotes
Summary: content property doesn't support quotes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P3 Normal
Assignee: Nobody
URL: http://www.designmeme.com/articles/cs...
Keywords:
Depends on: 55508
Blocks: 52126 3234
  Show dependency treegraph
 
Reported: 2006-01-12 07:20 PST by Duncan Wilcox
Modified: 2011-03-02 13:24 PST (History)
18 users (show)

See Also:


Attachments
first patch (almost finished) for 'content: *quotes' and 'quotes' properties (26.63 KB, patch)
2008-03-24 07:13 PDT, Vincent Ricard
hyatt: review-
Details | Formatted Diff | Diff
Final patch (34.85 KB, patch)
2008-04-06 07:48 PDT, Vincent Ricard
eric: review-
Details | Formatted Diff | Diff
Patch with some fix (34.16 KB, patch)
2008-05-04 11:19 PDT, Vincent Ricard
eric: review-
Details | Formatted Diff | Diff
Patch reusing the previous computed level (35.60 KB, patch)
2008-09-02 13:26 PDT, Vincent Ricard
darin: review-
Details | Formatted Diff | Diff
newer patch -- not ready to check in (76.65 KB, patch)
2008-10-12 15:10 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Proposed Patch. Similar to Darin's, but taking advantage of the recent changes to RenderObject (60.58 KB, patch)
2011-02-08 08:42 PST, Carol Szabo
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch. Added support for language specific defaults. (74.18 KB, patch)
2011-02-14 08:54 PST, Carol Szabo
no flags Details | Formatted Diff | Diff
Proposed Patch. Fixed problem with Chromium build system. (75.10 KB, patch)
2011-02-14 10:05 PST, Carol Szabo
no flags Details | Formatted Diff | Diff
Proposed Patch. Fixed problem with Chromium build system. (75.10 KB, patch)
2011-02-14 10:54 PST, Carol Szabo
no flags Details | Formatted Diff | Diff
Proposed Patch. Rebased and fixed typing warning for some indexes. (75.21 KB, patch)
2011-02-16 14:16 PST, Carol Szabo
no flags Details | Formatted Diff | Diff
Proposed Patch. Fixed typo in WebCore.gypi (75.21 KB, patch)
2011-02-16 15:05 PST, Carol Szabo
no flags Details | Formatted Diff | Diff
Proposed Patch. Fixed problem with Windows build. (75.58 KB, patch)
2011-02-18 11:14 PST, Carol Szabo
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch. Addressed DHyatt's concerns. (78.72 KB, patch)
2011-02-18 15:41 PST, Carol Szabo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Duncan Wilcox 2006-01-12 07:20:19 PST
According to http://www.w3.org/TR/REC-CSS2/generate.html#propdef-content the content property 
should allow:

Value:  	[ <string> | <uri> | <counter> | attr(X) | open-quote | close-quote | no-open-quote | no-
close-quote ]+ | inherit

The page doesn't show quotes around the last blockquote because webkit doesn't appear support open-
quote | close-quote | no-open-quote | no-close-quote.

A simple workaround in CSS is to use "\201c" instead of open-quote and "\201d" instead of close-quote.
Comment 1 Mark Rowe (bdash) 2006-01-12 07:29:08 PST
This looks to be related to bug 3234.  To properly support open-quote and close-quote we should 
support the quotes property as specified at http://www.w3.org/TR/CSS21/generate.html#propdef-
quotes.  From there the correct quotation marks can be used by open-quote and close-quote.
Comment 2 Mark Rowe (bdash) 2006-01-12 07:41:51 PST
A good test case exercising both open-quote/close-quote and the quotes property can be found at 
http://www.meyerweb.com/eric/css/tests/css2/sec12-04-01.htm
Comment 3 Benjamin Hawkes-Lewis 2007-12-21 02:37:30 PST
"A simple workaround in CSS is to use "\201c" instead of open-quote and "\201d"
instead of close-quote."

This isn't an effective workaround since it can't properly handle the alternation of quotation marks (e.g. between double and single in US English) when quotations are nested inside one another.
Comment 4 Vincent Ricard 2008-03-24 07:13:16 PDT
Created attachment 20006 [details]
first patch (almost finished) for 'content: *quotes' and 'quotes' properties

Here is a patch working for the both url provoding in this bug report.
I still have to write the corresponding test cases, but i'd like have some comments/suggestions/review since the code is almost done.

This patch also covers #3234.

One thing is not yet fixed: choose the default open/close quote. I think this should be language dependent; but where put that? in html.css ? a new quote-lang.css?

A second thing: i've to add the copyright/license header :)
Comment 5 Dave Hyatt 2008-03-25 14:12:55 PDT
Comment on attachment 20006 [details]
first patch (almost finished) for 'content: *quotes' and 'quotes' properties

Nice work adding this!

The convention of _ arguments in the QuotesValue is not something we use these days.  Also the & should be with const String& in the QuoteValue constructor.

The QuoteValue object should have m_ in front of its member variable names, e.g., m_openQuote, m_closeQuote.

Pointer placement is wrong in the RenderQuote constructor.

Document *node

should be

Document* node

Is redoing pref widths inside dirtyLineBoxes really necessary, or did you just copy this file from RenderCounter and accidentally pick that up?

You can't just do a raw pointer comparison in the equality operator of StyleRareInheritedData.

&& m_quotes == o.m_quotes;

You need to actually check if the quotes objects are equivalent.

Extraneous whitespace change:

-    unsigned wordWrap : 1; // EWordWrap 
+    unsigned wordWrap : 1; // EWordWrap

In CSSStyleSelector for the case where you clear out the quotes (CSSValueNone), can't you just set to 0 instead of to new QuotesData?
Comment 6 Vincent Ricard 2008-04-06 07:48:47 PDT
Created attachment 20365 [details]
Final patch

Here is the final patch (all corrections asked by Dave are inside).

About the overriding of 'dirtyLineBoxes': if i don't do that, it does not work... and i dont know why, i'm still a newbie with the rendering engine :-)

The default quotes put in html4.css are the english ones (we could merge the whole list provided in #3234).
Comment 7 Eric Seidel (no email) 2008-04-30 23:45:40 PDT
Comment on attachment 20365 [details]
Final patch

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.
Comment 8 Vincent Ricard 2008-05-04 11:18:20 PDT
> 1.  There are a bunch of style guideline violations re: placement of "*"
> http://webkit.org/coding/coding-style.html

Fixed.

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

Because CSSPrimitiveValue.h does not expose quoteString() for the subclasses (the function is only declared in CSSPrimitiveValue.cpp). So i had to copy/paste it.

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

I fixed this by calling setTextInternal in dirtyLineBoxes. In fact, i don't know how/when set the internal text; i need the render tree to compute the depth, so i can't do that in the constructor (since the parent is not yet known).

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

Fixed.

> Why can't:
> 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.

I missed the trick :-/ 'm_quotes == o.m_quotes' does not mean '*m_quotes == *o.m_quotes' ?

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

I'd prefer my patch only corrects the css property. We could open an other bugs dedicated to this refactoring.

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

Fixed.

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

Fixed.

> 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.

Fixed with OwnPtr.

Comment 9 Vincent Ricard 2008-05-04 11:19:19 PDT
Created attachment 20959 [details]
Patch with some fix

Here is the new patch
Comment 10 mitz 2008-05-04 13:16:58 PDT
Comment on attachment 20959 [details]
Patch with some fix

Presumably Vincent meant to ask for a review :-)
Comment 11 Eric Seidel (no email) 2008-07-28 15:38:53 PDT
Comment on attachment 20959 [details]
Patch with some fix

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.
Comment 12 Vincent Ricard 2008-09-02 13:26:07 PDT
Created attachment 23125 [details]
Patch reusing the previous computed level

Here is a patch fixing all Eric's requirements.
About the level computation and the tree invalidation, maybe could we do as for the counters: have an "invalidQuote" method, and call it from RenderObject::destroy()?
Comment 13 Dave Hyatt 2008-10-03 13:12:51 PDT
Comment on attachment 23125 [details]
Patch reusing the previous computed level

r=me
Comment 14 Dave Hyatt 2008-10-03 13:13:30 PDT
Comment on attachment 23125 [details]
Patch reusing the previous computed level

This does need to add RenderQuote.cpp/h to other projects though (Mac/Windows) etc. or the build will be broken.

Whoever lands it will need to do that.
Comment 15 Darin Adler 2008-10-12 14:01:51 PDT
Comment on attachment 23125 [details]
Patch reusing the previous computed level

This also has a bug in the inherit case that will result in multiple styles sharing the same QuoteData and multiply deleting it.

review-

I'm going to fix this and land it.
Comment 16 Darin Adler 2008-10-12 15:05:10 PDT
This patch also didn't include expected results for the tests. Also, the patch changes the results of some existing tests; seemingly breaking fast/css-generated-content/005.html because the quotes aren't defaulting to straight quotes.
Comment 17 Darin Adler 2008-10-12 15:10:30 PDT
Created attachment 24311 [details]
newer patch -- not ready to check in

This patch has some of the problems with the previous patch fixed. But it breaks our existing regression test fast/css-generated-content/005.html, so it's not ready to check in yet.

I'm also uncertain if the results of the tests are good. I'd really prefer to find a way to write dumpAsText tests, but that may not be possible for generated content.
Comment 18 Daniel Upstone 2009-08-01 08:53:47 PDT
This bug has still not been resolved and is a serious browser inconsistency issue for CSS2 implementation.
It may be worth noting that IE8 is the only browser to do this correctly (yes, you read that correctly). Firefox does render correctly, but c&p of the output shows that it's still using forced quotes on the q element rather than providing the rendered result or a no-generated-content result. Opera seems to have it's own issues (eg. a stray " sneaking in) but is otherwise fine. Webkit still lacks full support and renders incorrectly.

Test Case: http://www.zealous-studios.co.uk/content-quotes.xhtml
The *-quote values for the content property are still not supported. This breaks any attempt to correctly fix https://bugs.webkit.org/show_bug.cgi?id=3234

This bug needs to be fixed first before any progress on bug 3234 can be made.
Comment 19 Carol Szabo 2011-02-08 08:42:36 PST
Created attachment 81638 [details]
Proposed Patch. Similar to Darin's, but taking advantage of the recent changes to RenderObject
Comment 20 WebKit Review Bot 2011-02-08 09:09:49 PST
Attachment 81638 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7763130
Comment 21 Dave Hyatt 2011-02-08 10:14:48 PST
Comment on attachment 81638 [details]
Proposed Patch. Similar to Darin's, but taking advantage of the recent changes to RenderObject

View in context: https://bugs.webkit.org/attachment.cgi?id=81638&action=review

Glad you are tackling this!  Some initial comments:

> Source/WebCore/css/CSSStyleSelector.cpp:4599
> +        HANDLE_INHERIT(quotes, Quotes)

Where is the handling of "initial"?

> Source/WebCore/css/html.css:28
> +    quotes: '"' '"' "'" "'";

This really shouldn't be necessary.  The CSS 2.1 specification says the initial value "depends on the user agent," which implies to me that the initial value is the quotes you want to use and not none.  Also, quotes should obviously work for XML documents, so let's just fix the initial value.

The initial value should probably just be a special keyword like -webkit-quotes.  Then your front end code can eventually use the current document language to pick the correct set of quotes (instead of hard-coding the quotes listed above).

> Source/WebCore/rendering/RenderQuote.cpp:80
> +    for (RenderObject* predecesor = head->previousInPreOrder(); predecesor; predecesor = predecesor->previousInPreOrder())

Should be predecessor.

> Source/WebCore/rendering/RenderQuote.cpp:178
> +        if (descendant->isQuote()) {

I think this for loop would read nicer if you just did

if (!descendant->isQuote())
    continue;

and then you don't have to indent all the code that comes after.
Comment 22 Carol Szabo 2011-02-08 11:02:46 PST
(In reply to comment #21)
> > Source/WebCore/css/CSSStyleSelector.cpp:4599
> > +        HANDLE_INHERIT(quotes, Quotes)
> 
> Where is the handling of "initial"?
I do not see that initial is allowed for quotes. But maybe we should support it anyway.
> 
> > Source/WebCore/css/html.css:28
> > +    quotes: '"' '"' "'" "'";
> 
> This really shouldn't be necessary.  The CSS 2.1 specification says the initial value "depends on the user agent," which implies to me that the initial value is the quotes you want to use and not none.  Also, quotes should obviously work for XML documents, so let's just fix the initial value.
> 
> The initial value should probably just be a special keyword like -webkit-quotes.  Then your front end code can eventually use the current document language to pick the correct set of quotes (instead of hard-coding the quotes listed above).
I understand the part with using the quotes as per document language, but I am not sure how to implement that. An example of another feature using this would be helpful.
What I understand so far is that I could use a keyword (such as initial) instead of an actual list of quotes in html.css and in CSSStyleSelector::applyProperty, transform that keyword into a real list when creating the RenderStyle object for an element.
What I do not know is how to specify this keyword to the root element of any type of document so that it is inherited by all elements unless one element in the tree defines it otherwise, and then its children will inherit that redefined value.
Comment 23 Dave Hyatt 2011-02-08 12:54:58 PST
Comment on attachment 81638 [details]
Proposed Patch. Similar to Darin's, but taking advantage of the recent changes to RenderObject

View in context: https://bugs.webkit.org/attachment.cgi?id=81638&action=review

> Source/WebCore/rendering/style/StyleRareInheritedData.h:91
> +    Vector<String> quotes;

Basically I'd consider turning the Vector into a pointer and only allocate it when there are concrete quotes specified explicitly in CSS.  Then you could add a member like bool m_useUAQuotes; and have that be set to true for the initial value (and false when quotes is explicitly set, either to none or to a concrete value).

Vector<String>* m_authorQuotes;
bool m_useUAQuotes;

Then you just need a good place to resolve to a real value if m_useUAQuotes is set instead of having a Vector.  It seems like you could do that in placeQuote and have a global hashmap in RenderQuote that just stores the different languages.  For the first landing you can just make a static Vector sitting right in placeQuote for English and use that if the user agent value is set.

You still need to patch CSSComputedStyleDeclaration as well, although doing that in a follow-up patch would be fine.
Comment 24 Carol Szabo 2011-02-14 08:54:39 PST
Created attachment 82320 [details]
Proposed Patch. Added support for language specific defaults.
Comment 25 WebKit Review Bot 2011-02-14 09:32:13 PST
Attachment 82320 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7905669
Comment 26 Build Bot 2011-02-14 09:57:28 PST
Attachment 82320 [details] did not build on win:
Build output: http://queues.webkit.org/results/7913581
Comment 27 Carol Szabo 2011-02-14 10:05:12 PST
Created attachment 82329 [details]
Proposed Patch. Fixed problem with Chromium build system.
Comment 28 WebKit Review Bot 2011-02-14 10:42:05 PST
Attachment 82329 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7907670
Comment 29 Carol Szabo 2011-02-14 10:54:44 PST
Created attachment 82340 [details]
Proposed Patch. Fixed problem with Chromium build system.
Comment 30 Build Bot 2011-02-14 11:14:32 PST
Attachment 82329 [details] did not build on win:
Build output: http://queues.webkit.org/results/7908669
Comment 31 WebKit Review Bot 2011-02-14 11:32:12 PST
Attachment 82340 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7910675
Comment 32 Build Bot 2011-02-14 13:04:51 PST
Attachment 82340 [details] did not build on win:
Build output: http://queues.webkit.org/results/7910694
Comment 33 Carol Szabo 2011-02-16 14:16:53 PST
Created attachment 82693 [details]
Proposed Patch. Rebased and fixed typing warning for some indexes.
Comment 34 Eli Grey (:sephr) 2011-02-16 14:43:28 PST
(In reply to comment #33)
I'm opposed to the simpleQuotes default, and feel that the UA's preferred locale should be the default, or englishQuotes if this is not possible (which possibly should be changed to latinQuotes). {"'} isn't that appropriate in a case where the start and end of the quote are both known.
Comment 35 WebKit Review Bot 2011-02-16 14:54:42 PST
Attachment 82693 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7925347
Comment 36 Carol Szabo 2011-02-16 15:05:31 PST
Created attachment 82706 [details]
Proposed Patch. Fixed typo in WebCore.gypi
Comment 37 Build Bot 2011-02-16 17:24:10 PST
Attachment 82706 [details] did not build on win:
Build output: http://queues.webkit.org/results/7917606
Comment 38 Build Bot 2011-02-16 18:24:43 PST
Attachment 82706 [details] did not build on win:
Build output: http://queues.webkit.org/results/7929298
Comment 39 Carol Szabo 2011-02-18 11:14:17 PST
Created attachment 82985 [details]
Proposed Patch. Fixed problem with Windows build.
Comment 40 Dave Hyatt 2011-02-18 11:32:44 PST
Comment on attachment 82985 [details]
Proposed Patch. Fixed problem with Windows build.

View in context: https://bugs.webkit.org/attachment.cgi?id=82985&action=review

> Source/WebCore/css/CSSStyleSelector.cpp:4729
> +    case CSSPropertyQuotes:
> +        if (isInherit) {
> +            if (m_parentStyle)
> +                m_style->setQuotes(m_parentStyle->quotes());
> +            return;
> +        }

Still don't see the isInitial case here for handling the initial value.  You should probably make a test case that tests -webkit-initial explicitly, since this isn't being covered.  Put an object with quotes:-webkit-initial inside an object with quotes:none to see the problem.  If I'm understanding how you represent the initial value correctly (with a null pointer), then all your isInitial case needs to do is m_style->setQuotes(0).

> Source/WebCore/rendering/RenderQuote.cpp:245
> +    ASSERT(m_depth != UNKNOWN_DEPTH);
> +     const QuotesData* quotes = style()->quotes();
> +     if (!quotes)
> +         quotes = defaultQuotes(this);

Some funny indenting here.
Comment 41 Carol Szabo 2011-02-18 15:41:04 PST
Created attachment 83029 [details]
Proposed Patch. Addressed DHyatt's concerns.
Comment 42 Eli Grey (:sephr) 2011-02-18 16:29:59 PST
What is your response to my concerns?
Comment 43 Carol Szabo 2011-02-21 07:56:17 PST
(In reply to comment #34)
> (In reply to comment #33)
> I'm opposed to the simpleQuotes default, and feel that the UA's preferred locale should be the default, or englishQuotes if this is not possible (which possibly should be changed to latinQuotes). {"'} isn't that appropriate in a case where the start and end of the quote are both known.

I believe the default is a minor issue, a matter of preference. The quotes you do not like are used only when the language of the page cannot be determined. I feel that simple ASCII characters, which are available on most systems, should be an option.
The way the code is designed, any implementor of a WebKit based browser can change this default easily. My goal is to provide basic support for quotes and compliance with the standard. The defaults are not specified in the standard, so the only reason why I included a relatively elaborate logic for defaults is so that my patch gets approved by reviewers. I would have considered such a logic out of scope for now.
If you want a different default wait for my patch to be landed and make your proposal. See if reviewers approve it.

As far as the englishQuotes vs. latinQuotes part of your comment I have no idea what you mean. I come from a culture based on a Latin language, very close to Italian and we use different quotes; so even with my limited knowledge of correct orthography in different cultures, I do not find latinQuotes an appropriate name for the quotes used customarily in English texts. Please explain.
Comment 44 Dave Hyatt 2011-03-01 11:53:01 PST
Comment on attachment 83029 [details]
Proposed Patch. Addressed DHyatt's concerns.

r=me

Looks great!
Comment 45 Dave Hyatt 2011-03-01 11:58:00 PST
Comment on attachment 83029 [details]
Proposed Patch. Addressed DHyatt's concerns.

Two gotchas:

(1) You need to patch RenderStyle::diff for when quotes change dynamically to supply the appropriate hint (layout hint I think).
(2) It seems like:

&& *quotes.get() == *o.quotes.get()

will crash if you are comparing against objects with quotes:none, since you'll deref 0.
Comment 46 Dave Hyatt 2011-03-01 12:02:36 PST
Comment on attachment 83029 [details]
Proposed Patch. Addressed DHyatt's concerns.

View in context: https://bugs.webkit.org/attachment.cgi?id=83029&action=review

> Source/WebCore/rendering/style/QuotesData.cpp:42
> +    if (!&other || !this)
> +        return false;

I see you did handle the null check.  This would probably seem a little less strange if you made the operator== a friend function instead, but I don't feel that strongly about it.
Comment 47 Darin Adler 2011-03-01 12:06:58 PST
Also, no need to use get() if you are using *. The * operator works on the smart pointers directly.
Comment 48 Dave Hyatt 2011-03-01 12:07:22 PST
Comment on attachment 83029 [details]
Proposed Patch. Addressed DHyatt's concerns.

My concerns have been met.  Looks good.
Comment 49 WebKit Review Bot 2011-03-01 14:09:00 PST
http://trac.webkit.org/changeset/80037 might have broken SnowLeopard Intel Release (Build)
Comment 50 Carol Szabo 2011-03-01 14:09:09 PST
Comment on attachment 83029 [details]
Proposed Patch. Addressed DHyatt's concerns.

Manually committed due to failure to apply changes to the vcproj in some ews. r80037.
Comment 51 Csaba Osztrogonác 2011-03-01 14:35:12 PST
Reopen, because it was rolled out by http://trac.webkit.org/changeset/80041
Comment 52 Csaba Osztrogonác 2011-03-01 14:51:09 PST
The patch was rolled out because of SL build break, but I'd like to ask
some minor changes for the fixed patch:

Please add "#include <wtf/text/CString.h>" line to Source/WebCore/rendering/RenderQuote.cpp to make Qt-V8 build happy.

And please update the 3 layout test result: http://build.webkit.org/results/Qt%20Linux%20Release/r80040%20%2829126%29/results.html
Comment 53 Carol Szabo 2011-03-02 11:39:02 PST
Changeset 80151. Recommitted my patch after fixing the XCode project and including expected result changes and style changes suggested by Ossy.
Comment 54 WebKit Review Bot 2011-03-02 13:24:19 PST
http://trac.webkit.org/changeset/80151 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
fast/css-generated-content/005.html
fast/css-generated-content/beforeAfter-interdocument.html
fast/css-generated-content/no-openclose-quote.html
fast/invalid/residual-style.html