Summary: | Typo fix in SVGFEConvolveMatrixElement-dom-* tests | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Renata Hodovan <rhodovan.u-szeged> | ||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, commit-queue, eric, krit, mdelaney7, webkit.review.bot, zimmermann | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Renata Hodovan
2010-10-27 00:34:04 PDT
Created attachment 71995 [details]
Fix typo
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 (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 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..
(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. (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 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. > 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).
(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. Created attachment 72052 [details]
Fix typo
(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. (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 on attachment 72052 [details]
Fix typo
r=me
Comment on attachment 72052 [details] Fix typo Clearing flags on attachment: 72052 Committed r70686: <http://trac.webkit.org/changeset/70686> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/70686 might have broken GTK Linux 64-bit Debug |