Bug 4472 – Resource (paint server) information is not included in KCanvas render tree dumps

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

Resource (paint server) information is not included in KCanvas render tree dumps

for example:

 KCanvasItem {rect} at (274,109) size 101x141 [stroke={server=UNIMPLEMENTED}] [fill=
{server=UNIMPLEMENTED}] [data="M275.00,110.00L375.00,110.00L375.00,250.00L275.00,250.00"]

This will require being able to do a reverse lookup of canvas resources from their pointer value, as well as 
adding dumping of resources to the top of the dump file.  The two changes are not necessarily tied.

[Kimmo Kinnunen]

Created attachment 3531 [details]
Patch to dump KCanvasRegistry info on KRenderingPaintServers

This patch makes KCanvasTreeDebug dump more info about KRenderingPaintServer
instances found from KCanvasRegistry. KCanvasResources are not dumped because
there is not currently type() method for them, and base class doesn't have any
info to be dumped.

paint server pointer -> string id in registry mapping is not implemented,
rather registry that owns the paint server will set the id for the server. If
reverse mapping is really wanted instead of this, this needs to be changed.
If the current approach of saving QString id for each server/resource is ok,
then perhaps it'd be good to have KCanvasRegistryItem base class, which would
implement setIdInRegistry() and idInRegistry(). Each class that is to be stored
in registry would then implement that interface..
Looking for comments..

[Eric Seidel (no email)]

The patch looks fine.

Gradient stop lists should be trivial to implement, lets land that at the same time.

It also bothers me a little that the << operators for paint servers need to do so much static casting... I 
guess there are advantages to having this all in one file, but part of me feels like it would have been easier 
to have made the << operator virtual on KCanvasPaintServer and implement it for each server... doing the 
same for resources would make it possible to print them w/o a type() call.

Overalll, great work!

[Eric Seidel (no email)]

One thing I forgot, is that you should also attach a patch containing all of the layout-test updates.

[Kimmo Kinnunen]

Created attachment 3550 [details]
Alternative way of dumping paint servers and canvas resources

This is alternative approach for dumping the canvas registry. 
Every class that can somehow be navigated to from canvas registry has virtual
method externalRepresentation. By overloading it, you can dump the properties
of the objects in the tree.
In addition to that, every base class or type has related QTextStream
&operator<<(QTextStream  &, const Class &) -operator, which is actually used in
dumping code. 

Which is the preferred approach, this or the previous? This patch is not final,
as it lacks filter effect dumping and updated layout tests.

the result is something like following snipet. However, filter effect dumping
is not implemented fully, so  effects are only partial.
--

KCanvasRegistry 
 KCanvasResource {id=MyFilter[type=FILTER] at (0,0) size 200x120
[filterBoundingBoxMode=0] [effectBoundingBoxMode=0] [effects=[[in=SourceAlpha]
[result=blur] [subRegion=at (0,0) size 0x0], [in=blur] [result=offsetBlur]
[subRegion=at (0,0) size 0x0], [in=SourceGraphic] [result=] [subRegion=at (0,0)
size 0x0]]]}
KCanvasContainer at (0,0) size 479x359
  KCanvasContainer at (70,110) size 320x178 [stroke={server=0}]
[fill={server=}]
    KCanvasContainer at (70,110) size 320x178
[transform={m=((1.500000,0.000000)(0.000000,1.500000))
t=(80.000000,110.000000)}]
      KCanvasItem {rect} at (80,110) size 298x178 [stroke={server=}]
[fill={server=}] [data="M1.00,1.00L199.00,1.00L199.00,119.00L1.00,119.00"]
      KCanvasContainer at (70,147) size 320x105 [stroke={server=0}]
[fill={server=}] [filter=MyFilter]
	KCanvasContainer at (70,147) size 320x105 [stroke={server=0}]
[fill={server=}]
	  KCanvasItem {path} at (70,147) size 320x105 [stroke={server=, stroke
width=10.000000}] [fill={server=0}]
[data="M50.00,90.00C0.00,90.00,0.00,30.00,50.00,30.00L150.00,30.00C200.00,30.00,200.00,90.00,150.00,90.00"]

	  KCanvasItem {path} at (124,169) size 212x61 [stroke={server=0}]
[fill={server=}]
[data="M60.00,80.00C30.00,80.00,30.00,40.00,60.00,40.00L140.00,40.00C170.00,40.00,170.00,80.00,140.00,80.00"]

	  KCanvasContainer at (0,0) size 0x0 [stroke={server=}]
[fill={server=}]
  KCanvasItem {rect} at (0,0) size 479x359 [stroke={server=}] [fill={server=0}]
[data="M1.00,1.00L479.00,1.00L479.00,359.00L1.00,359.00"]

[Eric Seidel (no email)]

The second patch looks OK.  I'm not sure why you have both the externalRepresentation function *and* 
the << operator... why not just a virtual << operator on every subclass?  In the case of sub-classes where 
you already have a type() operator, you could go so far as to make << non-virtual, and then do  your own 
static_cast...  but that sounds a bit like over-kill.

[Kimmo Kinnunen]

(In reply to comment #5)   
> The second patch looks OK.  I'm not sure why you have both the   
externalRepresentation function *and*    
> the << operator... why not just a virtual << operator on every subclass?  In   
the case of sub-classes where    
> you already have a type() operator, you could go so far as to make <<   
non-virtual, and then do  your own    
> static_cast...  but that sounds a bit like over-kill.   
 
To my understanding stream operators in a class affect the class they're 
defined in, not the stream. So you need to have your instance of the class in 
the left hand side, ie "resource << stream", which doesn't make sense. "stream 
<< resource" affects the stream, and the operator cannot be defined in the 
class of resource -object. 
 
In other words, "stream << resource" calls Stream::operator<<(const Resource&) 
or operator<<(Stream&, const Resource&), but to my understanding you cannot 
make it call Resource::operator<<(Stream&), because it means different thing. 
 
   

[Kimmo Kinnunen]

Created attachment 3583 [details]
Dump KRenderingPaintServers and KCanvasResources and their properties

The patch dump KRenderingPaintServers and KCanvasResources and their properties
when using externalRepresentation()

[Kimmo Kinnunen]

Created attachment 3584 [details]
Layout-test diffs for the previous attachment

SVGSupport/layout-test diffs that are result of the previous patch (3583)

[Eric Seidel (no email)]

Looks good.  We discussed a few more issues over IRC:

1.  there were several "server=," that seems wrong.  Just get rid of "server=" all together.
2.  "server=0" should just be "none"
3.  PaintServer identifiers should aways be in quotes, both when listed in the registry, and when 
referenced  .  e.g.  fill="grad"
4.  While you're in there, can you add some setprecision(2) calls when printing things like width, etc?
5.  It would be nice to special case some KCanvasMatricies, like Identity, and print as such. (the more 
human readible the better).
6.  Also, when the registry is empty, print "KCanvasRegistry: empty" or some other indicator.

[Eric Seidel (no email)]

Hum... furthermore, this doesn't look quite right:

+ KCanvasResource {id=arithmetic50[type=FILTER] at (0,0) size 100x100 [filterBoundingBoxMode=1] 
[effectBoundingBoxMode=0] [effects=[[type=IMAGE] [in=] [result=blue] [subRegion=at (0,0) size 0x0], 
[type=IMAGE] [in=] [result=red] [subRegion=at (0,0) size 0x0], [type=COMPOSITE] [in=red] [result=] 
[subRegion=at (0,0) size 0x0] [in2=blue] [k1=0.000000 k2=0.000000 k3=0.000000 k4=0.000000]]]}

1.  there should be a space between the id (in quotes!) and the type.
2.  Why a location?  the bounding box is valid, I think...
3.  No need to print default values... decide which ones are default (bbox mode on? etc?) and don't print 
those.
4.  don't print empty values... like "in" or subregion...
5.  result should be in quotes result="blue"

Each filter effect shoudl probably have it's own line.  Print the filter as though it were a container, with 
each effect indented and on the lines below.

[Eric Seidel (no email)]

Comment on attachment 3583 [details]
Dump KRenderingPaintServers and KCanvasResources and their properties

This is a really great start, but we need a few more tweaks before we land. 
See my copious comments in the bug.

[Kimmo Kinnunen]

Created attachment 3636 [details]
Dump KRenderingPaintServers and KCanvasResources and their properties (2nd)

Patch which tries to  address previous comments. 
Added some checks to not dump default valued properties. However, I added those
checks only for the properties I knew the default value - it's hard to know it
because a) there's no initialization on construction b) there's no
documentation. Maybe somebody who knows better could add more..

Didn't add indentation, because it would need that the dumping context (ie. the
value of the indentation level) would need to be transferred to each dump
function. This would ofcourse be most naturally done with specific
DebugTreeStream, but I didn't want to make this overengineered / deviate from
webcore.. 

Adds QTextStream::precision() to KWQ/KWQTextStream.h/mm .. I don't know how
welcome this is..

[Kimmo Kinnunen]

Created attachment 3637 [details]
 Layout-test diffs for the previous attachment (2nd)

Layout-tests as result of 3636

[Eric Seidel (no email)]

Comment on attachment 3636 [details]
Dump KRenderingPaintServers and KCanvasResources and their properties (2nd)

Still could use a couple more tweaks we discussed over IRC.  Otherwise looks
good.

[Kimmo Kinnunen]

Created attachment 3638 [details]
Dump KRenderingPaintServers and KCanvasResources and their properties (3rd)

Another try at the resource dumping. removed server=, simplified
QTextStream::precision()
(whitespace diffs are still there..)

[Kimmo Kinnunen]

Created attachment 3639 [details]
Dump KRenderingPaintServers and KCanvasResources and their properties (4h)

Fixed few typos..

[Kimmo Kinnunen]

Created attachment 3640 [details]
Layout tests for 3639

layout tests that are result of 3639.

[Tobias Lidskog]

I think the following two lines in KCanvasTreeDebug.cpp will need to change from
if (o.style()->isStroked() && DIFFERS_FROM_PARENT(style()->strokePainter()) && o.style()-
>strokePainter()) [line 178]
if (o.style()->isFilled() && DIFFERS_FROM_PARENT(style()->fillPainter()) && o.style()->fillPainter()) [line 
180]
to
if (o.style()->isStroked() && DIFFERS_FROM_PARENT(style()->isStroked()) && DIFFERS_FROM_PARENT
(style()->strokePainter()) && o.style()->strokePainter())
if (o.style()->isFilled() && DIFFERS_FROM_PARENT(style()->isFilled()) && DIFFERS_FROM_PARENT(style()-
>fillPainter()) && o.style()->fillPainter())

This because, in the current implementation of KCanvasRenderingStyle, strokePainter() and fillPainter() 
will create a new painter if none already exists. Calling o.parent->style()->strokePainter() (which is 
what the macro will do) could therefor alter the state of the parent, an unwanted side effect.

[Tobias Lidskog]

(In reply to comment #18)
Come to think of it, my proposed change wont do. The logic will need to be something like:
if (o.style()->isStroked())
    if (!o.parent->isStroked() || DIFFERS_FROM_PARENT(style()->strokePainter()))
        ....

[Kimmo Kinnunen]

Created attachment 3669 [details]
DUMP KRenderingPaintServers and KCanvasResources and their properties (5th)

Added macro to test a predicate before using accessor member function which has
side-effects to parent.

Included marker dumping, they were accidentally printed as ptrs.

[Kimmo Kinnunen]

Created attachment 3670 [details]
layout-tests for 3669

Layout-tests for 3669. I hope I don't fill this bugzilla too much with the
patch-spam..

[Eric Seidel (no email)]

Comment on attachment 3669 [details]
DUMP KRenderingPaintServers and KCanvasResources and their properties (5th)

looks great!  r=me

[Eric Seidel (no email)]

Comment on attachment 3669 [details]
DUMP KRenderingPaintServers and KCanvasResources and their properties (5th)

We caught one error, on printing KCanvasPath, which I'd like to correct before
landing.

[Kimmo Kinnunen]

Created attachment 3695 [details]
Dump KRenderingPaintServers and KCanvasResources and their properties (6th)

Fixed paths to print relevant attributes only (depending on path command).
Moved path related dumping operators to KPathData.cpp, which didn't exist. This
means that the patch touches WebCore/WebCore.xcodeproj/project.pbxproject also.


Also fixes one small output formating detail (clip path names were not inside
hyphens "")

Note, that in order to compile WebCore+SVG currently I needed to add
EventNames.cpp to be compiled in WebCore+SVG project. This change is also
included in the patch, as I didn't feel confident enough to just delete the +
line from the patch.

[Kimmo Kinnunen]

Created attachment 3696 [details]
Layout-tests for 3695

Layout-tests for 3695

[Eric Seidel (no email)]

Comment on attachment 3695 [details]
Dump KRenderingPaintServers and KCanvasResources and their properties (6th)

This looks fine, and all the tests pass on my machine.	I would like to discuss
the license with you before landing, so I've posted an alternative patch.  We
can discuss and land tonight.

[Eric Seidel (no email)]

Created attachment 3706 [details]
A couple cleanups (including license)