Summary: | KCanvas needs to be redesigned to fill & stroke at once | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | rwlbuis | ||||||||||||
Priority: | P4 | ||||||||||||||
Version: | 420+ | ||||||||||||||
Hardware: | Mac | ||||||||||||||
OS: | OS X 10.4 | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 6447 | ||||||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2005-11-15 00:47:38 PST
This could be done by changing the fill and stroke painters to have "setup" and "teardown" methods instead of draw(), where you could just have them change the context state accordingly, and then actually do the drawing elsewhere. Basically the fill and paint servers should be removed. The paint servers should have setup and tear-down methods. KCanvasRenderingStyle should also be killed at the same time. Created attachment 5516 [details]
move paintserver/draw from fill/stroke painters into the callers
Comment on attachment 5516 [details]
move paintserver/draw from fill/stroke painters into the callers
We talked online. Although this is a good patch, I think that it makes more
sense (in terms of process overhead) to go a bit further along the removal
process first.
I encoruaged Alexander to take the approache of just removing all the set*
methods on KCanvasRenderingStyle and making all of the get methods dynamically
compute/lookup, etc. all the necessary info. That way none of the callers have
to change (more than they have in this patch), but KCanvasRenderingStyle
becomes a complete no-op and we can get rid of more of this uncessary code in
one fell swoop.
I also encoruaged alexander to make sure that he posts layout test changes, and
is sure to test with --leaks.
Created attachment 5536 [details]
broken patch
Comment on attachment 5536 [details]
broken patch
A few comments:
RenderPath::setupForDraw()
should now be part of draw. If something needs to fill, it fetches it's paint
server, it sets the active client (that will go away after my next patch) and
then paints with that paint server.
fillPaintServer should be a static method:
canvasStyle()->fillPaintServer(style(), this)
KCanvasRenderingStyle::fillPaintServer(style(), getDocument())
fillPaintServer(), strokePaintServer() shoudl return *shared* instances for
solid and image paint servers. In fact, PaintServerImage is slated to die, it
just hasn't yet.
disableFillPainter() and disableStrokePainter() should no longer be necessary.
Created attachment 5551 [details]
new renderstyle refactor - compiling, taking in most of previous suggestions with exception of static's and shared solid/image painter
Comment on attachment 5551 [details]
new renderstyle refactor - compiling, taking in most of previous suggestions with exception of static's and shared solid/image painter
We dicussed a few minor things over IRC. This patch in general looks fabulous.
A couple reminders:
1. ChangeLog entry should be included in your final patch
2. Make sure you update the copyright on any files you touch.
3. Please post a (separate) patch containing the expected changes to the
layout tests.
4. Also, if you think this could in any way affect normal rendering, please
post (a) new layout test(s) demonstrating those changes.
Created attachment 5555 [details]
take the time to cleanup the code while refactoring on suggestion from macdome
Comment on attachment 5555 [details]
take the time to cleanup the code while refactoring on suggestion from macdome
Again, looks great. A couple comments:
No need to check canvasStyle() or path() first, just have:
KRenderingPaintServer *fillPaintServer =
canvasStyle()->fillPaintServer(canvasStyle()->renderStyle(), this);
and then check if non-null, if non-null, setActiveClient(this) and draw()
overrideFillPaintServer overrideStrokePaintServer shoudl assume type IMAGE, and
shoudl ASSERT that the passed in paint server is of that type. Also, I woudl
encourage you to land a // FIXME: right next to those functions describing how
they were going away.
Otherwise this looks great.
You had mentioned there was a laytou test regression wrt Images. We'll wait to
land until that's fixed.
Created attachment 5565 [details]
renderstyle refactors, final patch
Comment on attachment 5565 [details]
renderstyle refactors, final patch
This patch is absolutely beyond words wonderful. Soooo much ugly code: all
gone! r=me.
|