Bug 48403 - Typo fix in SVGFEConvolveMatrixElement-dom-* tests
Summary: Typo fix in SVGFEConvolveMatrixElement-dom-* tests
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-27 00:34 PDT by Renata Hodovan
Modified: 2010-10-27 14:05 PDT (History)
7 users (show)

See Also:


Attachments
Fix typo (7.66 KB, patch)
2010-10-27 00:53 PDT, Renata Hodovan
krit: review-
krit: commit-queue-
Details | Formatted Diff | Diff
Fix typo (7.75 KB, patch)
2010-10-27 10:16 PDT, Renata Hodovan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Renata Hodovan 2010-10-27 00:34:04 PDT
Typo fix in SVGFEConvolveMatrixElement-dom-* tests
Comment 1 Renata Hodovan 2010-10-27 00:53:32 PDT
Created attachment 71995 [details]
Fix typo
Comment 2 Dirk Schulze 2010-10-27 01:05:11 PDT
Comment on attachment 71995 [details]
Fix typo

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

> LayoutTests/ChangeLog:8
> +        Since expecteds and pixel tests didn't change after correcting this attribute has no effect so it is removed.

Hm, what do you mean? Removing this attribute did not fix the currently failing tests, so we can remove it? :-P
Comment 3 Renata Hodovan 2010-10-27 01:10:39 PDT
(In reply to comment #2)
> (From update of attachment 71995 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71995&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        Since expecteds and pixel tests didn't change after correcting this attribute has no effect so it is removed.
> 
> Hm, what do you mean? Removing this attribute did not fix the currently failing tests, so we can remove it? :-P

These tests don't fail this time... I realized the preserveAlpha attribute is misspelled... I corrected it, but the expected didn't changed. It means the attribute doesn't have effect in this case, so we can remove it, right?
Comment 4 Nikolas Zimmermann 2010-10-27 01:15:42 PDT
Comment on attachment 71995 [details]
Fix typo

It's fine with me, but it indicates were missing a SVGFEConvolveMatrixElement-dom/svgdom-preserveAlpha* test, right?
If you create one, that shows it's working, you can remove the attribute from the other tests..
Comment 5 Dirk Schulze 2010-10-27 01:17:53 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 71995 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=71995&action=review
> > 
> > > LayoutTests/ChangeLog:8
> > > +        Since expecteds and pixel tests didn't change after correcting this attribute has no effect so it is removed.
> > 
> > Hm, what do you mean? Removing this attribute did not fix the currently failing tests, so we can remove it? :-P
> 
> These tests don't fail this time... I realized the preserveAlpha attribute is misspelled... I corrected it, but the expected didn't changed. It means the attribute doesn't have effect in this case, so we can remove it, right?

Ah, I saw that we have already preserverAlpha tests. Than I'm fine.

Niko and I noticed that we miss SVG DOM tests for feConvolveMatrix. Not sure if I missed it during the review process or someone else, but can you add SVG DOM tests in a new patch as well? After the changes of Niko to the SVG bindings, it shouldn't be a problem to test the kernelMatrix via SVG DOM.
Comment 6 Dirk Schulze 2010-10-27 01:19:03 PDT
(In reply to comment #4)
> (From update of attachment 71995 [details])
> It's fine with me, but it indicates were missing a SVGFEConvolveMatrixElement-dom/svgdom-preserveAlpha* test, right?
> If you create one, that shows it's working, you can remove the attribute from the other tests..

Hey Niko, don't comment on bugs I'm writing a comment on ;-)
Comment 7 Dirk Schulze 2010-10-27 01:21:55 PDT
Comment on attachment 71995 [details]
Fix typo

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

>>>> LayoutTests/ChangeLog:8
>>>> +        Since expecteds and pixel tests didn't change after correcting this attribute has no effect so it is removed.
>>> 
>>> Hm, what do you mean? Removing this attribute did not fix the currently failing tests, so we can remove it? :-P
>> 
>> These tests don't fail this time... I realized the preserveAlpha attribute is misspelled... I corrected it, but the expected didn't changed. It means the attribute doesn't have effect in this case, so we can remove it, right?
> 
> Ah, I saw that we have already preserverAlpha tests. Than I'm fine.
> 
> Niko and I noticed that we miss SVG DOM tests for feConvolveMatrix. Not sure if I missed it during the review process or someone else, but can you add SVG DOM tests in a new patch as well? After the changes of Niko to the SVG bindings, it shouldn't be a problem to test the kernelMatrix via SVG DOM.

Pleases write, that we already have a dom test for preserveAlpha which is not affected by this changes.
Comment 8 Renata Hodovan 2010-10-27 09:05:02 PDT
> Niko and I noticed that we miss SVG DOM tests for feConvolveMatrix. Not sure if I missed it during the review process or someone else, but can you add SVG DOM tests in a new patch as well? After the changes of Niko to the SVG bindings, it shouldn't be a problem to test the kernelMatrix via SVG DOM.
Yes.... SVG DOM tests are missing because my previous patch with DOM and SVG DOM tests was too big for bugzilla, so I should cut it into two part. But after then I couldn't generate the SVG DOM tests because filters were not invalidated at SVGImage object (and it's a part of the tests). Since it's solved already I'll send them ASAP (but in its own bug).
Comment 9 Dirk Schulze 2010-10-27 09:24:56 PDT
(In reply to comment #8)
> > Niko and I noticed that we miss SVG DOM tests for feConvolveMatrix. Not sure if I missed it during the review process or someone else, but can you add SVG DOM tests in a new patch as well? After the changes of Niko to the SVG bindings, it shouldn't be a problem to test the kernelMatrix via SVG DOM.
> Yes.... SVG DOM tests are missing because my previous patch with DOM and SVG DOM tests was too big for bugzilla, so I should cut it into two part. But after then I couldn't generate the SVG DOM tests because filters were not invalidated at SVGImage object (and it's a part of the tests). Since it's solved already I'll send them ASAP (but in its own bug).

Great! Please update the ChangeLog and I'll review the patch.
Comment 10 Renata Hodovan 2010-10-27 10:16:56 PDT
Created attachment 72052 [details]
Fix typo
Comment 11 Nikolas Zimmermann 2010-10-27 11:33:46 PDT
(In reply to comment #8)
> > Niko and I noticed that we miss SVG DOM tests for feConvolveMatrix. Not sure if I missed it during the review process or someone else, but can you add SVG DOM tests in a new patch as well? After the changes of Niko to the SVG bindings, it shouldn't be a problem to test the kernelMatrix via SVG DOM.
> Yes.... SVG DOM tests are missing because my previous patch with DOM and SVG DOM tests was too big for bugzilla, so I should cut it into two part. But after then I couldn't generate the SVG DOM tests because filters were not invalidated at SVGImage object (and it's a part of the tests). Since it's solved already I'll send them ASAP (but in its own bug).

You can always check the "Big File" checkbox, that allows you to upload more than 2MB.
Comment 12 Renata Hodovan 2010-10-27 11:35:34 PDT
(In reply to comment #11)
> (In reply to comment #8)
> > > Niko and I noticed that we miss SVG DOM tests for feConvolveMatrix. Not sure if I missed it during the review process or someone else, but can you add SVG DOM tests in a new patch as well? After the changes of Niko to the SVG bindings, it shouldn't be a problem to test the kernelMatrix via SVG DOM.
> > Yes.... SVG DOM tests are missing because my previous patch with DOM and SVG DOM tests was too big for bugzilla, so I should cut it into two part. But after then I couldn't generate the SVG DOM tests because filters were not invalidated at SVGImage object (and it's a part of the tests). Since it's solved already I'll send them ASAP (but in its own bug).
> 
> You can always check the "Big File" checkbox, that allows you to upload more than 2MB.

Thanks :$
Comment 13 Dirk Schulze 2010-10-27 11:47:49 PDT
Comment on attachment 72052 [details]
Fix typo

r=me
Comment 14 WebKit Commit Bot 2010-10-27 12:09:30 PDT
Comment on attachment 72052 [details]
Fix typo

Clearing flags on attachment: 72052

Committed r70686: <http://trac.webkit.org/changeset/70686>
Comment 15 WebKit Commit Bot 2010-10-27 12:09:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Review Bot 2010-10-27 14:05:23 PDT
http://trac.webkit.org/changeset/70686 might have broken GTK Linux 64-bit Debug