Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
135913002:
Do refactor in collectGradientAttributes() and renderStyleForLengthResolve() (Closed)
|
Created:
6 years, 11 months ago by gyuyoung-inactive Modified:
6 years, 11 months ago Reviewers:
Stephen Chennney CC:
blink-reviews, pdr, Stephen Chennney, krit, f(malita), rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionSome SVG functions have done a first iteration by using a boolean flag. This is one of poor implementations. This cl fix it by extracting a logic into a new method. Additionally it would be good to use do-while() loop instead of using while() in renderStyleForLengthResolving() because a first condition is always true.
BUG=N/A
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165358
Patch Set 1 : #
Total comments: 1
Patch Set 2 : #Patch Set 3 : #
Total comments: 1
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #
Total comments: 15
Patch Set 6 : #
Messages
Total messages: 17 (0 generated)
Stephen ? could you take a look this issue ?
On 2014/01/12 06:06:32, gyuyoung wrote: > Stephen ? could you take a look this issue ? This patch need to be modified. It looks this one break svg tests.
https://codereview.chromium.org/135913002/diff/60001/Source/core/svg/SVGRadia... File Source/core/svg/SVGRadialGradientElement.cpp (right): https://codereview.chromium.org/135913002/diff/60001/Source/core/svg/SVGRadia... Source/core/svg/SVGRadialGradientElement.cpp:174: if (current->hasTagName(SVGNames::radialGradientTag)) { This moving do wrong behavior. I will update this patch tomorrow.
Patch is fixed.
I'm not happy with this - I think you can do even better. In the gradient methods, I would prefer if you extracted the part of the loop that processes the attributes into a new method. That method can assume the element it is given is of the appropriate gradient class. You can then call the new method for "this", before entering the do/while loop to work through hrefs. This removes the need for isLinear while also avoiding the new cost in your current patch of checking the tag name of "this". https://codereview.chromium.org/135913002/diff/140001/Source/core/svg/SVGLine... File Source/core/svg/SVGLinearGradientElement.cpp (right): https://codereview.chromium.org/135913002/diff/140001/Source/core/svg/SVGLine... Source/core/svg/SVGLinearGradientElement.cpp:175: current = 0; Need braces here.
On 2014/01/14 13:44:15, Stephen Chenney wrote: > I'm not happy with this - I think you can do even better. > > In the gradient methods, I would prefer if you extracted the part of the loop > that processes the attributes into a new method. That method can assume the > element it is given is of the appropriate gradient class. You can then call the > new method for "this", before entering the do/while loop to work through hrefs. Let me think how to extract a part of the loop. > This removes the need for isLinear while also avoiding the new cost in your > current patch of checking the tag name of "this". This patch call a hasTagName() one more time than before. So, I thought it would be nicer to remove a flag. Anyway, let me consider how to remove it as well.
On 2014/01/14 13:44:15, Stephen Chenney wrote: > I'm not happy with this - I think you can do even better. > > In the gradient methods, I would prefer if you extracted the part of the loop > that processes the attributes into a new method. That method can assume the > element it is given is of the appropriate gradient class. You can then call the > new method for "this", before entering the do/while loop to work through hrefs. > > This removes the need for isLinear while also avoiding the new cost in your > current patch of checking the tag name of "this". > > https://codereview.chromium.org/135913002/diff/140001/Source/core/svg/SVGLine... > File Source/core/svg/SVGLinearGradientElement.cpp (right): > > https://codereview.chromium.org/135913002/diff/140001/Source/core/svg/SVGLine... > Source/core/svg/SVGLinearGradientElement.cpp:175: current = 0; > Need braces here. Stephen, I fixed this patch according to your suggestion. I wonder how do you think about latest one.
This is a much better patch. The comments below for radial also apply to linear. I think that patch as it is now will not behave the same as the previous code. https://codereview.chromium.org/135913002/diff/200001/Source/core/svg/SVGRadi... File Source/core/svg/SVGRadialGradientElement.cpp (right): https://codereview.chromium.org/135913002/diff/200001/Source/core/svg/SVGRadi... Source/core/svg/SVGRadialGradientElement.cpp:141: if (!current->renderer()) This "if" needs to be done for the initial "this" too. https://codereview.chromium.org/135913002/diff/200001/Source/core/svg/SVGRadi... Source/core/svg/SVGRadialGradientElement.cpp:144: if (!attributes.hasSpreadMethod() && current->hasAttribute(SVGNames::spreadMethodAttr)) All of these attribute checks should also go into your new method. It's fine to call them on a SVGGradientElement. https://codereview.chromium.org/135913002/diff/200001/Source/core/svg/SVGRadi... Source/core/svg/SVGRadialGradientElement.cpp:162: processedGradients.add(current); You'll need to do this for the initial "this" element too.
Stephen, thank you for your review. I reply to your comments. https://codereview.chromium.org/135913002/diff/200001/Source/core/svg/SVGRadi... File Source/core/svg/SVGRadialGradientElement.cpp (right): https://codereview.chromium.org/135913002/diff/200001/Source/core/svg/SVGRadi... Source/core/svg/SVGRadialGradientElement.cpp:141: if (!current->renderer()) On 2014/01/16 02:19:52, Stephen Chenney wrote: > This "if" needs to be done for the initial "this" too. Right. https://codereview.chromium.org/135913002/diff/200001/Source/core/svg/SVGRadi... Source/core/svg/SVGRadialGradientElement.cpp:144: if (!attributes.hasSpreadMethod() && current->hasAttribute(SVGNames::spreadMethodAttr)) On 2014/01/16 02:19:52, Stephen Chenney wrote: > All of these attribute checks should also go into your new method. It's fine to > call them on a SVGGradientElement. Yes, it is fine to call them in new method. However, these calls need be called both *radialGradientElement* and *linearGradientElement*. If we move those to new method, those are only called when element is *radialGradientElement* in my patch. It changes existing behavior. I only moved "if (isRadial) { ... }" into new method. That's why I didn't move this into new method. https://codereview.chromium.org/135913002/diff/200001/Source/core/svg/SVGRadi... Source/core/svg/SVGRadialGradientElement.cpp:162: processedGradients.add(current); On 2014/01/16 02:19:52, Stephen Chenney wrote: > You'll need to do this for the initial "this" element too. Is *this* element added by first iteration ? I don't understand well why I need to do this before entering do-while loop.
On 2014/01/16 02:34:01, gyuyoung wrote: > Stephen, thank you for your review. I reply to your comments. > > https://codereview.chromium.org/135913002/diff/200001/Source/core/svg/SVGRadi... > File Source/core/svg/SVGRadialGradientElement.cpp (right): > > https://codereview.chromium.org/135913002/diff/200001/Source/core/svg/SVGRadi... > Source/core/svg/SVGRadialGradientElement.cpp:141: if (!current->renderer()) > On 2014/01/16 02:19:52, Stephen Chenney wrote: > > This "if" needs to be done for the initial "this" too. > > Right. > > https://codereview.chromium.org/135913002/diff/200001/Source/core/svg/SVGRadi... > Source/core/svg/SVGRadialGradientElement.cpp:144: if > (!attributes.hasSpreadMethod() && > current->hasAttribute(SVGNames::spreadMethodAttr)) > On 2014/01/16 02:19:52, Stephen Chenney wrote: > > All of these attribute checks should also go into your new method. It's fine > to > > call them on a SVGGradientElement. > > Yes, it is fine to call them in new method. However, these calls need be called > both *radialGradientElement* and *linearGradientElement*. If we move those to > new method, those are only called when element is *radialGradientElement* in my > patch. It changes existing behavior. I only moved "if (isRadial) { ... }" into > new method. That's why I didn't move this into new method. > > https://codereview.chromium.org/135913002/diff/200001/Source/core/svg/SVGRadi... > Source/core/svg/SVGRadialGradientElement.cpp:162: > processedGradients.add(current); > On 2014/01/16 02:19:52, Stephen Chenney wrote: > > You'll need to do this for the initial "this" element too. > > Is *this* element added by first iteration ? I don't understand well why I need > to do this before entering do-while loop. Anyway, let me modify this patch again.
I fixed this patch again. How do you think ? https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGRadi... File Source/core/svg/SVGRadialGradientElement.cpp (left): https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGRadi... Source/core/svg/SVGRadialGradientElement.cpp:184: processedGradients.add(current); I think this line doesn't need to call out of do-while loop. Do you think this line should be called before entering do-while loop ?
Nearly there. We can make it a little more efficient and it's worth doing it now while you're changing the structure. I only commented on SVGLinear, but the same comments apply to radial. https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... File Source/core/svg/SVGLinearGradientElement.cpp (right): https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... Source/core/svg/SVGLinearGradientElement.cpp:124: return false; Should just be "if (!renderer()) return false;" at the very beginning of the method. https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... Source/core/svg/SVGLinearGradientElement.cpp:128: do { while (true) https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... Source/core/svg/SVGLinearGradientElement.cpp:139: break; return true; https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... Source/core/svg/SVGLinearGradientElement.cpp:147: current = 0; return true; https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... Source/core/svg/SVGLinearGradientElement.cpp:149: } while (current); } ASSERT_NOT_REACHED(); https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... File Source/core/svg/SVGLinearGradientElement.h (right): https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... Source/core/svg/SVGLinearGradientElement.h:47: void setAttributeAnimatedLength(SVGGradientElement*, LinearGradientAttributes&, bool isLinear = true); Rename to setGradientAttributes. and it should be a static method. https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGRadi... File Source/core/svg/SVGRadialGradientElement.cpp (left): https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGRadi... Source/core/svg/SVGRadialGradientElement.cpp:184: processedGradients.add(current); On 2014/01/16 03:34:11, gyuyoung wrote: > I think this line doesn't need to call out of do-while loop. Do you think this > line should be called before entering do-while loop ? I think it needs to appear before entering the loop (for the initial element) and then it only needs to be called inside the if statement, at the end of the block.
https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... File Source/core/svg/SVGLinearGradientElement.cpp (right): https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... Source/core/svg/SVGLinearGradientElement.cpp:124: return false; On 2014/01/16 15:39:34, Stephen Chenney wrote: > Should just be "if (!renderer()) return false;" at the very beginning of the > method. Done. https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... Source/core/svg/SVGLinearGradientElement.cpp:128: do { On 2014/01/16 15:39:34, Stephen Chenney wrote: > while (true) Done. https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... Source/core/svg/SVGLinearGradientElement.cpp:139: break; On 2014/01/16 15:39:34, Stephen Chenney wrote: > return true; In SVGRadialGradientElement, we can't return true because there is a work out of while() loop as below, else { current = 0; break; } } // Handle default values for fx/fy if (!attributes.hasFx()) attributes.setFx(attributes.cx()); if (!attributes.hasFy()) attributes.setFy(attributes.cy()); So, I use "break;" instead of returning "true". How do you think ? https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... Source/core/svg/SVGLinearGradientElement.cpp:147: current = 0; On 2014/01/16 15:39:34, Stephen Chenney wrote: > return true; Done. https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... Source/core/svg/SVGLinearGradientElement.cpp:149: } while (current); On 2014/01/16 15:39:34, Stephen Chenney wrote: > } > > ASSERT_NOT_REACHED(); Done. https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... File Source/core/svg/SVGLinearGradientElement.h (right): https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGLine... Source/core/svg/SVGLinearGradientElement.h:47: void setAttributeAnimatedLength(SVGGradientElement*, LinearGradientAttributes&, bool isLinear = true); On 2014/01/16 15:39:34, Stephen Chenney wrote: > Rename to setGradientAttributes. and it should be a static method. Done. https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGRadi... File Source/core/svg/SVGRadialGradientElement.cpp (left): https://codereview.chromium.org/135913002/diff/260001/Source/core/svg/SVGRadi... Source/core/svg/SVGRadialGradientElement.cpp:184: processedGradients.add(current); On 2014/01/16 15:39:34, Stephen Chenney wrote: > On 2014/01/16 03:34:11, gyuyoung wrote: > > I think this line doesn't need to call out of do-while loop. Do you think this > > line should be called before entering do-while loop ? > > I think it needs to appear before entering the loop (for the initial element) > and then it only needs to be called inside the if statement, at the end of the > block. Done.
LGTM. Thanks for working through it. Let's wait on the try bots before committing.
On 2014/01/17 15:42:07, Stephen Chenney wrote: > LGTM. Thanks for working through it. Let's wait on the try bots before > committing. Thank you for your nice review !
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gyuyoung.kim@samsung.com/135913002/400001
Message was sent while issue was closed.
Change committed as 165358 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
