Bug 60850 – SVG: Missing implementation of <altGlyphDef>, <altGlyphItem> and <glyphRef>

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Leo Yang]

Currently webkit is lack of <altGlyphDef>, <altGlyphItem> and <glyphRef> for SVG.

[Leo Yang]

Created attachment 93573 [details]
Draft patch

Not for review, just let Niko and Dirk know what I'm doing.

[Dirk Schulze]

Comment on attachment 93573 [details]
Draft patch

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

You missed attributeChanged svgAttributeChanged functions to handle DOM changes (SVGDOM changes, if altGlyph is settable via SVGDOM). Also, you may need to change the binding scripts to set attributes via JavaScript. Can you upload the final patches in smaller chunks? Maybe altGlyph first, altGlyphRef later, or the other way around?

> Source/WebCore/svg/SVGAltGlyphDefElement.cpp:2
> + * Copyright (C) 2011 Leo Yang <[e-mail]>

You are not implementing it for Torch mobile but in your free time?

> Source/WebCore/svg/SVGGlyphRefElement.h:50
> +    // DOM interface
> +    const AtomicString& glyphRef() const;
> +    void setGlyphRef(const AtomicString&, ExceptionCode&);
> +    const AtomicString& format() const;
> +    void setFormat(const AtomicString&, ExceptionCode&);
> +    float x() const { return m_x; }
> +    void setX(float, ExceptionCode&);
> +    float y() const { return m_y; }
> +    void setY(float, ExceptionCode&);
> +    float dx() const { return m_dx; }
> +    void setDx(float, ExceptionCode&);
> +    float dy() const { return m_dy; }
> +    void setDy(float, ExceptionCode&);

This should be generated. You may have to modify the bindings scripts.

> Source/WebCore/svg/SVGGlyphRefElement.h:63
> +    float m_x;
> +    float m_y;
> +    float m_dx;
> +    float m_dy;

Ditto.

[Dirk Schulze]

I need to read the spec first to give a general conclusion about your patch.

[Leo Yang]

(In reply to comment #2)
> (From update of attachment 93573 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93573&action=review
> 
> You missed attributeChanged svgAttributeChanged functions to handle DOM changes (SVGDOM changes, if altGlyph is settable via SVGDOM). Also, you may need to change the binding scripts to set attributes via JavaScript. Can you upload the final patches in smaller chunks? Maybe altGlyph first, altGlyphRef later, or the other way around?
> 
> > Source/WebCore/svg/SVGAltGlyphDefElement.cpp:2
> > + * Copyright (C) 2011 Leo Yang <[e-mail]>
> 
> You are not implementing it for Torch mobile but in your free time?
> 
No. I wrote the patch in my spare time.

> > Source/WebCore/svg/SVGGlyphRefElement.h:50
> > +    // DOM interface
> > +    const AtomicString& glyphRef() const;
> > +    void setGlyphRef(const AtomicString&, ExceptionCode&);
> > +    const AtomicString& format() const;
> > +    void setFormat(const AtomicString&, ExceptionCode&);
> > +    float x() const { return m_x; }
> > +    void setX(float, ExceptionCode&);
> > +    float y() const { return m_y; }
> > +    void setY(float, ExceptionCode&);
> > +    float dx() const { return m_dx; }
> > +    void setDx(float, ExceptionCode&);
> > +    float dy() const { return m_dy; }
> > +    void setDy(float, ExceptionCode&);
> 
> This should be generated. You may have to modify the bindings scripts.
> 
> > Source/WebCore/svg/SVGGlyphRefElement.h:63
> > +    float m_x;
> > +    float m_y;
> > +    float m_dx;
> > +    float m_dy;
> 
> Ditto.

[Rob Buis]

Hi Leo,

(In reply to comment #1)
> Created an attachment (id=93573) [details]
> Draft patch
> 
> Not for review, just let Niko and Dirk know what I'm doing.

Don't forget I am here as well these days and we work for the same company :) Great that you work on this and on first view looks good to me. One small nitpick, new svg element constructors should assert the given tagName parameter, see :

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

Cheers,

Rob.

[Leo Yang]

(In reply to comment #5)
> Hi Leo,
> 
> (In reply to comment #1)
> > Created an attachment (id=93573) [details] [details]
> > Draft patch
> > 
> > Not for review, just let Niko and Dirk know what I'm doing.
> 
> Don't forget I am here as well these days and we work for the same company :) Great that you work on this and on first view looks good to me. One small nitpick, new svg element constructors should assert the given tagName parameter, see :
> 
> https://bugs.webkit.org/show_bug.cgi?id=22576
> 
> Cheers,
> 
> Rob.

Hey Rob,
Nice to meet you here :). It's a good idea to assert tagName in the svg element constructors. I'll follow it when I submit patch for review.

[Nikolas Zimmermann]

Leo, did you already evaluate how to integrate your patch with the SVG Fonts rewrite? I think it shouldn't bee too hard, once the patch from bug 59085 lands.

[Leo Yang]

(In reply to comment #7)
> Leo, did you already evaluate how to integrate your patch with the SVG Fonts rewrite? I think it shouldn't bee too hard, once the patch from bug 59085 lands.

Niko, I looked at patch form 59085 which is a large one in my some pieces of upstream working time. I think I need to look at the patch again (or maybe third time:p) to think about how to integrate my patch.

[Nikolas Zimmermann]

Leo, the new patch at bug 59085 landed. Will you maybe find some time to update your patch? It would be great to have text-altglyph-01-b.svg fully fixed!

[Leo Yang]

It seems that after bug 59085 gets resolved, SVG_FONTS has been disabled for Qt port (Qt version < 4.8) because the patch of 59085 depends on font fast path. Maybe I need to switch to other port or update Qt to 4.8 to complete this feature.

But Niko I have a question: Does it mean SVG_FONTS feature depend on font fast path permanently?

[Nikolas Zimmermann]

(In reply to comment #10)
> It seems that after bug 59085 gets resolved, SVG_FONTS has been disabled for Qt port (Qt version < 4.8) because the patch of 59085 depends on font fast path. Maybe I need to switch to other port or update Qt to 4.8 to complete this feature.
> 
> But Niko I have a question: Does it mean SVG_FONTS feature depend on font fast path permanently?

Definitely, we want to share as much as code possible with the existing methods that measure/draw text for native fonts -- that requires using GlyphBuffer & WidthIterator, which are only available if font fast path is enabled (that's for all ports but Qt <= 4.8 - Andreas told me QRawFont support is being worked on at Nokia atm, and it will be enabled again soon, once some fixes in Qt are applied). I see no problem depending on the font fast path, it's definitely the way to go.

I looked again through your patch, and it think it's quite easy to integrate it within the new concept!

[Leo Yang]

(In reply to comment #11)
> (In reply to comment #10)
> > It seems that after bug 59085 gets resolved, SVG_FONTS has been disabled for Qt port (Qt version < 4.8) because the patch of 59085 depends on font fast path. Maybe I need to switch to other port or update Qt to 4.8 to complete this feature.
> > 
> > But Niko I have a question: Does it mean SVG_FONTS feature depend on font fast path permanently?
> 
> Definitely, we want to share as much as code possible with the existing methods that measure/draw text for native fonts -- that requires using GlyphBuffer & WidthIterator, which are only available if font fast path is enabled (that's for all ports but Qt <= 4.8 - Andreas told me QRawFont support is being worked on at Nokia atm, and it will be enabled again soon, once some fixes in Qt are applied). I see no problem depending on the font fast path, it's definitely the way to go.
> 
> I looked again through your patch, and it think it's quite easy to integrate it within the new concept!

Yes, I looked at the current code base, it's easy to integrate my patch. I'm going to switch to non-Qt port to integrate it.

[Leo Yang]

Created attachment 99690 [details]
Patch v1

Not for review.

Let buildbot check first. Not integrated with Mac build system yet.

[WebKit Review Bot]

Comment on attachment 99690 [details]
Patch v1

Attachment 99690 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8986373

[Leo Yang]

Created attachment 99770 [details]
Patch v2

Not for review. Attempt to fix Win build.

[WebKit Review Bot]

Comment on attachment 99770 [details]
Patch v2

Attachment 99770 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8990352

[WebKit Review Bot]

Comment on attachment 99770 [details]
Patch v2

Attachment 99770 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8987540

[Dirk Schulze]

Hi Leo. Great that you work on that. Can you check the new SVG test suite as well? IIRC new altGlyph tests were added. http://dev.w3.org/SVG/profiles/1.1F2/test/svg/

[Leo Yang]

(In reply to comment #18)
> Hi Leo. Great that you work on that. Can you check the new SVG test suite as well? IIRC new altGlyph tests were added. http://dev.w3.org/SVG/profiles/1.1F2/test/svg/

OK, thanks.

[Leo Yang]

Created attachment 100620 [details]
Patch v3

Not for review.
Integrating with Mac build system. Hopefully it works.

[WebKit Review Bot]

Comment on attachment 100620 [details]
Patch v3

Attachment 100620 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9050008

New failing tests:
fast/dom/prototype-inheritance.html

[WebKit Review Bot]

Created attachment 100624 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Comment on attachment 100620 [details]
Patch v3

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

Next round of comments for Leo, I think it's really ready to apply the r? flag :-)

> Source/WebCore/svg/SVGAltGlyphDefElement.cpp:60
> +    for (Node* child = firstChild(); child; child = child->nextSibling()) {
> +        if (!foundFirstAltGlyphItem && child->hasTagName(SVGNames::glyphRefTag)) {
> +            fountFirstGlyphRef = true;
> +            String referredGlyphName;
> +            if (static_cast<SVGGlyphRefElement*>(child)->hasValidGlyphElement(referredGlyphName))
> +                glyphNames.append(referredGlyphName);
> +            else
> +                return false;
> +        } else if (!fountFirstGlyphRef && child->hasTagName(SVGNames::altGlyphItemTag)) {
> +            foundFirstAltGlyphItem = true;
> +            Vector<String> referredGlyphNames;
> +            if (static_cast<SVGAltGlyphItemElement*>(child)->hasValidGlyphElements(glyphNames) && !glyphNames.isEmpty())
> +                return true;
> +        }
> +    }

I find this quite confusing.
http://www.w3.org/TR/SVG/text.html#AltGlyphDefElement explains the content model for <altGlyphDef>:

Content model:
Either:
one or more ‘glyphRef’ elements, or
one or more ‘altGlyphItem’ elements.

We have two possible solutions here:
1. The first child element of altGlyphDef determines the "content mode":
- if it's a <glyphRef> you should skip all following children, unless they are glyphRef elements.
- if it's a <altGlyphItem> you should skip all following children, unless they are altGlyphItem elements.

2. If we see a glyphRef element and an altGlyphItem element, consider it an error, and early return false.

The spec is strict about the content model so I think we should go with 2) and write tests for it:
<altGlyphDef><glyphRef..> <glyphRef...> </altGlyphDef> (should work)
<altGlyphDef><altGlyphItem..> <altGlyphItem...> </altGlyphDef> (should work)
<altGlyphDef><altGlyphItem..> <glyphRef...> </altGlyphDef> (should fail, aka. ignore the altGlyphDef completely)
<altGlyphDef><glyphRef..> <altGlyphItem...> </altGlyphDef> (should fail, aka. ignore the altGlyphDef completely)

I think this is a reasonable interpretation of the specs content model.
It would help if you could include spec quotations here in our usual style:

// Spec: http://link-to-relevant-paragraph
// The foo element should do foo.
your-code;

> Source/WebCore/svg/SVGAltGlyphItemElement.cpp:43
> +    for (Node* child = firstChild(); child; child = child->nextSibling()) {

// Spec: quotes would help here a lot:

In the more complex case, an ‘altGlyphDef’ contains one or more ‘altGlyphItem’ elements. Each ‘altGlyphItem’ represents a candidate set of substitute glyphs. Each ‘altGlyphItem’ contains one or more ‘glyphRef’ elements. Each ‘glyphRef’ element references a single glyph within a particular font. The first ‘altGlyphItem’ in which all referenced glyphs are available is chosen. The glyphs referenced from this ‘altGlyphItem’ are rendered instead of the character(s) that are inside of the referencing ‘altGlyph’ element. If none of the ‘altGlyphItem’ elements result in a successful match (i.e., none of the ‘altGlyphItem’ elements has all of its referenced glyphs available), then the character(s) that are inside of the ‘altGlyph’ element are rendered as if there were not an ‘altGlyph’ element surrounding those characters.

I'd split up this long paragraph into individual comments around your if branches.

> Source/WebCore/svg/SVGAltGlyphItemElement.cpp:44
> +        if (child->hasTagName(SVGNames::glyphRefTag)) {

You can early continue here if the child is not a glyphRef.

> Source/WebCore/svg/SVGAltGlyphItemElement.cpp:49
> +                glyphNames = Vector<String>();

glyphNames.clear() ?

> Source/WebCore/svg/SVGFontData.cpp:158
> +                    altGlyphNames = Vector<String>();

altGlyphNames.clear();

> Source/WebCore/svg/SVGFontData.cpp:165
> +    if (!altGlyphNames.isEmpty()) {
> +        size_t altGlyphNamesSize = altGlyphNames.size();

Micro-optimization: Maybe reverse these lines, and check if (altGlyphNamesSize > 0) to avoid asking twice for the size (explicit, and implicit through isEmpty).

> Source/WebCore/svg/SVGGlyphRefElement.cpp:33
> +
> +

One newline too much.

> Source/WebCore/svg/SVGGlyphRefElement.cpp:96
> +void SVGGlyphRefElement::setGlyphRef(const AtomicString&, ExceptionCode& ec)
> +{
> +    ec = NO_MODIFICATION_ALLOWED_ERR;
> +}
> +

The spec is written a bit misleading here, you do not need to raise exceptions here, it's wrong.
  attribute DOMString glyphRef setraises(DOMException);

Spec says:
Exceptions on setting
DOMException, code NO_MODIFICATION_ALLOWED_ERR
Raised on an attempt to change the value of a read only attribute.

Definition of read only attribute:
http://www.w3.org/TR/SVG/svgdom.html#ReadOnlyNodes
Some operations and attributes in the SVG DOM are defined to raise an exception when an attempt is made to modify a node in the DOM that is read only. Such read only nodes are not related to attributes declared in IDL with the readonly keyword. Rather, they are nodes that cannot be modified by virtue of being defined as readonly nodes by DOM Level 2 Core ([DOM2], Glossary appendix). Specifically, Entity and EntityReference nodes and their descendants are read only ([DOM2], section 1.3).

That means setGlyphRef/setX/setY/etc.. don't raise an exception as the elements are never readonly but should work as expected.
Luckily for glyphRef you do not need to implement glyphRef/setGlyphRef on your own but just mark the the attribute in the IDL with [Reflect].
        attribute [Reflect] DOMString glyphRef setter raises(DOMException);

> Source/WebCore/svg/SVGGlyphRefElement.cpp:106
> +const AtomicString& SVGGlyphRefElement::format() const
> +{
> +    return getAttribute(SVGNames::formatAttr);
> +}
> +
> +void SVGGlyphRefElement::setFormat(const AtomicString&, ExceptionCode& ec)
> +{
> +    ec = NO_MODIFICATION_ALLOWED_ERR;
> +}
> +

Ditto, this can just be removed and replaced by [Reflect] usage in the idl.

> Source/WebCore/svg/SVGGlyphRefElement.cpp:109
> +    ec = NO_MODIFICATION_ALLOWED_ERR;

[Reflect] doesn't support floats (yet, not sure it will). So you have to implement setx/setY/setDx/setDy on your own.
I guess sth like m_x = x; does the job.

The question is what happens if someone dynamically modifies the 'x' attribute or format or glyphRef of an <altGlyphRef> element - how do you react on that?
Currently it seems you're not reacting to any dynamic change, only respect the states of these attributes while parsing?
We need a way to trigger layout from here, right?

[Leo Yang]

(In reply to comment #23)
> (From update of attachment 100620 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100620&action=review
> 
> Next round of comments for Leo, I think it's really ready to apply the r? flag :-)
> 
> > Source/WebCore/svg/SVGAltGlyphDefElement.cpp:60
> > +    for (Node* child = firstChild(); child; child = child->nextSibling()) {
> > +        if (!foundFirstAltGlyphItem && child->hasTagName(SVGNames::glyphRefTag)) {
> > +            fountFirstGlyphRef = true;
> > +            String referredGlyphName;
> > +            if (static_cast<SVGGlyphRefElement*>(child)->hasValidGlyphElement(referredGlyphName))
> > +                glyphNames.append(referredGlyphName);
> > +            else
> > +                return false;
> > +        } else if (!fountFirstGlyphRef && child->hasTagName(SVGNames::altGlyphItemTag)) {
> > +            foundFirstAltGlyphItem = true;
> > +            Vector<String> referredGlyphNames;
> > +            if (static_cast<SVGAltGlyphItemElement*>(child)->hasValidGlyphElements(glyphNames) && !glyphNames.isEmpty())
> > +                return true;
> > +        }
> > +    }
> 
> I find this quite confusing.
> http://www.w3.org/TR/SVG/text.html#AltGlyphDefElement explains the content model for <altGlyphDef>:
> 
> Content model:
> Either:
> one or more ‘glyphRef’ elements, or
> one or more ‘altGlyphItem’ elements.
> 
> We have two possible solutions here:
> 1. The first child element of altGlyphDef determines the "content mode":
> - if it's a <glyphRef> you should skip all following children, unless they are glyphRef elements.
> - if it's a <altGlyphItem> you should skip all following children, unless they are altGlyphItem elements.
> 
> 2. If we see a glyphRef element and an altGlyphItem element, consider it an error, and early return false.
> 
> The spec is strict about the content model so I think we should go with 2) and write tests for it:
> <altGlyphDef><glyphRef..> <glyphRef...> </altGlyphDef> (should work)
> <altGlyphDef><altGlyphItem..> <altGlyphItem...> </altGlyphDef> (should work)
> <altGlyphDef><altGlyphItem..> <glyphRef...> </altGlyphDef> (should fail, aka. ignore the altGlyphDef completely)
> <altGlyphDef><glyphRef..> <altGlyphItem...> </altGlyphDef> (should fail, aka. ignore the altGlyphDef completely)
> 
> I think this is a reasonable interpretation of the specs content model.
Hmm... The spec doesn't say which content model we should choose. I thought we should apply 2. But after I looked at Opera 11, which applies 1, I chose 2. BTW: firefox 5 and IE 10 don't support <altGlyph>.

[Leo Yang]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 100620 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=100620&action=review
> > 
> > Next round of comments for Leo, I think it's really ready to apply the r? flag :-)
> > 
> > > Source/WebCore/svg/SVGAltGlyphDefElement.cpp:60
> > > +    for (Node* child = firstChild(); child; child = child->nextSibling()) {
> > > +        if (!foundFirstAltGlyphItem && child->hasTagName(SVGNames::glyphRefTag)) {
> > > +            fountFirstGlyphRef = true;
> > > +            String referredGlyphName;
> > > +            if (static_cast<SVGGlyphRefElement*>(child)->hasValidGlyphElement(referredGlyphName))
> > > +                glyphNames.append(referredGlyphName);
> > > +            else
> > > +                return false;
> > > +        } else if (!fountFirstGlyphRef && child->hasTagName(SVGNames::altGlyphItemTag)) {
> > > +            foundFirstAltGlyphItem = true;
> > > +            Vector<String> referredGlyphNames;
> > > +            if (static_cast<SVGAltGlyphItemElement*>(child)->hasValidGlyphElements(glyphNames) && !glyphNames.isEmpty())
> > > +                return true;
> > > +        }
> > > +    }
> > 
> > I find this quite confusing.
> > http://www.w3.org/TR/SVG/text.html#AltGlyphDefElement explains the content model for <altGlyphDef>:
> > 
> > Content model:
> > Either:
> > one or more ‘glyphRef’ elements, or
> > one or more ‘altGlyphItem’ elements.
> > 
> > We have two possible solutions here:
> > 1. The first child element of altGlyphDef determines the "content mode":
> > - if it's a <glyphRef> you should skip all following children, unless they are glyphRef elements.
> > - if it's a <altGlyphItem> you should skip all following children, unless they are altGlyphItem elements.
> > 
> > 2. If we see a glyphRef element and an altGlyphItem element, consider it an error, and early return false.
> > 
> > The spec is strict about the content model so I think we should go with 2) and write tests for it:
> > <altGlyphDef><glyphRef..> <glyphRef...> </altGlyphDef> (should work)
> > <altGlyphDef><altGlyphItem..> <altGlyphItem...> </altGlyphDef> (should work)
> > <altGlyphDef><altGlyphItem..> <glyphRef...> </altGlyphDef> (should fail, aka. ignore the altGlyphDef completely)
> > <altGlyphDef><glyphRef..> <altGlyphItem...> </altGlyphDef> (should fail, aka. ignore the altGlyphDef completely)
> > 
> > I think this is a reasonable interpretation of the specs content model.
> Hmm... The spec doesn't say which content model we should choose. I thought we should apply 2. But after I looked at Opera 11, which applies 1, I chose 2.
sorry, I chose 1.
 BTW: firefox 5 and IE 10 don't support <altGlyph>.

[Leo Yang]

(In reply to comment #23)
> (From update of attachment 100620 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100620&action=review
> 
> Next round of comments for Leo, I think it's really ready to apply the r? flag :-)
> 
> > Source/WebCore/svg/SVGAltGlyphDefElement.cpp:60
> > +    for (Node* child = firstChild(); child; child = child->nextSibling()) {
> > +        if (!foundFirstAltGlyphItem && child->hasTagName(SVGNames::glyphRefTag)) {
> > +            fountFirstGlyphRef = true;
> > +            String referredGlyphName;
> > +            if (static_cast<SVGGlyphRefElement*>(child)->hasValidGlyphElement(referredGlyphName))
> > +                glyphNames.append(referredGlyphName);
> > +            else
> > +                return false;
> > +        } else if (!fountFirstGlyphRef && child->hasTagName(SVGNames::altGlyphItemTag)) {
> > +            foundFirstAltGlyphItem = true;
> > +            Vector<String> referredGlyphNames;
> > +            if (static_cast<SVGAltGlyphItemElement*>(child)->hasValidGlyphElements(glyphNames) && !glyphNames.isEmpty())
> > +                return true;
> > +        }
> > +    }
> 
> I find this quite confusing.
> http://www.w3.org/TR/SVG/text.html#AltGlyphDefElement explains the content model for <altGlyphDef>:
> 
> Content model:
> Either:
> one or more ‘glyphRef’ elements, or
> one or more ‘altGlyphItem’ elements.
> 
> We have two possible solutions here:
> 1. The first child element of altGlyphDef determines the "content mode":
> - if it's a <glyphRef> you should skip all following children, unless they are glyphRef elements.
> - if it's a <altGlyphItem> you should skip all following children, unless they are altGlyphItem elements.
> 
> 2. If we see a glyphRef element and an altGlyphItem element, consider it an error, and early return false.
> 
> The spec is strict about the content model so I think we should go with 2) and write tests for it:
> <altGlyphDef><glyphRef..> <glyphRef...> </altGlyphDef> (should work)
> <altGlyphDef><altGlyphItem..> <altGlyphItem...> </altGlyphDef> (should work)
> <altGlyphDef><altGlyphItem..> <glyphRef...> </altGlyphDef> (should fail, aka. ignore the altGlyphDef completely)
> <altGlyphDef><glyphRef..> <altGlyphItem...> </altGlyphDef> (should fail, aka. ignore the altGlyphDef completely)
> 
> I think this is a reasonable interpretation of the specs content model.
See my previous 2 comments.

> It would help if you could include spec quotations here in our usual style:
> 
> // Spec: http://link-to-relevant-paragraph
> // The foo element should do foo.
> your-code;
> 
Good idea!

> > Source/WebCore/svg/SVGAltGlyphItemElement.cpp:43
> > +    for (Node* child = firstChild(); child; child = child->nextSibling()) {
> 
> // Spec: quotes would help here a lot:
>
Ditto.
 
> In the more complex case, an ‘altGlyphDef’ contains one or more ‘altGlyphItem’ elements. Each ‘altGlyphItem’ represents a candidate set of substitute glyphs. Each ‘altGlyphItem’ contains one or more ‘glyphRef’ elements. Each ‘glyphRef’ element references a single glyph within a particular font. The first ‘altGlyphItem’ in which all referenced glyphs are available is chosen. The glyphs referenced from this ‘altGlyphItem’ are rendered instead of the character(s) that are inside of the referencing ‘altGlyph’ element. If none of the ‘altGlyphItem’ elements result in a successful match (i.e., none of the ‘altGlyphItem’ elements has all of its referenced glyphs available), then the character(s) that are inside of the ‘altGlyph’ element are rendered as if there were not an ‘altGlyph’ element surrounding those characters.
> 
> I'd split up this long paragraph into individual comments around your if branches.
Ditto.

> 
> > Source/WebCore/svg/SVGAltGlyphItemElement.cpp:44
> > +        if (child->hasTagName(SVGNames::glyphRefTag)) {
> 
> You can early continue here if the child is not a glyphRef.
> 
> > Source/WebCore/svg/SVGAltGlyphItemElement.cpp:49
> > +                glyphNames = Vector<String>();
> 
> glyphNames.clear() ?
> 
Yes.

> > Source/WebCore/svg/SVGFontData.cpp:158
> > +                    altGlyphNames = Vector<String>();
> 
> altGlyphNames.clear();
Yes.

> 
> > Source/WebCore/svg/SVGFontData.cpp:165
> > +    if (!altGlyphNames.isEmpty()) {
> > +        size_t altGlyphNamesSize = altGlyphNames.size();
> 
> Micro-optimization: Maybe reverse these lines, and check if (altGlyphNamesSize > 0) to avoid asking twice for the size (explicit, and implicit through isEmpty).
>
Good.
 
> > Source/WebCore/svg/SVGGlyphRefElement.cpp:33
> > +
> > +
> 
> One newline too much.
> 
Will remove one.

> > Source/WebCore/svg/SVGGlyphRefElement.cpp:96
> > +void SVGGlyphRefElement::setGlyphRef(const AtomicString&, ExceptionCode& ec)
> > +{
> > +    ec = NO_MODIFICATION_ALLOWED_ERR;
> > +}
> > +
> 
> The spec is written a bit misleading here, you do not need to raise exceptions here, it's wrong.
>   attribute DOMString glyphRef setraises(DOMException);
> 
> Spec says:
> Exceptions on setting
> DOMException, code NO_MODIFICATION_ALLOWED_ERR
> Raised on an attempt to change the value of a read only attribute.
> 
> Definition of read only attribute:
> http://www.w3.org/TR/SVG/svgdom.html#ReadOnlyNodes
> Some operations and attributes in the SVG DOM are defined to raise an exception when an attempt is made to modify a node in the DOM that is read only. Such read only nodes are not related to attributes declared in IDL with the readonly keyword. Rather, they are nodes that cannot be modified by virtue of being defined as readonly nodes by DOM Level 2 Core ([DOM2], Glossary appendix). Specifically, Entity and EntityReference nodes and their descendants are read only ([DOM2], section 1.3).
> 
> That means setGlyphRef/setX/setY/etc.. don't raise an exception as the elements are never readonly but should work as expected.
> Luckily for glyphRef you do not need to implement glyphRef/setGlyphRef on your own but just mark the the attribute in the IDL with [Reflect].
>         attribute [Reflect] DOMString glyphRef setter raises(DOMException);
> 
V8 Code generator seems convert attribute to lower case when using [Reflect]. So it will generate code like SVGNames::glyphrefAttr which is not defined. If I change the case of the name, seems breaking naming convention in svg.

> > Source/WebCore/svg/SVGGlyphRefElement.cpp:106
> > +const AtomicString& SVGGlyphRefElement::format() const
> > +{
> > +    return getAttribute(SVGNames::formatAttr);
> > +}
> > +
> > +void SVGGlyphRefElement::setFormat(const AtomicString&, ExceptionCode& ec)
> > +{
> > +    ec = NO_MODIFICATION_ALLOWED_ERR;
> > +}
> > +
> 
> Ditto, this can just be removed and replaced by [Reflect] usage in the idl.
>
For formatAttr, I can use [Reflect]
 
> > Source/WebCore/svg/SVGGlyphRefElement.cpp:109
> > +    ec = NO_MODIFICATION_ALLOWED_ERR;
> 
> [Reflect] doesn't support floats (yet, not sure it will). So you have to implement setx/setY/setDx/setDy on your own.
> I guess sth like m_x = x; does the job.
>
You are right.
 
> The question is what happens if someone dynamically modifies the 'x' attribute or format or glyphRef of an <altGlyphRef> element - how do you react on that?
> Currently it seems you're not reacting to any dynamic change, only respect the states of these attributes while parsing?
> We need a way to trigger layout from here, right?
Right. But I have no idea about how to honor x and y attribute.glyphRef is not honored currently.

[Nikolas Zimmermann]

(In reply to comment #26)
> > I think this is a reasonable interpretation of the specs content model.
> See my previous 2 comments.
Ok, let's go with Opera and follow 1. Can you still add some comments to the code that explains this - would make our lives easier looking at it again in future.

> > Luckily for glyphRef you do not need to implement glyphRef/setGlyphRef on your own but just mark the the attribute in the IDL with [Reflect].
> >         attribute [Reflect] DOMString glyphRef setter raises(DOMException);
> > 
> V8 Code generator seems convert attribute to lower case when using [Reflect]. So it will generate code like SVGNames::glyphrefAttr which is not defined. If I change the case of the name, seems breaking naming convention in svg.
JSC is not affected? it should be an easy to fix the CodeGenerators - Reflect hasn't been tested yet for SVG attributes that require special naming sheme (glyphHref). Code for this is probably already existant either in CodeGenerator.pm directly or in dom/make_names.pl.

 
> > The question is what happens if someone dynamically modifies the 'x' attribute or format or glyphRef of an <altGlyphRef> element - how do you react on that?
> > Currently it seems you're not reacting to any dynamic change, only respect the states of these attributes while parsing?
> > We need a way to trigger layout from here, right?
> Right. But I have no idea about how to honor x and y attribute.glyphRef is not honored currently.

Okay, if x/y/glyphRef are not supported, you should add a bug report that these are ignored currently, and link to this bug report with a // FIXME: in the relevant methods.
For setX/setY/... you also want to add a FIXME that dynamic invalidation doesn't work right now.

I still think you're patch is great, even if it doesn't support everything so far :-)

[Leo Yang]

Created attachment 101283 [details]
Patch v4

[Leo Yang]

(In reply to comment #27)
> (In reply to comment #26)
> > > I think this is a reasonable interpretation of the specs content model.
> > See my previous 2 comments.
> Ok, let's go with Opera and follow 1. Can you still add some comments to the code that explains this - would make our lives easier looking at it again in future.
> 
> > > Luckily for glyphRef you do not need to implement glyphRef/setGlyphRef on your own but just mark the the attribute in the IDL with [Reflect].
> > >         attribute [Reflect] DOMString glyphRef setter raises(DOMException);
> > > 
> > V8 Code generator seems convert attribute to lower case when using [Reflect]. So it will generate code like SVGNames::glyphrefAttr which is not defined. If I change the case of the name, seems breaking naming convention in svg.
> JSC is not affected? it should be an easy to fix the CodeGenerators - Reflect hasn't been tested yet for SVG attributes that require special naming sheme (glyphHref). Code for this is probably already existant either in CodeGenerator.pm directly or in dom/make_names.pl.
I don't know if JSC is affected but will try it. I guess the generator was initially written for html in which the attribute name is case insensitive.

> 
> 
> > > The question is what happens if someone dynamically modifies the 'x' attribute or format or glyphRef of an <altGlyphRef> element - how do you react on that?
> > > Currently it seems you're not reacting to any dynamic change, only respect the states of these attributes while parsing?
> > > We need a way to trigger layout from here, right?
> > Right. But I have no idea about how to honor x and y attribute.glyphRef is not honored currently.
> 
> Okay, if x/y/glyphRef are not supported, you should add a bug report that these are ignored currently, and link to this bug report with a // FIXME: in the relevant methods.
> For setX/setY/... you also want to add a FIXME that dynamic invalidation doesn't work right now.
I added FIXME in patch v4. Will file bug to track the implementation.

> 
> I still think you're patch is great, even if it doesn't support everything so far :-)
Thanks! :)

[Leo Yang]

Created attachment 101285 [details]
Patch v5

Fixed a copy/paste character in patch v4 which isn't present by bugzilla perfectly.

[Leo Yang]

Created attachment 101286 [details]
Patch v6

OPS! another character in v5 need to be fixed. Sorry for annoying.

[Nikolas Zimmermann]

Comment on attachment 101285 [details]
Patch v5

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

Looks great, r=me! Please check the comments before landing. I'll switch state to r+ once cr-linux EWS results are available (as it runs all pixel tests), if there are no regressions.

> LayoutTests/ChangeLog:11
> +

You should mention here that you fixed this test!
Once this landed you want to update "https://spreadsheets.google.com/spreadsheet/ccc?key=0AtEuXGcF4tpXdF82VjVMNV83cUI5VkVOQjVjOG9NcVE&hl=en_US#gid=0" as well and turn this test to green. This document tracks our SVG 1.1 2nd Edition results.

> Source/WebCore/ChangeLog:14
> +        NOTE: x, y, dx, dy, format and glyphRef attributes on <glyphRef> are not
> +        honored yet, so neither are the reaction of dynamic change of them. They
> +        will be honored in separated patches.

Good.

> Source/WebCore/ChangeLog:16
> +        Tests: svg/W3C-SVG-1.1/text-altglyph-01-b.svg

You should mention you've fixed this test.

> Source/WebCore/svg/SVGAltGlyphDefElement.cpp:85
> +    // "i1" if they are valid and "g1" and "g2" are skipped.

Excellent comment!

> Source/WebCore/svg/SVGGlyphRefElement.cpp:59
> +{
> +    // FIXME: We only support xlink:href so far.

Please add a link here as well to the new bug, once you've filed it. (you can do this when landing, this doesn't block r+)

> Source/WebCore/svg/SVGGlyphRefElement.cpp:71
> +    if (attr->name() == SVGNames::xAttr)

At some point we want to handle error reporting here, CC'ing Tim Horton on that.
Doesn't block the initial landing either, but a comment should be added.

> Source/WebCore/svg/SVGGlyphRefElement.idl:25
> +        attribute DOMString glyphRef

Would be great if you could tweak the generators to make Reflect work here as well!

[Nikolas Zimmermann]

CC'ing Timothy on the error reporting. I'd like to hear his opinion, whether we should directly add the new error reporting facilities within all new files in SVG* or whether we want to transition them all in a later stage at-once.

[WebKit Review Bot]

Comment on attachment 101286 [details]
Patch v6

Attachment 101286 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9153081

New failing tests:
fast/dom/prototype-inheritance.html

[WebKit Review Bot]

Comment on attachment 101286 [details]
Patch v6

Attachment 101286 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9159055

[Leo Yang]

Created attachment 101290 [details]
Patch v7

Attempt to fix Mac build.

[Leo Yang]

Comment on attachment 101290 [details]
Patch v7

cancel r?, need to address some comments form Niko.

[WebKit Review Bot]

Comment on attachment 101290 [details]
Patch v7

Attachment 101290 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9153104

New failing tests:
fast/dom/prototype-inheritance.html

[Leo Yang]

Created attachment 101295 [details]
Patch v8

[WebKit Review Bot]

Comment on attachment 101295 [details]
Patch v8

Attachment 101295 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9170005

New failing tests:
fast/dom/prototype-inheritance.html

[Nikolas Zimmermann]

Comment on attachment 101295 [details]
Patch v8

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

Looks great, r=me. I've told Leo to watch the bots after landing to rebaseline all platforms that have unique prototype-inheritance.html results.

> Source/WebCore/svg/SVGGlyphRefElement.idl:25
> +        attribute DOMString glyphRef

Please add a FIXME here that this is supposed to use Reflect, but has a bug. Also please file a bug on this, that it's impossible to use Reflect on such attributes at present.
Even better would be a fix of the generators of course :-)

[Leo Yang]

Manually committed r91331: http://trac.webkit.org/changeset/91331