Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
144463009:
Refactor SVGPreserveAspectRatio::parseInternal() (Closed)
|
Created:
6 years, 10 months ago by gyuyoung-inactive Modified:
6 years, 10 months ago Reviewers:
Stephen Chennney CC:
blink-reviews, krit, ed+blinkwatch_opera.com, f(malita), gyuyoung.kim_webkit.org, Stephen Chennney, rwlbuis, pdr. Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionRefactor SVGPreserveAspectRatio::parseInternal()
This cl removes "goto" and replaces it with local variables, and fixes poor coding style.
R=schenney
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166927
Patch Set 1 : Work in progress #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 1
Messages
Total messages: 19 (0 generated)
It seems SVGPreserveAspectRatio::parseInternal() needs to be done refactor. This is initial patch for it. Next patch is going to be uploaded.
On 2014/02/09 05:36:11, gyuyoung wrote: > It seems SVGPreserveAspectRatio::parseInternal() needs to be done refactor. This > is initial patch for it. Next patch is going to be uploaded. Stephen, what do you think this patch ?
On 2014/02/09 15:33:43, gyuyoung wrote: > On 2014/02/09 05:36:11, gyuyoung wrote: > > It seems SVGPreserveAspectRatio::parseInternal() needs to be done refactor. > This > > is initial patch for it. Next patch is going to be uploaded. > > Stephen, what do you think this patch ? Not Stephen, but... Maybe you could keep the parse-results in local variables instead, and then "commit" just before returning true at the end - then the 'return bailOut...()' thing can just be 'return false' (given that the defaults are setup at the start - which they are already for setValueAsString - so maybe just make sure all callers of parseInternal does that). What do you (and Stephen) think?
On 2014/02/10 09:25:26, fs wrote: > On 2014/02/09 15:33:43, gyuyoung wrote: > > On 2014/02/09 05:36:11, gyuyoung wrote: > > > It seems SVGPreserveAspectRatio::parseInternal() needs to be done refactor. > > This > > > is initial patch for it. Next patch is going to be uploaded. > > > > Stephen, what do you think this patch ? > > Not Stephen, but... Maybe you could keep the parse-results in local variables > instead, and then "commit" just before returning true at the end - then the > 'return bailOut...()' thing can just be 'return false' (given that the defaults > are setup at the start - which they are already for setValueAsString - so maybe > just make sure all callers of parseInternal does that). > > What do you (and Stephen) think? Looks better idea. Let me try it !
On 2014/02/10 11:36:23, gyuyoung wrote: > On 2014/02/10 09:25:26, fs wrote: > > On 2014/02/09 15:33:43, gyuyoung wrote: > > > On 2014/02/09 05:36:11, gyuyoung wrote: > > > > It seems SVGPreserveAspectRatio::parseInternal() needs to be done > refactor. > > > This > > > > is initial patch for it. Next patch is going to be uploaded. > > > > > > Stephen, what do you think this patch ? > > > > Not Stephen, but... Maybe you could keep the parse-results in local variables > > instead, and then "commit" just before returning true at the end - then the > > 'return bailOut...()' thing can just be 'return false' (given that the > defaults > > are setup at the start - which they are already for setValueAsString - so > maybe > > just make sure all callers of parseInternal does that). > > > > What do you (and Stephen) think? > > Looks better idea. Let me try it ! fs, patch is updated ! How do you think ?
LGTM. The initial setXX may be unnecessary but it is also safer, so I think leave it in. I also updated the CL description to better reflect what's happening.
The CQ bit was checked by gyuyoung.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gyuyoung.kim@samsung.com/144463009/120001
On 2014/02/10 15:20:47, Stephen Chenney wrote: > LGTM. The initial setXX may be unnecessary but it is also safer, so I think > leave it in. > > I also updated the CL description to better reflect what's happening. Thank you for updating the CL description !
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
On 2014/02/10 17:04:12, I haz the power (commit-bot) wrote: > Retried try job too often on linux_blink_rel for step(s) webkit_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin... Let me fix the fail test. Then, try to land again.
The CQ bit was checked by gyuyoung.kim@samsung.com
The CQ bit was unchecked by gyuyoung.kim@samsung.com
https://codereview.chromium.org/144463009/diff/340001/Source/core/svg/SVGPres... File Source/core/svg/SVGPreserveAspectRatio.cpp (right): https://codereview.chromium.org/144463009/diff/340001/Source/core/svg/SVGPres... Source/core/svg/SVGPreserveAspectRatio.cpp:162: if (align != SVG_PRESERVEASPECTRATIO_NONE) This line caused the failure of test. This cl keep align's status, and update it at the end of this function. So, we need to compare this codition using *align* local variable.
The CQ bit was checked by gyuyoung.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gyuyoung.kim@samsung.com/144463009/340001
Message was sent while issue was closed.
Change committed as 166927 |
