Bug 48715 - Enable StrictTypeChecking for all types using SVG(Animated)PropertyTearOff
Summary: Enable StrictTypeChecking for all types using SVG(Animated)PropertyTearOff
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 47905
  Show dependency treegraph
 
Reported: 2010-10-30 06:24 PDT by Nikolas Zimmermann
Modified: 2010-11-01 02:12 PDT (History)
4 users (show)

See Also:


Attachments
Patch (111.75 KB, patch)
2010-10-31 13:38 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (113.43 KB, patch)
2010-10-31 13:51 PDT, Nikolas Zimmermann
rwlbuis: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2010-10-30 06:24:34 PDT
Enable StrictTypeChecking for all types using SVG(Animated)PropertyTearOff
Comment 1 Nikolas Zimmermann 2010-10-31 13:38:33 PDT
Created attachment 72475 [details]
Patch

Added lots of new tests, and a little code :-)
Comment 2 WebKit Review Bot 2010-10-31 13:42:23 PDT
Attachment 72475 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/svg/SVGLength.cpp:206:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
WebCore/bindings/v8/custom/V8SVGLengthCustom.cpp:82:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Nikolas Zimmermann 2010-10-31 13:51:15 PDT
Created attachment 72476 [details]
Patch v2

Fix style issues, more explicit ChangeLog.
Comment 4 Rob Buis 2010-11-01 01:01:08 PDT
Comment on attachment 72476 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=72476&action=review

I only found small issues that can easily be corrected, so r=me.

> WebCore/ChangeLog:11
> +        When testing SVGLength it became obvious that quite a lot functionality was missing.

quite a lot *of* functionality

> WebCore/ChangeLog:44
> +        * svg/SVGLength.cpp: Implement all missing features of SVGLength.e

Remove the extra e.

> WebCore/ChangeLog:47
> +        (WebCore::SVGLength::value): Add ExceptionCode parameter throw NOT_SUPPORTED_ERR when trying to obtain to relative units, when no context is given (SVGLength created by createSVGLength()).

Should be rephrased, hard to read.

> WebCore/ChangeLog:49
> +        (WebCore::SVGLength::valueAsPercentage): s/100.0f/100/, use m_valueInSpecifiedUnits instead of valueInSpecifiedUnits().

Maybe not mention s/100.0f/100/, since it is the only mention of adapting to the code guideline style but other places in SVGLength.cpp were adapted too.
Or have a general remark in the ChangeLog that this style is now preferred because of the Webkit Coding style guideline.

> WebCore/svg/SVGLength.cpp:421
> +    return value / style->fontSize();

Can reuse fontSize local variable here.

> LayoutTests/svg/dom/SVGAnimatedAngle-expected.txt:16
> +Check assigning to baseVal has no effect, as no setter is defined

Check that.

> LayoutTests/svg/dom/SVGAnimatedLength-expected.txt:16
> +Check assigning to baseVal has no effect, as no setter is defined

Check that.

> LayoutTests/svg/dom/SVGAnimatedLengthList-expected.txt:16
> +Check assigning to baseVal has no effect, as no setter is defined

Check that.

> LayoutTests/svg/dom/SVGAnimatedRect-expected.txt:16
> +Check assigning to baseVal has no effect, as no setter is defined

Ditto.
Comment 5 Nikolas Zimmermann 2010-11-01 01:53:58 PDT
Thanks for the quick review, fixed all issues, landing now...
Comment 6 Nikolas Zimmermann 2010-11-01 01:57:39 PDT
Landed in r71014.
Comment 7 Nikolas Zimmermann 2010-11-01 02:12:15 PDT
(In reply to comment #6)
> Landed in r71014.

Forgot to land the svg/dynamic-updates changes, fixed in r71015.