Mike, there is a *lot* of code in that patch that doesn't meet the coding style guidelines (<http://webkit.org/coding/coding-style.html>). There are commented out blocks of code, mismatching indentation levels, incorrect brace and parenthesis placement, functions with incorrect case in their names, and various other things.
(In reply to comment #2)
> Mike, there is a *lot* of code in that patch that doesn't meet the coding style
> guidelines (<http://webkit.org/coding/coding-style.html>). There are commented
> out blocks of code, mismatching indentation levels, incorrect brace and
> parenthesis placement, functions with incorrect case in their names, and
> various other things.
>
AffineTransform was ugly thats obvious.
You will have to give me some more info on where your seening problems.
Its a lot of code so a few pointers would be helpful.
I saw a few others like somehow the patch to RenderSVGContainter has tabs.
But outside of Affine which was junked I'm not seeing huge problems.
AffineTransform has lots of commented out code in it. Your second patch has changes to the header -- did you intend to include changes to the implementation too?
In GraphicsContextCairo::pathDebugString, and other places, you have inconsistent use of spaces around assignments. You also have the ()'s placed incorrectly next to if's in that function, and others.
You have braces around a single-line if inside Path::operator=. Path::strokeBoundingRect appears to have incorrect indentation, and incorrect spacing in the "if" statement.
The Page constructor in PageGdk.cpp has its member initialization incorrectly laid out. Page::windowRect has incorrect spacing around if statements.
I could easily go on, but as you say it is a lot of code.
(In reply to comment #5)
> AffineTransform has lots of commented out code in it. Your second patch has
> changes to the header -- did you intend to include changes to the
> implementation too?
>
> In GraphicsContextCairo::pathDebugString, and other places, you have
> inconsistent use of spaces around assignments. You also have the ()'s placed
> incorrectly next to if's in that function, and others.
>
> You have braces around a single-line if inside Path::operator=.
> Path::strokeBoundingRect appears to have incorrect indentation, and incorrect
> spacing in the "if" statement.
>
> The Page constructor in PageGdk.cpp has its member initialization incorrectly
> laid out. Page::windowRect has incorrect spacing around if statements.
>
> I could easily go on, but as you say it is a lot of code.
>
Okay fixed that one its easy once some one points it out. Thanks.
Not sure how to go forward I can keep resubmitting the patch but unless
I get a few pointers its going to be a long drawing out process.
I actually have dyslexia pretty bad so generally I use automated formatters
since I physically can't see stuff like this unless some points it out.
I had to stare at the windowRect function for several minutes before I even saw what your saying.
So far with the current coding style the tools I have cause more problems then they cure. I've got a mac mini but I did not see that you could set it up for indenting correctly.
I filed a bug a while back about auto formatting and with my condition it really really helps.
In the interim please point these issue out if I know to look for a certain issue I'll eventually see it. I did file a bug about auto-formatting and I do try honest and I hate to claim a weakness but on the same had I can't fix what I can't see.
(In reply to comment #6)
Here is the bug about auto-formatting I received no response on it.
http://bugs.webkit.org/show_bug.cgi?id=9671> (In reply to comment #5)
> > AffineTransform has lots of commented out code in it. Your second patch has
> > changes to the header -- did you intend to include changes to the
> > implementation too?
> >
> > In GraphicsContextCairo::pathDebugString, and other places, you have
> > inconsistent use of spaces around assignments. You also have the ()'s placed
> > incorrectly next to if's in that function, and others.
> >
> > You have braces around a single-line if inside Path::operator=.
> > Path::strokeBoundingRect appears to have incorrect indentation, and incorrect
> > spacing in the "if" statement.
> >
> > The Page constructor in PageGdk.cpp has its member initialization incorrectly
> > laid out. Page::windowRect has incorrect spacing around if statements.
> >
> > I could easily go on, but as you say it is a lot of code.
> >
>
> Okay fixed that one its easy once some one points it out. Thanks.
> Not sure how to go forward I can keep resubmitting the patch but unless
> I get a few pointers its going to be a long drawing out process.
>
> I actually have dyslexia pretty bad so generally I use automated formatters
> since I physically can't see stuff like this unless some points it out.
> I had to stare at the windowRect function for several minutes before I even saw
> what your saying.
>
> So far with the current coding style the tools I have cause more problems then
> they cure. I've got a mac mini but I did not see that you could set it up for
> indenting correctly.
>
> I filed a bug a while back about auto formatting and with my condition it
> really really helps.
>
> In the interim please point these issue out if I know to look for a certain
> issue I'll eventually see it. I did file a bug about auto-formatting and I do
> try honest and I hate to claim a weakness but on the same had I can't fix what
> I can't see.
>
Heya Mike!
First of all I really like your interessest regarding a Cairo/gdk based
SVG rendering device! Though I am not sure wheter it should go in, because:
- Since some months, there is following plan floating around:
KCanvas own RenderingDeviceContext should die, and the functionality
of it should be merged into GrahpicsContext directly. That will eliminate
a lot of confusion. The resources (masker, clipper, marker, patterns,
gradients, filters) etc. of need to be merged into platform/ code as an
follow-up patch.
We currently only have a Qt-device & Quartz rendering device in kcanvas/device
in WebKit. If the KRenderingDeviceContext kill happens, we already need to change & test two implementations, a third one in SVN is one more to fix.
As stated above, this area is supposed to change a lot. If you want to
commit this SVG rendering device in SVN (to save bitrod), this is no problem (for me) at all, though you are warned, that you'll probably need to fix
a lot, when this change happens.
For me, new semester started today, and it'll take some time until I have
sufficient time for SVG hacking in WebKit - though this GraphicsContext
<-> RenderingDeviceContext merge is up on my todo list). If you feel
like working on it, this would be highly appreciated :-)
Greetings,
Nikolas Zimmermann
One way to make this go much smoother would be to beak up you're patch into to smaller units. It's rather hard to give specific comments on so much code, so breaking it up would really help.
That said, I'm really excited about what you're trying to accomplish here. It's a big undertaking.
One way to make this go much smoother would be to beak up you're patch into to smaller units. It's rather hard to give specific comments on so much code, so breaking it up would really help.
That said, I'm really excited about what you're trying to accomplish here. It's a big undertaking.
(In reply to comment #8)
> Heya Mike!
>
> First of all I really like your interessest regarding a Cairo/gdk based
> SVG rendering device! Though I am not sure wheter it should go in, because:
> - Since some months, there is following plan floating around:
>
> KCanvas own RenderingDeviceContext should die, and the functionality
> of it should be merged into GrahpicsContext directly. That will eliminate
> a lot of confusion. The resources (masker, clipper, marker, patterns,
> gradients, filters) etc. of need to be merged into platform/ code as an
> follow-up patch.
>
> We currently only have a Qt-device & Quartz rendering device in kcanvas/device
> in WebKit. If the KRenderingDeviceContext kill happens, we already need to
> change & test two implementations, a third one in SVN is one more to fix.
>
> As stated above, this area is supposed to change a lot. If you want to
> commit this SVG rendering device in SVN (to save bitrod), this is no problem
> (for me) at all, though you are warned, that you'll probably need to fix
> a lot, when this change happens.
>
Not a problem I'm committed to to this SVG is the heart and sould of the Gdk
port otherwise it won't work.
I'd suggest you take a hard look at my patches there were some serious problems
with affine transforms and the html renderer I have to have foriegnObject working correctly. I'd suggest that before you do major work on svg you consider making the html renderer affine transform aware. It uses m_x,m_y fixed offeset and does not understand translation and there seemed to be a number of bugs with
embedding the html in SVG. Next if you allow and arbitrary transform for the embedded object then you need to go with my lightweight frame concept which has its own set of problems that need to be worked out since the hitdetection code
did not work correctly FrameView's coordinates were put at 0,0.
My patch contains changes that allow all this to work correctly despite the problems but its not the cleanest solution since I did not wan't to change
a lot of the basic render classes. My suggestion is to incorporate the transform into the basic rending first then look at further changes for svg.
Next as far as cairo goes. I think we must work closely with the cairo team to get a good implementation. And agian if you look at my code you will see that I made GraphicsContext equal to a cairo context and made path a context.
The problem with cairo is that path is not a first class object and probably never will be. This means its easy to take other drawing libraries and create a cairo like api but its really hard to take cairo and use it to implement a api it was not designed for.
So if their is any intrest in a good cairo port I really think we need to consider cairos unique global context first and make the api compatible with that. Other systems can trivially emulate the behavior with no loss in performance.
What this means is we have to define the destination surface at the very begining
before we even parse the svg. This will allow us to cache cairo_t's for the surface and use them later during the rendering process so the performance can be quite good. Once I accepted cairo's design I realized it was probably the best for high speed complex 2D drawing. The rendering engine should then use the cached cairo_t's for rendering. So if you go with cairo's design you can get fast rendering for cairo and its easy to emulate a cairo_t in other api's. If you don't then cairo will always be too slow. I think cairo has it right since it allows you to optimize the 2D drawing path for the destination so you don't have to do a lot of complex math during the rendering phase.
This means the target surface needs to be a global and if you change targets
say go to printing or offscreen we need a way to reset cairo info for the new target.
So cairo is a catch 22 api if you follow its design you can probably get faster 2D rendering then other approaches if you try and force an alternative api on cairo then cairo will be slow.
> For me, new semester started today, and it'll take some time until I have
> sufficient time for SVG hacking in WebKit - though this GraphicsContext
> <-> RenderingDeviceContext merge is up on my todo list). If you feel
> like working on it, this would be highly appreciated :-)
>
> Greetings,
> Nikolas Zimmermann
>
(In reply to comment #9)
> One way to make this go much smoother would be to beak up you're patch into to
> smaller units. It's rather hard to give specific comments on so much code, so
> breaking it up would really help.
>
> That said, I'm really excited about what you're trying to accomplish here.
> It's a big undertaking.
>
I wish I could its all inter-related the changes are actually a large branch.
The gdk port is based on the concept of implementing WebKit in WebKit with primitive support from the host system and SVG is the fundamental language for the port not HTML so this patch is laying the foundation for my port.
Believe it or not I did stop as soon as I had a reasonable base.
I made resolving the use of a autoformat tool a blocking bug for this patch.
It was suggested that eclipse might have enough formatting options to match the webkit style. The only request I have is that at least one of the auto format options is available on linux and free.
Created attachment 11153[details]
bakefile changes fixed for linux compile and a few misc changes
This patch is mainly changes for build I did make one reasonable change to document there is a comment inline in the patch. Basically proposed moving the flushing out of Frame and into the document itself. The functionality seemed misplaced.
Created attachment 11154[details]
Extensive changes to gdk to compile and support svg compenents
A major rewrite of the gdk port to support svg widgets lightweight frames upgrade of the curl download support etc.
Created attachment 11155[details]
Removed use of gtk for closing window added svg web browser demo
Please don't accept any patches with gtk calls.
If you think they are needed ask me.
A Gtk driver/port would be a different related project.
Created attachment 11156[details]
This patch allows you to get the correct offsets for svg foriegnObjects
This patch corrects problems with offest of a forgien object since svg uses affine transforms and html uses m_x my. It may be gdk specific not sure.
The long term fix is to move consistently to a affine transform at least for all containers.
Created attachment 11157[details]
astyle plus sed script for formatting
This is the script I used to format code it gets reasobly close.
astyle version is 1.19
I'd like to see it move into the WebKitTools/Scripts directory if its acceptable.
I'm willing to keep working on it till it can either format correctly or warn
if it detects a possible formatting problem.
Comment on attachment 11269[details]
Roll up patch with missing cairo files added
This patch is huge and includes a bunch of different unrelated changes. Also I think it has now been outdated by other changes. Please update, and I'd also appreciate if you could break this up into smaller patches for the separate issues like SVG support separate from things like ResourceLoader changes and scrolling changes.
(In reply to comment #24)
> (From update of attachment 11269[details] [edit])
> This patch is huge and includes a bunch of different unrelated changes. Also I
> think it has now been outdated by other changes. Please update, and I'd also
> appreciate if you could break this up into smaller patches for the separate
> issues like SVG support separate from things like ResourceLoader changes and
> scrolling changes.
>
I'm working on smaller patches now. The roll up patch was more to allow the port to build.
The plan is to submit two patches that address building the gdk port and resource loader changes.
Next I'll submit two more one adding Path support to cairo
and the next adding svg support changes.
I'm going to rework the way I did Path.
Created attachment 11413[details]
Patch to build the gdk port
This patch is only to allow the gdk port to build with the current head.
The changes to common code are minimal the SVG dependencies are removed.
It is not completely functional i.e page are not loading.
But if this patch can be reviewed and applied in a timely manner it will greatly
reduce the size and scope of the SVG patches.
I believe I've done as asked and split the patch to a smaller subset but please please review and apply this patch asap.
So I can get the port functioning again.
Additional patches for SVG/Cairo comming over the next few days.
Created attachment 11419[details]
patch to add path support for cairo
This patch overlaps the previous one in TemporaryLinkStubs.cpp and in the changelog with the gdk.patch
Comment on attachment 11419[details]
patch to add path support for cairo
To build now with this patch you need to add
platform/cairo/PathCairo.cpp
to the source bakefile
I did not add this to the patch for a obvious online change
The incoming svg one will include it along with the rest of the changes needed for svg
Created attachment 11509[details]
Smaller gdk patch to allow it to build and load URLS agian
Attached is a smaller patch to get gdk port working agian.
Mike, there are a large number of tabs introduced in your latest patch. As per the coding style guidelines, tabs should not be used for indentation. There are a number of other coding style issues: several commented out blocks of code, incorrect brace placement in class declarations in header files, #include statements in arbitrary order rather than sorted alphabetically, and so on. While skimming the patch I also noticed a block of code that looks a little suspicious:
+void FrameLoader::checkLoadCompleteForThisFrame()
+{
+ ASSERT(m_client->hasWebView());
+ notImplemented();
+
+ switch (m_state) {
+ case FrameStateProvisional: {
+ }
+
+ case FrameStateCommittedPage: {
Is it intended for the FrameStateProvisional case to fall through to the FrameStateCommittedPage case? The braces are extraneous either way, but if the intent _is_ to fall through I think it could be made more obvious.
(In reply to comment #33)
> Mike, there are a large number of tabs introduced in your latest patch. As per
> the coding style guidelines, tabs should not be used for indentation. There
> are a number of other coding style issues: several commented out blocks of
> code, incorrect brace placement in class declarations in header files, #include
> statements in arbitrary order rather than sorted alphabetically, and so on.
> While skimming the patch I also noticed a block of code that looks a little
> suspicious:
>
> +void FrameLoader::checkLoadCompleteForThisFrame()
> +{
> + ASSERT(m_client->hasWebView());
> + notImplemented();
> +
> + switch (m_state) {
> + case FrameStateProvisional: {
> + }
> +
> + case FrameStateCommittedPage: {
>
> Is it intended for the FrameStateProvisional case to fall through to the
> FrameStateCommittedPage case? The braces are extraneous either way, but if the
> intent _is_ to fall through I think it could be made more obvious.
>
Bummer.
I forgot to format.
As far as the code goes that was copied from the mac impl dunno if its right or not. I do know I can load www.google.com agian. Note the notImplemented call indicating I did not quite agree with the code either.
Sending a formatting patch.
(In reply to comment #37)
> Hey Mike - any news on your patches?
>
> Niko
>
The current ones are pretty much dead now unless someone refactor them for the current head. With that said it should not be too hard to get them to apply.
Since the gdk port itself is broken on the head you would have to test them under the windows port which I have not worked on.
If the qt port was factored to allow cairo it would be really nice since I could submit cairo only patches easily.
On my personal branch I've re factored webkit to make it a true platform independent library not dependent on any platform gui toolkit and removed most of the platform support so I've since gone in a different direction. Cairo is still required in this version but I've not updated to the svg support.
I will soon and I will resubmit the above patches against the head if needed.
I won't do anything to these patches but get them to work with the current head so if someone else gets them in before I do it would be fantastic.
The gdk port is not dead others will be working on it so hopefully it will come back to life soon.
Sorry for the long reply :)
2006-10-17 15:34 PDT, Michael Emmel
2006-10-17 20:46 PDT, Michael Emmel
2006-10-19 14:05 PDT, Michael Emmel
2006-10-19 14:07 PDT, Michael Emmel
2006-10-19 14:10 PDT, Michael Emmel
2006-10-19 14:13 PDT, Michael Emmel
2006-10-19 14:16 PDT, Michael Emmel
2006-10-19 14:21 PDT, Michael Emmel
2006-10-28 13:07 PDT, Michael Emmel
2006-10-28 13:35 PDT, Michael Emmel
2006-11-07 00:45 PST, Michael Emmel
2006-11-07 19:05 PST, Michael Emmel
2006-11-08 10:42 PST, Michael Emmel
2006-11-13 13:46 PST, Michael Emmel
2006-11-13 16:38 PST, Michael Emmel