Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
631233002:
Remove RenderSVGResourceContainer::cast() (Closed)
|
Created:
6 years, 2 months ago by fs Modified:
6 years, 2 months ago Reviewers:
pdr. CC:
blink-reviews, blink-reviews-rendering, zoltan1, rwlbuis, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, kouhei+svg_chromium.org, ed+blinkwatch_opera.com, krit, f(malita), gyuyoung.kim_webkit.org, jchaffraix+rendering, Stephen Chennney, pdr+svgwatchlist_chromium.org, rune+blink Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionRemove RenderSVGResourceContainer::cast()
Fold into getRenderSVGResourceById<Renderer>.
BUG=420022
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183484
Patch Set 1 #
Messages
Total messages: 14 (4 generated)
fs@opera.com changed reviewers: + pdr@chromium.org
Left the a comment in the originating review (https://codereview.chromium.org/630103002/), I suppose this couldbe considered a small improvement...
On 2014/10/07 at 16:25:38, fs wrote: > Left the a comment in the originating review (https://codereview.chromium.org/630103002/), I suppose this couldbe considered a small improvement... I like this, but I'm worried because you don't :P Can you explain your reluctance a bit more?
On 2014/10/07 18:25:09, pdr wrote: > On 2014/10/07 at 16:25:38, fs wrote: > > Left the a comment in the originating review > (https://codereview.chromium.org/630103002/), I suppose this couldbe considered > a small improvement... > > I like this, but I'm worried because you don't :P Can you explain your > reluctance a bit more? Well, it's mostly because you mentioned (or maybe alluded to) the DEFINE_RENDER_SVG_RESOURCE_TYPE_CASTS, but I couldn't really make those ends meet (in a good way). So, to clarify, I like this, but you made me think I might be missing something...
On 2014/10/08 at 08:12:03, fs wrote: > On 2014/10/07 18:25:09, pdr wrote: > > On 2014/10/07 at 16:25:38, fs wrote: > > > Left the a comment in the originating review > > (https://codereview.chromium.org/630103002/), I suppose this couldbe considered > > a small improvement... > > > > I like this, but I'm worried because you don't :P Can you explain your > > reluctance a bit more? > > Well, it's mostly because you mentioned (or maybe alluded to) the DEFINE_RENDER_SVG_RESOURCE_TYPE_CASTS, but I couldn't really make those ends meet (in a good way). So, to clarify, I like this, but you made me think I might be missing something... Haha :) So you don't think you're the one that's crazy: I was thinking of doing this with DEFINE_RENDER_SVG_RESOURCE_TYPE_CASTS but this is a nice as well. Off to the CQ!
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/631233002/1
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2014/10/09 02:23:10, pdr wrote: > On 2014/10/08 at 08:12:03, fs wrote: > > On 2014/10/07 18:25:09, pdr wrote: > > > On 2014/10/07 at 16:25:38, fs wrote: > > > > Left the a comment in the originating review > > > (https://codereview.chromium.org/630103002/), I suppose this couldbe > considered > > > a small improvement... > > > > > > I like this, but I'm worried because you don't :P Can you explain your > > > reluctance a bit more? > > > > Well, it's mostly because you mentioned (or maybe alluded to) the > DEFINE_RENDER_SVG_RESOURCE_TYPE_CASTS, but I couldn't really make those ends > meet (in a good way). So, to clarify, I like this, but you made me think I might > be missing something... > > Haha :) > > So you don't think you're the one that's crazy: I was thinking of doing this > with DEFINE_RENDER_SVG_RESOURCE_TYPE_CASTS but this is a nice as well. Off to > the CQ! Magic acronym missing =)
On 2014/10/09 at 06:43:45, fs wrote: > On 2014/10/09 02:23:10, pdr wrote: > > On 2014/10/08 at 08:12:03, fs wrote: > > > On 2014/10/07 18:25:09, pdr wrote: > > > > On 2014/10/07 at 16:25:38, fs wrote: > > > > > Left the a comment in the originating review > > > > (https://codereview.chromium.org/630103002/), I suppose this couldbe > > considered > > > > a small improvement... > > > > > > > > I like this, but I'm worried because you don't :P Can you explain your > > > > reluctance a bit more? > > > > > > Well, it's mostly because you mentioned (or maybe alluded to) the > > DEFINE_RENDER_SVG_RESOURCE_TYPE_CASTS, but I couldn't really make those ends > > meet (in a good way). So, to clarify, I like this, but you made me think I might > > be missing something... > > > > Haha :) > > > > So you don't think you're the one that's crazy: I was thinking of doing this > > with DEFINE_RENDER_SVG_RESOURCE_TYPE_CASTS but this is a nice as well. Off to > > the CQ! > > Magic acronym missing =) LGTM
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/631233002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1) as 183484 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
