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

Chris Dumez

Reported 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

Attachments
Patch (148.12 KB, patch) 2013-06-28 03:51 PDT, Chris Dumez	haraken: review+ buildbot: commit-queue-	Details Formatted Diff Diff
Attempt to fix mac-ews (154.52 KB, patch) 2013-06-28 04:36 PDT, Chris Dumez	buildbot: commit-queue-	Details Formatted Diff Diff
Another attempt to fix mac build (155.42 KB, patch) 2013-06-28 05:40 PDT, Chris Dumez	buildbot: commit-queue-	Details Formatted Diff Diff
Another attempt to fix mac build (159.70 KB, patch) 2013-06-28 06:27 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (164.47 KB, patch) 2013-06-28 06:56 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Chris Dumez

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

Created attachment 205694 [details] Patch

Kentaro Hara

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

Comment on attachment 205694 [details] Patch Looks OK.

Build Bot

Comment 3 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

Chris Dumez

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

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

Build Bot

Comment 5 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

Chris Dumez

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

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

Build Bot

Comment 7 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

Chris Dumez

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

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

Chris Dumez

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

Created attachment 205711 [details] Patch

Chris Dumez

Comment 10 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.

Kentaro Hara

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

Comment on attachment 205711 [details] Patch LGTM

Dirk Schulze

Comment 12 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.

Chris Dumez

Comment 13 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.

Dirk Schulze

Comment 14 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.

Chris Dumez

Comment 15 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 :)

WebKit Commit Bot

Comment 16 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>

WebKit Commit Bot

Comment 17 2013-06-28 08:58:42 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.