Bug 118178 – Introduce SVGGraphicsElement IDL interface

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

[Chris Dumez]

We should merge SVGLocatable and SVGTransformable into a new SVGGraphicsElement IDL interface, which inherits SVGElement, as per the latest specification:
https://svgwg.org/svg2-draft/types.html#InterfaceSVGGraphicsElement

This is one step towards getting rid of multiple inheritance in SVG, which is not longer supported by Web IDL.

Corresponding Blink patch:
https://src.chromium.org/viewvc/blink?revision=153166&view=revision

[Chris Dumez]

Created attachment 205694 [details]
Patch

[Kentaro Hara]

Comment on attachment 205694 [details]
Patch

Looks OK.

[Build Bot]

Comment on attachment 205694 [details]
Patch

Attachment 205694 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/974332

[Chris Dumez]

Created attachment 205697 [details]
Attempt to fix mac-ews

[Build Bot]

Comment on attachment 205697 [details]
Attempt to fix mac-ews

Attachment 205697 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/966735

[Chris Dumez]

Created attachment 205704 [details]
Another attempt to fix mac build

[Build Bot]

Comment on attachment 205704 [details]
Another attempt to fix mac build

Attachment 205704 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/981698

[Chris Dumez]

Created attachment 205707 [details]
Another attempt to fix mac build

[Chris Dumez]

Created attachment 205711 [details]
Patch

[Chris Dumez]

Kentaro, could you please take another quick look?

I merged the fix from https://codereview.chromium.org/18054014/ (Moving SVGTests related code from SVGGraphics subclasses to SVGGraphics class) since this patch had not landed yet.

Mac build should be OK now too.

[Kentaro Hara]

Comment on attachment 205711 [details]
Patch

LGTM

[Dirk Schulze]

(In reply to comment #11)
> (From update of attachment 205711 [details])
> LGTM

Could you please give some more hours for reviewers familiar with this code? This is was a simple renaming and cleanup. But follow up patches need a lot more attention and I would like have some one with a bit more experience to look over it before it gets reviewed - or at least committed. Thank you.

[Chris Dumez]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 205711 [details] [details])
> > LGTM
> 
> Could you please give some more hours for reviewers familiar with this code? This is was a simple renaming and cleanup. But follow up patches need a lot more attention and I would like have some one with a bit more experience to look over it before it gets reviewed - or at least committed. Thank you.

Sure. Let me know when this can be committed or if you have comments.

[Dirk Schulze]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (From update of attachment 205711 [details] [details] [details])
> > > LGTM
> > 
> > Could you please give some more hours for reviewers familiar with this code? This is was a simple renaming and cleanup. But follow up patches need a lot more attention and I would like have some one with a bit more experience to look over it before it gets reviewed - or at least committed. Thank you.
> 
> Sure. Let me know when this can be committed or if you have comments.

Looked over it. Looks great for me. More worried when it comes to changing SVGLocatable elements to SVGGraphicsElements. This needs a lot of testing. Especially text related elements can be tricky.

[Chris Dumez]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > (From update of attachment 205711 [details] [details] [details] [details])
> > > > LGTM
> > > 
> > > Could you please give some more hours for reviewers familiar with this code? This is was a simple renaming and cleanup. But follow up patches need a lot more attention and I would like have some one with a bit more experience to look over it before it gets reviewed - or at least committed. Thank you.
> > 
> > Sure. Let me know when this can be committed or if you have comments.
> 
> Looked over it. Looks great for me. More worried when it comes to changing SVGLocatable elements to SVGGraphicsElements. This needs a lot of testing. Especially text related elements can be tricky.

Ok, thanks for checking. I'll cc you from now on and give some time. I do agree that more complex patches are coming and help is welcome :)

[WebKit Commit Bot]

Comment on attachment 205711 [details]
Patch

Clearing flags on attachment: 205711

Committed r152167: <http://trac.webkit.org/changeset/152167>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.