Bug 3249 - Investigate merging the WebCore DOM and KDOM
Summary: Investigate merging the WebCore DOM and KDOM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 412
Hardware: Mac OS X 10.4
: P4 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
: 3914 (view as bug list)
Depends on: 5572 5583 5584 5585 5671
Blocks:
  Show dependency treegraph
 
Reported: 2005-06-01 16:21 PDT by Dave Hyatt
Modified: 2005-11-23 04:23 PST (History)
3 users (show)

See Also:


Attachments
First stab at the merger (977.73 KB, patch)
2005-11-01 03:15 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
khtml & kwq side of the merger (42.53 KB, patch)
2005-11-16 17:33 PST, Eric Seidel (no email)
hyatt: review-
Details | Formatted Diff | Diff
A revised patch with Hyatt's suggested changes. (44.79 KB, patch)
2005-11-17 12:57 PST, Eric Seidel (no email)
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2005-06-01 16:21:26 PDT
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.
Comment 1 Eric Seidel (no email) 2005-08-28 02:48:40 PDT
This one is on my plate... I just haven't found cycles for it yet.
Comment 2 Eric Seidel (no email) 2005-11-01 03:15:24 PST
Created attachment 4549 [details]
First stab at the merger
Comment 3 Eric Seidel (no email) 2005-11-01 03:18:08 PST
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...
Comment 4 Eric Seidel (no email) 2005-11-01 11:57:41 PST
More recent patch posted:
http://www.eseidel.com/quick.patch
Comment 5 Eric Seidel (no email) 2005-11-15 02:20:35 PST
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
Comment 6 Eric Seidel (no email) 2005-11-16 17:33:06 PST
Created attachment 4706 [details]
khtml & kwq side of the merger
Comment 7 Eric Seidel (no email) 2005-11-16 17:34:53 PST
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.
Comment 8 Dave Hyatt 2005-11-17 11:38:10 PST
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"
Comment 9 Dave Hyatt 2005-11-17 11:39:57 PST
Patch is awesome btw.  :)
Comment 10 Eric Seidel (no email) 2005-11-17 12:49:59 PST
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.
Comment 11 Eric Seidel (no email) 2005-11-17 12:57:05 PST
Created attachment 4713 [details]
A revised patch with Hyatt's suggested changes.

This revision does not include making SVGRenderStyle lazily created.
Comment 12 Dave Hyatt 2005-11-17 13:26:22 PST
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
Comment 13 Eric Seidel (no email) 2005-11-17 14:21:19 PST
Fixed.
Comment 14 Eric Seidel (no email) 2005-11-20 22:34:55 PST
*** Bug 3914 has been marked as a duplicate of this bug. ***