Bug 60850 - SVG: Missing implementation of <altGlyphDef>, <altGlyphItem> and <glyphRef>
Summary: SVG: Missing implementation of <altGlyphDef>, <altGlyphItem> and <glyphRef>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Leo Yang
URL:
Keywords:
Depends on: 59085
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-14 19:45 PDT by Leo Yang
Modified: 2011-07-19 19:50 PDT (History)
6 users (show)

See Also:


Attachments
Draft patch (96.76 KB, patch)
2011-05-14 19:47 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Patch v1 (91.61 KB, patch)
2011-07-05 03:21 PDT, Leo Yang
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v2 (93.20 KB, patch)
2011-07-05 18:35 PDT, Leo Yang
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v3 (120.83 KB, patch)
2011-07-12 21:15 PDT, Leo Yang
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (5.78 MB, application/zip)
2011-07-12 21:41 PDT, WebKit Review Bot
no flags Details
Patch v4 (130.20 KB, patch)
2011-07-19 00:50 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Patch v5 (130.15 KB, patch)
2011-07-19 01:11 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Patch v6 (130.11 KB, patch)
2011-07-19 01:16 PDT, Leo Yang
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v7 (130.09 KB, patch)
2011-07-19 02:23 PDT, Leo Yang
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v8 (130.55 KB, patch)
2011-07-19 03:05 PDT, Leo Yang
zimmermann: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Yang 2011-05-14 19:45:23 PDT
Currently webkit is lack of <altGlyphDef>, <altGlyphItem> and <glyphRef> for SVG.
Comment 1 Leo Yang 2011-05-14 19:47:22 PDT
Created attachment 93573 [details]
Draft patch

Not for review, just let Niko and Dirk know what I'm doing.
Comment 2 Dirk Schulze 2011-05-15 01:29:54 PDT
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 <leoyang@webkit.org>

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.
Comment 3 Dirk Schulze 2011-05-15 01:31:08 PDT
I need to read the spec first to give a general conclusion about your patch.
Comment 4 Leo Yang 2011-05-15 18:38:46 PDT
(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 <leoyang@webkit.org>
> 
> 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.
Comment 5 Rob Buis 2011-05-17 14:59:00 PDT
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.
Comment 6 Leo Yang 2011-05-17 18:43:10 PDT
(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.
Comment 7 Nikolas Zimmermann 2011-06-15 04:24:12 PDT
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.
Comment 8 Leo Yang 2011-06-15 18:52:30 PDT
(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.
Comment 9 Nikolas Zimmermann 2011-06-24 23:41:48 PDT
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!
Comment 10 Leo Yang 2011-07-04 03:35:23 PDT
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?
Comment 11 Nikolas Zimmermann 2011-07-04 07:25:27 PDT
(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!
Comment 12 Leo Yang 2011-07-04 18:28:09 PDT
(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.
Comment 13 Leo Yang 2011-07-05 03:21:44 PDT
Created attachment 99690 [details]
Patch v1

Not for review.

Let buildbot check first. Not integrated with Mac build system yet.
Comment 14 WebKit Review Bot 2011-07-05 04:29:31 PDT
Comment on attachment 99690 [details]
Patch v1

Attachment 99690 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8986373
Comment 15 Leo Yang 2011-07-05 18:35:24 PDT
Created attachment 99770 [details]
Patch v2

Not for review. Attempt to fix Win build.
Comment 16 WebKit Review Bot 2011-07-05 19:37:06 PDT
Comment on attachment 99770 [details]
Patch v2

Attachment 99770 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8990352
Comment 17 WebKit Review Bot 2011-07-05 19:53:22 PDT
Comment on attachment 99770 [details]
Patch v2

Attachment 99770 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8987540
Comment 18 Dirk Schulze 2011-07-06 00:16:22 PDT
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/
Comment 19 Leo Yang 2011-07-06 00:48:51 PDT
(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.
Comment 20 Leo Yang 2011-07-12 21:15:55 PDT
Created attachment 100620 [details]
Patch v3

Not for review.
Integrating with Mac build system. Hopefully it works.
Comment 21 WebKit Review Bot 2011-07-12 21:41:26 PDT
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
Comment 22 WebKit Review Bot 2011-07-12 21:41:32 PDT
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
Comment 23 Nikolas Zimmermann 2011-07-13 01:26:09 PDT
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?
Comment 24 Leo Yang 2011-07-13 03:22:39 PDT
(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>.
Comment 25 Leo Yang 2011-07-13 03:24:31 PDT
(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>.
Comment 26 Leo Yang 2011-07-18 23:28:42 PDT
(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.
Comment 27 Nikolas Zimmermann 2011-07-19 00:46:14 PDT
(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 :-)
Comment 28 Leo Yang 2011-07-19 00:50:58 PDT
Created attachment 101283 [details]
Patch v4
Comment 29 Leo Yang 2011-07-19 01:02:21 PDT
(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! :)
Comment 30 Leo Yang 2011-07-19 01:11:40 PDT
Created attachment 101285 [details]
Patch v5

Fixed a copy/paste character in patch v4 which isn't present by bugzilla perfectly.
Comment 31 Leo Yang 2011-07-19 01:16:12 PDT
Created attachment 101286 [details]
Patch v6

OPS! another character in v5 need to be fixed. Sorry for annoying.
Comment 32 Nikolas Zimmermann 2011-07-19 01:21:39 PDT
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!
Comment 33 Nikolas Zimmermann 2011-07-19 01:25:22 PDT
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.
Comment 34 WebKit Review Bot 2011-07-19 01:48:46 PDT
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
Comment 35 WebKit Review Bot 2011-07-19 01:57:35 PDT
Comment on attachment 101286 [details]
Patch v6

Attachment 101286 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9159055
Comment 36 Leo Yang 2011-07-19 02:23:00 PDT
Created attachment 101290 [details]
Patch v7

Attempt to fix Mac build.
Comment 37 Leo Yang 2011-07-19 02:32:35 PDT
Comment on attachment 101290 [details]
Patch v7

cancel r?, need to address some comments form Niko.
Comment 38 WebKit Review Bot 2011-07-19 02:51:43 PDT
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
Comment 39 Leo Yang 2011-07-19 03:05:45 PDT
Created attachment 101295 [details]
Patch v8
Comment 40 WebKit Review Bot 2011-07-19 03:41:30 PDT
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
Comment 41 Nikolas Zimmermann 2011-07-19 03:55:23 PDT
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 :-)
Comment 42 Leo Yang 2011-07-19 19:50:33 PDT
Manually committed r91331: http://trac.webkit.org/changeset/91331