Bug 23816 - Clean up RenderSVGRoot.cpp
Summary: Clean up RenderSVGRoot.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-07 06:58 PST by Rob Buis
Modified: 2009-02-09 12:55 PST (History)
0 users

See Also:


Attachments
Cleanup patch (4.22 KB, patch)
2009-02-07 07:00 PST, Rob Buis
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2009-02-07 06:58:42 PST
RenderSVGRoot has some unneeded includes and unneeded if tests, clean that up.
Comment 1 Rob Buis 2009-02-07 07:00:28 PST
Created attachment 27445 [details]
Cleanup patch

Straightforward cleanup of RenderSVGRoot.cpp.
Cheers,

Rob.
Comment 2 Eric Seidel (no email) 2009-02-09 11:10:51 PST
Comment on attachment 27445 [details]
Cleanup patch

Looks fine.  You could even handle the hasSetContainerSize() case up front.  That would result in more code, but might be cleaner.

if (svg-> hasSetContainerSize()) { 
   m_viewport = FloatRect(0, 0, width.value(svg), height.value(svg));
   return;
}

Then the rest could be a terinary:

w = (width.unitType() == LengthTypePercent) ? svg->relativeWidthValue() : width.value(svg);
h = (height.unitType() == LengthTypePercent) ? svg->relativeHeightValue() : height.value(svg);
m_viewport = FloatRect(0, 0, w, h);

I'm not sure if that's cleaner or not though.  Either way, looks great and doesn't need any further review.
Comment 3 Rob Buis 2009-02-09 12:55:59 PST
Hi Eric,

(In reply to comment #2)
> (From update of attachment 27445 [details] [review])
> Looks fine.  You could even handle the hasSetContainerSize() case up front. 
> That would result in more code, but might be cleaner.
> 
> if (svg-> hasSetContainerSize()) { 
>    m_viewport = FloatRect(0, 0, width.value(svg), height.value(svg));
>    return;
> }
> 
> Then the rest could be a terinary:
> 
> w = (width.unitType() == LengthTypePercent) ? svg->relativeWidthValue() :
> width.value(svg);
> h = (height.unitType() == LengthTypePercent) ? svg->relativeHeightValue() :
> height.value(svg);
> m_viewport = FloatRect(0, 0, w, h);
> 
> I'm not sure if that's cleaner or not though.  Either way, looks great and
> doesn't need any further review.

Thanks for the thumbs up, I did try to clean it up that way in my final commit.
Cheers,

Rob.