Bug 43254 - Expand SVG Attribute Macros
Summary: Expand SVG Attribute Macros
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-30 08:56 PDT by Fady Samuel
Modified: 2010-07-30 14:09 PDT (History)
4 users (show)

See Also:


Attachments
SVG Attribute macros expanded (17.92 KB, patch)
2010-07-30 09:00 PDT, Fady Samuel
dglazkov: review-
dglazkov: commit-queue-
Details | Formatted Diff | Diff
SVG Attribute Macros Expanded (24.81 KB, patch)
2010-07-30 09:41 PDT, Fady Samuel
dglazkov: review-
dglazkov: commit-queue-
Details | Formatted Diff | Diff
SVG Attribute Macros Expanded (18.66 KB, patch)
2010-07-30 10:30 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2010-07-30 08:56:05 PDT
SVG Attributes are currently macros. This often makes debugging SVG a nightmare as we can't trace through macros.
Comment 1 Fady Samuel 2010-07-30 09:00:55 PDT
Created attachment 63065 [details]
SVG Attribute macros expanded
Comment 2 Dimitri Glazkov (Google) 2010-07-30 09:35:49 PDT
Comment on attachment 63065 [details]
SVG Attribute macros expanded

It's missing a ChangeLog. Otherwise, looks good.
Comment 3 Fady Samuel 2010-07-30 09:41:24 PDT
Created attachment 63067 [details]
SVG Attribute Macros Expanded
Comment 4 Dimitri Glazkov (Google) 2010-07-30 09:57:33 PDT
Comment on attachment 63067 [details]
SVG Attribute Macros Expanded

I'll pick on you, but only because you're new here! :P

WebCore/ChangeLog:10
 +          No new tests.
Why no new test?

Usually a simple explanation is provided, like:

"No change in behavior, so no new tests."

WebCore/ChangeLog:130
 +          * rendering/style/SVGRenderStyleDefs.h:
This long list of changes is not very useful. Typically, the author of the ChangeLog takes this produced template and makes something useful out of it, like:

rendering/style/SVGRenderStyle.h: Expanded and removed references to <list of macros> macros.
rendering/style/SVGRenderStyleDefs.h: Removed definitons for expanded macros.

Then you can zap the rest of the list in the template.
Comment 5 Fady Samuel 2010-07-30 10:30:37 PDT
Created attachment 63076 [details]
SVG Attribute Macros Expanded

Made the change log nicer. Thanks dglazkov for your patience with my n00bness at writing useful changelogs.:)
Comment 6 Dimitri Glazkov (Google) 2010-07-30 10:42:14 PDT
Comment on attachment 63076 [details]
SVG Attribute Macros Expanded

ok.
Comment 7 Nikolas Zimmermann 2010-07-30 11:26:21 PDT
Thanks Fady, good start at cleaning up SVGRenderStyle & friends.
Comment 8 WebKit Commit Bot 2010-07-30 12:06:36 PDT
Comment on attachment 63076 [details]
SVG Attribute Macros Expanded

Clearing flags on attachment: 63076

Committed r64367: <http://trac.webkit.org/changeset/64367>
Comment 9 WebKit Commit Bot 2010-07-30 12:06:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Eric Seidel (no email) 2010-07-30 14:03:31 PDT
Comment on attachment 63076 [details]
SVG Attribute Macros Expanded

WebCore/ChangeLog:1
 +  2010-07-30  fsamuel@chromium.org  <fsamuel@chromium.org>
This should be your Full Name <email>, no email twice. :)

WebCore/rendering/style/SVGRenderStyle.h: 
 +      SVG_RS_DEFINE_ATTRIBUTE(EAlignmentBaseline, AlignmentBaseline, alignmentBaseline, AB_AUTO)
I'm not sure why expanding these is better... but OK.
Comment 11 Fady Samuel 2010-07-30 14:09:14 PDT
(In reply to comment #10)
> (From update of attachment 63076 [details])
> WebCore/ChangeLog:1
>  +  2010-07-30  fsamuel@chromium.org  <fsamuel@chromium.org>
> This should be your Full Name <email>, no email twice. :)
> 
> WebCore/rendering/style/SVGRenderStyle.h: 
>  +      SVG_RS_DEFINE_ATTRIBUTE(EAlignmentBaseline, AlignmentBaseline, alignmentBaseline, AB_AUTO)
> I'm not sure why expanding these is better... but OK.

Made debugging a snap now. 

Nikolas Zimmermann suggested I make this change in:

https://bugs.webkit.org/show_bug.cgi?id=43254
Comment 12 Nikolas Zimmermann 2010-07-30 14:09:29 PDT
(In reply to comment #10)
> (From update of attachment 63076 [details])
> WebCore/ChangeLog:1
>  +  2010-07-30  fsamuel@chromium.org  <fsamuel@chromium.org>
> This should be your Full Name <email>, no email twice. :)
> 
> WebCore/rendering/style/SVGRenderStyle.h: 
>  +      SVG_RS_DEFINE_ATTRIBUTE(EAlignmentBaseline, AlignmentBaseline, alignmentBaseline, AB_AUTO)
> I'm not sure why expanding these is better... but OK.

It was your request, a long time ago, you even added the FIXME :-)