Make RenderSVGResource::requestPaintingResource an implementation detail

Make RenderSVGResource::requestPaintingResource an implementation detail

This allows "simple color" and resource-based paint-servers to be
separated, and clears the path for removing RenderSVGResource from the
inheritance chain.
Rename requestPaintingResource to requestPaint, because it no longer
return (only) a 'resource'.
RenderSVGResource::sharedSolidPaintingResource becomes unused - as well
as RenderSVGResourceSolidColor, so remove those.

BUG=420022
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184094

[fs, 2014-10-17 18:10:54]

fs@opera.com changed reviewers:
+ fmalita@chromium.org, pdr@chromium.org, schenney@chromium.org

[fs, 2014-10-17 18:10:55]

Potential first step on the path of removing RenderSVGResource.

Approximate follow-up steps:

* Move markForLayoutAndParentResourceInvalidation somewhere else
(RenderSVGResourceContainer?)

* Rename RenderSVGResource and interpose it between RenderSVGResourceContainer
and paint-server resource containers.

[fs, 2014-10-20 13:18:21]

On 2014/10/17 18:10:55, fs wrote:
> Potential first step on the path of removing RenderSVGResource.
> 
> Approximate follow-up steps:
> 
> * Move markForLayoutAndParentResourceInvalidation somewhere else
> (RenderSVGResourceContainer?)
> 
> * Rename RenderSVGResource and interpose it between RenderSVGResourceContainer
> and paint-server resource containers.

Ping!?

[f(malita), 2014-10-20 14:51:07]

https://codereview.chromium.org/658333003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResource.h (right):

https://codereview.chromium.org/658333003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResource.h:83: struct PaintDescription {
This smells close enough to a paint server to make me wonder why we're not just
building/returning one of those instead.

I think {fill,stroke}Contains() are the only users not turning this into a paint
server immediately, no? Not sure whether returning a fully resolved server is
going to show a perf impact there, but if it does we could add a simplified
hasValidPaintServer() for them.

Is this feasible?

https://codereview.chromium.org/658333003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGResources.cpp (left):

https://codereview.chromium.org/658333003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGResources.cpp:443: case SolidColorResourceType:
let's switch this to a default case and leave the assert.

https://codereview.chromium.org/658333003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGResourcesCycleSolver.cpp (left):

https://codereview.chromium.org/658333003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGResourcesCycleSolver.cpp:168: case
SolidColorResourceType:
ditto.

[fs, 2014-10-20 15:20:21]

https://codereview.chromium.org/658333003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResource.h (right):

https://codereview.chromium.org/658333003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResource.h:83: struct PaintDescription {
On 2014/10/20 14:51:06, Florin Malita wrote:
> This smells close enough to a paint server to make me wonder why we're not
just
> building/returning one of those instead.
> 
> I think {fill,stroke}Contains() are the only users not turning this into a
paint
> server immediately, no? Not sure whether returning a fully resolved server is
> going to show a perf impact there, but if it does we could add a simplified
> hasValidPaintServer() for them.

It's the case you mention and the DRT code. For DRT, the only missing piece is
the id off the element. It's quite possible we could just pass that from the
SVGRenderStyle (since it should be validated already.)

As for hasValidPaintServer, I had that in a previous CL, but ended up
disappearing it... It should be unproblematic to reappear it though. I'll try it
out.

[fs, 2014-10-20 15:52:25]

https://codereview.chromium.org/658333003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResource.h (right):

https://codereview.chromium.org/658333003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResource.h:83: struct PaintDescription {
On 2014/10/20 15:20:20, fs wrote:
> On 2014/10/20 14:51:06, Florin Malita wrote:
> > This smells close enough to a paint server to make me wonder why we're not
> just
> > building/returning one of those instead.
> > 
> > I think {fill,stroke}Contains() are the only users not turning this into a
> paint
> > server immediately, no? Not sure whether returning a fully resolved server
is
> > going to show a perf impact there, but if it does we could add a simplified
> > hasValidPaintServer() for them.
> 
> It's the case you mention and the DRT code. For DRT, the only missing piece is
> the id off the element. It's quite possible we could just pass that from the
> SVGRenderStyle (since it should be validated already.)

Oh, and it'll not be possible to tell a fallback color from a regular color.
That doesn't seem like a big issue though (and I'll let the try-bots figure that
out...)

[fs, 2014-10-20 16:03:04]

Ok, minor re-org done. The extra struct us still there, but it no longer visible
in the interface. There could be some LayoutTests fall-out (fallback color vs.
"regular" color), but hopefully minor (haven't check yet).

[f(malita), 2014-10-20 16:37:39]

On 2014/10/20 16:03:04, fs wrote:
> Ok, minor re-org done. The extra struct us still there, but it no longer
visible
> in the interface. There could be some LayoutTests fall-out (fallback color vs.
> "regular" color), but hopefully minor (haven't check yet).

Looks good, assuming we get layout tests under control (mostly id formatting
noise AFAICT, although I do see a few text expectations switching from gradient
to solid color which is troubling).

[fs, 2014-10-20 16:45:24]

On 2014/10/20 16:37:39, Florin Malita wrote:
> On 2014/10/20 16:03:04, fs wrote:
> > Ok, minor re-org done. The extra struct us still there, but it no longer
> visible
> > in the interface. There could be some LayoutTests fall-out (fallback color
vs.
> > "regular" color), but hopefully minor (haven't check yet).
> 
> Looks good, assuming we get layout tests under control (mostly id formatting
> noise AFAICT, although I do see a few text expectations switching from
gradient
> to solid color which is troubling).

Yes, hopefully the noise has been reduced with PS4 - the "gradient" -> "solid
color" (and similar) will be because it'll now get the actual (used)
paint-server. (Cases where preparePaintServer return  SVGPaintServer::invalid()
pretty much.)

[fs, 2014-10-20 19:26:29]

On 2014/10/20 16:45:24, fs wrote:
> On 2014/10/20 16:37:39, Florin Malita wrote:
> > On 2014/10/20 16:03:04, fs wrote:
> > > Ok, minor re-org done. The extra struct us still there, but it no longer
> > visible
> > > in the interface. There could be some LayoutTests fall-out (fallback color
> vs.
> > > "regular" color), but hopefully minor (haven't check yet).
> > 
> > Looks good, assuming we get layout tests under control (mostly id formatting
> > noise AFAICT, although I do see a few text expectations switching from
> gradient
> > to solid color which is troubling).
> 
> Yes, hopefully the noise has been reduced with PS4 - the "gradient" -> "solid
> color" (and similar) will be because it'll now get the actual (used)
> paint-server. (Cases where preparePaintServer return 
SVGPaintServer::invalid()
> pretty much.)

svg/W3C-SVG-1.1-SE/pservers-grad-17-b.svg
svg/W3C-SVG-1.1-SE/pservers-grad-20-b.svg
svg/W3C-SVG-1.1/pservers-grad-17-b.svg
svg/custom/gradient-with-1d-boundingbox.svg

  Empty bboxes => fallback color

svg/custom/dominant-baseline-hanging.svg

  Empty bbox, no explicit fallback => none

svg/W3C-SVG-1.1-SE/pservers-pattern-03-f.svg

  Empty (zero dimensions) patterns => fallback color

svg/custom/resource-invalidation-crash.svg

  Empty (zero dimensions) patterns, no explicit fallback => none

svg/custom/nested-pattern-boundingBoxModeContent.svg
svg/custom/resources-css-scaled.html

  In these it looks like DRT ends up mutating the (cached) paint-server or
something...

[fs, 2014-10-21 11:41:54]

On 2014/10/20 19:26:29, fs wrote:
> On 2014/10/20 16:45:24, fs wrote:
> > On 2014/10/20 16:37:39, Florin Malita wrote:
> > > On 2014/10/20 16:03:04, fs wrote:
> > > > Ok, minor re-org done. The extra struct us still there, but it no longer
> > > visible
> > > > in the interface. There could be some LayoutTests fall-out (fallback
color
> > vs.
> > > > "regular" color), but hopefully minor (haven't check yet).
> > > 
> > > Looks good, assuming we get layout tests under control (mostly id
formatting
> > > noise AFAICT, although I do see a few text expectations switching from
> > gradient
> > > to solid color which is troubling).
> > 
> > Yes, hopefully the noise has been reduced with PS4 - the "gradient" ->
"solid
> > color" (and similar) will be because it'll now get the actual (used)
> > paint-server. (Cases where preparePaintServer return 
> SVGPaintServer::invalid()
> > pretty much.)

I think we need to decide on what's acceptable or not here.

> svg/W3C-SVG-1.1-SE/pservers-grad-17-b.svg
> svg/W3C-SVG-1.1-SE/pservers-grad-20-b.svg
> svg/W3C-SVG-1.1/pservers-grad-17-b.svg
> svg/custom/gradient-with-1d-boundingbox.svg
> 
>   Empty bboxes => fallback color
> 
> svg/custom/dominant-baseline-hanging.svg
> 
>   Empty bbox, no explicit fallback => none
> 
> svg/W3C-SVG-1.1-SE/pservers-pattern-03-f.svg
> 
>   Empty (zero dimensions) patterns => fallback color
> 
> svg/custom/resource-invalidation-crash.svg
> 
>   Empty (zero dimensions) patterns, no explicit fallback => none

All the above stem from the same behavioral change - DRT will now see the
actual/resolved/fallback evaluated paint-server. If that's not OK, then I can't
really think of a better way forward than rolling back PS2 (and add 'case ...'
-> 'default'). (Otherwise there'll still be either a "special" interface for
DRT, and also see below.)

> svg/custom/nested-pattern-boundingBoxModeContent.svg
> svg/custom/resources-css-scaled.html
> 
>   In these it looks like DRT ends up mutating the (cached) paint-server or
> something...

Caused by the context sensitiveness of RenderSVGResourcePattern
(calculateDeviceSpaceTransformation/related). Something like
https://codereview.chromium.org/453653003/ would fix this, but it seems this has
stalled. I suppose we could do a hybrid - i.e. cache display-list (fewer) and
generate (and cache) ImageBuffers (per client and context-scale). Seems a lot of
work for a unified API towards DRT though =). I'd be willing to give it a shot
though, if you think it's worthwhile.

So, I'm seeing the following options:

1) PS2 + minor fixups (=> no DRT changes/rebaselines required)
2) Rewritten https://codereview.chromium.org/453653003/ + PS4:ish (=>
rebaselines needed perm above)
A) Abort
I) Ignore
R) Retry

Your choice? (1/2/A/I/R)

[f(malita), 2014-10-21 12:27:33]

On 2014/10/21 11:41:54, fs wrote:
> On 2014/10/20 19:26:29, fs wrote:
> > On 2014/10/20 16:45:24, fs wrote:
> > > On 2014/10/20 16:37:39, Florin Malita wrote:
> > > > On 2014/10/20 16:03:04, fs wrote:
> > > > > Ok, minor re-org done. The extra struct us still there, but it no
longer
> > > > visible
> > > > > in the interface. There could be some LayoutTests fall-out (fallback
> color
> > > vs.
> > > > > "regular" color), but hopefully minor (haven't check yet).
> > > > 
> > > > Looks good, assuming we get layout tests under control (mostly id
> formatting
> > > > noise AFAICT, although I do see a few text expectations switching from
> > > gradient
> > > > to solid color which is troubling).
> > > 
> > > Yes, hopefully the noise has been reduced with PS4 - the "gradient" ->
> "solid
> > > color" (and similar) will be because it'll now get the actual (used)
> > > paint-server. (Cases where preparePaintServer return 
> > SVGPaintServer::invalid()
> > > pretty much.)
> 
> I think we need to decide on what's acceptable or not here.
> 
> > svg/W3C-SVG-1.1-SE/pservers-grad-17-b.svg
> > svg/W3C-SVG-1.1-SE/pservers-grad-20-b.svg
> > svg/W3C-SVG-1.1/pservers-grad-17-b.svg
> > svg/custom/gradient-with-1d-boundingbox.svg
> > 
> >   Empty bboxes => fallback color
> > 
> > svg/custom/dominant-baseline-hanging.svg
> > 
> >   Empty bbox, no explicit fallback => none
> > 
> > svg/W3C-SVG-1.1-SE/pservers-pattern-03-f.svg
> > 
> >   Empty (zero dimensions) patterns => fallback color
> > 
> > svg/custom/resource-invalidation-crash.svg
> > 
> >   Empty (zero dimensions) patterns, no explicit fallback => none
> 
> All the above stem from the same behavioral change - DRT will now see the
> actual/resolved/fallback evaluated paint-server. If that's not OK, then I
can't
> really think of a better way forward than rolling back PS2 (and add 'case ...'
> -> 'default'). (Otherwise there'll still be either a "special" interface for
> DRT, and also see below.)
> 
> > svg/custom/nested-pattern-boundingBoxModeContent.svg
> > svg/custom/resources-css-scaled.html
> > 
> >   In these it looks like DRT ends up mutating the (cached) paint-server or
> > something...
> 
> Caused by the context sensitiveness of RenderSVGResourcePattern
> (calculateDeviceSpaceTransformation/related). Something like
> https://codereview.chromium.org/453653003/ would fix this, but it seems this
has
> stalled. I suppose we could do a hybrid - i.e. cache display-list (fewer) and
> generate (and cache) ImageBuffers (per client and context-scale). Seems a lot
of
> work for a unified API towards DRT though =). I'd be willing to give it a shot
> though, if you think it's worthwhile.
> 
> So, I'm seeing the following options:
> 
> 1) PS2 + minor fixups (=> no DRT changes/rebaselines required)
> 2) Rewritten https://codereview.chromium.org/453653003/ + PS4:ish (=>
> rebaselines needed perm above)
> A) Abort
> I) Ignore
> R) Retry
> 
> Your choice? (1/2/A/I/R)


Thanks for trying these out!

I was actually just taking another look at
https://codereview.chromium.org/453653003, so #2 may soon be an option. For now
#1 should do - LGTM.

[fs, 2014-10-21 13:22:15]

On 2014/10/21 12:27:33, Florin Malita wrote:
> On 2014/10/21 11:41:54, fs wrote:
> > On 2014/10/20 19:26:29, fs wrote:
> > > On 2014/10/20 16:45:24, fs wrote:
> > > > On 2014/10/20 16:37:39, Florin Malita wrote:
> > > > > On 2014/10/20 16:03:04, fs wrote:
> > > > > > Ok, minor re-org done. The extra struct us still there, but it no
> longer
> > > > > visible
> > > > > > in the interface. There could be some LayoutTests fall-out (fallback
> > color
> > > > vs.
> > > > > > "regular" color), but hopefully minor (haven't check yet).
> > > > > 
> > > > > Looks good, assuming we get layout tests under control (mostly id
> > formatting
> > > > > noise AFAICT, although I do see a few text expectations switching from
> > > > gradient
> > > > > to solid color which is troubling).
> > > > 
> > > > Yes, hopefully the noise has been reduced with PS4 - the "gradient" ->
> > "solid
> > > > color" (and similar) will be because it'll now get the actual (used)
> > > > paint-server. (Cases where preparePaintServer return 
> > > SVGPaintServer::invalid()
> > > > pretty much.)
> > 
> > I think we need to decide on what's acceptable or not here.
> > 
> > > svg/W3C-SVG-1.1-SE/pservers-grad-17-b.svg
> > > svg/W3C-SVG-1.1-SE/pservers-grad-20-b.svg
> > > svg/W3C-SVG-1.1/pservers-grad-17-b.svg
> > > svg/custom/gradient-with-1d-boundingbox.svg
> > > 
> > >   Empty bboxes => fallback color
> > > 
> > > svg/custom/dominant-baseline-hanging.svg
> > > 
> > >   Empty bbox, no explicit fallback => none
> > > 
> > > svg/W3C-SVG-1.1-SE/pservers-pattern-03-f.svg
> > > 
> > >   Empty (zero dimensions) patterns => fallback color
> > > 
> > > svg/custom/resource-invalidation-crash.svg
> > > 
> > >   Empty (zero dimensions) patterns, no explicit fallback => none
> > 
> > All the above stem from the same behavioral change - DRT will now see the
> > actual/resolved/fallback evaluated paint-server. If that's not OK, then I
> can't
> > really think of a better way forward than rolling back PS2 (and add 'case
...'
> > -> 'default'). (Otherwise there'll still be either a "special" interface for
> > DRT, and also see below.)
> > 
> > > svg/custom/nested-pattern-boundingBoxModeContent.svg
> > > svg/custom/resources-css-scaled.html
> > > 
> > >   In these it looks like DRT ends up mutating the (cached) paint-server or
> > > something...
> > 
> > Caused by the context sensitiveness of RenderSVGResourcePattern
> > (calculateDeviceSpaceTransformation/related). Something like
> > https://codereview.chromium.org/453653003/ would fix this, but it seems this
> has
> > stalled. I suppose we could do a hybrid - i.e. cache display-list (fewer)
and
> > generate (and cache) ImageBuffers (per client and context-scale). Seems a
lot
> of
> > work for a unified API towards DRT though =). I'd be willing to give it a
shot
> > though, if you think it's worthwhile.
> > 
> > So, I'm seeing the following options:
> > 
> > 1) PS2 + minor fixups (=> no DRT changes/rebaselines required)
> > 2) Rewritten https://codereview.chromium.org/453653003/ + PS4:ish (=>
> > rebaselines needed perm above)
> > A) Abort
> > I) Ignore
> > R) Retry
> > 
> > Your choice? (1/2/A/I/R)
> 
> 
> Thanks for trying these out!
> 
> I was actually just taking another look at
> https://codereview.chromium.org/453653003,

Excellent! =)

> so #2 may soon be an option. For now
> #1 should do - LGTM.

Reverted to PS2, and then tried to keep as much of the "hiding" as possible (and
indicating that SVGPaintDescription shouldn't be used for anything but DRT). Oh,
and renamed PaintDescription and moved it to into blink:: (realized that's what
I wanted all along when working on follow-ups.)

A new look could be warranted.

[f(malita), 2014-10-21 13:57:36]

re-lgtm

https://codereview.chromium.org/658333003/diff/80001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResource.h (right):

https://codereview.chromium.org/658333003/diff/80001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResource.h:96: static SVGPaintDescription
requestPaintForDRT(const RenderObject&, const RenderStyle*,
RenderSVGResourceMode);
nit: I would prefer not to have DRT-specific method names here (even if DRT is
the only client) - the comment should suffice. I'll leave it up to you though.

[fs, 2014-10-21 14:08:23]

On 2014/10/21 13:57:36, Florin Malita wrote:
> re-lgtm

Thanks.
 
>
https://codereview.chromium.org/658333003/diff/80001/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGResource.h (right):
> 
>
https://codereview.chromium.org/658333003/diff/80001/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGResource.h:96: static SVGPaintDescription
> requestPaintForDRT(const RenderObject&, const RenderStyle*,
> RenderSVGResourceMode);
> nit: I would prefer not to have DRT-specific method names here (even if DRT is
> the only client) - the comment should suffice. I'll leave it up to you though.

Renamed it to requestPaintDescription.

[fs, 2014-10-21 15:39:34]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-10-21 15:40:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/658333003/100001

[commit-bot: I haz the power, 2014-10-21 16:00:56]

Committed patchset #6 (id:100001) as 184094