Bug 28936 - Allow [Reflect] on SVG elements
Summary: Allow [Reflect] on SVG elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-03 00:26 PDT by Cameron McCormack (:heycam)
Modified: 2009-10-11 02:52 PDT (History)
2 users (show)

See Also:


Attachments
Patch v1 (16.50 KB, patch)
2009-09-03 00:36 PDT, Cameron McCormack (:heycam)
eric: review-
Details | Formatted Diff | Diff
Patch v2 (26.07 KB, patch)
2009-09-07 20:51 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch v3 (21.50 KB, patch)
2009-09-10 20:30 PDT, Cameron McCormack (:heycam)
sam: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2009-09-03 00:26:51 PDT
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.
Comment 1 Cameron McCormack (:heycam) 2009-09-03 00:36:11 PDT
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.
Comment 2 Eric Seidel (no email) 2009-09-03 01:25:06 PDT
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.
Comment 3 Cameron McCormack (:heycam) 2009-09-03 17:13:09 PDT
(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.
Comment 4 Cameron McCormack (:heycam) 2009-09-07 20:51:51 PDT
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.
Comment 5 Eric Seidel (no email) 2009-09-08 09:48:14 PDT
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.
Comment 6 Cameron McCormack (:heycam) 2009-09-09 01:47:50 PDT
(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.
Comment 7 Cameron McCormack (:heycam) 2009-09-10 20:30:53 PDT
Created attachment 39406 [details]
Patch v3

This one uses [Reflect] on SVGElement::id.
Comment 8 Cameron McCormack (:heycam) 2009-09-10 20:31:50 PDT
Comment on attachment 39406 [details]
Patch v3

Er, v3.
Comment 9 Sam Weinig 2009-09-13 18:08:23 PDT
Comment on attachment 39406 [details]
Patch v3

Yeah for more automatic reflection! r=me
Comment 10 WebKit Commit Bot 2009-09-18 13:07:31 PDT
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.
Comment 11 Eric Seidel (no email) 2009-09-18 13:11:01 PDT
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. :(
Comment 12 Cameron McCormack (:heycam) 2009-10-11 02:52:17 PDT
Committed: http://trac.webkit.org/changeset/49424