Bug 48822

Summary: Convert SVGPreserveAspectRatio to the new SVGPropertyTearOff concept
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, mdelaney7, webkit-ews, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 47905    
Attachments:
Description Flags
Patch
none
Patch v2 rwlbuis: review+

Description Nikolas Zimmermann 2010-11-02 00:17:16 PDT
Convert SVGPreserveAspectRatio to the new SVGPropertyTearOff concept.
Comment 1 Nikolas Zimmermann 2010-11-02 00:23:21 PDT
Created attachment 72634 [details]
Patch
Comment 2 Nikolas Zimmermann 2010-11-02 00:31:15 PDT
Created attachment 72635 [details]
Patch v2

Recreate patch, after updating to trunk, so it applies.
Comment 3 WebKit Review Bot 2010-11-02 00:34:00 PDT
Attachment 72635 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/svg/SVGPreserveAspectRatio.cpp:168:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
WebCore/svg/SVGPreserveAspectRatio.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Nikolas Zimmermann 2010-11-02 00:37:40 PDT
(In reply to comment #3)
> Attachment 72635 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
> WebCore/svg/SVGPreserveAspectRatio.cpp:168:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
> WebCore/svg/SVGPreserveAspectRatio.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 2 in 30 files

Fixed the second issue, ignoring the first on purpose.
switch (blub) {
case foo:
{
    foobar
}
}

is just fine, the style guide doesn't mention how braces should be formatted when used inside a case statement. The bot only doesn't complain if I would have used:

switch (blub) {
case foo:
    {
        foobar
    }
}

which looks crazy.
Comment 5 Rob Buis 2010-11-02 00:48:57 PDT
Comment on attachment 72635 [details]
Patch v2

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

Found only one minor issue, so r=me.

> LayoutTests/svg/dom/script-tests/SVGPreserveAspectRatio.js:1
> +description("This test checks the SVGPreserveAspectRatio API");

may add the  "utilizing" explanation for consistency :)
Comment 6 Early Warning System Bot 2010-11-02 01:10:19 PDT
Attachment 72635 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4979013
Comment 7 Nikolas Zimmermann 2010-11-02 01:36:14 PDT
Fixed style issue two, qt and efl builds (all related to SVGNames.h include missing in two places, funny that I don't need those includes on mac though....). Anyhow, landed in r71103. Closing bug after I get all bot results.
Comment 8 Eric Seidel (no email) 2010-11-02 02:19:26 PDT
Attachment 72635 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4959011
Comment 9 Nikolas Zimmermann 2010-11-04 08:53:49 PDT
Forgot to close.