Bug 28936 – Allow [Reflect] on SVG elements

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Cameron McCormack (:heycam)]

Update the JS binding generators to reference SVGNames instead of HTMLNames, if [Reflect]ing an attribute on an SVG element.

Also make [Reflect] on an attribute with a setter exception work in ObjC bindings.

[Cameron McCormack (:heycam)]

Created attachment 38970 [details]
Patch v1

Haven't added any tests since no IDL attributes on SVG interfaces currently use [Reflect].

I've visually inspected output from generating COM and V8 bindings but I haven't really tested them.

[Eric Seidel (no email)]

Comment on attachment 38970 [details]
Patch v1

In general this looks fine.

I would like to see a real example use this.  Since I think it makes sense to convert one attribute as a test and make that part of this patch.

Also, some SVG attributes, like "class" come from HTMLNames.  We probably need a fallback behavior on a per-name basis, instead of a idl-level decision.  All SVG and HTML attributes are in the null namespace, so it doesn't really matter which we're using so long as it has the name we're looking for.

[Cameron McCormack (:heycam)]

(In reply to comment #2)
> I would like to see a real example use this.  Since I think it makes sense to
> convert one attribute as a test and make that part of this patch.

OK, I'll pick one.

> Also, some SVG attributes, like "class" come from HTMLNames.  We probably need
> a fallback behavior on a per-name basis, instead of a idl-level decision.

Hmm, why is that "id" and "class" for example aren't in SVGNames, yet others that do duplicate them, like "title" and "style", are?  There are 11 duplicates, it seems.  I guess it would be good if this duplication were removed to save a bit of space.

For the moment, would you suggest just hard coding in some checks for "id", "class" etc. in the code generators to make them reference HTMLNames?

> All SVG and HTML attributes are in the null namespace, so it doesn't really
> matter which we're using so long as it has the name we're looking for.

Yep, OK.

[Cameron McCormack (:heycam)]

Created attachment 39169 [details]
Patch v2

I've updated the bindings scripts to special case certain attribute names that can be found in HTMLNames.  See the hash and sub added in CodeGenerator.pm.

I also made the format and glyphRef attributes on SVGAltGlyphElement use [Reflect] to test the new functionality.  In doing so, assigning to .format and .glyphRef now does indeed update the attribute on the element, rather than throwing immediately.

[Eric Seidel (no email)]

Comment on attachment 39169 [details]
Patch v2

This is slightly confusing because you're both changing behavior as well as converting a setter to use [reflect].  Can we convert a setter which does not change behavior?  I'd need to look at the spec again to convince myself why it's OK to set altGlyph.format, and when it is that the setter should throw.

[Cameron McCormack (:heycam)]

(In reply to comment #5)
> (From update of attachment 39169 [details])
> This is slightly confusing because you're both changing behavior as well as
> converting a setter to use [reflect].  Can we convert a setter which does not
> change behavior?

It seems that all of the null-namespace attributes on SVG elements (which don't have constants on HTMLNames) would either have a change of processing (for the better) by using [Reflect] or are on elements that aren't implemented currently.

If you'd rather no change in behaviour I could do, for example, SVGElement::id, but that'd use HTMLNames.

> I'd need to look at the spec again to convince myself why
> it's OK to set altGlyph.format, and when it is that the setter should throw.

The setter would throw in the same situation as one of the previous bugs I filed, i.e. it would throw only if the setAttribute() would throw.

[Cameron McCormack (:heycam)]

Created attachment 39406 [details]
Patch v3

This one uses [Reflect] on SVGElement::id.

[Cameron McCormack (:heycam)]

Comment on attachment 39406 [details]
Patch v3

Er, v3.

[Sam Weinig]

Comment on attachment 39406 [details]
Patch v3

Yeah for more automatic reflection! r=me

[WebKit Commit Bot]

Comment on attachment 39406 [details]
Patch v3

Rejecting patch 39406 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=39406 from bug 28936 failed to download and apply.

[Eric Seidel (no email)]

We'll need a new patch if you want the queue to land this.

1 out of 2 hunks FAILED -- saving rejects to file WebCore/bindings/scripts/CodeGenerator.pm.rej

Ideally we would have cq+'d this earlier and then it would not have been a problem. :(

[Cameron McCormack (:heycam)]

Committed: http://trac.webkit.org/changeset/49424