Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1046673006:
Add svgAttributeChanged function to SVGFEComponentTransferElement class (Closed)
|
Created:
5 years, 8 months ago by hyunjunekim2 Modified:
5 years, 8 months ago CC:
blink-reviews, krit, ed+blinkwatch_opera.com, f(malita), gyuyoung.kim_webkit.org, kouhei+svg_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd svgAttributeChanged function to SVGFEComponentTransferElement class.
This CL is to add missing code. Add svgAttributeChanged function to SVGFEComponentTransferElement class.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193027
Patch Set 1 #
Total comments: 4
Patch Set 2 : #Patch Set 3 : fixed #Patch Set 4 : Add testcase #Patch Set 5 : add a repaint testcase. #Patch Set 6 : #Patch Set 7 : modified testcases. #
Total comments: 7
Patch Set 8 : #Patch Set 9 : modify testcase-2 #
Total comments: 1
Patch Set 10 : modify testcase-3 #
Messages
Total messages: 22 (2 generated)
hyunjune.kim@samsung.com changed reviewers: + fs@opera.com, mkwst@chromium.org, pdr@chromium.org, pfeldman@chromium.org
pdr, Mike West, fs, pfeldman, Hi, I'm hyunjunekim (hyunjune.kim@samsung.com, hykim0777@gamil.com). Added missing code. Could you give me feedback? Thank you.
Does this fix any existing test? If not, then please add a new test. https://codereview.chromium.org/1046673006/diff/1/Source/core/svg/SVGFECompon... File Source/core/svg/SVGFEComponentTransferElement.cpp (right): https://codereview.chromium.org/1046673006/diff/1/Source/core/svg/SVGFECompon... Source/core/svg/SVGFEComponentTransferElement.cpp:51: void SVGFEComponentTransferElement::svgAttributeChanged(const QualifiedName& attrName) Please put this method after isSupportedAttribute. https://codereview.chromium.org/1046673006/diff/1/Source/core/svg/SVGFECompon... Source/core/svg/SVGFEComponentTransferElement.cpp:57: Add SVGElement::InvalidationGuard invalidationGuard(this); here. https://codereview.chromium.org/1046673006/diff/1/Source/core/svg/SVGFECompon... Source/core/svg/SVGFEComponentTransferElement.cpp:62: } ASSERT_NOT_REACHED() https://codereview.chromium.org/1046673006/diff/1/Source/core/svg/SVGFECompon... File Source/core/svg/SVGFEComponentTransferElement.h (right): https://codereview.chromium.org/1046673006/diff/1/Source/core/svg/SVGFECompon... Source/core/svg/SVGFEComponentTransferElement.h:41: virtual void svgAttributeChanged(const QualifiedName&) override; After isSupportedAttribute please.
On 2015/03/31 08:25:50, fs wrote: > Does this fix any existing test? If not, then please add a new test. > > https://codereview.chromium.org/1046673006/diff/1/Source/core/svg/SVGFECompon... > File Source/core/svg/SVGFEComponentTransferElement.cpp (right): > > https://codereview.chromium.org/1046673006/diff/1/Source/core/svg/SVGFECompon... > Source/core/svg/SVGFEComponentTransferElement.cpp:51: void > SVGFEComponentTransferElement::svgAttributeChanged(const QualifiedName& > attrName) > Please put this method after isSupportedAttribute. > > https://codereview.chromium.org/1046673006/diff/1/Source/core/svg/SVGFECompon... > Source/core/svg/SVGFEComponentTransferElement.cpp:57: > Add > > SVGElement::InvalidationGuard invalidationGuard(this); > > here. > > https://codereview.chromium.org/1046673006/diff/1/Source/core/svg/SVGFECompon... > Source/core/svg/SVGFEComponentTransferElement.cpp:62: } > ASSERT_NOT_REACHED() > > https://codereview.chromium.org/1046673006/diff/1/Source/core/svg/SVGFECompon... > File Source/core/svg/SVGFEComponentTransferElement.h (right): > > https://codereview.chromium.org/1046673006/diff/1/Source/core/svg/SVGFECompon... > Source/core/svg/SVGFEComponentTransferElement.h:41: virtual void > svgAttributeChanged(const QualifiedName&) override; > After isSupportedAttribute please. I am going to make test-case. thank you for your review.
Deferring to fs@ here. He's a much better SVG reviewer than I would be.
On 2015/03/31 09:39:08, Mike West wrote: > Deferring to fs@ here. He's a much better SVG reviewer than I would be. fs, Hi, I modified test-case & some codes. Could you give me feedback?
On 2015/03/31 16:17:59, hyunjunekim2 wrote: > On 2015/03/31 09:39:08, Mike West wrote: > > Deferring to fs@ here. He's a much better SVG reviewer than I would be. > > fs, Hi, I modified test-case & some codes. Could you give me feedback? Code change looks good, but please just add a new test for this - one in svg/dynamic-updates is ok. The modifications you did to the existing tests doesn't actually appear to test the effects of the code change.
On 2015/03/31 16:41:27, fs wrote: > On 2015/03/31 16:17:59, hyunjunekim2 wrote: > > On 2015/03/31 09:39:08, Mike West wrote: > > > Deferring to fs@ here. He's a much better SVG reviewer than I would be. > > > > fs, Hi, I modified test-case & some codes. Could you give me feedback? > > Code change looks good, but please just add a new test for this - one in > svg/dynamic-updates is ok. The modifications you did to the existing tests > doesn't actually appear to test the effects of the code change. fs, I will make a new test about FEComponentTransferElement. Thank you.
On 2015/03/31 16:41:27, fs wrote: > On 2015/03/31 16:17:59, hyunjunekim2 wrote: > > On 2015/03/31 09:39:08, Mike West wrote: > > > Deferring to fs@ here. He's a much better SVG reviewer than I would be. > > > > fs, Hi, I modified test-case & some codes. Could you give me feedback? > > Code change looks good, but please just add a new test for this - one in > svg/dynamic-updates is ok. The modifications you did to the existing tests > doesn't actually appear to test the effects of the code change. fs, Hello, I made a test case to test the effects of the code change. Could you give me feedback? Thank you.
https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/dynami... File LayoutTests/svg/dynamic-updates/script-tests/SVGFEComponentTransferElement-dom-amplitude-attr.js (right): https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/dynami... LayoutTests/svg/dynamic-updates/script-tests/SVGFEComponentTransferElement-dom-amplitude-attr.js:27: var feComponentTransferElement = createSVGElement("feComponentTransfer"); If you want to fix this typo in a separate CL. https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/repain... File LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg (right): https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/repain... LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:2: width="300" height="300" onload="runRepaintAndPixelTest()"> Should be no need to make a pixel test for this - repaint is enough (a reftest would've worked too.) https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/repain... LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:3: <script xlink:href="../../fast/repaint/resources/text-based-repaint.js" /> If you put the test in an HTML wrapper you can drop a lot of the ugliness: <!DOCTYPE html> <script src="../../fast/repaint/resources/text-based-repaint.js"></script> <script> function ... onload = runRepaintTest; </script> <svg> ... </svg> (no xlink:href, no CDATA, no xmlns) https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/repain... LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:10: <linearGradient id="MyGradient" gradientUnits="userSpaceOnUse" x1="10" y1="0" x2="590" y2="0"> There should be no need for a gradient here. Just use a simple color ("green"). https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/repain... LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:14: <filter id="SourceGraphic" x="0%" y="0%" width="100%" height="100%"> Just call it something simple like 'f' ('SourceGraphic' is easily confusable with the filter references) Also, you don't need the percentage here - actually, just leave out x/y/etc because the defaults should work out fine here. https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/repain... LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:16: <feFuncG type="gamma" amplitude="1" exponent="0.5" offset="0"/> I think you could just use type="identity" here. https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/repain... LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:19: <rect x="0" y="0" width="100" height="100" fill="url(#MyGradient)" filter="url(#SourceGraphic)"/> Drop x="0" and y="0" (they're the default values)
On 2015/04/01 16:21:42, fs wrote: > https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/dynami... > File > LayoutTests/svg/dynamic-updates/script-tests/SVGFEComponentTransferElement-dom-amplitude-attr.js > (right): > > https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/dynami... > LayoutTests/svg/dynamic-updates/script-tests/SVGFEComponentTransferElement-dom-amplitude-attr.js:27: > var feComponentTransferElement = createSVGElement("feComponentTransfer"); > If you want to fix this typo in a separate CL. > > https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/repain... > File LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg (right): > > https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/repain... > LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:2: width="300" > height="300" onload="runRepaintAndPixelTest()"> > Should be no need to make a pixel test for this - repaint is enough (a reftest > would've worked too.) > > https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/repain... > LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:3: <script > xlink:href="../../fast/repaint/resources/text-based-repaint.js" /> > If you put the test in an HTML wrapper you can drop a lot of the ugliness: > > <!DOCTYPE html> > <script src="../../fast/repaint/resources/text-based-repaint.js"></script> > <script> > function ... > onload = runRepaintTest; > </script> > <svg> > ... > </svg> > > (no xlink:href, no CDATA, no xmlns) > > https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/repain... > LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:10: <linearGradient > id="MyGradient" gradientUnits="userSpaceOnUse" x1="10" y1="0" x2="590" y2="0"> > There should be no need for a gradient here. Just use a simple color ("green"). > > https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/repain... > LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:14: <filter > id="SourceGraphic" x="0%" y="0%" width="100%" height="100%"> > Just call it something simple like 'f' ('SourceGraphic' is easily confusable > with the filter references) > > Also, you don't need the percentage here - actually, just leave out x/y/etc > because the defaults should work out fine here. > > https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/repain... > LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:16: <feFuncG > type="gamma" amplitude="1" exponent="0.5" offset="0"/> > I think you could just use type="identity" here. > > https://codereview.chromium.org/1046673006/diff/120001/LayoutTests/svg/repain... > LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:19: <rect x="0" y="0" > width="100" height="100" fill="url(#MyGradient)" filter="url(#SourceGraphic)"/> > Drop x="0" and y="0" (they're the default values) fs, Could you check this? Thank you.
> ><filter id="SourceGraphic" x="0%" y="0%" width="100%" height="100%">
> > Just call it something simple like 'f' ('SourceGraphic' is easily confusable
> > with the filter references)
> >
> > Also, you don't need the percentage here - actually, just leave out x/y/etc
> > because the defaults should work out fine here.
fs,
I cheked filtr's default value.
filter element's x, y default value - "-10%"
filter element's width, height default value - "120%"
I need the percentage here.
Could you check this one? I modified a test-case for your feedback.
On 2015/04/02 05:25:43, hyunjunekim2 wrote:
> > ><filter id="SourceGraphic" x="0%" y="0%" width="100%" height="100%">
> > > Just call it something simple like 'f' ('SourceGraphic' is easily
confusable
> > > with the filter references)
> > >
> > > Also, you don't need the percentage here - actually, just leave out
x/y/etc
> > > because the defaults should work out fine here.
>
> fs,
> I cheked filtr's default value.
> filter element's x, y default value - "-10%"
> filter element's width, height default value - "120%"
> I need the percentage here.
>
> Could you check this one? I modified a test-case for your feedback.
FYI
http://www.w3.org/TR/SVG/filters.html#Filtereffectsregion
If ‘x’ or ‘y’ is not specified, the effect is as if a value of -10% were
specified.
If ‘width’ or ‘height’ is not specified, the effect is as if a value of 120%
were specified.
On 2015/04/02 05:29:41, hyunjunekim2 wrote:
> On 2015/04/02 05:25:43, hyunjunekim2 wrote:
> > > ><filter id="SourceGraphic" x="0%" y="0%" width="100%" height="100%">
> > > > Just call it something simple like 'f' ('SourceGraphic' is easily
> confusable
> > > > with the filter references)
> > > >
> > > > Also, you don't need the percentage here - actually, just leave out
> x/y/etc
> > > > because the defaults should work out fine here.
> >
> > fs,
> > I cheked filtr's default value.
> > filter element's x, y default value - "-10%"
> > filter element's width, height default value - "120%"
> > I need the percentage here.
> >
> > Could you check this one? I modified a test-case for your feedback.
>
> FYI
> http://www.w3.org/TR/SVG/filters.html#Filtereffectsregion
> If ‘x’ or ‘y’ is not specified, the effect is as if a value of -10% were
> specified.
> If ‘width’ or ‘height’ is not specified, the effect is as if a value of 120%
> were specified.
The reason you don't need them is that everything in the will be transparent,
and hence not affected by the filter (the filter, as specified, does not
transform transparent pixels into non-transparent ones.)
On 2015/04/02 09:03:40, fs wrote:
> On 2015/04/02 05:29:41, hyunjunekim2 wrote:
> > On 2015/04/02 05:25:43, hyunjunekim2 wrote:
> > > > ><filter id="SourceGraphic" x="0%" y="0%" width="100%" height="100%">
> > > > > Just call it something simple like 'f' ('SourceGraphic' is easily
> > confusable
> > > > > with the filter references)
> > > > >
> > > > > Also, you don't need the percentage here - actually, just leave out
> > x/y/etc
> > > > > because the defaults should work out fine here.
> > >
> > > fs,
> > > I cheked filtr's default value.
> > > filter element's x, y default value - "-10%"
> > > filter element's width, height default value - "120%"
> > > I need the percentage here.
> > >
> > > Could you check this one? I modified a test-case for your feedback.
> >
> > FYI
> > http://www.w3.org/TR/SVG/filters.html#Filtereffectsregion
> > If ‘x’ or ‘y’ is not specified, the effect is as if a value of -10% were
> > specified.
> > If ‘width’ or ‘height’ is not specified, the effect is as if a value of 120%
> > were specified.
>
> The reason you don't need them is that everything in the will be transparent,
> and hence not affected by the filter (the filter, as specified, does not
> transform transparent pixels into non-transparent ones.)
(The repaint rect will be different, but the fact that there'll be one should be
enough.)
https://codereview.chromium.org/1046673006/diff/160001/LayoutTests/svg/repain... File LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg (right): https://codereview.chromium.org/1046673006/diff/160001/LayoutTests/svg/repain... LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:11: <feFuncG type="identity" amplitude="1" exponent="0.5" offset="0"/> You can drop 'amplitude="1" exponent="0.5" offset="0"'
On 2015/04/02 09:05:58, fs wrote: > https://codereview.chromium.org/1046673006/diff/160001/LayoutTests/svg/repain... > File LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg (right): > > https://codereview.chromium.org/1046673006/diff/160001/LayoutTests/svg/repain... > LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:11: <feFuncG > type="identity" amplitude="1" exponent="0.5" offset="0"/> > You can drop 'amplitude="1" exponent="0.5" offset="0"' fs, I modified this. Could you check this one? And are there feature to need implement in SVG of Blink? If there are, Could you tell me it? I want to study it. Thank you. :)
On 2015/04/02 09:29:15, hyunjunekim2 wrote: > On 2015/04/02 09:05:58, fs wrote: > > > https://codereview.chromium.org/1046673006/diff/160001/LayoutTests/svg/repain... > > File LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg (right): > > > > > https://codereview.chromium.org/1046673006/diff/160001/LayoutTests/svg/repain... > > LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:11: <feFuncG > > type="identity" amplitude="1" exponent="0.5" offset="0"/> > > You can drop 'amplitude="1" exponent="0.5" offset="0"' > > fs, I modified this. Could you check this one? lgtm > And are there feature to need implement in SVG of Blink? If there are, Could you > tell me it? I want to study it. You can search for the Cr-Blink-SVG label in the bug tracker.
On 2015/04/02 09:32:07, fs wrote: > On 2015/04/02 09:29:15, hyunjunekim2 wrote: > > On 2015/04/02 09:05:58, fs wrote: > > > > > > https://codereview.chromium.org/1046673006/diff/160001/LayoutTests/svg/repain... > > > File LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg (right): > > > > > > > > > https://codereview.chromium.org/1046673006/diff/160001/LayoutTests/svg/repain... > > > LayoutTests/svg/repaint/fecomponenttransfer-in1-change.svg:11: <feFuncG > > > type="identity" amplitude="1" exponent="0.5" offset="0"/> > > > You can drop 'amplitude="1" exponent="0.5" offset="0"' > > > > fs, I modified this. Could you check this one? > > lgtm > > > And are there feature to need implement in SVG of Blink? If there are, Could > you > > tell me it? I want to study it. > > You can search for the Cr-Blink-SVG label in the bug tracker. fs, Wow~ I will search it. Thank you for your review and answer. ;)
The CQ bit was checked by hyunjune.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1046673006/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://src.chromium.org/viewvc/blink?view=rev&revision=193027 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
