Bug 3249 – Investigate merging the WebCore DOM and KDOM

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

[Dave Hyatt]

This is tracking bug for the discussion of merging the DOM used by WebCore with KDOM in such a way 
that KSVG can be ported to WebCore.

[Eric Seidel (no email)]

This one is on my plate... I just haven't found cycles for it yet.

[Eric Seidel (no email)]

Created attachment 4549 [details]
First stab at the merger

[Eric Seidel (no email)]

So I went ahead and took my first stab at this merger this weekend.  I have attached below a list of 
(most of) the issues I encountered and solved, as well as some which I have encountered, but have yet 
to solve.

Integration issues:

- Headers (temporarily every kdom header maps to all webcore headers)
- Namespace mapping (KDOM -> DOM, khtml)
- Various KDOM stub classes (temporary)
- Various DOMStringImpl -> DOMString changes
- DOMString::string() -> DOMString::qstring()
- DOMString::handle() -> DOMString::impl()
- DocumentImpl, DocumentType  ::impl() -> ::implentation()
- Moving NodeImpl::Id to QualifiedString/AtomicString
- SVGNames.*, SVGElementFactory.* (and perl script to generate them)
- EventImpl::id() -> type() with AtomicString, removal of SVGEventImpl
- Adding svg document and event creation in xml_docimpl.h
- Cleanup event dispatch, in SVGDocumentImpl, using SharedPtr
- exceptioncode additions, for appendNode, createEvent, dispatchEvent, etc.
- Mapped KSVGPart, KDOMPart, KSVGView, KDOMView to KHTMLPart, KHTMLView
- Removed all throw calls, replacing with exceptioncode parameters
- SVGElement derives from StyledElementImpl for now.


Outstanding:
- DocumentBuilder vs. Tokenizer
- Merger of the Ecma systems
- CSS Parsing systems
- Event dispatch/Listening issues
- Merge KSVGView properties into KHTMLView
- requestDocument & CachedDocument vs. requestFrame and CachedObject...

[Eric Seidel (no email)]

More recent patch posted:
http://www.eseidel.com/quick.patch

[Eric Seidel (no email)]

Additional things I've done:
- Corrected attribute casing (viewBoxAttr vs. viewboxAttr)
- Moved TimeScheduler from Doc -> SVGSVGElement (temporary, wrong, solution)
- Moved off of DocPtr onto DocumentImpl * directly
- Made SVGRenderStyle a member of khtml::RenderStyle
- Worked KSVGCSSParser into CSSParser
- Worked SVGCSSStyleSelector into CSSStyleSelector
- Changed parseAttribute -> parseMappedAttribute
- Moved KCanvasContainer onto RenderContainer
- Renamed KCanvasItem, RenderPath, moved onto RenderObject
- Added bbox(), isRenderPath(), isKCanvasContainer() to RenderObject
- Moved KCanvasContainer and RenderPath from draw() -> paint()
- Removed RenderSVGBox
- Gutted KCanvasItem (now RenderPath) and KCanvasContainer
- Merged KCanvasTreeDebug into KWQRenderTreeDebug
- Made SVGNames.* SVGElementFactory.* generation automatic (build phase)

Most recent patch (not including previous changes): http://www.eseidel.com/quick3.patch

[Eric Seidel (no email)]

Created attachment 4706 [details]
khtml & kwq side of the merger

[Eric Seidel (no email)]

Comment on attachment 4706 [details]
khtml & kwq side of the merger

I would like to have at least Dave, and possible Maciej look at this before I
land.  Note that *NONE* of this is code will be enabled by default, yet. 
SVG_SUPPORT is still turned off in the main WebCore target.  Once this is
approved I can land the entire KDOM-DOM merger.

[Dave Hyatt]

Comment on attachment 4706 [details]
khtml & kwq side of the merger

(1) You need to add the SVG MIME type to khtml/ecma/xmlhttprequest.cpp.

(2) I don't like the name KRenderingDeviceQuartz used directly in RenderCanvas.
 I have several problems with this name.  First of all, the convention for
RenderTheme was to use RenderThemeMac (not Quartz).  I think it's better to
name the platform rather than the graphics library for platform-specific code. 
Second is this object really even platform-specific?  If it is, then
RenderCanvas should not contain direct references to platform-specific objects.
 If it isn't, then the file is misnamed.

(3) I think you should rename your KCanvasContainers to SVGRenderContainers and
the KCanvasRegistry to something like the SVGRenderingRegistry.

(4) You make an SVGRenderStyle for every style object.	These should either be
lazily allocated or the SVG styles should use DataRef structs like the other
CSS structs for efficient sharing.

(5) The SVG <style> tag support has a funny code style that doesn't match
guidelines.  No spaces after the ifs, braces on the wrong lines, etc.

(6) Typo in rendererIsNeeded:

"+    // SVG ignores arbitrary xml elements in its render tree in contrary to
the normal CSS/XML behavior.
"

Change "in contrary" to just be "contrary"

[Dave Hyatt]

Patch is awesome btw.  :)

[Eric Seidel (no email)]

Hyatt:   Thanks for the response.  I've answered your comments inline:

(In reply to comment #8)
> (From update of attachment 4706 [details] [edit])
> (1) You need to add the SVG MIME type to khtml/ecma/xmlhttprequest.cpp.
done.  I also added DOMImplementationImpl::isXMLMIMEType so as to have a single override point.

> (2) I don't like the name KRenderingDeviceQuartz used directly in RenderCanvas.
>  I have several problems with this name.  First of all, the convention for
> RenderTheme was to use RenderThemeMac (not Quartz).  I think it's better to
> name the platform rather than the graphics library for platform-specific code. 
> Second is this object really even platform-specific?  If it is, then
> RenderCanvas should not contain direct references to platform-specific objects.
>  If it isn't, then the file is misnamed.

Agreed.  I'm not planning on making any more renames in this patch, however I'm happy to abstract the 
direct reference via the use of QPainter (which is already abstracted via KWQ.  Would that suffice?  All of 
KCanvas will be renamed shortly to use Render* names.  But not in this patch.

> (3) I think you should rename your KCanvasContainers to SVGRenderContainers and
> the KCanvasRegistry to something like the SVGRenderingRegistry.

Same as above.  These are historical names, which I do not plan to correct in this patch.  
KCanvasRegistry will be removed soon anyway.

> (4) You make an SVGRenderStyle for every style object.	These should either be
> lazily allocated or the SVG styles should use DataRef structs like the other
> CSS structs for efficient sharing.

You'll have to explain this in person.  I think I could make a lazily allocated version, I had not planned 
to do so for this patch. All of these blocks of code are of course in SVG_SUPPORT which is off in the 
normal WebCore target.  I will obviously have to correct some of these glaring performance issues 
before turning SVG on for the main target.

> (5) The SVG <style> tag support has a funny code style that doesn't match
> guidelines.  No spaces after the ifs, braces on the wrong lines, etc.

I can correct that.  This was copied from KSVG2 which has (inconsistant but) different style than 
WebCore.  I am slowly cleaning up part of KSVG2, but also trying not to make needless style changes in 
an effort to maintain source compatitibility with kde.

> (6) Typo in rendererIsNeeded:
> 
> "+    // SVG ignores arbitrary xml elements in its render tree in contrary to
> the normal CSS/XML behavior."
>
> Change "in contrary" to just be "contrary"

fixed.

[Eric Seidel (no email)]

Created attachment 4713 [details]
A revised patch with Hyatt's suggested changes.

This revision does not include making SVGRenderStyle lazily created.

[Dave Hyatt]

Comment on attachment 4713 [details]
A revised patch with Hyatt's suggested changes.

Ok, I made sure to mention these issues just so they can be tracked.  They
don't have to hold up the landing.

Looks like domparser.cpp needs to use the xmlmimetype function you made.  It
also doesn't handle SVG.

Fix that and r=me

[Eric Seidel (no email)]

Fixed.

[Eric Seidel (no email)]

*** Bug 3914 has been marked as a duplicate of this bug. ***