Bug 118178

Summary:

Introduce SVGGraphicsElement IDL interface

Product:

WebKit

Reporter:

Chris Dumez <cdumez>

Component:

SVG

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, benjamin, buildbot, commit-queue, darin, d-r, esprehn+autocc, fmalita, glenn, gyuyoung.kim, haraken, krit, laszlo.gombos, pdr, rakuco, rniwa, schenney, thorton, zimmermann

Priority:

Keywords:

BlinkMergeCandidate, WebExposed

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

URL:

https://svgwg.org/svg2-draft/types.html#InterfaceSVGGraphicsElement

Bug Depends on:

Bug Blocks:

118353

Attachments:

Description	Flags
Patch	haraken: review+, buildbot: commit-queue-
Attempt to fix mac-ews	buildbot: commit-queue-
Another attempt to fix mac build	buildbot: commit-queue-
Another attempt to fix mac build	none
Patch	none

Description Chris Dumez 2013-06-28 02:46:53 PDT

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

Comment 1 Chris Dumez 2013-06-28 03:51:03 PDT

Created attachment 205694 [details]
Patch

Comment 2 Kentaro Hara 2013-06-28 04:13:49 PDT

Comment on attachment 205694 [details]
Patch

Looks OK.

Comment 3 Build Bot 2013-06-28 04:22:00 PDT

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

Comment 4 Chris Dumez 2013-06-28 04:36:18 PDT

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

Comment 5 Build Bot 2013-06-28 05:08:14 PDT

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

Comment 6 Chris Dumez 2013-06-28 05:40:17 PDT

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

Comment 7 Build Bot 2013-06-28 05:59:04 PDT

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

Comment 8 Chris Dumez 2013-06-28 06:27:44 PDT

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

Comment 9 Chris Dumez 2013-06-28 06:56:54 PDT

Created attachment 205711 [details]
Patch

Comment 10 Chris Dumez 2013-06-28 06:58:54 PDT

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.

Comment 11 Kentaro Hara 2013-06-28 07:01:06 PDT

Comment on attachment 205711 [details]
Patch

LGTM

Comment 12 Dirk Schulze 2013-06-28 07:56:37 PDT

(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.

Comment 13 Chris Dumez 2013-06-28 08:11:53 PDT

(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.

Comment 14 Dirk Schulze 2013-06-28 08:18:11 PDT

(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.

Comment 15 Chris Dumez 2013-06-28 08:36:57 PDT

(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 :)

Comment 16 WebKit Commit Bot 2013-06-28 08:58:37 PDT

Comment on attachment 205711 [details]
Patch

Clearing flags on attachment: 205711

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

Comment 17 WebKit Commit Bot 2013-06-28 08:58:42 PDT

All reviewed patches have been landed.  Closing bug.