Bug 48403 – Typo fix in SVGFEConvolveMatrixElement-dom-* tests

• Component: SVG
• Status: RESOLVED FIXED

[Renata Hodovan]

Typo fix in SVGFEConvolveMatrixElement-dom-* tests

[Renata Hodovan]

Created attachment 71995 [details]
Fix typo

[Dirk Schulze]

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

[Renata Hodovan]

(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?

[Nikolas Zimmermann]

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..

[Dirk Schulze]

(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.

[Dirk Schulze]

(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 ;-)

[Dirk Schulze]

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.

[Renata Hodovan]

> 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).

[Dirk Schulze]

(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.

[Renata Hodovan]

Created attachment 72052 [details]
Fix typo

[Nikolas Zimmermann]

(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.

[Renata Hodovan]

(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 :$

[Dirk Schulze]

Comment on attachment 72052 [details]
Fix typo

r=me

[WebKit Commit Bot]

Comment on attachment 72052 [details]
Fix typo

Clearing flags on attachment: 72052

Committed r70686: <http://trac.webkit.org/changeset/70686>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/70686 might have broken GTK Linux 64-bit Debug